Add support for 'href' (w/o XLink NS) for various SVG elements

Add support for 'href' (w/o XLink NS) for various SVG elements

This CL adds a new wrapper type SVGAnimatedHref that wraps 'href' (the
new, null/default NS version) and '(xlink:)href' (the XLink NS version).
This wrapper type is used by SVGURIReference and thus most uses of
'href' is covered by this part. SVGAnimatedHref is intended as a wrapper
for accessing the underlying value of either 'href' or 'xlink:href'. Any
updates due to setAttribute or will go directly to the underlying values
which means that synchronization etc. does not apply to the wrapper.
This is one of the "pro"s of this approach - it does not require any
modification to synching between the SVGString object and the attribute
in certain cases (like when the "active" attribute is removed). A "con"
would be that there's a lot of dead space in the object since the
wrapper itself still needs to implement the SVGAnimatedString interface
even if just forwards to the actual SVGString.
The UseCounter for href.baseVal/animVal are moved to the new wrapper.

For cases where SVGURIReference is not used, new code-paths are added to
select the right attribute.

Animation code is updated to target the 'href' in the null/default NS.

BUG=584142
Committed: https://crrev.com/2f0b655515af2df03d41d4f894e0dd59d5cb6411
Cr-Commit-Position: refs/heads/master@{#375192}

[fs, 2016-02-08 15:05:17]

Description was changed from

==========
Add support for 'href' (w/o XLink NS) for various SVG elements

This CL adds a new wrapper type SVGAnimatedHref that wraps 'href' (the
new, null/default NS version) and '(xlink:)href' (the XLink NS version).
This wrapper type is used by SVGURIReference and thus most uses of
'href' is covered by this part. SVGAnimatedHref is intended as a wrapper
for accessing the underlying value of either 'href' or 'xlink:href'. Any
updates due to setAttribute or will go directly to the underlying values
which means that synchronization etc. does not apply to the wrapper.
This is one of the "pro"s of this approach - it does not require any
modification to synching between the SVGString object and the attribute
in certain cases (like when the "active" attribute is removed). A "con"
would be that there's a lot of dead space in the object since the
wrapper itself still needs to implement the SVGAnimatedString interface
even if just forwards to the actual SVGString.

For cases where SVGURIReference is not used, new code-paths are added to
select the right attribute.

BUG=584142
==========

to

==========
Add support for 'href' (w/o XLink NS) for various SVG elements

This CL adds a new wrapper type SVGAnimatedHref that wraps 'href' (the
new, null/default NS version) and '(xlink:)href' (the XLink NS version).
This wrapper type is used by SVGURIReference and thus most uses of
'href' is covered by this part. SVGAnimatedHref is intended as a wrapper
for accessing the underlying value of either 'href' or 'xlink:href'. Any
updates due to setAttribute or will go directly to the underlying values
which means that synchronization etc. does not apply to the wrapper.
This is one of the "pro"s of this approach - it does not require any
modification to synching between the SVGString object and the attribute
in certain cases (like when the "active" attribute is removed). A "con"
would be that there's a lot of dead space in the object since the
wrapper itself still needs to implement the SVGAnimatedString interface
even if just forwards to the actual SVGString.

For cases where SVGURIReference is not used, new code-paths are added to
select the right attribute.

Animation code is updated to target the 'href' in the null/default NS.

BUG=584142
==========

[fs, 2016-02-08 15:11:28]

Description was changed from

==========
Add support for 'href' (w/o XLink NS) for various SVG elements

This CL adds a new wrapper type SVGAnimatedHref that wraps 'href' (the
new, null/default NS version) and '(xlink:)href' (the XLink NS version).
This wrapper type is used by SVGURIReference and thus most uses of
'href' is covered by this part. SVGAnimatedHref is intended as a wrapper
for accessing the underlying value of either 'href' or 'xlink:href'. Any
updates due to setAttribute or will go directly to the underlying values
which means that synchronization etc. does not apply to the wrapper.
This is one of the "pro"s of this approach - it does not require any
modification to synching between the SVGString object and the attribute
in certain cases (like when the "active" attribute is removed). A "con"
would be that there's a lot of dead space in the object since the
wrapper itself still needs to implement the SVGAnimatedString interface
even if just forwards to the actual SVGString.

For cases where SVGURIReference is not used, new code-paths are added to
select the right attribute.

Animation code is updated to target the 'href' in the null/default NS.

BUG=584142
==========

to

==========
Add support for 'href' (w/o XLink NS) for various SVG elements

This CL adds a new wrapper type SVGAnimatedHref that wraps 'href' (the
new, null/default NS version) and '(xlink:)href' (the XLink NS version).
This wrapper type is used by SVGURIReference and thus most uses of
'href' is covered by this part. SVGAnimatedHref is intended as a wrapper
for accessing the underlying value of either 'href' or 'xlink:href'. Any
updates due to setAttribute or will go directly to the underlying values
which means that synchronization etc. does not apply to the wrapper.
This is one of the "pro"s of this approach - it does not require any
modification to synching between the SVGString object and the attribute
in certain cases (like when the "active" attribute is removed). A "con"
would be that there's a lot of dead space in the object since the
wrapper itself still needs to implement the SVGAnimatedString interface
even if just forwards to the actual SVGString.
The UseCounter for href.baseVal/animVal are moved to the new wrapper.

For cases where SVGURIReference is not used, new code-paths are added to
select the right attribute.

Animation code is updated to target the 'href' in the null/default NS.

BUG=584142
==========

[fs, 2016-02-08 18:19:10]

fs@opera.com changed reviewers:
+ alancutter@chromium.org, pdr@chromium.org

[fs, 2016-02-08 18:19:10]

alancutter: A minor, hopefully uncontroversial, change to SVG (Web) Animations.
Maybe mostly FYI.

pdr: Could you handle the rest? If not, delegate as needed.

[alancutter (OOO until 2018), 2016-02-08 23:35:56]

On 2016/02/08 at 18:19:10, fs wrote:
> alancutter: A minor, hopefully uncontroversial, change to SVG (Web)
Animations. Maybe mostly FYI.
> 
> pdr: Could you handle the rest? If not, delegate as needed.

lgtm to core/animation changes.

[pdr., 2016-02-09 23:17:06]

pdr@chromium.org changed reviewers:
+ davve@opera.com

[pdr., 2016-02-09 23:17:07]

+cc davve for more reviewer eyes.

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/dom/href-semantics.html (right):

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/href-semantics.html:1: <!DOCTYPE html>
These tests seem reasonable to me. Can you check to see how Edge performs on
these, to ensure we match their behavior?

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGAnimatedHref.h (right):

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedHref.h:19: class SVGAnimatedHref
final : public SVGAnimatedString {
So much complexity that will bite us in the future :( I'm worried about this
class and all the callsites that need to check both xlink:href and href.

Is there not a way to simply alias this? E.g., in the getter and setter for
xlink:href, get/set href?

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp:242:
AtomicString href = getAttribute(SVGNames::hrefAttr);
Can we use Element::hrefURL here?

[fs, 2016-02-10 14:44:46]

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/dom/href-semantics.html (right):

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/dom/href-semantics.html:1: <!DOCTYPE html>
On 2016/02/09 at 23:17:07, pdr wrote:
> These tests seem reasonable to me. Can you check to see how Edge performs on
these, to ensure we match their behavior?

These are all PASS in Edge.

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGAnimatedHref.h (right):

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedHref.h:19: class SVGAnimatedHref
final : public SVGAnimatedString {
On 2016/02/09 at 23:17:07, pdr wrote:
> So much complexity that will bite us in the future :(

Well this is actually the less-complex solution... =P A lot of this class is
just overrides for the sake of asserts though - asserts to try and catch any
infelicities. I guess this in itself shows that there are several facets that
combine in this interface (value-interfacing; value-synchronization; animation)

> I'm worried about this class and all the callsites that need to check both
xlink:href and href.

The ones using this class (==implements SVGURIReference) are the ones that does
_not_ need to worry, because this class sorts out the right 'href' for them.

> Is there not a way to simply alias this? E.g., in the getter and setter for
xlink:href, get/set href?

I'm afraid this is the "simple alias" (backingHref being what determines the
active alias). For the DOM interface(s) we need to return one and only one
object - anything else would be mildly confusing to say the least. If we didn't
need to cater for the DOM interface, then this would likely be radically
simpler...

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp:242:
AtomicString href = getAttribute(SVGNames::hrefAttr);
On 2016/02/09 at 23:17:07, pdr wrote:
> Can we use Element::hrefURL here?

We probably could (considering SVGSMILElements are never layouted etc.) - I
don't think it would be without peril though. And then there's the issue of
String vs. KURL (which hrefURL returns), so there'd need to be some impedance
matching here. I'd be happy to try and clean this up in a follow-up though
(never really been a fan of the somewhat ad-hoc looking code in
SVGURIReference.)

[davve, 2016-02-10 17:47:40]

Reading through makes me share some of pdr's complexity concerns, especially
about multiple call sites having to look for href at two places in the correct
order.

However, in general I don't see a way how the complexity here could be avoided.
Maybe getting rid of some parts of SVG1DOM will help in the future. I dunno.

Time's up for today, but I'll look more at this tomorrow.

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

https://codereview.chromium.org/1681553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedString.h:47: virtual String
baseVal();
Do these three need to be virtual/still exist? As I understand it they were
added in https://codereview.chromium.org/1086873004 for counting. Now the
counting moves to SVGAnimatedHref, but these three are still around doing
nothing but delegate to the base class?

[fs, 2016-02-10 19:31:15]

On 2016/02/10 at 17:47:40, davve wrote:
> Reading through makes me share some of pdr's complexity concerns, especially
about multiple call sites having to look for href at two places in the correct
order.

I guess I'll have to try to do some form of cleanup around hrefURL (but we'll
need something that returns the string/unresolved rep. at least for
VisitedLinkState... and hrefString is taken =/)

> However, in general I don't see a way how the complexity here could be
avoided. Maybe getting rid of some parts of SVG1DOM will help in the future. I
dunno.

Yes, the scariest part here is that it's pretty unlikely we'll ever get rid of
this duality... Time to build that time-machine and go back to 2000 and get
someone to formally object...

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

https://codereview.chromium.org/1681553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedString.h:47: virtual String
baseVal();
On 2016/02/10 at 17:47:39, David Vest wrote:
> Do these three need to be virtual/still exist? As I understand it they were
added in https://codereview.chromium.org/1086873004 for counting. Now the
counting moves to SVGAnimatedHref, but these three are still around doing
nothing but delegate to the base class?

Since we call these through a SVGAnimatedString* these need to be virtual for
the dispatch to reach the SVGAnimatedHref overrides when using
SVGURIReference.href. (These are not virtual in the base/ancestor class.)

[davve, 2016-02-11 07:37:09]

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

https://codereview.chromium.org/1681553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedString.h:47: virtual String
baseVal();
On 2016/02/10 19:31:14, fs wrote:
> On 2016/02/10 at 17:47:39, David Vest wrote:
> > Do these three need to be virtual/still exist? As I understand it they were
> added in https://codereview.chromium.org/1086873004 for counting. Now the
> counting moves to SVGAnimatedHref, but these three are still around doing
> nothing but delegate to the base class?
> 
> Since we call these through a SVGAnimatedString* these need to be virtual for
> the dispatch to reach the SVGAnimatedHref overrides when using
> SVGURIReference.href. (These are not virtual in the base/ancestor class.)

Of course, it's an SVGAnimatedString in SVGURIReference.idl. Got a bit fooled by
SVGURIReference::href() returning SVGAnimatedHref* instead of
SVGAnimatedString*, but that's perhaps customary. Bindings still only see
SVGAnimatedString, as I understand it.

[fs, 2016-02-11 09:48:19]

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

https://codereview.chromium.org/1681553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedString.h:47: virtual String
baseVal();
On 2016/02/11 at 07:37:08, David Vest wrote:
> On 2016/02/10 19:31:14, fs wrote:
> > On 2016/02/10 at 17:47:39, David Vest wrote:
> > > Do these three need to be virtual/still exist? As I understand it they
were
> > added in https://codereview.chromium.org/1086873004 for counting. Now the
> > counting moves to SVGAnimatedHref, but these three are still around doing
> > nothing but delegate to the base class?
> > 
> > Since we call these through a SVGAnimatedString* these need to be virtual
for
> > the dispatch to reach the SVGAnimatedHref overrides when using
> > SVGURIReference.href. (These are not virtual in the base/ancestor class.)
> 
> Of course, it's an SVGAnimatedString in SVGURIReference.idl. Got a bit fooled
by SVGURIReference::href() returning SVGAnimatedHref* instead of
SVGAnimatedString*, but that's perhaps customary. Bindings still only see
SVGAnimatedString, as I understand it.

The reason for having SVGURIReference::href() return SVGAnimatedHref* is
actually to avoid having to make more accessors virtual (most prominently
currentValue.)

[davve, 2016-02-11 10:12:21]

Reading through this closer I'm starting to feel confident this is more or less
as simple as possible given how the spec is written and how complex the problem
space is.

lgtm but pdr should have a say too, I think.

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

https://codereview.chromium.org/1681553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGAnimatedString.h:47: virtual String
baseVal();
On 2016/02/11 09:48:18, fs wrote:

> The reason for having SVGURIReference::href() return SVGAnimatedHref* is
> actually to avoid having to make more accessors virtual (most prominently
> currentValue.)

Thanks for explaining.

[fs, 2016-02-11 13:35:12]

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/1681553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp:242:
AtomicString href = getAttribute(SVGNames::hrefAttr);
On 2016/02/10 at 14:44:46, fs wrote:
> On 2016/02/09 at 23:17:07, pdr wrote:
> > Can we use Element::hrefURL here?
> 
> We probably could (considering SVGSMILElements are never layouted etc.) - I
don't think it would be without peril though. And then there's the issue of
String vs. KURL (which hrefURL returns), so there'd need to be some impedance
matching here. I'd be happy to try and clean this up in a follow-up though
(never really been a fan of the somewhat ad-hoc looking code in
SVGURIReference.)

I did an attempt at "hiding" the details a bit. Not a huge success IMHO. See
latest PS.

[pdr., 2016-02-12 03:03:36]

LGTM here too. Somewhat unfortunate constraints but I couldn't find a better
approach.

https://codereview.chromium.org/1681553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/1681553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp:291: //
'xlink:href' should map to 'href' for animation purposes. (It's possible that I
just dreamt this.)
Nit: Can you shoot a quick email to www-svg to make this true and remove the
parenthetical?

[davve, 2016-02-12 07:26:14]

https://codereview.chromium.org/1681553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/1681553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp:291: //
'xlink:href' should map to 'href' for animation purposes. (It's possible that I
just dreamt this.)
On 2016/02/12 03:03:35, pdr wrote:
> Nit: Can you shoot a quick email to www-svg to make this true and remove the
> parenthetical?

I think fs may be aware of this already but there is "Animation elements treat
attributeName='xlink:href' as being an alias for targetting the ‘href’
attribute." in a note (so non-normative?) in
https://svgwg.org/svg2-draft/single-page.html#types-__svg__SVGURIReference__href

[fs, 2016-02-12 11:49:58]

https://codereview.chromium.org/1681553002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/1681553002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp:291: //
'xlink:href' should map to 'href' for animation purposes. (It's possible that I
just dreamt this.)
On 2016/02/12 at 07:26:14, David Vest wrote:
> On 2016/02/12 03:03:35, pdr wrote:
> > Nit: Can you shoot a quick email to www-svg to make this true and remove the
> > parenthetical?
> 
> I think fs may be aware of this already but there is "Animation elements treat
attributeName='xlink:href' as being an alias for targetting the ‘href’
attribute." in a note (so non-normative?) in
https://svgwg.org/svg2-draft/single-page.html#types-__svg__SVGURIReference__href

Ah, there's where it was - I knew I had read it somewhere in the spec - thanks!
I'll replace the "parenthetical" with a spec-ref. (The one above or an
equivalent one.)

[fs, 2016-02-12 16:08:39]

The CQ bit was checked by fs@opera.com

[fs, 2016-02-12 16:08:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org, davve@opera.com, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1681553002/#ps100001
(title: "Rebase; update comment")

[commit-bot: I haz the power, 2016-02-12 16:09:15]

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

[commit-bot: I haz the power, 2016-02-12 16:15:18]

Committed patchset #6 (id:100001)

[carlosk, 2016-02-12 16:47:14]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1692673004/ by carlosk@chromium.org.

The reason for reverting is: This CL seems to be breaking the compilation step
of a lot of Android bots. Reverting..

[commit-bot: I haz the power, 2016-02-16 22:43:28]

Description was changed from

==========
Add support for 'href' (w/o XLink NS) for various SVG elements

This CL adds a new wrapper type SVGAnimatedHref that wraps 'href' (the
new, null/default NS version) and '(xlink:)href' (the XLink NS version).
This wrapper type is used by SVGURIReference and thus most uses of
'href' is covered by this part. SVGAnimatedHref is intended as a wrapper
for accessing the underlying value of either 'href' or 'xlink:href'. Any
updates due to setAttribute or will go directly to the underlying values
which means that synchronization etc. does not apply to the wrapper.
This is one of the "pro"s of this approach - it does not require any
modification to synching between the SVGString object and the attribute
in certain cases (like when the "active" attribute is removed). A "con"
would be that there's a lot of dead space in the object since the
wrapper itself still needs to implement the SVGAnimatedString interface
even if just forwards to the actual SVGString.
The UseCounter for href.baseVal/animVal are moved to the new wrapper.

For cases where SVGURIReference is not used, new code-paths are added to
select the right attribute.

Animation code is updated to target the 'href' in the null/default NS.

BUG=584142
==========

to

==========
Add support for 'href' (w/o XLink NS) for various SVG elements

This CL adds a new wrapper type SVGAnimatedHref that wraps 'href' (the
new, null/default NS version) and '(xlink:)href' (the XLink NS version).
This wrapper type is used by SVGURIReference and thus most uses of
'href' is covered by this part. SVGAnimatedHref is intended as a wrapper
for accessing the underlying value of either 'href' or 'xlink:href'. Any
updates due to setAttribute or will go directly to the underlying values
which means that synchronization etc. does not apply to the wrapper.
This is one of the "pro"s of this approach - it does not require any
modification to synching between the SVGString object and the attribute
in certain cases (like when the "active" attribute is removed). A "con"
would be that there's a lot of dead space in the object since the
wrapper itself still needs to implement the SVGAnimatedString interface
even if just forwards to the actual SVGString.
The UseCounter for href.baseVal/animVal are moved to the new wrapper.

For cases where SVGURIReference is not used, new code-paths are added to
select the right attribute.

Animation code is updated to target the 'href' in the null/default NS.

BUG=584142

Committed: https://crrev.com/2f0b655515af2df03d41d4f894e0dd59d5cb6411
Cr-Commit-Position: refs/heads/master@{#375192}
==========

[commit-bot: I haz the power, 2016-02-16 22:43:29]

Patchset 6 (id:??) landed as
https://crrev.com/2f0b655515af2df03d41d4f894e0dd59d5cb6411
Cr-Commit-Position: refs/heads/master@{#375192}