Hey Gagan: Thanks for the responses. Brief comments inline, then continuing the review.
On Wed, Jul 14, 2010 at 11:20 PM, <[email protected]> wrote: > > 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. No worries, I once accidentally committed test code that pointed to my local directory structure. > > > 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) Agreed w/ the rationale; I'm just suggesting not changing the API's semantics out of an abundance of caution. Perhaps it's over-paranoid, but I'd prefer to see a new API added that does addHeader rather than setHeader. The two could be differentiated on a boolean, w/ both deferring to a helper method (basically this one, w/ a new bool applied). > > > 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. Agreed, feel free to add one. I did a triple-take when reading this CL myself due to this existing confusion. > > > http://codereview.appspot.com/1822041/show >
