Bug 214690

Created attachment 405052 [details]
Patch

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.

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

Created attachment 405058 [details]
Patch

Created attachment 405062 [details]
Patch

Created attachment 405063 [details]
Patch

Created attachment 405065 [details]
Patch

Created attachment 405069 [details]
Patch

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

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

<rdar://problem/66011698>