CSS rewriter doesnt take container into account when rewriting embedded URL's

I ran into a container configuration related issue while testing out RC2, which is that the CSS rewriter doesn't seem to be taking the container into account when rewriting embedded URL's -- it always uses the default container configuration which ends up giving me invalid proxy URL's in my rewritten CSS. This appears to be due to the fact that the CssResponseRewriter uses the DomWalker.makeGadget method to create a Gadget instance to pass to the ProxyUriManager.ProxyUri constructor, but the GadgetContext inside that Gadget instance doesnt have the container set, which makes code that runs later in the ProxyUriManager to generate the proxy URL use the default container configuration instead of my custom container configuration.

I was able to come up with a patch to work around this issue, but I'm not sure if its the best way to go about it... There is a lot of code around the whole gadget rewriting process and I definitely don't have my head wrapped around all of it -- but I thought I'd take a crack at it anyway, if for nothing else to at least highlight where the issue is occurring. The two major changes I made were to add another parameter to the DomWalker.makeGadget method for the container, and passing the container down through the call stack in the CssResponseRewriter. For any other usages of the affected classes which broke -- if there was an obvious place to pull the container from (like a Gadget instance) I used it -- otherwise I just passed in null to trigger the default behavior (which is to return the default container). 

The JIRA ticket for this patch is:

https://issues.apache.org/jira/browse/SHINDIG-1411

[zhoresh, 2010-08-18 22:12:40]

http://codereview.appspot.com/2004042/diff/13001/14004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):

http://codereview.appspot.com/2004042/diff/13001/14004#newcode218
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:218:
public static Gadget makeGadget(final Uri context, final String container) {
Maybe we can remove this overload function and everywhere pass alone the
request? (the other makeGadget function)
This way if later we find out we need more paramaters from the requests, they
will be available.
(Assuming again we do not create fake request just for this)

Places we don't have proper request, will require a different method to create
Gadget then this.

[gagan.goku, 2010-08-19 06:06:20]

Thanks for making this change Jesse. Just some small comments / nit picks.
Its okay to fix the problem in multiple cl's as long as we keep each cl coherent
and well documented. Your cl description is already good enough :)

http://codereview.appspot.com/2004042/diff/13001/14004
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):

http://codereview.appspot.com/2004042/diff/13001/14004#newcode218
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:218:
public static Gadget makeGadget(final Uri context, final String container) {
On 2010/08/18 22:12:40, zhoresh wrote:
> Maybe we can remove this overload function and everywhere pass alone the
> request? (the other makeGadget function)
> This way if later we find out we need more paramaters from the requests, they
> will be available.
> (Assuming again we do not create fake request just for this)
> 
> Places we don't have proper request, will require a different method to create
> Gadget then this.
> 

I agree that its better to remove this function altogether. I tried to achieve
similar effect by using DomWalker.makeGadget(HttpRequest) in
CssResponseRewriter. However, the side effect was that that i needed to add the
HttpRequest to the GadgetContext (it was required in
StyleTagProxyEmbeddedUrlsVisitor).

Given that we only need information like container etc, it might be better to
pass along a GadgetContext object rather than just container. The makeGadget
method could look something like this:

makeGadget(Uri uri, GadgetContext context) {
    try {
      final GadgetSpec spec = new GadgetSpec(context,
        "<Module><ModulePrefs author=\"a\" title=\"t\"></ModulePrefs>" +
        "<Content></Content></Module>");
      return new Gadget() {
        @Override
        public GadgetSpec getSpec() {
          return spec;
        }
      }.setContext(new GadgetContext(context) {
        @Override
        public Uri getUrl() {
          return uri;
        }
      });
    } catch (Exception e) {
      throw new RuntimeException("Unexpected boilerplate parse failure");
    }
}

We could then refactor the other functions to pass along the gadget context
rather than just the container string.
In places where you have the http request, you could pass on
DomWalker.makeGadget(httpRequest).getContext().

Thoughts ?

http://codereview.appspot.com/2004042/diff/13001/14007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/2004042/diff/13001/14007#newcode138
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:138:
Gadget gadget = DomWalker.makeGadget(requestUri, null);
It would be nice if you could pass on the HttpRequest object instead of
requestUri. Then you could say

Gadget gadget = DomWalker.makeGadget(httpRequest)

and this would have the correct container information.

Or, just leave a todo for me to take care of it later :)

http://codereview.appspot.com/2004042/diff/13001/14008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/2004042/diff/13001/14008#newcode51
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:51:
Gadget gadget = DomWalker.makeGadget(requestUri, null);
I was thinking of changing the signature of this method to be something like:

