SVG should consider base URI when resolving fragment URIs

SVG should consider base URI when resolving fragment URIs

When SVG resolving fragment URIs, it should always consider  base URIs first.

BUG=449027
Committed: https://crrev.com/d97f56157c1d119d3a0dda5d5d341e099f7f2b26
Cr-Commit-Position: refs/heads/master@{#360061}

[Shanmuga Pandi, 2015-11-16 09:45:57]

Description was changed from

==========
SVG should consider base URI when resolving fragment URIs

BUG=449027
==========

to

==========
SVG should consider base URI when resolving fragment URIs

BUG=449027
R=SamsungPeerReview
==========

[Shanmuga Pandi, 2015-11-16 09:45:57]

shanmuga.m@samsung.com changed reviewers:
+ rob.buis@samsung.com

[Shanmuga Pandi, 2015-11-16 09:46:15]

PTAL!!

[rwlbuis, 2015-11-16 18:30:50]

Looks good with some style nits.

https://codereview.chromium.org/1450823002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/custom/baseuri-href-expected.html
(right):

https://codereview.chromium.org/1450823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/baseuri-href-expected.html:5: <rect
id="rect" width="100" height="100" fill="green" />
Ditto.

https://codereview.chromium.org/1450823002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/svg/custom/baseuri-href.html (right):

https://codereview.chromium.org/1450823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/baseuri-href.html:4: <base href="/foo"
/>
Better stick with one style, i.e. end here should be "/foo"/>

https://codereview.chromium.org/1450823002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/svg/custom/baseuri-href.html:8: <rect id="rect"
width="100" height="100" fill="green" />
Ditto.

[Shanmuga Pandi, 2015-11-17 05:12:30]

Description was changed from

==========
SVG should consider base URI when resolving fragment URIs

BUG=449027
R=SamsungPeerReview
==========

to

==========
SVG should consider base URI when resolving fragment URIs

When SVG resolving fragment URIs, it should always consider  base URIs first.

BUG=449027
==========

[Shanmuga Pandi, 2015-11-17 05:12:30]

shanmuga.m@samsung.com changed reviewers:
+ ed@chromium.org, fs@opera.com, pdr@chromium.org

[Shanmuga Pandi, 2015-11-17 06:15:01]

Please check this.
thanks

[fs, 2015-11-17 09:07:55]

https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGURIReference.h (right):

https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGURIReference.h:45: if (document.baseURL()
== document.url() && uri.startsWith('#'))
How is this any different from just removing the startsWith(...) check
altogether?

[Shanmuga Pandi, 2015-11-17 09:15:35]

https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGURIReference.h (right):

https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGURIReference.h:45: if (document.baseURL()
== document.url() && uri.startsWith('#'))
On 2015/11/17 09:07:54, fs wrote:
> How is this any different from just removing the startsWith(...) check
> altogether?


If a content does not have <base> property , then it will be treated as internal
URI and do early return, else it will be considered as external URI.

[Shanmuga Pandi, 2015-11-17 09:18:27]

On 2015/11/17 09:15:35, Shanmuga Pandi wrote:
>
https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/svg/SVGURIReference.h (right):
> 
>
https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/svg/SVGURIReference.h:45: if
(document.baseURL()
> == document.url() && uri.startsWith('#'))
> On 2015/11/17 09:07:54, fs wrote:
> > How is this any different from just removing the startsWith(...) check
> > altogether?
> 
> 
> If a content does not have <base> property , then it will be treated as
internal
> URI and do early return, else it will be considered as external URI.

As you said , even if we remove this check altogether, still it will work.

[fs, 2015-11-17 09:53:16]

On 2015/11/17 at 09:18:27, shanmuga.m wrote:
> On 2015/11/17 09:15:35, Shanmuga Pandi wrote:
> >
https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/svg/SVGURIReference.h (right):
> > 
> >
https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/svg/SVGURIReference.h:45: if
(document.baseURL()
> > == document.url() && uri.startsWith('#'))
> > On 2015/11/17 09:07:54, fs wrote:
> > > How is this any different from just removing the startsWith(...) check
> > > altogether?
> > 
> > 
> > If a content does not have <base> property , then it will be treated as
internal
> > URI and do early return, else it will be considered as external URI.
> 
> As you said , even if we remove this check altogether, still it will work.

Yes, then I prefer to remove the check =)

[Shanmuga Pandi, 2015-11-17 10:02:41]

On 2015/11/17 09:53:16, fs wrote:
> On 2015/11/17 at 09:18:27, shanmuga.m wrote:
> > On 2015/11/17 09:15:35, Shanmuga Pandi wrote:
> > >
>
https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/svg/SVGURIReference.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1450823002/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/svg/SVGURIReference.h:45: if
> (document.baseURL()
> > > == document.url() && uri.startsWith('#'))
> > > On 2015/11/17 09:07:54, fs wrote:
> > > > How is this any different from just removing the startsWith(...) check
> > > > altogether?
> > > 
> > > 
> > > If a content does not have <base> property , then it will be treated as
> internal
> > > URI and do early return, else it will be considered as external URI.
> > 
> > As you said , even if we remove this check altogether, still it will work.
> 
> Yes, then I prefer to remove the check =)

Done :)

[fs, 2015-11-17 10:22:19]

lgtm

[Shanmuga Pandi, 2015-11-17 10:31:44]

The CQ bit was checked by shanmuga.m@samsung.com

[commit-bot: I haz the power, 2015-11-17 10:32:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1450823002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1450823002/60001

[commit-bot: I haz the power, 2015-11-17 12:02:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-17 12:02:55]

Patchset 4 (id:??) landed as
https://crrev.com/d97f56157c1d119d3a0dda5d5d341e099f7f2b26
Cr-Commit-Position: refs/heads/master@{#360061}