http://codereview.appspot.com/1726041/diff/6001/7002 File java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java (right):
http://codereview.appspot.com/1726041/diff/6001/7002#newcode171 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:171: protected String getBaseHref(Document doc) { On 2010/06/17 21:38:38, johnfargo wrote:
On 2010/06/17 21:21:39, gagan.goku wrote: > On 2010/06/17 20:59:54, johnfargo wrote: > > this can probably be private -- when would you see a need to
customize?
> > made private. IMO reusable helper functions should be made
protected, but this
> is fine.
Agreed in general, but in that case they should be public static
methods (none
of these require state). In this case I don't think that's necessary -
I'd just
make this private. You test it by way of testing the actual
functionality of
AbsolutePathReferenceVisitor.
Done. http://codereview.appspot.com/1726041/diff/11001/12001 File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java (right): http://codereview.appspot.com/1726041/diff/11001/12001#newcode48 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:48: } On 2010/06/17 21:38:39, johnfargo wrote:
nit: newline here
Done. http://codereview.appspot.com/1726041/diff/11001/12001#newcode52 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:52: } On 2010/06/17 21:38:39, johnfargo wrote:
same
Done. http://codereview.appspot.com/1726041/diff/11001/12001#newcode114 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:114: Element baseTag = elem("base", "href", "http://www.google.com/";); On 2010/06/17 21:38:39, johnfargo wrote:
nit: convention is http://www.example.org
Done. http://codereview.appspot.com/1726041/diff/11001/12001#newcode132 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:132: assertNull(visitor.getBaseHref(html.getOwnerDocument())); On 2010/06/17 21:38:39, johnfargo wrote:
IMO if you're testing the contract of AbsolutePathReferenceVisitor you
don't
have to test the helper methods. I'd do so only if you choose to
expose them as
public static, which seems unnecessary to me.
just wanted to test a one off case when base tag is there without href attribute. Anyways, do you want me to remove the test ? http://codereview.appspot.com/1726041/diff/11001/12001#newcode141 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:141: Element baseTag2 = elem("base", "href", "http://www.google.com/";); On 2010/06/17 21:38:39, johnfargo wrote:
the href value should be different to ensure the first base tag is
being used. Done. http://codereview.appspot.com/1726041/show
