Refactoring ResponseRewriterRegistry to take into account container

Refactoring ResponseRewriterRegistry to take into account the container of the request being processed.

This change adds a ContextAwareRegistry which determines the list of rewriters to apply using the container of the http request being processed and the rewrite flow id specified.

Each handler (ProxyHandler , AccelHandler etc.) injects ResponseRewriterRegistry and specifies the rewrite flow id as shown below:

@ResponseRewriters(rewriteFlow="accelerate") ResponseRewriterRegistry
@ResponseRewriters(rewriteFlow="requestPipeline") ResponseRewriterRegistry
@ResponseRewriters(rewriteFlow="default") ResponseRewriterRegistry


ResponseRewriters for a given rewrite flow and container are provided as follows:

@Provides
@Singleton
@ResponseRewritersList(rewriteFlow="accelerate", container="default")
protected List<ResponseRewriter> provide() {
  return ImmutableList.of(absolutePathReferenceRewriter);
}
....

A RewritePath is defined as a tuple of (container, rewriteFlow).
A map [RewritePath -> List of rewriters] is then generated by injecting all the ResponseRewritersList's.

[Kuntal Loya, 2010-09-01 08:09:03]

Could we have the responseRewriterRegistry implementation have the flow
information as well?
Right now, I got totally confused with flow and context being talked about at
multiple places.

Lets discuss the design in person later.

http://codereview.appspot.com/2058042/diff/15020/9013
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java
(right):

http://codereview.appspot.com/2058042/diff/15020/9013#newcode36
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java:36:
protected final Map<String, List<ResponseRewriter>>
containerToResponseRewriters;
containerRewriters - does that sound any better?

