http://codereview.appspot.com/1032045/diff/1/2 File java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java (right):
http://codereview.appspot.com/1032045/diff/1/2#newcode123 java/common/src/main/java/org/apache/shindig/common/servlet/HttpUtil.java:123: throw new IllegalArgumentException("Wrong format for parameter 'callback' specified. Must match: " + nit: > 100char http://codereview.appspot.com/1032045/diff/1/4 File java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java (right): http://codereview.appspot.com/1032045/diff/1/4#newcode89 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:89: if (HttpUtil.isJSONP(servletRequest)) { the body of this if-statement should be a helper ie. dispatchJsonp http://codereview.appspot.com/1032045/diff/1/4#newcode95 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:95: if ((content.indexOf('[') != -1) && content.indexOf('[') < content.indexOf('{')) { duplicated elsewhere in the code: should be broken out into at least an isBatch method http://codereview.appspot.com/1032045/diff/1/4#newcode108 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:108: } this whole if/else block is identical to that found in doPost, but with a JSONP wrapper. let's just wrap ServletResponse in a helper class that either does a Jsonp wrap or not -- or break this out into a separate method that takes boolean isJsonp. http://codereview.appspot.com/1032045/diff/1/4#newcode115 java/common/src/main/java/org/apache/shindig/protocol/JsonRpcServlet.java:115: throw new IllegalArgumentException("could not find data to process"); IMO would be preferable not to throw a RuntimeException http://codereview.appspot.com/1032045/show