public Uri parseAndNormalize(HttpRequest httpRequest) throws GadgetException;

In case you are not comfortable making adding this change in this cl, please
leave a todo for me. I'l take care of it in my next cl :)

[gagan.goku, 2010-08-19 06:07:59]

Btw, the other similar codereview trying to fix this problem is
http://codereview.appspot.com/1888046/. Also see Henry's comment on not
injecting HttpRequest into GadgetContext which i think is valid and clean.

[jcian, 2010-08-19 20:46:15]

Thanks for all the comments so far from everyone.

I think Gagan’s suggestion of passing along a GadgetContext sounds reasonable so
I went ahead and took a shot at seeing what that might look like.  I haven’t had
a chance to actually test the updated code out yet, but it does build and pass
all the tests.  If this looks like a potentially reasonable approach I can do
some testing with it and also address the other comments about trying to fix
other places where this same issue might be occurring (outside of just the
CssResponseRewriter which is where I started).

As a side note -- I don’t really like the constructor I had to add to the
GadgetContext class, but it seemed better than having a bunch of anonymous inner
classes spread all over the place which do the same thing just to get instances
of GadgetContext passed into the updated DomWalker.makeGadget method.  If anyone
has additional thoughts on a better way to go about that I'd be glad to hear
them.  I guess part of my problem is that I don’t really understand the
motivations around why the GadgetContext class is implemented the way it is --
it seems like it's designed to have instances created by either extending it or
creating anonymous classes that extend it and override different pieces.  Is
there some reason why it doesn’t have private final instance variables defined
with constructors that set them and accessors that return default values if the
instance variables are null?

I'll upload a new patch set in just a couple of minutes with my latest changes.

Thanks all!

[jcian, 2010-08-19 20:49:17]

Updates per comments

[johnfargo, 2010-08-21 17:21:21]

Hi Jesse:

Thanks for taking this on. The code involved here is representative of some
classes which started out as an appropriate idea, increasingly are burdensome
(continuous passing of "fake" Gadget/GadgetContext and container fields, in
particular).

Structurally speaking, we'll eventually want to either:
A) refactor a bit into more general context objects
B) introduce a request-scoped context object that has key configuration hanging
off of it. Within Google's installation of Shindig, we actually already do this.
I'd like to port the impl to Shindig. Basically anywhere you need it you:
@Inject Provider<RequestContext>
...from which various contextual data are made available.

But that's for another time! Apropos of this CL:
1. I'd prefer not to introduce the new GadgetContext ctor, to keep that class's
API surface smaller.
2. There are a few cases here where you could pass the "right" container as
well.

Thanks again,
John

http://codereview.appspot.com/2004042/diff/24002/29001
File java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29001#newcode41
java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java:41:
this.delegate = new GadgetContext() {
stylistically I'd prefer to minimize the number of constructors here -- this
functionality can be achieved just by constructing the new GadgetContext()
wherever it's used.

http://codereview.appspot.com/2004042/diff/24002/29003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/CajaResponseRewriter.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29003#newcode100
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/CajaResponseRewriter.java:100:
proxyFetcher(req, contextUri), proxyUriPolicy(contextUri));
HttpRequest has .getContainer() hanging off of it, which should be used to
obtain the container here.

http://codereview.appspot.com/2004042/diff/24002/29008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29008#newcode125
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:125:
public ProxyUriManager.ProxyUri getProxyUri(Uri requestUri) throws
GadgetException {
may as well add a method:
public ProxyUriManager.ProxyUri getProxyUri(Uri reqUri, String container)

...and pass request.getContainer() to the second argument when called in this
method.

[gagan.goku, 2010-08-22 16:19:11]

General thought:
Would it be better to pass on the GadgetContext object rather than String
container ? GadgetContext has more context [ duh :) ] and this might take us
closer to what John was suggesting.

http://codereview.appspot.com/2004042/diff/24002/29001
File java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29001#newcode41
java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java:41:
this.delegate = new GadgetContext() {
On 2010/08/21 17:21:21, johnfargo wrote:
> stylistically I'd prefer to minimize the number of constructors here -- this
> functionality can be achieved just by constructing the new GadgetContext()
> wherever it's used.

Agreed, this constructor might not be needed with small refactoring.

http://codereview.appspot.com/2004042/diff/24002/29002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29002#newcode191
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java:191:
new ProxyUriManager.ProxyUri(DomWalker.makeGadget(new GadgetContext(context,
null)), inboundUri));
Better to fix this case as well. Currently we are passing null for container
which is equivalent for default container. If possible, lets fix the issue by
passing along the correct GadgetContext / container.
Or, lets leave a TODO which clearly states the bug so that we fix it correctly
when we get to it :)

