http://codereview.appspot.com/7259/diff/1/3 File java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/RpcServlet.java (right):
http://codereview.appspot.com/7259/diff/1/3#newcode46 Line 46: static final String GET_REQUEST_CALLBACK_PATTERN = "[A-Za-z\\.]+"; Underscores need to be supported as well. http://codereview.appspot.com/7259/diff/1/3#newcode133 Line 133: values[0], getRequestCharacterEncoding(request)); This isn't necessary. getParameter will automatically decode a url encoded parameter value. http://codereview.appspot.com/7259/diff/1/3#newcode176 Line 176: private class ParameterException extends Exception { This checked exception isn't adding much value here. As John says, an IllegalArgumentException is more fitting for the inputs. http://codereview.appspot.com/7259/diff/1/3#newcode182 Line 182: private class Result { This should be a static class. http://codereview.appspot.com/7259/diff/1/2 File java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/RpcServletTest.java (right): http://codereview.appspot.com/7259/diff/1/2#newcode35 Line 35: public class RpcServletTest extends ServletTestFixture { Please don't write any new code depending on ServletTestFixture. It's deprecated. New tests should use junit4 conventions. See other tests for examples. http://codereview.appspot.com/7259/diff/1/2#newcode39 Line 39: private class TestJsonRpcHandler extends JsonRpcHandler { inner classes should come after test methods. http://codereview.appspot.com/7259/diff/1/2#newcode47 Line 47: super(executor, processor, urlGenerator); None of these are used. Passing nulls to the parent ctor or simply mocking a JsonRpcHandler is strongly preferred here. http://codereview.appspot.com/7259/diff/1/2#newcode151 Line 151: String[] reqParamValues, You don't need to bother testing multiple parameters being passed. The handling of getParameter is well defined as part of the HTTP standard. http://codereview.appspot.com/7259
