Issue 186252: Create Html Accelerate servlet

Can't Edit
Can't Publish+Mail
Start Review

Created:
15 years, 7 months ago by zhoresh

Modified:
15 years, 7 months ago

Reviewers:
johnfargo, chirag, shindig.remailer, awiner1

Base URL:
http://svn.apache.org/repos/asf/incubator/shindig/trunk/

Visibility:
Public.

Description

Take advantage of gadget rewritter and apply it on arbitrary html page. This done using new servlet (gadgets/accel?url=...). Which for now fage a gadget spec with content from the specified url. This is just the first step in making this servlet usable.

Patch Set 1 #

Total comments: 30

Patch Set 2 : Update according to comment #

Patch Set 3 : Added tests #

Patch Set 4 : Fix couple of bugs, and add separate list of rewritters for accel servlet #

Total comments: 12

Created: 15 years, 7 months ago

Download [raw] [tar.bz2]

Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+528 lines, -9 lines)			Patch
features/src/main/javascript/features/features.txt	View		1 chunk	+1 line, -0 lines	1 comment	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java	View	2 3	3 chunks	+11 lines, -3 lines	1 comment	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java	View		1 chunk	+57 lines, -0 lines	1 comment	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/HtmlRenderer.java	View		2 chunks	+5 lines, -4 lines	0 comments	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java	View		3 chunks	+25 lines, -1 line	2 comments	Download
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java	View	1 2 3	1 chunk	+195 lines, -0 lines	4 comments	Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java	View		5 chunks	+23 lines, -1 line	0 comments	Download
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/HtmlAccelServletTest.java	View		1 chunk	+176 lines, -0 lines	3 comments	Download
java/server/src/main/webapp/WEB-INF/web.xml	View		2 chunks	+12 lines, -0 lines	0 comments	Download
java/server/src/main/webapp/WEB-INF/web.full.xml	View		2 chunks	+13 lines, -0 lines	0 comments	Download
java/server/src/test/java/org/apache/shindig/server/JettyLauncher.java	View		4 chunks	+10 lines, -0 lines	0 comments	Download

Messages

Total messages: 11

Expand All Messages | Collapse All Messages

johnfargo

For some reason the code review tool can't DL the *.xml or Jetty*.java files, but ...

15 years, 7 months ago (2010-01-21 07:19:23 UTC) #2

For some reason the code review tool can't DL the *.xml or Jetty*.java files,
but most of my comments focus on the Servlet anyway.

In addition to these -- unit tests en route?

http://codereview.appspot.com/186252/diff/1/5
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/186252/diff/1/5#newcode57
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:57:
public static final String RAW_GADGETSPEC_XML_PARAM_NAME = "rawxml";
you should refer to DefaultGadgetSpecFactory.RAW_GADGETSPEC_XML_PARAM_NAME for
this, and document that using DefaultGadgetSpecFactory is an implicit
requirement for this servlet to work at this time

http://codereview.appspot.com/186252/diff/1/5#newcode71
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:71:
this.gadgetServlet = servlet;
it doesn't matter too much I suppose, but why not subclass?

