Bug 237604 – Use PropertyRegistry consistently in svgAttributeChanged

• Component: SVG
• Status: RESOLVED FIXED

[Rob Buis]

Use PropertyRegistry consistently in svgAttributeChanged.

[Rob Buis]

Created attachment 454118 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 454118 [details]
Patch

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

Unofficial review: Looks good, just a few comments.

> Source/WebCore/ChangeLog:8
> +        Use PropertyRegistry consistently in svgAttributeChanged.

This is a bit too thin, I'd state that most svgAttributeChanged() methods start with if (PropReg::isKnownAttr(...)) and some are missing, and that you're unifying this for consistency reasons.

> Source/WebCore/svg/SVGFEBlendElement.cpp:86
> +    if (PropertyRegistry::isKnownAttribute(attrName)) {

out of curiosity, did you check that each of the isKnownAttribute() functions has exactly the same attributes registered as we atually handle in svgAttributeChanged().
Could be that we missed something in the past, that's now fixed, by this usage pattern (no explicit check for all the attribute names, but instead use isKnownAttribute()).

> Source/WebCore/svg/SVGFEComponentTransferElement.cpp:65
> +void SVGFEComponentTransferElement::svgAttributeChanged(const QualifiedName& attrName)

This needs a test :/

> Source/WebCore/svg/SVGImageElement.cpp:107
> +        else {

else if (auto* ..

[Rob Buis]

Created attachment 454244 [details]
Patch

[Rob Buis]

Comment on attachment 454118 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Use PropertyRegistry consistently in svgAttributeChanged.
> 
> This is a bit too thin, I'd state that most svgAttributeChanged() methods start with if (PropReg::isKnownAttr(...)) and some are missing, and that you're unifying this for consistency reasons.

Done.

>> Source/WebCore/svg/SVGFEBlendElement.cpp:86
>> +    if (PropertyRegistry::isKnownAttribute(attrName)) {
> 
> out of curiosity, did you check that each of the isKnownAttribute() functions has exactly the same attributes registered as we atually handle in svgAttributeChanged().
> Could be that we missed something in the past, that's now fixed, by this usage pattern (no explicit check for all the attribute names, but instead use isKnownAttribute()).

I did by cross checking all changed files. One thing I missed though is that SVGFEDiffuseLightingElement::svgAttributeChanged incorrectly checked for SVGNames::lighting_colorAttr!

>> Source/WebCore/svg/SVGFEComponentTransferElement.cpp:65
>> +void SVGFEComponentTransferElement::svgAttributeChanged(const QualifiedName& attrName)
> 
> This needs a test :/

Ok I removed this part since it is not high priority at all.

>> Source/WebCore/svg/SVGImageElement.cpp:107
>> +        else {
> 
> else if (auto* ..

Done.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #4)
> > This needs a test :/
> 
> Ok I removed this part since it is not high priority at all.

Can you still create a ticket, add a FIXME that the svgAttributeChanged() implementation is missing with a ref to the new ticket?

Other than that, the last iteration looks great, unofficial r=me :-)

[Rob Buis]

Created attachment 454327 [details]
Patch

[Martin Robinson]

Comment on attachment 454327 [details]
Patch

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

Looks good to me, assuming that PropertyRegistry::isKnownAttribute works knowing the calling class. I would also add ASSERTs in the places where the code is making assumptions about what the remaining attributes to handle are.

> Source/WebCore/svg/SVGFEBlendElement.cpp:94
> +    if (PropertyRegistry::isKnownAttribute(attrName)) {
>          InstanceInvalidationGuard guard(*this);
> -        primitiveAttributeChanged(attrName);
> -        return;
> -    }
> -
> -    if (attrName == SVGNames::inAttr || attrName == SVGNames::in2Attr) {
> -        InstanceInvalidationGuard guard(*this);
> -        invalidate();
> +        if (attrName == SVGNames::modeAttr)
> +            primitiveAttributeChanged(attrName);
> +        else
> +            invalidate();
>          return;
>      }
>  

I believe in all the cases where the code assumes that a particular attribute is being handled in an else or a default code path, an assertion will make things a lot more understandable for the reader. Before it was easier to see what behavior corresponded to what attribute and the assertion will help to maintain the same readability. So here you could ASSERT(attrName == SVGNames::inAttr || attrName == SVGNames::in2Attr) in the else path.

> Source/WebCore/svg/SVGPathElement.cpp:69
> -    if (attrName == SVGNames::dAttr) {
> +    if (PropertyRegistry::isKnownAttribute(attrName)) {
>          InstanceInvalidationGuard guard(*this);

I think in these cases as well, an assertion could make the code a bit more understandable.

[Rob Buis]

Created attachment 454332 [details]
Patch

[EWS]

Committed r291108 (248270@main): <https://commits.webkit.org/248270@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/90098010>