http://codereview.appspot.com/2058042/diff/15020/9013#newcode38
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java:38:
public ContextAwareRegistry(GadgetHtmlParser htmlParser,
There can be a constructor which takes just the htmlParser and there should be a
method to set the responseRewriters for a container and another method to add a
responseRewriter for a container.

http://codereview.appspot.com/2058042/diff/15020/9013#newcode56
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java:56:
if (rewriters != null) {
null check is not required.

http://codereview.appspot.com/2058042/diff/15020/9012
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/15020/9012#newcode46
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:46:
ResponseRewriterList reg = m.getAnnotation(ResponseRewriterList.class);
reg?

[gagan.goku, 2010-09-07 05:59:29]

http://codereview.appspot.com/2058042/diff/15020/9013
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java
(right):

http://codereview.appspot.com/2058042/diff/15020/9013#newcode36
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java:36:
protected final Map<String, List<ResponseRewriter>>
containerToResponseRewriters;
On 2010/09/01 08:09:04, Kuntal Loya wrote:
> containerRewriters - does that sound any better?

changed the signature. please take another look.

http://codereview.appspot.com/2058042/diff/15020/9013#newcode38
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java:38:
public ContextAwareRegistry(GadgetHtmlParser htmlParser,
On 2010/09/01 08:09:04, Kuntal Loya wrote:
> There can be a constructor which takes just the htmlParser and there should be
a
> method to set the responseRewriters for a container and another method to add
a
> responseRewriter for a container.

Refactored to take all flow id and container rewriters. Please take another
look.

http://codereview.appspot.com/2058042/diff/15020/9013#newcode56
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java:56:
if (rewriters != null) {
On 2010/09/01 08:09:04, Kuntal Loya wrote:
> null check is not required.

Done.

http://codereview.appspot.com/2058042/diff/15020/9012
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/15020/9012#newcode46
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:46:
ResponseRewriterList reg = m.getAnnotation(ResponseRewriterList.class);
On 2010/09/01 08:09:04, Kuntal Loya wrote:
> reg?

renamed to rewriterList

[zhoresh, 2010-09-15 21:25:13]

http://codereview.appspot.com/2058042/diff/53001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java
(right):

http://codereview.appspot.com/2058042/diff/53001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java:40:
protected final Map<Pair<String, String>, Provider<List<ResponseRewriter>>>
responseRewriters;
I think Map<String, Map<String, Providwer<>>> make more sense to me. Basically a
map from container to map from flow to provider.
Then the selection would be first to check if container have specific config and
if not use the default container, then check for the specific flow, and again if
flow does not have specific provider use the default flow.

[gagan.goku, 2010-10-03 20:40:55]

http://codereview.appspot.com/2058042/diff/53001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java
(right):

http://codereview.appspot.com/2058042/diff/53001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContextAwareRegistry.java:40:
protected final Map<Pair<String, String>, Provider<List<ResponseRewriter>>>
responseRewriters;
On 2010/09/15 21:25:13, zhoresh wrote:
> I think Map<String, Map<String, Providwer<>>> make more sense to me. Basically
a
> map from container to map from flow to provider.
> Then the selection would be first to check if container have specific config
and
> if not use the default container, then check for the specific flow, and again
if
> flow does not have specific provider use the default flow.

Hi Ziv.
Changed the signature of map to take container as first key and rewrite flow as
2nd.
Fallback behavior of default container has been added in this cl. I don't think
wel need the default rewrite flow fallback anytime soon so im not adding it in
this cl as it adds another layer of complexity.

[anupama.dutta, 2010-10-12 04:37:40]

http://codereview.appspot.com/2058042/diff/72001/java/common/src/main/java/or...
File java/common/src/main/java/org/apache/shindig/common/Pair.java (right):

http://codereview.appspot.com/2058042/diff/72001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/common/Pair.java:36: @Override
Probably can be moved to a separate patch, if we do not use Pair in this patch?

http://codereview.appspot.com/2058042/diff/72001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/72001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:47:
* Go over all provider methods which provide {@code List<ResponseRewriter>}
Move this to just before ResponseRewriterRegistryProvider class definition for
better readability.

[gagan.goku, 2010-10-12 08:28:28]

http://codereview.appspot.com/2058042/diff/72001/java/common/src/main/java/or...
File java/common/src/main/java/org/apache/shindig/common/Pair.java (right):

http://codereview.appspot.com/2058042/diff/72001/java/common/src/main/java/or...
java/common/src/main/java/org/apache/shindig/common/Pair.java:36: @Override
On 2010/10/12 04:37:41, anupama.dutta wrote:
> Probably can be moved to a separate patch, if we do not use Pair in this
patch?

reverted from this change.

http://codereview.appspot.com/2058042/diff/72001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/72001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:47:
* Go over all provider methods which provide {@code List<ResponseRewriter>}
On 2010/10/12 04:37:41, anupama.dutta wrote:
> Move this to just before ResponseRewriterRegistryProvider class definition for
> better readability.

Done.

[johnfargo, 2010-10-15 00:32:45]

Pretty slick! But unfortunately it limits our utility, while the code is fairly
confusing as well. Looking over this in detail, it appears you're no longer
using Guice's @Provides annotations here at all. Because the binder method
iterates over methods in RewriteModule, one can never add additional
ResponseRewriterRegistries.

We have a few requirements/goals here:
1. Don't create all sorts of redundant ResponseRewriterRegistry classes just to
bind a new list of ResponseRewriters.
2. Bind the list of rewriters to use by some kind of meaningful name.
3. Allow rewriter lists to apply per-container.

With a solution to #1, we have a solution to #2, so we can consolidate those.

Especially if we can get rid of #3, it sounds like we might be able to get away
w/ using a simple Guice MapBinder.

@see
http://google-guice.googlecode.com/svn/trunk/latest-javadoc/com/google/inject...

This would work by:
A) replacing ResponseRewriterRegistry.rewrite(HttpReq, HttpRespBuilder) method
with:
  rewrite(FlowId, HttpReq, HttpResp)

B) Let FlowId be either 1) an enum of FlowIDs, or 2) [more complex, not
convinced we actually need #3] a pair of Container and FlowID enum

C) Change callers to pass in a FlowID (w/ or w/o container is fine) to get
rewriting behavior. Example would be in DefaultRequestPipeline, which would
call:
registry.rewrite(FlowID.DEFAULT, request, response);

