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.
Created attachment 238272 [details] Patch
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.
Created attachment 238311 [details] Patch
I made all the updates except for those in HTMLScriptElement because those methods are virtual and actually return String types for SVG overrides.
Comment on attachment 238311 [details] Patch Clearing flags on attachment: 238311 Committed r173724: <http://trac.webkit.org/changeset/173724>
All reviewed patches have been landed. Closing bug.
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.