[webkit-reviews] review denied: [Bug 92965] Optimize ChildNode{Insertion, Removal}Notifier::notify() by lazily taking a snapshot of child nodes : [Attachment 156040] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Kentaro Hara <haraken at chromium.org>'s
request for review:
Bug 92965: Optimize ChildNode{Insertion,Removal}Notifier::notify() by lazily
taking a snapshot of child nodes
https://bugs.webkit.org/show_bug.cgi?id=92965

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=156040&action=review


Nice idea.  Some detailed comments below.

> Source/WebCore/dom/ContainerNode.h:304
> +static ChildNodesSnapshot* latestSnapshot = 0;

Please don't add static global variables to header files.  That will add
storage for this variable to every complication unit that includes this header!


> Source/WebCore/dom/ContainerNode.h:324
> +	   if (m_childNodes)
> +	       delete m_childNodes;

Can we use an OwnPtr to have this delete happen automatically?

> Source/WebCore/dom/ContainerNode.h:338
> +	   Node* ret = (*m_childNodes)[m_currentIndex++].get();

ret -> node (prefer complete words in variable names)

> Source/WebCore/dom/ContainerNode.h:344
> +	   if (!hasSnapshot()) {

Prefer early return.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:69
> +    ChildNodesSnapshot snapshot(node);

I'd put the word "Lazy" into the name of this class to make it clear that the
snapshot is happening lazily.