Thanks Ziv, let me know when the tests are added. On Thu, Jan 21, 2010 at 6:34 PM, <[email protected]> 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 >
