[Webkit-unassigned] [Bug 33696] let's cache nodelists instead of dynamicnodelist::cache

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33696


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #51772|review?                     |review+
               Flag|                            |




--- Comment #44 from Darin Adler <darin at apple.com>  2010-03-26 12:47:40 PST ---
(From update of attachment 51772)
> +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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.