Issue 1949051: Refactoring ProxyingVisitor so that its functionality can be extended by other visitors

Issue 1949051: Refactoring ProxyingVisitor so that its functionality can be extended by other visitors (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
15 years, 5 months ago by gagan.goku

Modified:
15 years, 5 months ago

Reviewers:
Paul Lindner, dev

CC:
cool-shindig-committers_googlegroups.com, satya3656, anupama.dutta

Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/

Visibility:
Public.

Description

Refactoring ProxyingVisitor so that its functionality can be extended by other visitors like: - proxying resources - making url's absolute - prefetching images - domain balancing etc.

Patch Set 2 : 'addressing_anupamas_comment' #

Total comments: 10

Patch Set 3 : 'addressing_pauls_comments' #

Patch Set 4 : 'addressing_pauls_comments' #

Patch Set 5 : 'inheritDoc' #

Created: 15 years, 5 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+149 lines, -95 lines)			Patch
	M	java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java	View	1 2	2 chunks	+6 lines, -95 lines	0 comments	Download
	A	java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java	View	1 2 3 4	1 chunk	+143 lines, -0 lines	0 comments	Download

Messages

Total messages: 7

Expand All Messages | Collapse All Messages

anupama.dutta

LGTM. http://codereview.appspot.com/1949051/diff/1/3 File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java (right): http://codereview.appspot.com/1949051/diff/1/3#newcode39 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:39: * resource links to proxied versions of the ...

15 years, 5 months ago (2010-08-30 10:38:34 UTC) #2

gagan.goku

http://codereview.appspot.com/1949051/diff/1/3 File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java (right): http://codereview.appspot.com/1949051/diff/1/3#newcode39 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:39: * resource links to proxied versions of the same. ...

15 years, 5 months ago (2010-08-30 15:39:03 UTC) #3

Paul Lindner

some small comments. Thanks for the patch http://codereview.appspot.com/1949051/diff/5001/6001 File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java (left): http://codereview.appspot.com/1949051/diff/5001/6001#oldcode85 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:85: Is this ...

15 years, 5 months ago (2010-08-30 20:03:35 UTC) #4

gagan.goku

http://codereview.appspot.com/1949051/diff/5001/6001 File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java (left): http://codereview.appspot.com/1949051/diff/5001/6001#oldcode85 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:85: On 2010/08/30 20:03:35, Paul Lindner wrote: > Is this ...

15 years, 5 months ago (2010-08-31 03:36:34 UTC) #5

http://codereview.appspot.com/1949051/diff/5001/6001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
(left):

http://codereview.appspot.com/1949051/diff/5001/6001#oldcode85
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:85:

On 2010/08/30 20:03:35, Paul Lindner wrote:
> Is this @Inject strictly necessary?  It appears that this is instantiated
> directly in ProxyingContentRewriter.  I also doubt that guice supports ...
> syntax for injection, we may require a List or Collection instead..

Nice catch. The @Inject was meaningless. Don't know what guice does for ...
though
maybe it just passes null

http://codereview.appspot.com/1949051/diff/5001/6002
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java
(right):

http://codereview.appspot.com/1949051/diff/5001/6002#newcode84
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:84:
this.resourceTags = new HashMap<String, String>();
On 2010/08/30 20:03:35, Paul Lindner wrote:
> Make this Immutable?

Done.

http://codereview.appspot.com/1949051/diff/5001/6002#newcode89
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:89:

On 2010/08/30 20:03:35, Paul Lindner wrote:
> Please add some javadoc here.

Done.

http://codereview.appspot.com/1949051/diff/5001/6002#newcode115
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:115:

On 2010/08/30 20:03:35, Paul Lindner wrote:
> Please add some javadoc here.
> 

Done.

http://codereview.appspot.com/1949051/diff/5001/6002#newcode132
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:132:

On 2010/08/30 20:03:35, Paul Lindner wrote:
> Please add some javadoc here.
> Also can this be a Collection instead of List?
> 

Done. Collection looks cleaner as well.

Paul Lindner

submitted. Thanks! On 2010/08/31 03:36:34, gagan.goku wrote: > http://codereview.appspot.com/1949051/diff/5001/6001 > File > java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java > (left): ...

15 years, 5 months ago (2010-08-31 05:56:20 UTC) #6

submitted.

Thanks!

On 2010/08/31 03:36:34, gagan.goku wrote:
> http://codereview.appspot.com/1949051/diff/5001/6001
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java
> (left):
> 
> http://codereview.appspot.com/1949051/diff/5001/6001#oldcode85
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ProxyingVisitor.java:85:
> 
> On 2010/08/30 20:03:35, Paul Lindner wrote:
> > Is this @Inject strictly necessary?  It appears that this is instantiated
> > directly in ProxyingContentRewriter.  I also doubt that guice supports ...
> > syntax for injection, we may require a List or Collection instead..
> 
> Nice catch. The @Inject was meaningless. Don't know what guice does for ...
> though
> maybe it just passes null
> 
> http://codereview.appspot.com/1949051/diff/5001/6002
> File
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java
> (right):
> 
> http://codereview.appspot.com/1949051/diff/5001/6002#newcode84
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:84:
> this.resourceTags = new HashMap<String, String>();
> On 2010/08/30 20:03:35, Paul Lindner wrote:
> > Make this Immutable?
> 
> Done.
> 
> http://codereview.appspot.com/1949051/diff/5001/6002#newcode89
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:89:
> 
> On 2010/08/30 20:03:35, Paul Lindner wrote:
> > Please add some javadoc here.
> 
> Done.
> 
> http://codereview.appspot.com/1949051/diff/5001/6002#newcode115
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:115:
> 
> On 2010/08/30 20:03:35, Paul Lindner wrote:
> > Please add some javadoc here.
> > 
> 
> Done.
> 
> http://codereview.appspot.com/1949051/diff/5001/6002#newcode132
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ResourceMutateVisitor.java:132:
> 
> On 2010/08/30 20:03:35, Paul Lindner wrote:
> > Please add some javadoc here.
> > Also can this be a Collection instead of List?
> > 
> 
> Done. Collection looks cleaner as well.

Thanks Paul

Expand All Messages | Collapse All Messages