Bindings then look like:
MapBinder<FlowID, List<ResponseRewriter>> mapbinder =
  MapBinder.newMapBinder(binder(), FlowID.class, TypeLiteral...>();
mapbinder.addBinding(FlowID.DEFAULT).toInstance(getDefaultList());
mapbinder.addBinding(FlowID.ACCEL).toInstance(getAccelList());

...and so on.

WDYT?

<code comments below relate to the existing impl>

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:51:
@RewriterRegistry(rewriteFlow = "requestPipeline")
The flow IDs should be consolidated into a single class, perhaps an enum.

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResponseRewriterList.java
(right):

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResponseRewriterList.java:20:
String rewriteFlow();
make this an enum, with default = DEFAULT

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:24:
import com.google.inject.*;
explicitly list imports.

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:27:
import org.apache.shindig.gadgets.render.*;
same

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:65:
// TODO: Remove unnecessary code.
I'm pretty sure this is a universal TODO for all code at all times :) More
specific pls?

http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:99:
@Provides @Singleton @ResponseRewriterList(rewriteFlow = "accelerate", container
= "accel")
feels like "accel" should be a constant somewhere as well (public static final
String)

[gagan.goku, 2010-10-15 19:32:08]

Hi John

Thanks for spending time on this cl. Responses inline:

On Fri, Oct 15, 2010 at 6:02 AM, <johnfargo@gmail.com> wrote:

> Pretty slick! But unfortunately it limits our utility, while the code is
> fairly confusing as well. Looking over this in detail, it appears you're
> no longer using Guice's @Provides annotations here at all. Because the
> binder method iterates over methods in RewriteModule, one can never add
> additional ResponseRewriterRegistries.
>

@Provides is still getting used. The only difference is that instead of
providing a @Named annotation, it now provides a @ResponseRewriterList. Both
are equivalent in Guice in the sense that injector.getProvider(key) would
yield the same result [where key = respective instance of annotation].

The ResponseRewriterRegistryProvider iterates through these providers to
populate the list of rewriters per container and rewrite-flow. This is for
convenience so you don’t have to fill them into the map manually.

Also, if you want to provide your own ResponseRewriterRegistry for say
ServletX, you just have to say:
public ServletX(@Named(“blah”) ResponseRewriterRegistry registry);

and provide it in RewriteModule the same way we used to:
@Provides @Singleton @Named(“blah”)
List<ResponseRewriter> provideBlahRewriters(Rewriter r1, ...) {
}

Alternately, you could use the new annotations to say:
public ServletX(@ResponseRegistry(flow=”X”) ResponseRewriterRegistry
registry);

and bind it in RewriteModule as:
bind(ResponseRewriterRegistry.class)
    .annotatedWith(new ResponseRegistryImpl(“X”))
    .to(<your instance>);

or as:
@Provides @Singleton @ResponseRewriterRegistry(flow=”X”)
List<ResponseRewriter> provideBlahRewriters(Rewriter1 r1) {
}


What existing functionality are we losing with this change?


>
> We have a few requirements/goals here:
> 1. Don't create all sorts of redundant ResponseRewriterRegistry classes
> just to bind a new list of ResponseRewriters.
> 2. Bind the list of rewriters to use by some kind of meaningful name.
> 3. Allow rewriter lists to apply per-container.
>
> With a solution to #1, we have a solution to #2, so we can consolidate
> those.
>
> Especially if we can get rid of #3, it sounds like we might be able to
> get away w/ using a simple Guice MapBinder.
>

#3 is the real reason why we needed this change - essentially, for accel, we
want to define a different rewriter list for some of the flows.
For instance, for both proxy and concat flows in accel container, we will
keep only CssResponseRewriter. We want to keep the minimal set of rewriters
that work correctly for our use-case.
Even for the pre-cache rewriter list, we think we need different set of
rewriters for accel container. We have some ideas on how to implement
rewritten response caching, but we are still figuring this out.
So, per-container rewriter list is really a must for us.


> @see
>
>
http://google-guice.googlecode.com/svn/trunk/latest-javadoc/com/google/inject...
>
> This would work by:
> A) replacing ResponseRewriterRegistry. rewrite(HttpReq, HttpRespBuilder)
> method with:
>  rewrite(FlowId, HttpReq, HttpResp)
>
>
ResponseRewriterRegistry is an interface. Your suggestion requires we change
the interface and all its existing implementations (which would touch a lot
of code). Since we have a solution which does not change the interface, we
feel better using it.


> B) Let FlowId be either 1) an enum of FlowIDs, or 2) [more complex, not
> convinced we actually need #3] a pair of Container and FlowID enum
>
>
Good suggestion, we will implement (1) : i.e. enum of flowIDs. This would
make the change more readable and less error prone to typos.
(2): i.e. pair of container and rewrite-flow, was something we discussed
with Ziv and Ziv suggested that we make the registry hold a map from
container -> [map from rewrite-flow -> rewriter-list-provider]. The
reasoning is that container is logically the first key (and we should
fall-back to default container in absense of required container) and then
the rewrite-flow.

Given that we absolutely need per-container rewriters, we decided to go with
Ziv’s suggestion as it made sense.

C) Change callers to pass in a FlowID (w/ or w/o container is fine) to
> get rewriting behavior. Example would be in DefaultRequestPipeline,
> which would call:
> registry.rewrite(FlowID. DEFAULT, request, response);
>

