Re: Create Html Accelerate servlet (issue186252)

[John Hjelmstad <jo...@gmail.com>]

Re-committed, this time after remembering to svn add the new files.

On Fri, Jan 29, 2010 at 1:57 PM, <jo...@gmail.com> wrote:

> Hey Ziv:
>
> Just a few little cleanups, which I've already done myself and
> committed. Thanks!
>
> --John
>
>
> http://codereview.appspot.com/186252/diff/4001/4011
> File features/src/main/javascript/features/features.txt (right):
>
> http://codereview.appspot.com/186252/diff/4001/4011#newcode33
> features/src/main/javascript/features/features.txt:33:
> features/core.none/feature.xml
> out of order
>
> http://codereview.appspot.com/186252/diff/4001/4005
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java
> (right):
>
> http://codereview.appspot.com/186252/diff/4001/4005#newcode72
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java:72:
> HtmlAccelServlet.ACCEL_GADGET_PARAM_VALUE) {
> we should abstract this into its own (static) method on HtmlAccelServlet
> (which, for dependency reasons, could be switched out to a helper file
> if needed later)
>
> http://codereview.appspot.com/186252/diff/4001/4009
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java
> (right):
>
> http://codereview.appspot.com/186252/diff/4001/4009#newcode51
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java:51:
> HtmlAccelServlet.ACCEL_GADGET_PARAM_VALUE) {
> helper used here
>
> http://codereview.appspot.com/186252/diff/4001/4006
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
> (right):
>
> http://codereview.appspot.com/186252/diff/4001/4006#newcode79
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:79:
> private static class AccelRewritersProvider implements
> Provider<List<GadgetRewriter>> {
> looks fine that this is written as such, though @Provides methods would
> work just as well
>
> http://codereview.appspot.com/186252/diff/4001/4006#newcode82
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:82:
> @SuppressWarnings("unused")
> is this needed?
>
> http://codereview.appspot.com/186252/diff/4001/4010
>
> File
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
> (right):
>
> http://codereview.appspot.com/186252/diff/4001/4010#newcode79
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:79:
> + "</Module>\n";
> we can just change these two to use String.format(tpl,
> htmlEscaped(data));
>
> http://codereview.appspot.com/186252/diff/4001/4010#newcode85
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:85:
> public void setRequestPipeLine(RequestPipeline pipeline) {
> nit: lower-case l
>
> http://codereview.appspot.com/186252/diff/4001/4010#newcode117
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:117:
> !data.getHeader(CONTENT_TYPE).contains(HTML_CONTENT)) {
> perhaps factor this into a little helper method
>
> http://codereview.appspot.com/186252/diff/4001/4010#newcode180
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:180:
> !CONTENT_LENGTH.equals(entry.getKey())) {
> I think we can get rid of these. For each we should set them ourselves.
>
> http://codereview.appspot.com/186252/diff/4001/4003
> File
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java
> (right):
>
> http://codereview.appspot.com/186252/diff/4001/4003#newcode59
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java:59:
> public void testHtmlAccelNoData() throws Exception {
> stylistically, people usually remove the "test" prefix with jUnit4 tests
> since they're already annotated. Not a big deal though.
>
> http://codereview.appspot.com/186252/diff/4001/4003#newcode106
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java:106:
> expect(request.getRequestURL()).andReturn(new
> StringBuffer("gmodules.com/gadgets/accel")).once();
>
>> 100 char
>>
>
> http://codereview.appspot.com/186252/diff/4001/4003#newcode144
>
> java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java:144:
> expect(request.getRequestURL()).andReturn(new
> StringBuffer("gmodules.com/gadgets/accel")).once();
> same
>
>
> http://codereview.appspot.com/186252/show
>