Somehow the patch got out of sync w/ the current Shindig (chunk mismatch) -- give it a quick update. Thx
http://codereview.appspot.com/1822041/diff/33001/34001 File main/java/org/apache/shindig/gadgets/http/HttpRequest.java (right): http://codereview.appspot.com/1822041/diff/33001/34001#newcode54 main/java/org/apache/shindig/gadgets/http/HttpRequest.java:54: // TODO: Convert to Map<String, List<String>> to allow multiple values for a key. @see comment in UriUtils. These are internal params, not resource URI params, so the String, String mapping is intentional. http://codereview.appspot.com/1822041/diff/33001/34002 File main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java (right): http://codereview.appspot.com/1822041/diff/33001/34002#newcode53 main/java/org/apache/shindig/gadgets/rewrite/ProxyingContentRewriter.java:53: // new ConcatVisitor.Css(config, concatUriManager), Why commenting these out? That there are 3 visitors here is key to the rewriter's functionality. It ensures that concatenated resources don't later get proxied. http://codereview.appspot.com/1822041/diff/33001/34003 File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right): http://codereview.appspot.com/1822041/diff/33001/34003#newcode182 main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:182: // Copy the post body. if it exists, presumably http://codereview.appspot.com/1822041/diff/33001/34005 File main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right): http://codereview.appspot.com/1822041/diff/33001/34005#newcode76 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:76: public enum DisallowedRequestHeaders { could just simplify this by calling it DisallowedHeaders. Whether they're used in Request or Response context can be a matter of convention (naming) and/or simple use. http://codereview.appspot.com/1822041/diff/33001/34005#newcode146 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:146: resp.addHeader(entry.getKey(), entry.getValue()); I'd argue that set is still appropriate, since the semantics of copyResponseHeadersAndStatusCode have been to fully replace the response's headers w/ those provided. http://codereview.appspot.com/1822041/diff/33001/34005#newcode181 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:181: if (headerValues != null && isValidHeaderName(header) && probably want to do a headerValues.hasMoreElements() check. http://codereview.appspot.com/1822041/diff/33001/34005#newcode213 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:213: req.setParam(paramName, value); This underscores a really confusing note in our API. HttpRequest.set/getParam() isn't a representation of URI params -- it's a representation of "internal"/contextual params used by the system. Specifically, get/setParam() is only used for image processing, as a way of sending resizing parameters to the rewriter mechanism. That's because these params appear in the proxy URI, *not* the resource URI. In the case of Accel, params should appear in the resource URI, I presume. Or did you intend this for passing custom internal config params to accel's handlers? http://codereview.appspot.com/1822041/show
