Bug 77574 – Avoid creating NamedNodeMap unnecessarily

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

[Caio Marcelo de Oliveira Filho]

Avoid creating NamedNodeMap unnecessarily

[Caio Marcelo de Oliveira Filho]

This is related to work I'm doing for bug 75069.

[Caio Marcelo de Oliveira Filho]

Created attachment 125007 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 125007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125007&action=review

> Source/WebCore/dom/Element.h:220
> +    NamedNodeMap* ensureAttributes() const;

I'd call this one ensureUpdatedAttributes() to be consistent.

> Source/WebCore/dom/Node.cpp:1836
> -                NamedNodeMap* attrs = elem->attributes();
> -                
> +                NamedNodeMap* attrs = elem->attributeMap();
> +

It seems dangerous to rely on the fact hasAttributes have updated the attributes. Given that updateInvalidAttributes should be fast when there are no updates required, I'd prefer leaving as is (or replace it with updatedAttributes).

> Source/WebCore/dom/Node.cpp:1923
> -                NamedNodeMap *attrs = elem->attributes();
> +                NamedNodeMap* attrs = elem->attributeMap();

Ditto.

> Source/WebCore/dom/Node.cpp:1979
> +        if (NamedNodeMap* attrs = toElement(this)->attributeMap()) {

Ditto.

> Source/WebCore/editing/CompositeEditCommand.cpp:316
> +    if (!node->isElementNode())
> +        return true;
> +
> +    const NamedNodeMap* attributeMap = toElement(node)->updatedAttributes();
>      if (!attributeMap || attributeMap->isEmpty())
>          return true;

We can just call hasAttributes here.

> Source/WebCore/editing/htmlediting.cpp:1168
> +    // Neither element have attributes, so they are the same.

I don't think we need this comment.

> Source/WebCore/xml/XPathFunctions.cpp:595
> +        if (node->isElementNode() && toElement(node)->hasAttributes())
> +            languageAttribute = toElement(node)->attributeMap()->getAttributeItem(XMLNames::langAttr);

You're adding unnecessary updates of style and SVG animation attributes implicitly in hasAttributes. I don't think we should do this.

[Ryosuke Niwa]

In general, this refactoring looks great. It clarifies semantics in many call sites of attributes.

[WebKit Review Bot]

Comment on attachment 125007 [details]
Patch

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

[Caio Marcelo de Oliveira Filho]

Comment on attachment 125007 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125007&action=review

>> Source/WebCore/dom/Element.h:220
>> +    NamedNodeMap* ensureAttributes() const;
> 
> I'd call this one ensureUpdatedAttributes() to be consistent.

Fixed.

>> Source/WebCore/dom/Node.cpp:1836
>> +
> 
> It seems dangerous to rely on the fact hasAttributes have updated the attributes. Given that updateInvalidAttributes should be fast when there are no updates required, I'd prefer leaving as is (or replace it with updatedAttributes).

I agree. I changed this pattern to "if (NamedNodeMap* attrs = elem->updatedAttributes())". If is empty we will enter the conditional but not run any iteration in the loop inside.

>> Source/WebCore/editing/CompositeEditCommand.cpp:316
>>          return true;
> 
> We can just call hasAttributes here.

Of course! :-) Fixed.

>> Source/WebCore/editing/htmlediting.cpp:1168
>> +    // Neither element have attributes, so they are the same.
> 
> I don't think we need this comment.

Fixed.

>> Source/WebCore/xml/XPathFunctions.cpp:595
>> +            languageAttribute = toElement(node)->attributeMap()->getAttributeItem(XMLNames::langAttr);
> 
> You're adding unnecessary updates of style and SVG animation attributes implicitly in hasAttributes. I don't think we should do this.

No. Previous call to node->attributes() (now known as ensureUpdatedAttributes()) would update style and SVG animation attributes.

I also rewrote the code to avoid using hasAttribute()/attributeMap().

[Caio Marcelo de Oliveira Filho]

Created attachment 125030 [details]
Patch

[Ryosuke Niwa]

(In reply to comment #5)
> (From update of attachment 125007 [details])
> Attachment 125007 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11396581

It appears that you have to modify Source/WebKit/chromium/src/WebPageSerializerImpl.cpp

[Caio Marcelo de Oliveira Filho]

Created attachment 125033 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125033 [details]
Patch

Clearing flags on attachment: 125033

Committed r106515: <http://trac.webkit.org/changeset/106515>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.