http://codereview.appspot.com/2004042/diff/24002/29003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/CajaResponseRewriter.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29003#newcode100
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/CajaResponseRewriter.java:100:
proxyFetcher(req, contextUri), proxyUriPolicy(contextUri));
On 2010/08/21 17:21:21, johnfargo wrote:
> HttpRequest has .getContainer() hanging off of it, which should be used to
> obtain the container here.

Here again its easy to pass on the HttpRequest which has the container
information.

http://codereview.appspot.com/2004042/diff/24002/29005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29005#newcode218
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/DomWalker.java:218:
public static Gadget makeGadget(GadgetContext context) {
Good job getting rid of this misleading constructor.

http://codereview.appspot.com/2004042/diff/24002/29008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29008#newcode125
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:125:
public ProxyUriManager.ProxyUri getProxyUri(Uri requestUri) throws
GadgetException {
On 2010/08/21 17:21:21, johnfargo wrote:
> may as well add a method:
> public ProxyUriManager.ProxyUri getProxyUri(Uri reqUri, String container)
> 
> ...and pass request.getContainer() to the second argument when called in this
> method.

Or better, pass in the original HttpRequest and do makeGadget(httpRequest)

http://codereview.appspot.com/2004042/diff/24002/29008#newcode139
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:139:
Gadget gadget = DomWalker.makeGadget(new GadgetContext(requestUri, null));
Lets leave a TODO here to pass on the container information correctly, rather
than passing null container.

The problem is that getProxyUri() only takes a Uri. If it takes the HttpRequest,
we will have all the information we need.
Take a look at http://codereview.appspot.com/1949048/diff/7001/2004.

http://codereview.appspot.com/2004042/diff/24002/29009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29009#newcode52
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:52:
Gadget gadget = DomWalker.makeGadget(new GadgetContext(requestUri, null));
Lets leave a TODO to pass on the correct container here as well.

Take a look at http://codereview.appspot.com/1949048/diff/7001/2006 for another
way of fixing this issue.

http://codereview.appspot.com/2004042/diff/24002/29010
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29010#newcode232
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java:232:
}
We should really add a test here to ensure that non "default" container does
percolate and shows in the proxied uri.

[jcian, 2010-08-24 12:34:20]

I've uploaded a new patch set that should address all comments -- please let me
know if I've missed anything or if there is anything else needed.

http://codereview.appspot.com/2004042/diff/24002/29001
File java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29001#newcode41
java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetContext.java:41:
this.delegate = new GadgetContext() {
On 2010/08/22 16:19:12, gagan.goku wrote:
> On 2010/08/21 17:21:21, johnfargo wrote:
> > stylistically I'd prefer to minimize the number of constructors here -- this
> > functionality can be achieved just by constructing the new GadgetContext()
> > wherever it's used.
> 
> Agreed, this constructor might not be needed with small refactoring.

Done.

http://codereview.appspot.com/2004042/diff/24002/29002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29002#newcode191
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java:191:
new ProxyUriManager.ProxyUri(DomWalker.makeGadget(new GadgetContext(context,
null)), inboundUri));
On 2010/08/22 16:19:12, gagan.goku wrote:
> Better to fix this case as well. Currently we are passing null for container
> which is equivalent for default container. If possible, lets fix the issue by
> passing along the correct GadgetContext / container.
> Or, lets leave a TODO which clearly states the bug so that we fix it correctly
> when we get to it :)

I took a look at the methods that call this method (and the methods that call
those) and didn't see anything which would give us access to the container in
all cases -- so I went ahead and added a TODO to fix it in a later CL.

http://codereview.appspot.com/2004042/diff/24002/29003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/CajaResponseRewriter.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29003#newcode100
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/CajaResponseRewriter.java:100:
proxyFetcher(req, contextUri), proxyUriPolicy(contextUri));
On 2010/08/22 16:19:12, gagan.goku wrote:
> On 2010/08/21 17:21:21, johnfargo wrote:
> > HttpRequest has .getContainer() hanging off of it, which should be used to
> > obtain the container here.
> 
> Here again its easy to pass on the HttpRequest which has the container
> information.

Done.

