[webkit-reviews] review denied: [Bug 110926] Can't use fastGetAttribute with WebCore::HTMLNames::classAttr when element is a SVGStyledElement : [Attachment 191566] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Tim 'mithro' Ansell
<mithro at mithis.com>'s request for review:
Bug 110926: Can't use fastGetAttribute with WebCore::HTMLNames::classAttr when
element is a SVGStyledElement
https://bugs.webkit.org/show_bug.cgi?id=110926

Attachment 191566: Patch
https://bugs.webkit.org/attachment.cgi?id=191566&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=191566&action=review


review- because of the hasClass thing -- I’m also pretty sure we don’t need a
new getClassAttribute function

> Source/WebCore/dom/Element.h:830
> +inline const AtomicString& Element::getClassAttribute() const
> +{
> +#if ENABLE(SVG)
> +    if (isSVGElement())
> +	   return getAttribute(HTMLNames::classAttr);
> +#endif
> +    if (!hasClass())
> +	   return nullAtom;
> +    return fastGetAttribute(HTMLNames::classAttr);
> +}

The consensus in the comments seems to be that the hasClass check is not a
worthwhile optimization.

If we do want it, then I believe the hasClass check can go before the SVG
element check.

Is this measurably more efficient than calling getAttribute(classAttr)? I
suspect it is not. And if not, then we should have the code generator call
getAttribute and not add a new getClassAttribute function.

If this is, then we should consider doing this for every attribute other than
styleAttr. The only thing that is specific to the class attribute here is the
hasClass function.

Otherwise this looks pretty good to me.