[webkit-reviews] review granted: [Bug 237604] Use PropertyRegistry consistently in svgAttributeChanged : [Attachment 454327] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Rob Buis
<rbuis at igalia.com>'s request for review:
Bug 237604: Use PropertyRegistry consistently in svgAttributeChanged
https://bugs.webkit.org/show_bug.cgi?id=237604

Attachment 454327: Patch

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




--- Comment #7 from Martin Robinson <mrobinson at webkit.org> ---
Comment on attachment 454327
  --> https://bugs.webkit.org/attachment.cgi?id=454327
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.