http://codereview.appspot.com/2004042/diff/24002/29008
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29008#newcode125
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:125:
public ProxyUriManager.ProxyUri getProxyUri(Uri requestUri) throws
GadgetException {
On 2010/08/22 16:19:12, gagan.goku wrote:
> On 2010/08/21 17:21:21, johnfargo wrote:
> > may as well add a method:
> > public ProxyUriManager.ProxyUri getProxyUri(Uri reqUri, String container)
> > 
> > ...and pass request.getContainer() to the second argument when called in
this
> > method.
> 
> Or better, pass in the original HttpRequest and do makeGadget(httpRequest)

Done.

http://codereview.appspot.com/2004042/diff/24002/29008#newcode139
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:139:
Gadget gadget = DomWalker.makeGadget(new GadgetContext(requestUri, null));
On 2010/08/22 16:19:12, gagan.goku wrote:
> Lets leave a TODO here to pass on the container information correctly, rather
> than passing null container.
> 
> The problem is that getProxyUri() only takes a Uri. If it takes the
HttpRequest,
> we will have all the information we need.
> Take a look at http://codereview.appspot.com/1949048/diff/7001/2004.

Done.

http://codereview.appspot.com/2004042/diff/24002/29009
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29009#newcode52
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:52:
Gadget gadget = DomWalker.makeGadget(new GadgetContext(requestUri, null));
On 2010/08/22 16:19:12, gagan.goku wrote:
> Lets leave a TODO to pass on the correct container here as well.
> 
> Take a look at http://codereview.appspot.com/1949048/diff/7001/2006 for
another
> way of fixing this issue.

Done.

http://codereview.appspot.com/2004042/diff/24002/29010
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java
(right):

http://codereview.appspot.com/2004042/diff/24002/29010#newcode232
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/CssResponseRewriterTest.java:232:
}
On 2010/08/22 16:19:12, gagan.goku wrote:
> We should really add a test here to ensure that non "default" container does
> percolate and shows in the proxied uri.

The CssResponseRewriterTest already had a test in it that sent an alternate
container (testCssWithContainerProxy) but it was using the PassthruManager which
doesn’t do anything with the container.  I moved it over to use the
DefaultProxyUriManager instead and updated the test code to look for the right
container in the rewritten URL's.  

Should we also move SanitizingGadgetRewriterTest and CajaCssSanitizerTest to use
DefaultProxyUriManager instead as well?  SanitizingGadgetRewriterTest in
particular has a couple of tests in it which seem to imply that they will test
for the nocache and debug flags by name
(enforceCssImportLinkRewrittenNoCacheAndDebug,
enforceImageSrcProxiedNoCacheAndDebug), but they never seem to look for the
flags in the rewritten URL's (which the PassthruManager doesn’t output anyway).

[gagan.goku, 2010-08-24 13:01:13]

Hi Jesse

I have not gone through the tests thoroughly (at first glance they look nice).
Overall this code change looks awesome now.

The TODO that you added in CajaCssSanitizer.java affects
SanitizingGadgetRewriter.java and SanitizingResponseRewriter.java. But since you
have added a TODO in CajaCssSanitizer, no need to change these files in this cl.
Let me make a follow up change fixing these 2 files.

Also, you can either move the tests to point to DefaultProxyUriManager, or add
container information in PassThruManager.

Thanks
Gagan

[jcian, 2010-08-24 13:30:02]

On 2010/08/24 13:01:13, gagan.goku wrote:
> Hi Jesse
> 
> I have not gone through the tests thoroughly (at first glance they look nice).
> Overall this code change looks awesome now.
> 
> The TODO that you added in CajaCssSanitizer.java affects
> SanitizingGadgetRewriter.java and SanitizingResponseRewriter.java. But since
you
> have added a TODO in CajaCssSanitizer, no need to change these files in this
cl.
> Let me make a follow up change fixing these 2 files.
> 
> Also, you can either move the tests to point to DefaultProxyUriManager, or add
> container information in PassThruManager.
> 
> Thanks
> Gagan

Hi Gagan,

For the question of moving tests to DefaultProxyUriManager vs. adding additional
functionality to PassThruManager -- I had actually started out by adding the
additional functionality to PassThruManager but decided it was probably starting
to defeat the purpose of the class, which I think is to have a simple
implementation of ProxyUriManager to be used in places where you don't care
about all the extra details that DefaultProxyUriManager handles (like container,
debug, versioning, etc).

