[webkit-reviews] review granted: [Bug 136900] Use fastHasAttribute() / fastGetAttribute() when possible : [Attachment 238272] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 136900: Use fastHasAttribute() / fastGetAttribute() when possible
https://bugs.webkit.org/show_bug.cgi?id=136900

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.