On Mon, May 3, 2010 at 6:45 PM, <[email protected]> wrote: > fixed nits, will commit if no objections. >
No objections per se, but commentary. > > > > http://codereview.appspot.com/1032045/diff/6001/7001 > File > java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java > (right): > > http://codereview.appspot.com/1032045/diff/6001/7001#newcode130 > > java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java:130: > return true; > On 2010/05/03 23:04:54, johnfargo wrote: > >> why not just return the matched callback param to encapsulate logic? >> > > This logic has three states - callback valid, callback invalid, no > callback, so just returning the param doesn't cut it. An IAE seems > appropriate since malformed callbacks should be relatively rare and > should result in a 400 response. > I assumed callback invalid -> exception, so valid vs. no callback is null vs. String. > > http://codereview.appspot.com/1032045/diff/6001/7003 > File > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java > (right): > > http://codereview.appspot.com/1032045/diff/6001/7003#newcode86 > > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:86: > if (!("GET".equals(method) || "POST".equals(method))) { > On 2010/05/03 23:04:54, johnfargo wrote: > >> can't this just be enforced by direct use? >> > > not sure what you mean here. I combined doGet and doPost here to > eliminate duplicate code paths. In doing so we need to reject methods > we can't handle. > I was advocating for getting rid of the check. If you only call this from doGet and doPost, you're already implicitly checking method. > > http://codereview.appspot.com/1032045/diff/6001/7003#newcode87 > > java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:87: > sendError(servletResponse, new > ResponseItem(HttpServletResponse.SC_BAD_REQUEST, "Only POST/GET")); > On 2010/05/03 23:04:54, johnfargo wrote: > >> still >100 char >> > > Done. > > http://codereview.appspot.com/1032045/diff/6001/7004 > File > > java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java > (right): > > http://codereview.appspot.com/1032045/diff/6001/7004#newcode132 > > java/common/src/test/java/org/apache/shindig/common/util/JsonConversionUtilTest.java:132: > } > On 2010/05/03 23:04:54, johnfargo wrote: > >> nit: 1-space off indent >> > > Done. > > http://codereview.appspot.com/1032045/diff/6001/7005 > File > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java > (right): > > http://codereview.appspot.com/1032045/diff/6001/7005#newcode65 > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java:65: > HttpUtil.isJSONP(request); > On 2010/05/03 23:04:54, johnfargo wrote: > >> is this just for the side-effect of param validation? >> > Yes. Comment added to describe this > > http://codereview.appspot.com/1032045/show >
