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. On 2010/07/14 20:39:37, johnfargo wrote:
@see comment in UriUtils. These are internal params, not resource URI
params, so
the String, String mapping is intentional.
Makes sense then. Reverting this now. 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), On 2010/07/14 20:39:37, johnfargo wrote:
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.
Sorry about that. I commented these out for internal testing and forgot to revert this file. Really sorry. 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. On 2010/07/14 20:39:37, johnfargo wrote:
if it exists, presumably
Done. 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 { On 2010/07/14 20:39:37, johnfargo wrote:
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.
Wow. Now that you mention it, it really sounds so simple. Done. 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()); On 2010/07/14 20:39:37, johnfargo wrote:
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.
But there could be multiple header values for the same header, like: Set-Cookie: NAME=value; domain=hello Set-Cookie: NAME2=value2; domain=buffalo Set header would just retain the last one (i hope thats why HttpServletResponse has addHeader and setHeader functions) http://codereview.appspot.com/1822041/diff/33001/34005#newcode181 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:181: if (headerValues != null && isValidHeaderName(header) && On 2010/07/14 20:39:37, johnfargo wrote:
probably want to do a headerValues.hasMoreElements() check.
Done. http://codereview.appspot.com/1822041/diff/33001/34005#newcode213 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:213: req.setParam(paramName, value); On 2010/07/14 20:39:37, johnfargo wrote:
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?
I had thought that it was the equivalent of HttpServletRequest params, which are get/post parameter values. Might make sense to add a comment on the param variable so its more clear. http://codereview.appspot.com/1822041/show
