[webkit-reviews] review denied: [Bug 136830] Use an AtomicString as key for caching ClassNodeList objects : [Attachment 238137] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 136830: Use an AtomicString as key for caching ClassNodeList objects
https://bugs.webkit.org/show_bug.cgi?id=136830

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=238137&action=review


> Source/WebCore/ChangeLog:23
> +	   No new tests, no behavior change.

I disagree. :)

If not already covered, you should have a test ensuring that there are no name
conflicts in the cache.

For example:
   getElemensByTagName("foo")
   getElementsByClassNames("foo")
   ... etc.
should have independent node lists.

If such a test already exist, you should mention it in the changelog.

> Source/WebCore/dom/ClassNodeList.h:45
> +    static PassRef<ClassNodeList> create(ContainerNode& rootNode, const
AtomicString& classNames)
>      {
>	   return adoptRef(*new ClassNodeList(rootNode, classNames));
>      }

Looks like poor inlining. The create() function is inline and the constructor
is out of line.

> Source/WebCore/dom/ContainerNode.cpp:1057
>  PassRefPtr<NodeList> ContainerNode::getElementsByClassName(const String&
classNames)

You should convert this argument to AtomicString to avoid any conversion on the
call sites.

> Source/WebCore/dom/NodeRareData.h:-139
> -	   NodeListNameCacheMap::AddResult result =
m_nameCaches.fastAdd(namedNodeListKey<T>(name), nullptr);

Shouldn't you also update namedNodeListKey() to remove the generalization?