You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@shindig.apache.org by li...@inuus.com on 2010/08/30 22:03:35 UTC

Re: Refactoring ProxyingVisitor so that its functionality can be extended by other visitors (issue1949051)

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 @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..

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>();
Make this Immutable?

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

Please add some javadoc here.

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

Please add some javadoc here.

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

Please add some javadoc here.
Also can this be a Collection instead of List?

http://codereview.appspot.com/1949051/