With respect to moving the SanitizingGadgetRewriterTest and CajaCssSanitizerTest
over to use the DefaultProxyUriManager -- if you're going to be working on a
follow up patch for SanitizingGadgetRewriter anyway then maybe it would make
sense for you to go ahead and make those changes then.  What do you think?

Thanks,

--Jesse

[gagan.goku, 2010-08-24 15:01:13]

I'm fine with TODO's in appropriate places.

@Ziv, John: What are your thoughts ?

On Tue, Aug 24, 2010 at 7:00 PM, <jcian@mitre.org> wrote:

> On 2010/08/24 13:01:13, gagan.goku wrote:
>
>> Hi Jesse
>>
>
>  I have not gone through the tests thoroughly (at first glance they
>>
> look nice).
>
>> Overall this code change looks awesome now.
>>
>
>  The TODO that you added in CajaCssSanitizer.java affects
>> SanitizingGadgetRewriter.java and SanitizingResponseRewriter.java. But
>>
> since you
>
>> have added a TODO in CajaCssSanitizer, no need to change these files
>>
> in this cl.
>
>> Let me make a follow up change fixing these 2 files.
>>
>
>  Also, you can either move the tests to point to
>>
> DefaultProxyUriManager, or add
>
>> container information in PassThruManager.
>>
>
>  Thanks
>> Gagan
>>
>
> Hi Gagan,
>
> For the question of moving tests to DefaultProxyUriManager vs. adding
> additional functionality to PassThruManager -- I had actually started
> out by adding the additional functionality to PassThruManager but
> decided it was probably starting to defeat the purpose of the class,
> which I think is to have a simple implementation of ProxyUriManager to
> be used in places where you don't care about all the extra details that
> DefaultProxyUriManager handles (like container, debug, versioning, etc).
>
> With respect to moving the SanitizingGadgetRewriterTest and
> CajaCssSanitizerTest over to use the DefaultProxyUriManager -- if you're
> going to be working on a follow up patch for SanitizingGadgetRewriter
> anyway then maybe it would make sense for you to go ahead and make those
> changes then.  What do you think?
>
> Thanks,
>
> --Jesse
>
>
> http://codereview.appspot.com/2004042/
>

[johnfargo, 2010-08-24 15:10:43]

TODOs are fine by me.

On Tue, Aug 24, 2010 at 8:00 AM, Gagandeep singh <gagan.goku@gmail.com>wrote:

> I'm fine with TODO's in appropriate places.
>
> @Ziv, John: What are your thoughts ?
>
>
> On Tue, Aug 24, 2010 at 7:00 PM, <jcian@mitre.org> wrote:
>
>> On 2010/08/24 13:01:13, gagan.goku wrote:
>>
>>> Hi Jesse
>>>
>>
>>  I have not gone through the tests thoroughly (at first glance they
>>>
>> look nice).
>>
>>> Overall this code change looks awesome now.
>>>
>>
>>  The TODO that you added in CajaCssSanitizer.java affects
>>> SanitizingGadgetRewriter.java and SanitizingResponseRewriter.java. But
>>>
>> since you
>>
>>> have added a TODO in CajaCssSanitizer, no need to change these files
>>>
>> in this cl.
>>
>>> Let me make a follow up change fixing these 2 files.
>>>
>>
>>  Also, you can either move the tests to point to
>>>
>> DefaultProxyUriManager, or add
>>
>>> container information in PassThruManager.
>>>
>>
>>  Thanks
>>> Gagan
>>>
>>
>> Hi Gagan,
>>
>> For the question of moving tests to DefaultProxyUriManager vs. adding
>> additional functionality to PassThruManager -- I had actually started
>> out by adding the additional functionality to PassThruManager but
>> decided it was probably starting to defeat the purpose of the class,
>> which I think is to have a simple implementation of ProxyUriManager to
>> be used in places where you don't care about all the extra details that
>> DefaultProxyUriManager handles (like container, debug, versioning, etc).
>>
>> With respect to moving the SanitizingGadgetRewriterTest and
>> CajaCssSanitizerTest over to use the DefaultProxyUriManager -- if you're
>> going to be working on a follow up patch for SanitizingGadgetRewriter
>> anyway then maybe it would make sense for you to go ahead and make those
>> changes then.  What do you think?
>>
>> Thanks,
>>
>> --Jesse
>>
>>
>> http://codereview.appspot.com/2004042/
>>
>
>

[gagan.goku, 2010-08-25 17:52:14]

