Cleaning up HtmlAccelServlet

Removing old way of registering GadgetRewriter for HtmlAccelServlet and some boilerplate code used for it since now accel servlet uses ResponseRewriterRegistry.

[gagan.goku, 2010-07-12 07:00:51]

fixing bad things

[vikaas.arora, 2010-07-14 04:51:10]

Add some comment to the issue description defining the scope of the change.

http://codereview.appspot.com/1798042/diff/7001/7
File main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java (left):

http://codereview.appspot.com/1798042/diff/7001/7#oldcode72
main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java:72: }
Seems you are now removing special code required for handling of AccelServlet.
Can you add few lines pertaining to the scope of the change to the CL/Issue
description.

http://codereview.appspot.com/1798042/diff/7001/8
File main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java
(left):

http://codereview.appspot.com/1798042/diff/7001/8#oldcode52
main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java:52: }
Now there will only be one set of rewriters defined for Gadgets. No special set
for Accel Servlet. Where (in GGS ?) do you plan to tailor this list of rewriters
for AccelServlet ?

http://codereview.appspot.com/1798042/diff/7001/9
File main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java (right):

http://codereview.appspot.com/1798042/diff/7001/9#newcode44
main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:44: public
void setAccelHandler(AccelHandler accelHandler) {
The public method 'setHandler' name was apt. This Class (HtmlAccelServlet)
already signifies that the handler is Accel specific.

[gagan.goku, 2010-07-14 15:51:01]

addressing comments

[gagan.goku, 2010-07-14 15:52:00]

http://codereview.appspot.com/1798042/diff/7001/7
File main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java (left):

http://codereview.appspot.com/1798042/diff/7001/7#oldcode72
main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java:72: }
On 2010/07/14 04:51:10, vikaas.arora wrote:
> Seems you are now removing special code required for handling of AccelServlet.
> Can you add few lines pertaining to the scope of the change to the CL/Issue
> description.

Added. This code is not required now because accel has moved to using
ResponseRewriterRegistry now.

http://codereview.appspot.com/1798042/diff/7001/8
File main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java
(left):

http://codereview.appspot.com/1798042/diff/7001/8#oldcode52
main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java:52: }
On 2010/07/14 04:51:10, vikaas.arora wrote:
> Now there will only be one set of rewriters defined for Gadgets. No special
set
> for Accel Servlet. Where (in GGS ?) do you plan to tailor this list of
rewriters
> for AccelServlet ?

Take a look at provideAccelResponseRewriters.

http://codereview.appspot.com/1798042/diff/7001/9
File main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java (right):

http://codereview.appspot.com/1798042/diff/7001/9#newcode44
main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:44: public
void setAccelHandler(AccelHandler accelHandler) {
On 2010/07/14 04:51:10, vikaas.arora wrote:
> The public method 'setHandler' name was apt. This Class (HtmlAccelServlet)
> already signifies that the handler is Accel specific.

Done.

[johnfargo, 2010-07-14 20:18:34]

LGTM, nice cleanup. Committing.

On 2010/07/14 15:52:00, gagan.goku wrote:
> http://codereview.appspot.com/1798042/diff/7001/7
> File main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java
(left):
> 
> http://codereview.appspot.com/1798042/diff/7001/7#oldcode72
> main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java:72: }
> On 2010/07/14 04:51:10, vikaas.arora wrote:
> > Seems you are now removing special code required for handling of
AccelServlet.
> > Can you add few lines pertaining to the scope of the change to the CL/Issue
> > description.
> 
> Added. This code is not required now because accel has moved to using
> ResponseRewriterRegistry now.
> 
> http://codereview.appspot.com/1798042/diff/7001/8
> File main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java
> (left):
> 
> http://codereview.appspot.com/1798042/diff/7001/8#oldcode52
> main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java:52: }
> On 2010/07/14 04:51:10, vikaas.arora wrote:
> > Now there will only be one set of rewriters defined for Gadgets. No special
> set
> > for Accel Servlet. Where (in GGS ?) do you plan to tailor this list of
> rewriters
> > for AccelServlet ?
> 
> Take a look at provideAccelResponseRewriters.
> 
> http://codereview.appspot.com/1798042/diff/7001/9
> File main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):
> 
> http://codereview.appspot.com/1798042/diff/7001/9#newcode44
> main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:44: public
> void setAccelHandler(AccelHandler accelHandler) {
> On 2010/07/14 04:51:10, vikaas.arora wrote:
> > The public method 'setHandler' name was apt. This Class (HtmlAccelServlet)
> > already signifies that the handler is Accel specific.
> 
> Done.

[gagan.goku, 2010-07-14 20:20:17]

Thanks
Gagan

-- 
The only thing missing in life is background music.
-- Gagandeep Singh


On Thu, Jul 15, 2010 at 1:48 AM, <johnfargo@gmail.com> wrote:

> LGTM, nice cleanup. Committing.
>
>
> On 2010/07/14 15:52:00, gagan.goku wrote:
>
>> http://codereview.appspot.com/1798042/diff/7001/7
>> File
>>
> main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java
> (left):
>
>  http://codereview.appspot.com/1798042/diff/7001/7#oldcode72
>> main/java/org/apache/shindig/gadgets/DefaultGadgetSpecFactory.java:72:
>>
> }
>
>> On 2010/07/14 04:51:10, vikaas.arora wrote:
>> > Seems you are now removing special code required for handling of
>>
> AccelServlet.
>
>> > Can you add few lines pertaining to the scope of the change to the
>>
> CL/Issue
>
>> > description.
>>
>
>  Added. This code is not required now because accel has moved to using
>> ResponseRewriterRegistry now.
>>
>
>  http://codereview.appspot.com/1798042/diff/7001/8
>> File
>>
> main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java
>
>> (left):
>>
>
>  http://codereview.appspot.com/1798042/diff/7001/8#oldcode52
>>
>
>
> main/java/org/apache/shindig/gadgets/render/GadgetRewritersProvider.java:52:
> }
>
>> On 2010/07/14 04:51:10, vikaas.arora wrote:
>> > Now there will only be one set of rewriters defined for Gadgets. No
>>
> special
>
>> set
>> > for Accel Servlet. Where (in GGS ?) do you plan to tailor this list
>>
> of
>
>> rewriters
>> > for AccelServlet ?
>>
>
>  Take a look at provideAccelResponseRewriters.
>>
>
>  http://codereview.appspot.com/1798042/diff/7001/9
>> File
>>
> main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
> (right):
>
>  http://codereview.appspot.com/1798042/diff/7001/9#newcode44
>> main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:44:
>>
> public
>
>> void setAccelHandler(AccelHandler accelHandler) {
>> On 2010/07/14 04:51:10, vikaas.arora wrote:
>> > The public method 'setHandler' name was apt. This Class
>>
> (HtmlAccelServlet)
>
>> > already signifies that the handler is Accel specific.
>>
>
>  Done.
>>
>
>
>
> http://codereview.appspot.com/1798042/show
>