Bug 214690 – Add support for SVGAElement's rel / relList attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Chris Dumez]

Add support for SVGAElement's rel / relList attributes:
https://www.w3.org/TR/SVG/linking.html#InterfaceSVGAElement

[Chris Dumez]

Created attachment 405052 [details]
Patch

[Darin Adler]

Comment on attachment 405052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405052&action=review

> Source/WebCore/svg/SVGAElement.cpp:86
> +        return;

Wish I understood whether we should call to base or return in cases like this. I suppose return is a slight optimization, whereas calling through to base is a little more consistent and allows for each level to implement part of attribute handling, which could be handy in some cases.

> Source/WebCore/svg/SVGAElement.cpp:231
> +        m_relList = makeUnique<DOMTokenList>(const_cast<SVGAElement&>(*this), SVGNames::relAttr, [](Document&, StringView token) {

This const_cast makes me wonder about ever using const for functions in these element classes. Is there anything meaningful to the use of const in the class definition? I understand that conceptually it does not change the state of the element. But also, making a DOMTokenList doesn’t seem to change the element either, so why does it take a non-const reference. Except for the fact that everything takes non-const references to classes like Node and Element.

> Source/WebCore/svg/SVGAElement.cpp:236
> +#if USE(SYSTEM_PREVIEW)
> +            return equalIgnoringASCIICase(token, "noreferrer") || equalIgnoringASCIICase(token, "noopener") || equalIgnoringASCIICase(token, "ar");
> +#else
> +            return equalIgnoringASCIICase(token, "noreferrer") || equalIgnoringASCIICase(token, "noopener");
> +#endif

Is there a less repetitive way to write this? Maybe:

#if USE(SYSTEM_PREVIEW)
    if (equalIgnoringASCIICase(token, "ar"))
        return true;
#endif

Then the rest doesn’t have to be inside #if.

> Source/WebCore/svg/SVGAElement.idl:30
> +    [SameObject, PutForwards=value] readonly attribute DOMTokenList relList;

Not sure I understand the meaning of PutForwards=value, but whatever it is, glad the generator handles it.

[Chris Dumez]

Comment on attachment 405052 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405052&action=review

>> Source/WebCore/svg/SVGAElement.cpp:86
>> +        return;
> 
> Wish I understood whether we should call to base or return in cases like this. I suppose return is a slight optimization, whereas calling through to base is a little more consistent and allows for each level to implement part of attribute handling, which could be handy in some cases.

Yes, this is a slight optimization and seems to be the existing pattern in parseAttribute() functions.

>> Source/WebCore/svg/SVGAElement.cpp:231
>> +        m_relList = makeUnique<DOMTokenList>(const_cast<SVGAElement&>(*this), SVGNames::relAttr, [](Document&, StringView token) {
> 
> This const_cast makes me wonder about ever using const for functions in these element classes. Is there anything meaningful to the use of const in the class definition? I understand that conceptually it does not change the state of the element. But also, making a DOMTokenList doesn’t seem to change the element either, so why does it take a non-const reference. Except for the fact that everything takes non-const references to classes like Node and Element.

While constructing a DOMTokenList does not modify the element, the JS can modify an element (the element's attribute) via the DOMTokenList. The DOMTokenList needs to be able to update attributes on the element, which is why it takes in a non-const reference. I guess I should just drop the const on relList() function.

>> Source/WebCore/svg/SVGAElement.cpp:236
>> +#endif
> 
> Is there a less repetitive way to write this? Maybe:
> 
> #if USE(SYSTEM_PREVIEW)
>     if (equalIgnoringASCIICase(token, "ar"))
>         return true;
> #endif
> 
> Then the rest doesn’t have to be inside #if.

Ok.

>> Source/WebCore/svg/SVGAElement.idl:30
>> +    [SameObject, PutForwards=value] readonly attribute DOMTokenList relList;
> 
> Not sure I understand the meaning of PutForwards=value, but whatever it is, glad the generator handles it.

causes this:
a.relList = "foo"
to be equivalent to:
a.relList.value = "foo"

The most popular use is likely for window.location which actually sets window.location.href.

https://heycam.github.io/webidl/#PutForwards

[Chris Dumez]

Created attachment 405058 [details]
Patch

[Chris Dumez]

Created attachment 405062 [details]
Patch

[Chris Dumez]

Created attachment 405063 [details]
Patch

[Chris Dumez]

Created attachment 405065 [details]
Patch

[Chris Dumez]

Created attachment 405069 [details]
Patch

[EWS]

Committed r264789: <https://trac.webkit.org/changeset/264789>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405069 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66011698>