[webkit-reviews] review granted: [Bug 67394] Use fastGetAttribute() and fastHasAttribute() where appropriate. : [Attachment 105974] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 67394: Use fastGetAttribute() and fastHasAttribute() where appropriate.
https://bugs.webkit.org/show_bug.cgi?id=67394

Attachment 105974: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=105974&action=review

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


> Source/WebCore/ChangeLog:10
> +	   Change call sites that don't check the "style" or SVG animatable
> +	   attributes to use fastGetAttribute()/fastHasAttribute() instead
> +	   of getAttribute()/hasAttribute().

How is that you know none of these are SVG animatable attributes? That’s one
thing that held me back in the past. I wasn’t sure which attributes could be
animated by SVG.

>> Source/WebCore/dom/Document.cpp:632
>> +	    const AtomicString& accessKey =
element->fastGetAttribute(accesskeyAttr);
> 
> would not this be simpler if the check was done internally to
::getAttribute() instead of changing all call sites?

As Andreas implied in the change log, the getAttribute function is visible to
the public DOM and has to handle the style attribute properly and animated SVG
attributes properly.

We can use fastGetAttribute at call sites where we know the attribute is not
styleAttr and where we know animated SVG attributes can’t possibly be involved.
That’s why the call sites need to change.

> Source/WebCore/dom/Element.cpp:2006
> +    const AtomicString& value = fastGetAttribute(HTMLNames::spellcheckAttr);


Not sure why this has the HTMLNames prefix; other uses in this file don’t have
it.