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); http://codereview.appspot.com/186252/show
