http://codereview.appspot.com/1822041/diff/9001/10004 File main/java/org/apache/shindig/gadgets/uri/UriUtils.java (right):
http://codereview.appspot.com/1822041/diff/9001/10004#newcode70 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:70: // Headers that the fetcher itself would like to fill. On 2010/07/13 15:48:50, anupama.dutta wrote:
This comment seems slightly cryptic to me - could you give a few more
details as
to why the fetcher needs to fill this. Could you also add a comment
(similar to
38-42) just before the enum to explain what this enum is meant for?
Done. http://codereview.appspot.com/1822041/diff/9001/10004#newcode71 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:71: POST_INCOMPATIBLE_HEADERS(ImmutableSet.of("content-length")); On 2010/07/13 15:48:50, anupama.dutta wrote:
In order to use consistent terminology, would
POST_INCOMPATIBLE_DIRECTIVES be a
better name here?
Done. http://codereview.appspot.com/1822041/diff/9001/10004#newcode154 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:154: public static void copyRequestHeaders(HttpServletRequest request, On 2010/07/13 15:48:50, anupama.dutta wrote:
Similar to the previous method, using "data" for this parameter might
make this
more readable.
Done. http://codereview.appspot.com/1822041/diff/9001/10004#newcode158 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:158: for (DisallowedRequestHeaders h : disallowedResponseHeaders) { On 2010/07/13 15:48:50, anupama.dutta wrote:
disallowedResponseHeaders -> disallowedRequestHeaders.
Done. http://codereview.appspot.com/1822041/diff/9001/10004#newcode162 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:162: Enumeration<String> headerNames = request.getHeaderNames(); On 2010/07/13 15:48:50, anupama.dutta wrote:
Wouldn't using a block similar to 136-141 work here as well (with a
small change
that uses addHeader instead of setHeader)?
I couldnt find an easy way of getting headers from HttpServletRequest. As far as i know, this is the only way. In that case, this block is way too different from the previous one :( http://codereview.appspot.com/1822041/diff/9001/10004#newcode190 main/java/org/apache/shindig/gadgets/uri/UriUtils.java:190: Enumeration<String> paramNames = request.getParameterNames(); On 2010/07/13 15:48:50, anupama.dutta wrote:
Just wondering - Isn't there a shorter Map.Entry<String, String> kind
of block
that can be used here?
There is a getParameterMap function in ServletRequest which seems to return Map, but not Map<String, String[]> I dont know how to safely convert it to Map<String, String[]> Also, that block will probably be the same number of lines. http://codereview.appspot.com/1822041/show
