Issue 1726041: Adding capability in AbsolutePathReferenceVisitor to resolve relative to base tag url (Closed)
|
Created:
15 years, 10 months ago by gagan.goku Modified:
15 years, 9 months ago CC:
cool-shindig-committers_googlegroups.com Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
DescriptionAdding capability in AbsolutePathReferenceVisitor to resolve relative to base tag url
Base tag url looks like this:
<base href="http://www.google.com/">
And then any resource like:
<img src="/hello.jpg">
should be resolved to "http://www.google.com/hello.jpg"
Patch Set 1 #Patch Set 2 : adding_tests #Patch Set 3 : merging_changes_after_syning_to_r955719 #Patch Set 4 : fixing_small_bug #
Total comments: 16
Patch Set 5 : fixing_comments #
Total comments: 11
Patch Set 6 : fixing_more_comments #
Total comments: 2
Patch Set 7 : really_fixing_comments #
MessagesTotal messages: 10
Synced to r955719.
Sign in to reply to this message.
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#newcode42 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:42: public enum TagsToMakeAbsolute { nit: since this is already a subclass of AbsolutePathReferenceVisitor you can probably just call this Tags http://codereview.appspot.com/1726041/diff/6001/7002#newcode60 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:60: TagsToMakeAbsolute(Map<String, String> resourceTags) { make private http://codereview.appspot.com/1726041/diff/6001/7002#newcode62 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:62: } nit: missing space here http://codereview.appspot.com/1726041/diff/6001/7002#newcode69 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:69: Map<String, String> tagsToMakeAbsolute; should be private final http://codereview.appspot.com/1726041/diff/6001/7002#newcode88 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:88: Uri baseUri = getUriToResolveUrisRelativeTo(gadget, node); just call this getBaseUri, or perhaps getBaseResolutionUri 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) { this can probably be private -- when would you see a need to customize? http://codereview.appspot.com/1726041/diff/6001/7002#newcode177 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:177: Attr attr = (Attr) list.item(0).getAttributes().getNamedItem("href"); getAttributes() can return null - either: A. check this to avoid NPE, or B. cast list.item(0) to an Element (getElementsByTagName's NodeList is populated exclusively with them by contract) and use getAttribute("href") (note that this API returns "" rather than null if the attr doesn't exist)
Sign in to reply to this message.
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#newcode42 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:42: public enum TagsToMakeAbsolute { On 2010/06/17 20:59:54, johnfargo wrote: > nit: since this is already a subclass of AbsolutePathReferenceVisitor you can > probably just call this Tags Done. http://codereview.appspot.com/1726041/diff/6001/7002#newcode60 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:60: TagsToMakeAbsolute(Map<String, String> resourceTags) { On 2010/06/17 20:59:54, johnfargo wrote: > make private Done. http://codereview.appspot.com/1726041/diff/6001/7002#newcode62 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:62: } On 2010/06/17 20:59:54, johnfargo wrote: > nit: missing space here Done. http://codereview.appspot.com/1726041/diff/6001/7002#newcode69 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:69: Map<String, String> tagsToMakeAbsolute; On 2010/06/17 20:59:54, johnfargo wrote: > should be private final Done. http://codereview.appspot.com/1726041/diff/6001/7002#newcode88 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:88: Uri baseUri = getUriToResolveUrisRelativeTo(gadget, node); On 2010/06/17 20:59:54, johnfargo wrote: > just call this getBaseUri, or perhaps getBaseResolutionUri Done. 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 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. http://codereview.appspot.com/1726041/diff/6001/7002#newcode177 java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitor.java:177: Attr attr = (Attr) list.item(0).getAttributes().getNamedItem("href"); On 2010/06/17 20:59:54, johnfargo wrote: > getAttributes() can return null - either: > A. check this to avoid NPE, or > B. cast list.item(0) to an Element (getElementsByTagName's NodeList is populated > exclusively with them by contract) and use getAttribute("href") (note that this > API returns "" rather than null if the attr doesn't exist) checked for null.
Sign in to reply to this message.
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: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. 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: } nit: newline here http://codereview.appspot.com/1726041/diff/11001/12001#newcode52 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:52: } same 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/"); nit: convention is www.example.org 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())); 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. 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/"); the href value should be different to ensure the first base tag is being used.
Sign in to reply to this message.
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.
Sign in to reply to this message.
On Thu, Jun 17, 2010 at 2:52 PM, <gagan.goku@gmail.com> 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 >
Sign in to reply to this message.
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#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:52:32, gagan.goku wrote: > On 2010/06/17 21:38:39, johnfargo wrote: > > nit: convention is http://www.example.org > > Done. I call your bluff! :) http://codereview.appspot.com/1726041/diff/17001/18001#newcode142 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:142: same
Sign in to reply to this message.
This time really Done. http://codereview.appspot.com/1726041/diff/17001/18001 File java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java (right): http://codereview.appspot.com/1726041/diff/17001/18001#newcode142 java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:142: Element baseTag1 = elem("base", "href", "www.google.com/"); On 2010/06/17 21:55:04, johnfargo wrote: > same Done.
Sign in to reply to this message.
Committed. On Thu, Jun 17, 2010 at 3:00 PM, <gagan.goku@gmail.com> wrote: > This time really Done. > > > > http://codereview.appspot.com/1726041/diff/17001/18001 > File > > java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java > (right): > > http://codereview.appspot.com/1726041/diff/17001/18001#newcode142 > > java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:142: > Element baseTag1 = elem("base", "href", "www.google.com/"); > On 2010/06/17 21:55:04, johnfargo wrote: > >> same >> > > Done. > > > http://codereview.appspot.com/1726041/show >
Sign in to reply to this message.
On 2010/06/17 23:04:30, johnfargo wrote: > Committed. > > On Thu, Jun 17, 2010 at 3:00 PM, <mailto:gagan.goku@gmail.com> wrote: > > > This time really Done. > > > > > > > > http://codereview.appspot.com/1726041/diff/17001/18001 > > File > > > > > java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java > > (right): > > > > http://codereview.appspot.com/1726041/diff/17001/18001#newcode142 > > > > > java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/AbsolutePathReferenceVisitorTest.java:142: > > Element baseTag1 = elem("base", "href", "www.google.com/"); > > On 2010/06/17 21:55:04, johnfargo wrote: > > > >> same > >> > > > > Done. > > > > > > http://codereview.appspot.com/1726041/show > > > Thanks John.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
