http://codereview.appspot.com/1770043/diff/13001/14001 File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (left):
http://codereview.appspot.com/1770043/diff/13001/14001#oldcode211 main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:211: throws IOException { On 2010/07/14 06:10:25, vikaas.arora wrote:
This method (and it's constituent method calls) wasn't throwing
IOException.
Good to have this cleaned up.
Thanks http://codereview.appspot.com/1770043/diff/13001/14001 File main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java (right): http://codereview.appspot.com/1770043/diff/13001/14001#newcode86 main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:86: IOUtils.copy(results.getResponse(), response.getOutputStream()); On 2010/07/14 06:10:25, vikaas.arora wrote:
You can add another small function (10 odd lines) for block #75-#86
and name it
CreateResponse()
Done. http://codereview.appspot.com/1770043/diff/13001/14001#newcode90 main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:90: protected HttpRequest buildHttpRequest(HttpServletRequest request) On 2010/07/14 06:10:25, vikaas.arora wrote:
Move this overloaded method 'buildHttpRequest' near the other method
(that takes
two args).
Done. http://codereview.appspot.com/1770043/diff/13001/14001#newcode108 main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:108: // throw appropriate gadget exception. On 2010/07/14 06:10:25, vikaas.arora wrote:
Are we throwing GadgetException ? If not, revisit the comment.
Done. http://codereview.appspot.com/1770043/diff/13001/14003 File main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java (right): http://codereview.appspot.com/1770043/diff/13001/14003#newcode100 main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java:100: response.getWriter().write(UNPARSEABLE_CRUFT + output); On 2010/07/14 06:10:25, vikaas.arora wrote:
Block #91-#100 -> CreateResponse()
I am currently adding a todo for the same as moving that out will require a lot of refactoring as it uses the httpservletrequest. Will fix later if ppl like the idea. http://codereview.appspot.com/1770043/diff/13001/14004 File main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java (right): http://codereview.appspot.com/1770043/diff/13001/14004#newcode203 main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java:203: protected HttpRequest buildHttpRequest(HttpServletRequest request) On 2010/07/14 06:10:25, vikaas.arora wrote:
Don't you want to make this method abstract. If for a particular case, this method implementation is null, let that
specific
ProxyHandler define this will null body.
Otherwise, just don't declare this method in this abstract class
(interface) (if
you feel that this is not generic enough. Also in that case, make this
method
private in the other ProxyHandlers).
made abstract. http://codereview.appspot.com/1770043/diff/13001/14005 File main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (left): http://codereview.appspot.com/1770043/diff/13001/14005#oldcode89 main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:89: } On 2010/07/14 06:22:12, vikaas.arora wrote:
Shouldn't you retain this check as the first statement in the
doFetch(). You
have the similar check in 'processRequest'
I can make it the first statement, but then it doesn't fit in well with the current refactoring. So i though it'd be better to move it to processRequest. http://codereview.appspot.com/1770043/diff/13001/14005 File main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java (right): http://codereview.appspot.com/1770043/diff/13001/14005#newcode105 main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:105: } On 2010/07/14 06:22:12, vikaas.arora wrote:
Embed this block in a separate method (prepareRequest())
Done. http://codereview.appspot.com/1770043/diff/13001/14005#newcode175 main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java:175: HttpRequest rcr = buildHttpRequest(request, proxyUri, proxyUri.getResource()); On 2010/07/14 06:10:25, vikaas.arora wrote:
What does 'rcr' signify/mean ?
no idea. I just kept the same variable name :) Changed to httpRequest. http://codereview.appspot.com/1770043/diff/13001/14008 File test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java (right): http://codereview.appspot.com/1770043/diff/13001/14008#newcode79 test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:79: // expect(request.getHeaders("Host")).andReturn(makeEnumeration(host)); On 2010/07/14 06:22:12, vikaas.arora wrote:
Remove this commented code from the test.
Done. http://codereview.appspot.com/1770043/show
