Avoid creating NamedNodeMap unnecessarily
This is related to work I'm doing for bug 75069.
Created attachment 125007 [details] Patch
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.
In general, this refactoring looks great. It clarifies semantics in many call sites of attributes.
Comment on attachment 125007 [details] Patch Attachment 125007 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11396581
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().
Created attachment 125030 [details] Patch
(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
Created attachment 125033 [details] Patch
Comment on attachment 125033 [details] Patch Clearing flags on attachment: 125033 Committed r106515: <http://trac.webkit.org/changeset/106515>
All reviewed patches have been landed. Closing bug.