This can be done away with by injecting the right ContextAwareRegistry for
each rewrite-flow. Basically, you pass the rewrite-flow as a constructor
argument and use it later when rewriting.


>
> Bindings then look like:
> MapBinder<FlowID, List<ResponseRewriter>> mapbinder =
>  MapBinder.newMapBinder( binder(), FlowID.class, TypeLiteral...>();
> mapbinder.addBinding(FlowID.DEFAULT).toInstance(getDefaultList());
> mapbinder.addBinding(FlowID.ACCEL).toInstance(getAccelList());
>
> ...and so on.
>

WDYT?
>
>
There is no problem in this approach, but the downside is that you have
repetition:
1) Provide rewriter lists for each {Rewrite-flow x Container} configuration
like the way we do now using @Provides methods.
2) Enumerate all the {Rewrite-flow x Container} configurations separately in
the mapbinder.

By creating the @ResponseRegistry named annotation, you can automate the 2nd
step, and the first step become clearer.

Thanks
Gagan

 <code comments below relate to the existing impl>
>
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
> (right):
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java:51:
> @RewriterRegistry(rewriteFlow = "requestPipeline")
> The flow IDs should be consolidated into a single class, perhaps an
> enum.
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResponseRewriterList.java
> (right):
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResponseRewriterList.java:20:
> String rewriteFlow();
> make this an enum, with default = DEFAULT
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
>
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
> (right):
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:24:
> import com.google.inject.*;
> explicitly list imports.
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:27:
> import org.apache.shindig.gadgets.render.*;
> same
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:65:
> // TODO: Remove unnecessary code.
> I'm pretty sure this is a universal TODO for all code at all times :)
> More specific pls?
>
>
>
http://codereview.appspot.com/2058042/diff/92001/java/gadgets/src/main/java/o...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:99:
> @Provides @Singleton @ResponseRewriterList(rewriteFlow = "accelerate",
> container = "accel")
> feels like "accel" should be a constant somewhere as well (public static
> final String)
>
>
> http://codereview.appspot.com/2058042/
>

[gagan.goku, 2010-10-19 03:08:31]

Hi Fargo

Could you respond to my comments?

Thanks
Gagan

[johnfargo, 2010-10-27 23:37:24]

This seems more aligned w/ Guice's features, though again, it's a bit of a
hexagonal peg in a round hole :)

http://codereview.appspot.com/2058042/diff/116001/java/gadgets/src/main/java/...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/116001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:59:
MapBinder<String, Map<RewriteFlow, Provider<List<ResponseRewriter>>>> mapbinder
=
This is looking reasonable -- we could simplify it some more by creating a
compound key out of RewriteFlow + Container ID, ie something like:

public class FlowId {
  RewriteFlow rewriteFlow;
  String container;

  FlowID(RewriteFlow, String) { ... }

  @Override
  hashCode() and equals()
}

http://codereview.appspot.com/2058042/diff/116001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:80:
mapbinder.addBinding(AccelUriManager.CONTAINER).toInstance(flowToRewriterMap);
if you create a helper methods:
* getKey(container, flow) [the Key.get(...) piece], and
* getProviderForKey(key) [the getProvider method below), the bindings then
become:

mapbinder.addBinding(getKey(container, flow)).toProvider(getKey(container,
flow));
(which you could also distill into a helper method)

WDYT?

[gagan.goku, 2010-10-29 10:07:09]

http://codereview.appspot.com/2058042/diff/116001/java/gadgets/src/main/java/...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/116001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:59:
MapBinder<String, Map<RewriteFlow, Provider<List<ResponseRewriter>>>> mapbinder
=
On 2010/10/27 23:37:24, johnfargo wrote:
> This is looking reasonable -- we could simplify it some more by creating a
> compound key out of RewriteFlow + Container ID, ie something like:
> 
> public class FlowId {
>   RewriteFlow rewriteFlow;
>   String container;
> 
>   FlowID(RewriteFlow, String) { ... }
> 
>   @Override
>   hashCode() and equals()
> }

Done.

