[webkit-reviews] review granted: [Bug 221243] Avoid creating JS wrapper on a removed node when the subtree is not observable : [Attachment 419109] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 221243: Avoid creating JS wrapper on a removed node when the subtree is not
observable
https://bugs.webkit.org/show_bug.cgi?id=221243

Attachment 419109: Patch

https://bugs.webkit.org/attachment.cgi?id=419109&action=review




--- Comment #7 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 419109
  --> https://bugs.webkit.org/attachment.cgi?id=419109
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=419109&action=review

r=me

>>> Source/WebCore/dom/ContainerNode.cpp:116
>>> +		 hasChildListChanged = true;
>> 
>> I don't understand why we care if children have changed.
>> 
>> If a child has been removed, that is not an external reference to this
subtree.
>> 
>> If a child has been added, that is not an external reference to this subtree
unless the child has an external reference, which the refcount check will
cover.
>> 
>> If a child has been moved, that is just a removal followed by an addition,
so it is covered by the logic above.
> 
> I guess the issue here is that, if an event fired above, and then added a
node to the subtree, we don't know what refCount check to do. Nodes in the
children set should be checked for > 2, and other nodes should be checked for >
1, and we don't have an obvious way to tell the difference.

^ I guess this is worth a comment next to hasChildListChanged; but I can't
think of an obviously better alternative.