Bug 77674 – Move attribute storage from NamedNodeMap to ElementAttributeData

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caio Marcelo de Oliveira Filho]

Move attribute storage from NamedNodeMap to ElementAttributeData

[Caio Marcelo de Oliveira Filho]

Created attachment 125170 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125170 [details]
Patch

Attachment 125170 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11389972

[Caio Marcelo de Oliveira Filho]

Created attachment 125316 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 125316 [details]
Patch

After talking with rniwa and kling on IRC, we concluded this patch is too big.

So the plan is to first remove the noise from it by making many of the clients of NamedNodeMap to interact with Element itself. IOW, expose a way to get Attribute* from Element. Then this patch for moving attribute storage will be smaller.

[Caio Marcelo de Oliveira Filho]

Created attachment 126495 [details]
Patch

[Caio Marcelo de Oliveira Filho]

rniwa, as discussed I made this patch smaller by taking care of the noise in another patch. Compared to the previous one, I let two more things for separate changes: implementation of DOM methods (because I think they can move to Element itself, even make the code less redundant in some cases) and the parsing code that interacts with NamedNodeMap.

Note that the first of those separate changes will "resolve" kling's issues with ExceptionCode appearing in ElementAttributeData in the previous patch.

Could you take a look when you have time?

[Andreas Kling]

Comment on attachment 126495 [details]
Patch

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.

[Andreas Kling]

(In reply to comment #7)
> (From update of attachment 126495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126495&action=review
> 
> As suggested by Darin, we should rename ElementAttributeData to ElementAttributes at some point.

Actually, let's hold off on that as it clashes with the NamedNodeMap* Node::attributes() we expose to the web.

[Caio Marcelo de Oliveira Filho]

Committed r107570: <http://trac.webkit.org/changeset/107570>