[webkit-reviews] review granted: [Bug 110025] Element: Avoid unrelated attribute synchronization on other attribute access. : [Attachment 188725] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 110025: Element: Avoid unrelated attribute synchronization on other
attribute access.
https://bugs.webkit.org/show_bug.cgi?id=110025

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

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


I like the basic idea here, although I’m not sure that all the changes here are
improvements.

> Source/WebCore/dom/Element.cpp:351
> +void Element::synchronizeAttributeIfNeeded(const QualifiedName& name) const

I don’t think “if needed” is needed in this name.

> Source/WebCore/dom/Element.cpp:367
> +void Element::synchronizeAttributeIfNeeded(const AtomicString& localName)
const

The whole point of this function is to do the synchronizeAttribute operation
without creating a QualifiedName. I think it needs a comment to explain that.

Do we want part of this inlined for getAttribute?

> Source/WebCore/dom/Element.cpp:769
> +    const AtomicString& caseAdjustedLocalName =
shouldIgnoreAttributeCase(this) ? localName.lower() : localName;

This does a lot of unneeded work when the name is already all lowercase.

I looked at AtomicString::lower and it’s super-funny! It has a comment saying
that it’s a hot function, but does not have either of the two obvious
optimizations:

1) Fast case for when the string is already all lower case.
2) Avoid allocating and deleting an extra StringImpl in the common case where
the lower case version of the string is already in the atomic string table.

> Source/WebCore/dom/Element.cpp:1853
> +    if (const Attribute* attribute =
elementData()->getAttributeItem(localName, shouldIgnoreAttributeCase(this)))
> +	   return ensureAttr(attribute->name());
> +    return 0;

I like the old style with early return for the error case better than this,
even though this scopes the local variable more tightly.

> Source/WebCore/dom/Element.cpp:1864
> +    if (const Attribute* attribute = elementData()->getAttributeItem(qName))

> +	   return ensureAttr(attribute->name());
> +    return 0;

Same here.