[webkit-reviews] review granted: [Bug 33696] let's cache nodelists instead of dynamicnodelist::cache : [Attachment 51772] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted anton muhin <antonm at chromium.org>'s
request for review:
Bug 33696: let's cache nodelists instead of dynamicnodelist::cache
https://bugs.webkit.org/show_bug.cgi?id=33696

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +template <class It>
> +void markNodeLists(It begin, It end, MarkStack& markStack, JSGlobalData&
globalData)
> +{
> +    for (It it = begin; it != end; ++it)
> +	   markDOMObjectWrapper(markStack, globalData, it->second.get());
> +}

I'm not a big fan of abbreviations. How about "typename IteratorType" instead
of "class It"?

But as long as you are using templates, I suggest putting the begin and end
calls in this function too.

    template <class NodeListMap>
    void markNodeLists(const NodeListMap& map, MarkStack& markStack,
JSGlobalData& globalData)
    {
	for (NodeListMap::const_iterator it = map.begin(); it != map.end; ++it)

	    markDOMObjectWrapper(markStack, globalData, it->second.get());
    }

Then the call site will be cleaner.

> +	   // NodeLists may present, if it's the case they need to be marked.

Better grammar would be: "Node lists may be present. If so, they need to be
marked."

> +	   NodeListsNodeData* nodeLists = node->rareData()->nodeLists();
> +	   if (nodeLists) {

I suggest putting the definition of the nodeLists local variable inside the if
statement.

> +    bool hasRareData() const { return m_hasRareData; }
> +    NodeRareData* rareData() const;

Instead of making these public, I suggest making the code to mark node lists a
member of the Node class. This is the approach used in EventTarget for the
markJSEventListeners function.

I'm going to say r=me as-is, but I believe my suggestions would make the code
significantly better.