lgtm. Small nitpicks inline (mostly files to be reverted because they have been
committed as part of http://codereview.appspot.com/1949048/):

http://codereview.appspot.com/2004042/diff/27003/3015
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3015#newcode192
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java:192:
//TODO: Refactor this method to take an additional parameter which gives us
access to the
Small nitpick: Clean up todo a bit:
// TODO: Refactor this method to pass on the container as well.

http://codereview.appspot.com/2004042/diff/27003/3003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(left):

http://codereview.appspot.com/2004042/diff/27003/3003#oldcode77
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:77:
ProxyUriManager.ProxyUri proxyUri = getProxyUri(request.getUri());
You can revert this file now that Ziv has committed
http://codereview.appspot.com/1949048/

http://codereview.appspot.com/2004042/diff/27003/3016
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3016#newcode25
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:25:
import org.apache.shindig.gadgets.http.HttpRequest;
You can revert this file now that Ziv has committed
http://codereview.appspot.com/1949048/

http://codereview.appspot.com/2004042/diff/27003/3005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3005#newcode51
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:51:
public Uri parseAndNormalize(HttpRequest request) throws GadgetException {
You can revert this file now that Ziv has committed
http://codereview.appspot.com/1949048/

http://codereview.appspot.com/2004042/diff/27003/3006
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3006#newcode97
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java:97:
Gadget gadget = DomWalker.makeGadget(new
HttpRequest(Uri.parse("http://1.com/")));
Can we test that container information is being used by setting the container
here as well ?

http://codereview.appspot.com/2004042/diff/27003/3010
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManagerTest.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3010#newcode69
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManagerTest.java:69:
}
You can revert this file now that Ziv has committed
http://codereview.appspot.com/1949048/

[jcian, 2010-08-25 20:01:27]

http://codereview.appspot.com/2004042/diff/27003/3015
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3015#newcode192
java/gadgets/src/main/java/org/apache/shindig/gadgets/parse/caja/CajaCssSanitizer.java:192:
//TODO: Refactor this method to take an additional parameter which gives us
access to the
On 2010/08/25 17:52:14, gagan.goku wrote:
> Small nitpick: Clean up todo a bit:
> // TODO: Refactor this method to pass on the container as well.

Done.

http://codereview.appspot.com/2004042/diff/27003/3003
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(left):

http://codereview.appspot.com/2004042/diff/27003/3003#oldcode77
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java:77:
ProxyUriManager.ProxyUri proxyUri = getProxyUri(request.getUri());
On 2010/08/25 17:52:14, gagan.goku wrote:
> You can revert this file now that Ziv has committed
> http://codereview.appspot.com/1949048/

Done.

http://codereview.appspot.com/2004042/diff/27003/3016
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3016#newcode25
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/AccelUriManager.java:25:
import org.apache.shindig.gadgets.http.HttpRequest;
On 2010/08/25 17:52:14, gagan.goku wrote:
> You can revert this file now that Ziv has committed
> http://codereview.appspot.com/1949048/

Done.

http://codereview.appspot.com/2004042/diff/27003/3005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3005#newcode51
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManager.java:51:
public Uri parseAndNormalize(HttpRequest request) throws GadgetException {
On 2010/08/25 17:52:14, gagan.goku wrote:
> You can revert this file now that Ziv has committed
> http://codereview.appspot.com/1949048/

Done.

http://codereview.appspot.com/2004042/diff/27003/3006
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3006#newcode97
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitorTest.java:97:
Gadget gadget = DomWalker.makeGadget(new
HttpRequest(Uri.parse("http://1.com/")));
On 2010/08/25 17:52:14, gagan.goku wrote:
> Can we test that container information is being used by setting the container
> here as well ?

Done.

http://codereview.appspot.com/2004042/diff/27003/3010
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManagerTest.java
(right):

http://codereview.appspot.com/2004042/diff/27003/3010#newcode69
java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/DefaultAccelUriManagerTest.java:69:
}
On 2010/08/25 17:52:14, gagan.goku wrote:
> You can revert this file now that Ziv has committed
> http://codereview.appspot.com/1949048/

Done.

[zhoresh, 2010-08-26 02:06:38]

lgtm, committed at r989422

[gagan.goku, 2010-08-26 02:10:34]

Thanks Ziv. This should fix container related bugs.

On Thu, Aug 26, 2010 at 7:36 AM, <zhoresh@gmail.com> wrote:

> lgtm, committed at r989422
>
>
> http://codereview.appspot.com/2004042/
>

[gagan.goku, 2010-08-27 15:42:13]

You should close this issue now that it has been submitted.