----------------------------------------------------------- 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 > >
