Use PropertyRegistry consistently in svgAttributeChanged.
Created attachment 454118 [details] Patch
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* ..
Created attachment 454244 [details] Patch
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.
(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 :-)
Created attachment 454327 [details] Patch
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.
Created attachment 454332 [details] Patch
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].
<rdar://problem/90098010>