http://codereview.appspot.com/186252/diff/1/5#newcode93
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:93:
if (!StringUtils.equals(data.getHeader(CONTENT_TYPE), HTML_CONTENT)) {
use contains rather than equals to catch situations such as:
Content-Type: text/html; charset=utf8;

http://codereview.appspot.com/186252/diff/1/5#newcode94
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:94:
createServletResponse(data,resp);
nit: I'd call this "respondVerbatim(...)"

http://codereview.appspot.com/186252/diff/1/5#newcode105
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:105:
if (name == ACCEL_GADGET_PARAM_NAME) {
quick comment here as well that echoes the above explanation of "=="

http://codereview.appspot.com/186252/diff/1/5#newcode119
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:119:
doGet(req,resp);
this delegates down to GadgetRenderingServlet.doGet(...) in the fallback case,
which doesn't seem right to me

http://codereview.appspot.com/186252/diff/1/5#newcode124
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:124:
String theUrl = req.getParameter(URL_PARAM_NAME);
thought: rather than parsing out the param manually, we could just use
HttpGadgetContext, which in turn parses out "url", "debug", and "nocache", all
useful params that feed into HttpRequest (such as in GadgetRenderingServlet)

http://codereview.appspot.com/186252/diff/1/5#newcode137
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:137:
private void createServletResponse(HttpResponse results, HttpServletResponse
response) throws IOException {
>100char

http://codereview.appspot.com/186252/diff/1/5#newcode139
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:139:
for ( Map.Entry<String, String> entry : results.getHeaders().entries()) {
nit: space btw ( and Map

http://codereview.appspot.com/186252/diff/1/5#newcode144
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:144:
response.sendError(results.getHttpStatusCode());
1. In these cases we typically just pass 404 (SC_NOT_FOUND) or 400
(SC_BAD_REQUEST), since it's not  an error from the part of Shindig (eg. status
50x) to get to this point.
  - 30x (redirects) are weird but we assume the HttpFetcher would have followed
them, and thus don't proxy that response code.
2. May as well pass the response body to sendError(...) as well, then return;

http://codereview.appspot.com/186252/diff/1/5#newcode155
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:155:
b.append("               author=\"Google\"\n");
not Google, Apache Shindig :)

http://codereview.appspot.com/186252/diff/1/5#newcode158
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:158:
b.append("    ${localization}\n");
what's this var for?

http://codereview.appspot.com/186252/diff/1/5#newcode165
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:165:
b.append("<![CDATA[").append(content).append("]]>\n");
this whole String should be private static final String FAKE_SPEC_TPL, with the
content piece substitutable eg. "<![CDATA[%s]]>", then substituted using:
String.format(FAKE_SPEC_TPL, content);

chirag

http://codereview.appspot.com/186252/diff/1/5 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java (right): http://codereview.appspot.com/186252/diff/1/5#newcode58 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:58: public static final String CONTENT_TYPE = "content-Type"; Shouldn't this ...

15 years, 7 months ago (2010-01-21 08:13:32 UTC) #3

zhoresh

Updated according to comment (Test is next) http://codereview.appspot.com/186252/diff/1/5 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java (right): http://codereview.appspot.com/186252/diff/1/5#newcode57 java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:57: public static ...

15 years, 7 months ago (2010-01-22 02:34:38 UTC) #5

Updated according to comment 
(Test is next)

http://codereview.appspot.com/186252/diff/1/5
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/186252/diff/1/5#newcode57
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:57:
public static final String RAW_GADGETSPEC_XML_PARAM_NAME = "rawxml";
On 2010/01/21 07:19:23, johnfargo wrote:
> you should refer to DefaultGadgetSpecFactory.RAW_GADGETSPEC_XML_PARAM_NAME for
> this, and document that using DefaultGadgetSpecFactory is an implicit
> requirement for this servlet to work at this time

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode58
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:58:
public static final String CONTENT_TYPE = "content-Type";
On 2010/01/21 08:13:32, chirag wrote:
> Shouldn't this be "Content-Type"

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode71
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:71:
this.gadgetServlet = servlet;
On 2010/01/21 07:19:23, johnfargo wrote:
> it doesn't matter too much I suppose, but why not subclass?

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode93
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:93:
if (!StringUtils.equals(data.getHeader(CONTENT_TYPE), HTML_CONTENT)) {
On 2010/01/21 07:19:23, johnfargo wrote:
> use contains rather than equals to catch situations such as:
> Content-Type: text/html; charset=utf8;

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode94
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:94:
createServletResponse(data,resp);
On 2010/01/21 07:19:23, johnfargo wrote:
> nit: I'd call this "respondVerbatim(...)"

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode105
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:105:
if (name == ACCEL_GADGET_PARAM_NAME) {
On 2010/01/21 07:19:23, johnfargo wrote:
> quick comment here as well that echoes the above explanation of "=="

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode119
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:119:
doGet(req,resp);
On 2010/01/21 07:19:23, johnfargo wrote:
> this delegates down to GadgetRenderingServlet.doGet(...) in the fallback case,
> which doesn't seem right to me

This is the actual rewriting, done by the gadget renderer

http://codereview.appspot.com/186252/diff/1/5#newcode124
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:124:
String theUrl = req.getParameter(URL_PARAM_NAME);
On 2010/01/21 07:19:23, johnfargo wrote:
> thought: rather than parsing out the param manually, we could just use
> HttpGadgetContext, which in turn parses out "url", "debug", and "nocache", all
> useful params that feed into HttpRequest (such as in GadgetRenderingServlet)

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode126
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:126:
return null;
On 2010/01/21 08:13:32, chirag wrote:
> It might be useful to send an error message saying "Missing parameter" or
> "Missing url parameter"

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode137
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:137:
private void createServletResponse(HttpResponse results, HttpServletResponse
response) throws IOException {
On 2010/01/21 07:19:23, johnfargo wrote:
> >100char

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode139
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:139:
for ( Map.Entry<String, String> entry : results.getHeaders().entries()) {
On 2010/01/21 07:19:23, johnfargo wrote:
> nit: space btw ( and Map

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode144
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:144:
response.sendError(results.getHttpStatusCode());
On 2010/01/21 07:19:23, johnfargo wrote:
> 1. In these cases we typically just pass 404 (SC_NOT_FOUND) or 400
> (SC_BAD_REQUEST), since it's not  an error from the part of Shindig (eg.
status
> 50x) to get to this point.
>   - 30x (redirects) are weird but we assume the HttpFetcher would have
followed
> them, and thus don't proxy that response code.
> 2. May as well pass the response body to sendError(...) as well, then return;

Actually here we just try to send back what we received.
So I changed it to just copy the return code, nothing more.

http://codereview.appspot.com/186252/diff/1/5#newcode155
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:155:
b.append("               author=\"Google\"\n");
On 2010/01/21 07:19:23, johnfargo wrote:
> not Google, Apache Shindig :)

Done.

http://codereview.appspot.com/186252/diff/1/5#newcode158
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:158:
b.append("    ${localization}\n");
On 2010/01/21 07:19:23, johnfargo wrote:
> what's this var for?

Not needed

http://codereview.appspot.com/186252/diff/1/5#newcode165
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:165:
b.append("<![CDATA[").append(content).append("]]>\n");
On 2010/01/21 07:19:23, johnfargo wrote:
> this whole String should be private static final String FAKE_SPEC_TPL, with
the
> content piece substitutable eg. "<![CDATA[%s]]>", then substituted using:
> String.format(FAKE_SPEC_TPL, content);

Done.

johnfargo

Thanks Ziv, let me know when the tests are added. On Thu, Jan 21, 2010 ...

15 years, 7 months ago (2010-01-22 02:36:09 UTC) #6

Thanks Ziv, let me know when the tests are added.

On Thu, Jan 21, 2010 at 6:34 PM, <zhoresh@gmail.com> wrote:

> Updated according to comment
> (Test is next)
>
>
>
> http://codereview.appspot.com/186252/diff/1/5
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
> (right):
>
> http://codereview.appspot.com/186252/diff/1/5#newcode57
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:57:
> public static final String RAW_GADGETSPEC_XML_PARAM_NAME = "rawxml";
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> you should refer to
>>
> DefaultGadgetSpecFactory.RAW_GADGETSPEC_XML_PARAM_NAME for
>
>> this, and document that using DefaultGadgetSpecFactory is an implicit
>> requirement for this servlet to work at this time
>>
>
> Done.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode58
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:58:
> public static final String CONTENT_TYPE = "content-Type";
> On 2010/01/21 08:13:32, chirag wrote:
>
>> Shouldn't this be "Content-Type"
>>
>
> Done.
>
> http://codereview.appspot.com/186252/diff/1/5#newcode71
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:71:
> this.gadgetServlet = servlet;
>
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> it doesn't matter too much I suppose, but why not subclass?
>>
>
> Done.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode93
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:93:
> if (!StringUtils.equals(data.getHeader(CONTENT_TYPE), HTML_CONTENT)) {
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> use contains rather than equals to catch situations such as:
>> Content-Type: text/html; charset=utf8;
>>
>
> Done.
>
> http://codereview.appspot.com/186252/diff/1/5#newcode94
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:94:
> createServletResponse(data,resp);
>
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> nit: I'd call this "respondVerbatim(...)"
>>
>
> Done.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode105
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:105:
> if (name == ACCEL_GADGET_PARAM_NAME) {
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> quick comment here as well that echoes the above explanation of "=="
>>
>
> Done.
>
> http://codereview.appspot.com/186252/diff/1/5#newcode119
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:119:
> doGet(req,resp);
>
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> this delegates down to GadgetRenderingServlet.doGet(...) in the
>>
> fallback case,
>
>> which doesn't seem right to me
>>
>
> This is the actual rewriting, done by the gadget renderer
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode124
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:124:
> String theUrl = req.getParameter(URL_PARAM_NAME);
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> thought: rather than parsing out the param manually, we could just use
>> HttpGadgetContext, which in turn parses out "url", "debug", and
>>
> "nocache", all
>
>> useful params that feed into HttpRequest (such as in
>>
> GadgetRenderingServlet)
>
> Done.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode126
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:126:
> return null;
> On 2010/01/21 08:13:32, chirag wrote:
>
>> It might be useful to send an error message saying "Missing parameter"
>>
> or
>
>> "Missing url parameter"
>>
>
> Done.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode137
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:137:
> private void createServletResponse(HttpResponse results,
> HttpServletResponse response) throws IOException {
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> >100char
>>
>
> Done.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode139
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:139:
> for ( Map.Entry<String, String> entry : results.getHeaders().entries())
> {
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> nit: space btw ( and Map
>>
>
> Done.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode144
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:144:
> response.sendError(results.getHttpStatusCode());
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> 1. In these cases we typically just pass 404 (SC_NOT_FOUND) or 400
>> (SC_BAD_REQUEST), since it's not  an error from the part of Shindig
>>
> (eg. status
>
>> 50x) to get to this point.
>>   - 30x (redirects) are weird but we assume the HttpFetcher would have
>>
> followed
>
>> them, and thus don't proxy that response code.
>> 2. May as well pass the response body to sendError(...) as well, then
>>
> return;
>
> Actually here we just try to send back what we received.
> So I changed it to just copy the return code, nothing more.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode155
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:155:
> b.append("               author=\"Google\"\n");
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> not Google, Apache Shindig :)
>>
>
> Done.
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode158
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:158:
> b.append("    ${localization}\n");
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> what's this var for?
>>
>
> Not needed
>
>
> http://codereview.appspot.com/186252/diff/1/5#newcode165
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:165:
> b.append("<![CDATA[").append(content).append("]]>\n");
> On 2010/01/21 07:19:23, johnfargo wrote:
>
>> this whole String should be private static final String FAKE_SPEC_TPL,
>>
> with the
>
>> content piece substitutable eg. "<![CDATA[%s]]>", then substituted
>>
> using:
>
>> String.format(FAKE_SPEC_TPL, content);
>>
>
> Done.
>
>
> http://codereview.appspot.com/186252/show
>

zhoresh

Fargo, Tests are included, and I also fixed couple of bugs: - Need to handle ...

15 years, 7 months ago (2010-01-23 01:20:25 UTC) #8

zhoresh

Fix couple of bugs, and add separate list of rewritters for accel servlet

15 years, 7 months ago (2010-01-26 23:26:22 UTC) #9

johnfargo

Hey Ziv: Just a few little cleanups, which I've already done myself and committed. Thanks! ...

15 years, 7 months ago (2010-01-29 21:57:01 UTC) #10

johnfargo

15 years, 7 months ago (2010-02-01 20:06:01 UTC) #11

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

On Fri, Jan 29, 2010 at 1:57 PM, <johnfargo@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
>

Expand All Messages | Collapse All Messages