[webkit-reviews] review denied: [Bug 73853] Inserting nodes is slow due to Node::notifyNodeListsAttributeChanged (20%+) : [Attachment 120253] kling's approach

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 73853: Inserting nodes is slow due to Node::notifyNodeListsAttributeChanged
(20%+)
https://bugs.webkit.org/show_bug.cgi?id=73853

Attachment 120253: kling's approach
https://bugs.webkit.org/attachment.cgi?id=120253&action=review

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


> Source/WebCore/dom/ChildNodeList.cpp:39
> +    if (m_caches->m_isLengthCacheValid)
> +	   return m_caches->m_length;

It’s strange to call this “length cache” in one case, and “length” in another.
I’d like to see these two data members use the same terminology.

> Source/WebCore/dom/DynamicNodeList.h:48
> -	   unsigned cachedLength;
> -	   Node* lastItem;
> -	   unsigned lastItemOffset;
> -	   bool isLengthCacheValid : 1;
> -	   bool isItemCacheValid : 1;
> +
> +	   unsigned m_length;
> +	   Node* m_lastItem;
> +	   unsigned m_lastItemOffset;
> +	   bool m_isLengthCacheValid : 1;
> +	   bool m_isItemCacheValid : 1;

I don’t like this change: Given that these are public data members, they should
not have the m_ prefix added. If we want to use the m_ prefix, then we should
make this a class with accessor functions instead.

> Source/WebCore/dom/DynamicNodeList.h:120
> +	   DynamicNodeList::Caches::m_length;
> +	   DynamicNodeList::Caches::m_lastItem;
> +	   DynamicNodeList::Caches::m_lastItemOffset;
> +	   DynamicNodeList::Caches::m_isLengthCacheValid;
> +	   DynamicNodeList::Caches::m_isItemCacheValid;

I have never seen this syntax before and I am surprised it works. I think the
correct syntax is:

    using DynamicNodeList::Caches::m_length;

But also, this syntax works:

    using Caches::m_length;

I believe deriving from a base class makes the class name usable without a
prefix.

> Source/WebCore/dom/Node.cpp:1023
> -void Node::invalidateNodeListsCacheAfterAttributeChanged(const
QualifiedName& attrName)
> +void Node::invalidateNodeListsCacheAfterAttributeChanged(const
QualifiedName&)

We should remove the argument entirely, not just have it be unused.

> Source/WebCore/dom/Node.cpp:2353
>  #if ENABLE(MICRODATA)
>      invalidateMicrodataItemListCaches();
>  #endif
> +    
>  }

Shouldn’t add this blank line.