Bug 136900 – Use fastHasAttribute() / fastGetAttribute() when possible

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Use fastHasAttribute() / fastGetAttribute() when possible, that is when the attribute is not SVG-animated or the |style| attribute, to avoid synchronizing attributes unnecessarily.
We should also avoid calling hasAttribute(xxxAttr) then getAttribute(xxxAttr) and it causes 2 linear searches. It is best to call getAttribute(xxxAttr) directly and check if it returns the nullAtom.

[Chris Dumez]

Created attachment 238272 [details]
Patch

[Benjamin Poulain]

Comment on attachment 238272 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1386
> -    if (!element->hasAttribute(contenteditableAttr))
> +    const AtomicString& contentEditableValue = element->fastGetAttribute(contenteditableAttr);
> +    if (contentEditableValue.isNull())
>          return false;
>      
> -    const AtomicString& contentEditableValue = element->fastGetAttribute(contenteditableAttr);
>      // Both "true" (case-insensitive) and the empty string count as true.
>      return contentEditableValue.isEmpty() || equalIgnoringCase(contentEditableValue, "true");

Nothing to do with your patch, but this code looks very fishy. I am not convinced it is correct.

> Source/WebCore/html/HTMLAppletElement.cpp:132
>          paramNames.append("codeBase");

ASCIILiteral() missing here.

> Source/WebCore/html/HTMLAppletElement.cpp:144
>          paramNames.append("archive");

ditto.

> Source/WebCore/html/HTMLAppletElement.cpp:153
>          paramNames.append("mayScript");

ditto.

> Source/WebCore/html/HTMLButtonElement.cpp:197
>  String HTMLButtonElement::value() const

Maybe change the return type to const AtomicString& ?

> Source/WebCore/html/HTMLDocument.cpp:113
>      HTMLElement* b = body();
>      if (!b)
>          return String();
> -    return b->getAttribute(dirAttr);
> +    return b->fastGetAttribute(dirAttr);

Let's fix this:
    if (HTMLElement* body = this->body())
        return body->fastXXX
    return String();

> Source/WebCore/html/HTMLImageElement.cpp:209
>      // also heavily discussed by Hixie on bugzilla
> -    String alt = getAttribute(altAttr);
> +    String alt = fastGetAttribute(altAttr);
>      // fall back to title attribute
>      if (alt.isNull())
> -        alt = getAttribute(titleAttr);
> +        alt = fastGetAttribute(titleAttr);
>      return alt;

Fix everything to const AtomicString&?

> Source/WebCore/html/HTMLScriptElement.cpp:118
>  String HTMLScriptElement::sourceAttributeValue() const

Change return type?

> Source/WebCore/html/HTMLScriptElement.cpp:123
>  String HTMLScriptElement::charsetAttributeValue() const

ditto?

> Source/WebCore/html/HTMLScriptElement.cpp:133
>  String HTMLScriptElement::languageAttributeValue() const

ditto?

> Source/WebCore/html/HTMLScriptElement.cpp:138
>  String HTMLScriptElement::forAttributeValue() const

ditto?

> Source/WebCore/html/HTMLScriptElement.cpp:143
>  String HTMLScriptElement::eventAttributeValue() const

ditto?

> Source/WebCore/html/HTMLTableElement.cpp:563
>  String HTMLTableElement::rules() const

ditto?

> Source/WebCore/html/HTMLTableElement.cpp:568
>  String HTMLTableElement::summary() const

ditto?

> Source/WebCore/html/HTMLTableSectionElement.cpp:108
>  String HTMLTableSectionElement::align() const

ditto?

> Source/WebCore/html/HTMLTableSectionElement.cpp:118
>  String HTMLTableSectionElement::ch() const

ditto?

> Source/WebCore/html/HTMLTableSectionElement.cpp:138
>  String HTMLTableSectionElement::vAlign() const

ditto?

> Source/WebCore/loader/FormSubmission.cpp:157
>          String attributeValue;

Should be AtomicString.

[Chris Dumez]

Created attachment 238311 [details]
Patch

[Chris Dumez]

I made all the updates except for those in HTMLScriptElement because those methods are virtual and actually return String types for SVG overrides.

[WebKit Commit Bot]

Comment on attachment 238311 [details]
Patch

Clearing flags on attachment: 238311

Committed r173724: <http://trac.webkit.org/changeset/173724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 238272 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1383
> +    if (contentEditableValue.isNull())
>          return false;

The code change is fine, but the logic here is wrong. Looking at HTMLElement::collectStyleForPresentationAttribute, it seems that null means we inherit the attribute from the parent, and does not mean "false", unlike "false".

This code is missing proper handling for "plaintext-only", treating it as false, and it also incorrectly treats all other unknown strings the same as "false", while the real code treats them the same as a missing attribute.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:1386
>>      return contentEditableValue.isEmpty() || equalIgnoringCase(contentEditableValue, "true");
> 
> Nothing to do with your patch, but this code looks very fishy. I am not convinced it is correct.

This part matches the code in HTMLElement::collectStyleForPresentationAttribute, so it’s at least consistent with the actual WebKit behavior.

> Source/WebCore/editing/EditingStyle.cpp:282
>          return 0;

Missed a chance to make this nullptr.

> Source/WebCore/editing/EditingStyle.cpp:307
>          return 0;

Missed a chance to make this nullptr.

> Source/WebCore/editing/EditingStyle.cpp:310
>          return 0;

Missed a chance to make this nullptr.

> Source/WebCore/editing/SplitElementCommand.cpp:94
> +    const AtomicString& id = m_element1->fastGetAttribute(HTMLNames::idAttr);

I think this should be using getIdAttribute, although that’s just a tiny optimization for when there is no value. But since we have it, I think we should use it.

> Source/WebCore/editing/SplitElementCommand.cpp:96
> +        m_element2->setAttribute(HTMLNames::idAttr, id);

I think this should be using setIdAttribute, although it compiles to the same code that’s here.

>> Source/WebCore/html/HTMLDocument.cpp:113
>> +    return b->fastGetAttribute(dirAttr);
> 
> Let's fix this:
>     if (HTMLElement* body = this->body())
>         return body->fastXXX
>     return String();

Variable name fix would be good, but is it really good to reverse it just to scope the variable? I like the early return version better, even though it's one line longer, because the main flow of the function stays on the left inside of flowing into an if statement.

> Source/WebCore/page/Frame.cpp:479
> +    return matchLabelsAgainstString(labels, element->fastGetAttribute(idAttr));

Can use getIdAttribute.

> Source/WebCore/rendering/RenderTableCell.cpp:182
> +        String nowrap = element()->fastGetAttribute(nowrapAttr);
>          if (!nowrap.isNull() && w.isFixed())

Should be fastHasAttribute; nothing ever uses the return value except to check for null. Also seems we should put that into the if statement above rather than calling styleOrColLogicalWidth even when it's not used. Maybe switch style to early return, not sure since this is only part of the function’s job.