Adding StyleTagProxyImportRewriter

StyleTagProxyImportRewriter: Rewriter that replaces @import urls and background urls in <style> ... </style> with their proxied versions.

[gagan.goku, 2010-07-07 20:38:48]

with licence

[gagan.goku, 2010-07-07 22:52:52]

better tests

[gagan.goku, 2010-07-07 23:02:03]

better name of class.

[vikaas.arora, 2010-07-08 04:57:54]

Very high level comment and suggestion if we can refactor the existing Visitor
'StyleTagExtractorVisitor' to do the functionality (of rewriting the inlined CSS
Styles.

http://codereview.appspot.com/1704047/diff/14001/15002
File
main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java
(right):

http://codereview.appspot.com/1704047/diff/14001/15002#newcode34
main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java:34:
public class StyleTagProxyEmbeddedUrlsVisitor implements DomWalker.Visitor {
This new visitor looks similar to existing visitor 'StyleTagExtractorVisitor'.
The only major difference being this one calls the CssResponseRewriter.rewrite
with 'extractImports' bool flag as false, while the other one calls with
'extractImports' flag as true.

Can't we abstract out the the 'extractImports' bool flag in the interface for
the visitor and rename 'StyleTagExtractorVisitor' to some suitable name. That
way we don't require this additional Visitor.

[gagan.goku, 2010-07-08 19:23:44]

On 2010/07/08 04:57:54, vikaas.arora wrote:
> Very high level comment and suggestion if we can refactor the existing Visitor
> 'StyleTagExtractorVisitor' to do the functionality (of rewriting the inlined
CSS
> Styles.
> 
> http://codereview.appspot.com/1704047/diff/14001/15002
> File
>
main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java
> (right):
> 
> http://codereview.appspot.com/1704047/diff/14001/15002#newcode34
>
main/java/org/apache/shindig/gadgets/rewrite/StyleTagProxyEmbeddedUrlsVisitor.java:34:
> public class StyleTagProxyEmbeddedUrlsVisitor implements DomWalker.Visitor {
> This new visitor looks similar to existing visitor 'StyleTagExtractorVisitor'.
> The only major difference being this one calls the CssResponseRewriter.rewrite
> with 'extractImports' bool flag as false, while the other one calls with
> 'extractImports' flag as true.
> 
> Can't we abstract out the the 'extractImports' bool flag in the interface for
> the visitor and rename 'StyleTagExtractorVisitor' to some suitable name. That
> way we don't require this additional Visitor.

Hi Vikas

Agreed that you have a raised a good point, this visitor is very similar to
StyleTagExtractorVisitor. I have made StyleTagExtractorVisitor inherit from
StyleTagProxyEmbeddedUrlsVisitor  now. This cuts down some of the boilerplate
code. The other difference is that we really dont care about what is
enabled/disabled in the config.

The actual revisit function of StyleTagExtractorVisitor actually does lot more.
So i have left that unchanged.
Writing a function to abstract out the 'extractImports' bool seems useless now
as the function call is just 2 lines anyway.

[gagan.goku, 2010-07-08 19:26:51]

new

[johnfargo, 2010-07-08 22:32:11]

Only one comment -- I already took care of it and committed.

http://codereview.appspot.com/1704047/diff/22001/23001
File main/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitor.java
(right):

http://codereview.appspot.com/1704047/diff/22001/23001#newcode48
main/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitor.java:48:
return super.visit(gadget, node);
I appreciate the insistence on not duplicating code, but in this case we're
trading 3 lines for extension of another class -- I'd just revert this file.

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

Thanks John.

http://codereview.appspot.com/1704047/diff/22001/23001
File main/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitor.java
(right):

http://codereview.appspot.com/1704047/diff/22001/23001#newcode48
main/java/org/apache/shindig/gadgets/rewrite/StyleTagExtractorVisitor.java:48:
return super.visit(gadget, node);
On 2010/07/08 22:32:11, johnfargo wrote:
> I appreciate the insistence on not duplicating code, but in this case we're
> trading 3 lines for extension of another class -- I'd just revert this file.

okay :)