You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Marshall Shi <sh...@cn.ibm.com> on 2013/01/03 09:26:53 UTC
Re: Review Request: If-Modified-Since header not handled properly in gadgets
js servlet
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8766/
-----------------------------------------------------------
(Updated Jan. 3, 2013, 8:26 a.m.)
Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and Rich Thompson.
Description
-------
The gadgets js servlet response can never be 304. Even if the If-Modified-Since header present in the request header and the gadgets js content has been cached in browser.
Shindig provides required js processors and optional js processors. In current implementation, the IfModifiedSinceProcessor is doing the right thing to set a 304 response status code and stop the other optional js processors. But CompilationProcessor, which is a required js processor, will always run and always use a 200 to overwrite the 304 status code. Thus in the gadget js servlet layer, it won't return 304.
The proposed fix is to add a check in the beginning of CompilationProcessor, if the status code is already set to 304, the closure compilor won't start. In the gadget js servlet, also add a check for 304 status code.
This addresses bug SHINDIG-1890.
https://issues.apache.org/jira/browse/SHINDIG-1890
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistry.java 1406188
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsServingPipelineModule.java 1406188
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java 1406188
Diff: https://reviews.apache.org/r/8766/diff/
Testing
-------
Thanks,
Marshall Shi
Re: Review Request: If-Modified-Since header not handled properly in gadgets
js servlet
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8766/#review15508
-----------------------------------------------------------
Ship it!
Committed r1435421.
Please close this review and make sure the patch in the Jira is up to date.
- Dan Dumont
On Jan. 15, 2013, 2:22 a.m., Marshall Shi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8766/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2013, 2:22 a.m.)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and Rich Thompson.
>
>
> Description
> -------
>
> The gadgets js servlet response can never be 304. Even if the If-Modified-Since header present in the request header and the gadgets js content has been cached in browser.
>
> Shindig provides required js processors and optional js processors. In current implementation, the IfModifiedSinceProcessor is doing the right thing to set a 304 response status code and stop the other optional js processors. But CompilationProcessor, which is a required js processor, will always run and always use a 200 to overwrite the 304 status code. Thus in the gadget js servlet layer, it won't return 304.
>
> The proposed fix is to move IfModifiedSinceProcessor to a third type of processor: preprocessors. If one of the preprocessors return false, the entire process will be returned. So the closure compilor won't start. In the gadget js servlet, also add a check for 304 status code.
>
>
> This addresses bug SHINDIG-1890.
> https://issues.apache.org/jira/browse/SHINDIG-1890
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistry.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsServingPipelineModule.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistryTest.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1406188
>
> Diff: https://reviews.apache.org/r/8766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Marshall Shi
>
>
Re: Review Request: If-Modified-Since header not handled properly in gadgets
js servlet
Posted by Dan Dumont <dd...@us.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8766/#review15507
-----------------------------------------------------------
Ship it!
Ship It!
- Dan Dumont
On Jan. 15, 2013, 2:22 a.m., Marshall Shi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8766/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2013, 2:22 a.m.)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and Rich Thompson.
>
>
> Description
> -------
>
> The gadgets js servlet response can never be 304. Even if the If-Modified-Since header present in the request header and the gadgets js content has been cached in browser.
>
> Shindig provides required js processors and optional js processors. In current implementation, the IfModifiedSinceProcessor is doing the right thing to set a 304 response status code and stop the other optional js processors. But CompilationProcessor, which is a required js processor, will always run and always use a 200 to overwrite the 304 status code. Thus in the gadget js servlet layer, it won't return 304.
>
> The proposed fix is to move IfModifiedSinceProcessor to a third type of processor: preprocessors. If one of the preprocessors return false, the entire process will be returned. So the closure compilor won't start. In the gadget js servlet, also add a check for 304 status code.
>
>
> This addresses bug SHINDIG-1890.
> https://issues.apache.org/jira/browse/SHINDIG-1890
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistry.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsServingPipelineModule.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistryTest.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1406188
>
> Diff: https://reviews.apache.org/r/8766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Marshall Shi
>
>
Re: Review Request: If-Modified-Since header not handled properly in gadgets
js servlet
Posted by Marshall Shi <sh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8766/
-----------------------------------------------------------
(Updated Jan. 15, 2013, 2:22 a.m.)
Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and Rich Thompson.
Changes
-------
- update test cases
Description
-------
The gadgets js servlet response can never be 304. Even if the If-Modified-Since header present in the request header and the gadgets js content has been cached in browser.
Shindig provides required js processors and optional js processors. In current implementation, the IfModifiedSinceProcessor is doing the right thing to set a 304 response status code and stop the other optional js processors. But CompilationProcessor, which is a required js processor, will always run and always use a 200 to overwrite the 304 status code. Thus in the gadget js servlet layer, it won't return 304.
The proposed fix is to move IfModifiedSinceProcessor to a third type of processor: preprocessors. If one of the preprocessors return false, the entire process will be returned. So the closure compilor won't start. In the gadget js servlet, also add a check for 304 status code.
This addresses bug SHINDIG-1890.
https://issues.apache.org/jira/browse/SHINDIG-1890
Diffs (updated)
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistry.java 1406188
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsServingPipelineModule.java 1406188
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java 1406188
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistryTest.java 1406188
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1406188
Diff: https://reviews.apache.org/r/8766/diff/
Testing
-------
Thanks,
Marshall Shi
Re: Review Request: If-Modified-Since header not handled properly in gadgets
js servlet
Posted by Marshall Shi <sh...@cn.ibm.com>.
> On Jan. 14, 2013, 4:15 p.m., Ryan Baxter wrote:
> > Marshall I am seeing some compilation problems when applying this patch can you take a look?
> >
> >
> > testProcessorModifiesResponse(org.apache.shindig.gadgets.js.DefaultJsProcessorRegistryTest): Unresolved compilation problem:
> > The constructor DefaultJsProcessorRegistry(ImmutableList<JsProcessor>, ImmutableList<JsProcessor>) is undefined
> >
> > testTwoProcessorsAreRunOneAfterAnother(org.apache.shindig.gadgets.js.DefaultJsProcessorRegistryTest): Unresolved compilation problem:
> > The constructor DefaultJsProcessorRegistry(ImmutableList<JsProcessor>, ImmutableList<JsProcessor>) is undefined
> >
> > testProcessorStopsProcessingWhenItReturnsFalse(org.apache.shindig.gadgets.js.DefaultJsProcessorRegistryTest): Unresolved compilation problem:
> > The constructor DefaultJsProcessorRegistry(ImmutableList<JsProcessor>, ImmutableList<JsProcessor>) is undefined
Ryan, I skipped the unit test last time, now they are fixed.
- Marshall
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8766/#review15314
-----------------------------------------------------------
On Jan. 15, 2013, 2:22 a.m., Marshall Shi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8766/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2013, 2:22 a.m.)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and Rich Thompson.
>
>
> Description
> -------
>
> The gadgets js servlet response can never be 304. Even if the If-Modified-Since header present in the request header and the gadgets js content has been cached in browser.
>
> Shindig provides required js processors and optional js processors. In current implementation, the IfModifiedSinceProcessor is doing the right thing to set a 304 response status code and stop the other optional js processors. But CompilationProcessor, which is a required js processor, will always run and always use a 200 to overwrite the 304 status code. Thus in the gadget js servlet layer, it won't return 304.
>
> The proposed fix is to move IfModifiedSinceProcessor to a third type of processor: preprocessors. If one of the preprocessors return false, the entire process will be returned. So the closure compilor won't start. In the gadget js servlet, also add a check for 304 status code.
>
>
> This addresses bug SHINDIG-1890.
> https://issues.apache.org/jira/browse/SHINDIG-1890
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistry.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsServingPipelineModule.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistryTest.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/JsServletTest.java 1406188
>
> Diff: https://reviews.apache.org/r/8766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Marshall Shi
>
>
Re: Review Request: If-Modified-Since header not handled properly in gadgets
js servlet
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8766/#review15314
-----------------------------------------------------------
Marshall I am seeing some compilation problems when applying this patch can you take a look?
testProcessorModifiesResponse(org.apache.shindig.gadgets.js.DefaultJsProcessorRegistryTest): Unresolved compilation problem:
The constructor DefaultJsProcessorRegistry(ImmutableList<JsProcessor>, ImmutableList<JsProcessor>) is undefined
testTwoProcessorsAreRunOneAfterAnother(org.apache.shindig.gadgets.js.DefaultJsProcessorRegistryTest): Unresolved compilation problem:
The constructor DefaultJsProcessorRegistry(ImmutableList<JsProcessor>, ImmutableList<JsProcessor>) is undefined
testProcessorStopsProcessingWhenItReturnsFalse(org.apache.shindig.gadgets.js.DefaultJsProcessorRegistryTest): Unresolved compilation problem:
The constructor DefaultJsProcessorRegistry(ImmutableList<JsProcessor>, ImmutableList<JsProcessor>) is undefined
- Ryan Baxter
On Jan. 10, 2013, 5:41 a.m., Marshall Shi wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8766/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2013, 5:41 a.m.)
>
>
> Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and Rich Thompson.
>
>
> Description
> -------
>
> The gadgets js servlet response can never be 304. Even if the If-Modified-Since header present in the request header and the gadgets js content has been cached in browser.
>
> Shindig provides required js processors and optional js processors. In current implementation, the IfModifiedSinceProcessor is doing the right thing to set a 304 response status code and stop the other optional js processors. But CompilationProcessor, which is a required js processor, will always run and always use a 200 to overwrite the 304 status code. Thus in the gadget js servlet layer, it won't return 304.
>
> The proposed fix is to move IfModifiedSinceProcessor to a third type of processor: preprocessors. If one of the preprocessors return false, the entire process will be returned. So the closure compilor won't start. In the gadget js servlet, also add a check for 304 status code.
>
>
> This addresses bug SHINDIG-1890.
> https://issues.apache.org/jira/browse/SHINDIG-1890
>
>
> Diffs
> -----
>
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistry.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsServingPipelineModule.java 1406188
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java 1406188
>
> Diff: https://reviews.apache.org/r/8766/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Marshall Shi
>
>
Re: Review Request: If-Modified-Since header not handled properly in gadgets
js servlet
Posted by Marshall Shi <sh...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8766/
-----------------------------------------------------------
(Updated Jan. 10, 2013, 5:41 a.m.)
Review request for shindig, Ryan Baxter, Dan Dumont, Stanton Sievers, and Rich Thompson.
Description (updated)
-------
The gadgets js servlet response can never be 304. Even if the If-Modified-Since header present in the request header and the gadgets js content has been cached in browser.
Shindig provides required js processors and optional js processors. In current implementation, the IfModifiedSinceProcessor is doing the right thing to set a 304 response status code and stop the other optional js processors. But CompilationProcessor, which is a required js processor, will always run and always use a 200 to overwrite the 304 status code. Thus in the gadget js servlet layer, it won't return 304.
The proposed fix is to move IfModifiedSinceProcessor to a third type of processor: preprocessors. If one of the preprocessors return false, the entire process will be returned. So the closure compilor won't start. In the gadget js servlet, also add a check for 304 status code.
This addresses bug SHINDIG-1890.
https://issues.apache.org/jira/browse/SHINDIG-1890
Diffs
-----
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/DefaultJsProcessorRegistry.java 1406188
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/js/JsServingPipelineModule.java 1406188
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java 1406188
Diff: https://reviews.apache.org/r/8766/diff/
Testing
-------
Thanks,
Marshall Shi