[webkit-reviews] review granted: [Bug 107074] Remove NodeListsNodeData when it's no longer needed : [Attachment 183081] Fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 107074: Remove NodeListsNodeData when it's no longer needed
https://bugs.webkit.org/show_bug.cgi?id=107074

Attachment 183081: Fixes the bug
https://bugs.webkit.org/attachment.cgi?id=183081&action=review

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


Seems OK, although I have a few substantive thoughts below.

Somewhere there should be a comment about how we decided to delete these when
no longer needed rather than keeping them around to optimize re-using them
later. It’s the performance test, but something in the code should talk about
how we made the tradeoff, if possible.

> Source/WebCore/ChangeLog:10
> +	   If we detect that we have only one node lists left in the data
structure,

Should be “node list” instead of “node lists”.

> Source/WebCore/ChangeLog:20
> +	   (Node):

Bogus line should be removed. Or fix the script so it doesn’t add these.

> Source/WebCore/ChangeLog:26
> +	   (NodeListsNodeData):

Bogus line should be removed. Or fix the script so it doesn’t add these.

> Source/WebCore/dom/NodeRareData.h:74
> +	   if (!removeThisIfOnlyOneListIsLeft(list->ownerNode()))
> +	       m_childNodeList = 0;

I think I’d like these better if they used an early return statement instead of
nesting the remaining code. The logic is “if I can do this just by self
destructing, then do that and return, otherwise do it the normal way”.

> Source/WebCore/dom/NodeRareData.h:225
> +    bool removeThisIfOnlyOneListIsLeft(Node*);

This function is confusing for a few reasons. The idea of this function is that
we call it just before removing a list, which is why it checks for one
remaining list rather than zero. That was completely non-obvious to me and I
had to read the patch over and over again to figure that out. So I think that
either the function name or a comment needs to explain that.

The trickiness and danger of using this function without looking at the boolean
result would be clearer if it used the word “delete” instead of “remove”. On
the other hand, “delete” does make it sound like this just deletes the object
rather than cleanly removing it from the node.

Maybe a better name is deleteThisIfAboutToRemoveLastList? Or maybe
selfDestructIfAboutToRemoveLastList? Maybe you can come up with an even better
name.

I’m also not sure the factoring is quite right. It seems awkward to call this
class to remove a list, then have this class recover the node pointer so it can
do work on the node’s rare data. It’s kind of a messy relationship between the
node, rare data, and lists classes. I understand that this is an easy place to
correctly make the change, so I see the attraction of doing it this way, but
the need to recover the owner node pointer is a factoring danger sign.

> Tools/Scripts/webkitpy/performance_tests/perftest.py:209
>	   # Following is for Parser/html-parser.html
> -	   re.compile(re.escape("""CONSOLE MESSAGE: Blocked script execution in
'html-parser.html' because the document's frame is sandboxed and the
'allow-scripts' permission is not set.""")),
> +	   re.compile(r"CONSOLE MESSAGE: Blocked script execution in
'[A-Za-z0-9\-\.]+' because the document's frame is sandboxed and the
'allow-scripts' permission is not set."),

Comment now seems wrong.