[webkit-reviews] review granted: [Bug 77674] Move attribute storage from NamedNodeMap to ElementAttributeData : [Attachment 126495] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Caio Marcelo de Oliveira Filho
<cmarcelo at webkit.org>'s request for review:
Bug 77674: Move attribute storage from NamedNodeMap to ElementAttributeData
https://bugs.webkit.org/show_bug.cgi?id=77674

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126495&action=review


r=me

As suggested by Darin, we should rename ElementAttributeData to
ElementAttributes at some point.

> Source/WebCore/dom/Attr.h:43
>      friend class NamedNodeMap;
> +    friend class ElementAttributeData;

Nit: Alphabetical order.

> Source/WebCore/dom/Element.h:242
> +    ElementAttributeData* ensureAttributeData() const;
> +    ElementAttributeData* updatedAttributeData() const;
> +    ElementAttributeData* ensureUpdatedAttributeData() const;

As discussed on IRC, we have way too many of these.
It should be relatively easy to remove the need for
ensureUpdatedAttributeData(), though not in the scope of this patch.

> Source/WebCore/dom/ElementAttributeData.cpp:170
> +void ElementAttributeData::clearAttributes()
> +{
> +    clearClass();
> +    detachAttributesFromElement();
> +    m_attributes.clear();
> +}

This method doesn't clear idForStyleResolution!
I know you are just moving code around, so it's fine for this patch, but we
need to fix this.

> Source/WebCore/dom/ElementAttributeData.cpp:172
> +void ElementAttributeData::replaceAttribute(size_t index,
PassRefPtr<Attribute> prpAttribute, WebCore::Element* element)

No need for the WebCore:: prefix.

> Source/WebCore/dom/ElementAttributeData.h:57
> +    bool isEmpty() const { return !length(); }

return m_attributes.isEmpty(); seems more straightforward.