Your choice; I'll LGTM this either way. On Mon, May 3, 2010 at 6:52 PM, Paul Lindner <[email protected]> wrote:
> 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 >> > >> > >
