On Mon, May 3, 2010 at 6:47 PM, John Hjelmstad <[email protected]> wrote:
> 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. okay, that seems fine. > > > > > 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. > > I suppose I could rename service() to something like dispatch() and have doGet/doPost call that. Seems a little cluttered imho, but I can make the change. > > > > > 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 > > >
