On Thu, Jun 17, 2010 at 2:52 PM, <[email protected]> wrote: > > 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 ?
No the test is great; my point was just that you needn't test the result of the getBaseUri method itself -- it's tested as part of testing the actual functionality of the class. > > > 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 >