http://codereview.appspot.com/2058042/diff/116001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:80:
mapbinder.addBinding(AccelUriManager.CONTAINER).toInstance(flowToRewriterMap);
On 2010/10/27 23:37:24, johnfargo wrote:
> if you create a helper methods:
> * getKey(container, flow) [the Key.get(...) piece], and
> * getProviderForKey(key) [the getProvider method below), the bindings then
> become:
> 
> mapbinder.addBinding(getKey(container, flow)).toProvider(getKey(container,
> flow));
> (which you could also distill into a helper method)
> 
> WDYT?

Made a new class called RewritePath and encapsulated container and flow into it.
Please take a look.

[johnfargo, 2010-11-17 02:01:26]

Looks pretty good to me at this point. The multibinding stuff is a little
confusing, but more inherently than in spite of itself :) Thanks for all the
work. Just a few small suggestions.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:84:
Provider<List<ResponseRewriter>> list) {
I'd make this protected.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:92:
new ResponseRewriterListImpl(container, rewriteFlow))));
suggest breaking the Key.get(...) piece into a helper method.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:165:
@ResponseRewriterList(container = ContainerConfig.DEFAULT_CONTAINER,
no need for DEFAULT_CONTAINER since it's the default container value.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:175:
@ResponseRewriterList(container = ContainerConfig.DEFAULT_CONTAINER,
no need for DEFAULT_CONTAINER since it's the default container value.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewritePath.java
(right):

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewritePath.java:25:
public class RewritePath extends ResponseRewriterListImpl {
I'd suggest just getting rid of ResponseRewriterListImpl and putting the impl
here.

[gagan.goku, 2010-11-17 04:15:09]

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
(right):

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:84:
Provider<List<ResponseRewriter>> list) {
On 2010/11/17 02:01:26, johnfargo wrote:
> I'd make this protected.

Done.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:92:
new ResponseRewriterListImpl(container, rewriteFlow))));
On 2010/11/17 02:01:26, johnfargo wrote:
> suggest breaking the Key.get(...) piece into a helper method.

Done.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:165:
@ResponseRewriterList(container = ContainerConfig.DEFAULT_CONTAINER,
On 2010/11/17 02:01:26, johnfargo wrote:
> no need for DEFAULT_CONTAINER since it's the default container value.

Done.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:175:
@ResponseRewriterList(container = ContainerConfig.DEFAULT_CONTAINER,
On 2010/11/17 02:01:26, johnfargo wrote:
> no need for DEFAULT_CONTAINER since it's the default container value.

Done.

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewritePath.java
(right):

http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewritePath.java:25:
public class RewritePath extends ResponseRewriterListImpl {
On 2010/11/17 02:01:26, johnfargo wrote:
> I'd suggest just getting rid of ResponseRewriterListImpl and putting the impl
> here.

Done.

[johnfargo, 2010-11-17 07:00:07]

LGTM

On 2010/11/17 04:15:09, gagan.goku wrote:
>
http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java
> (right):
> 
>
http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:84:
> Provider<List<ResponseRewriter>> list) {
> On 2010/11/17 02:01:26, johnfargo wrote:
> > I'd make this protected.
> 
> Done.
> 
>
http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:92:
> new ResponseRewriterListImpl(container, rewriteFlow))));
> On 2010/11/17 02:01:26, johnfargo wrote:
> > suggest breaking the Key.get(...) piece into a helper method.
> 
> Done.
> 
>
http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:165:
> @ResponseRewriterList(container = ContainerConfig.DEFAULT_CONTAINER,
> On 2010/11/17 02:01:26, johnfargo wrote:
> > no need for DEFAULT_CONTAINER since it's the default container value.
> 
> Done.
> 
>
http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewriteModule.java:175:
> @ResponseRewriterList(container = ContainerConfig.DEFAULT_CONTAINER,
> On 2010/11/17 02:01:26, johnfargo wrote:
> > no need for DEFAULT_CONTAINER since it's the default container value.
> 
> Done.
> 
>
http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
> File
> java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewritePath.java
> (right):
> 
>
http://codereview.appspot.com/2058042/diff/126001/java/gadgets/src/main/java/...
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/RewritePath.java:25:
> public class RewritePath extends ResponseRewriterListImpl {
> On 2010/11/17 02:01:26, johnfargo wrote:
> > I'd suggest just getting rid of ResponseRewriterListImpl and putting the
impl
> > here.
> 
> Done.

[gagan.goku, 2010-11-17 07:38:09]

svn uped.
Build looks good.
Committed as r1035940.

Thanks for the reviews everyone.