fixed nits, will commit if no objections.
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. 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. 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
