[webkit-reviews] review granted: [Bug 195776] Reduce the size of Node::deref by eliminating an explicit parentNode check : [Attachment 364773] 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 195776: Reduce the size of Node::deref by eliminating an explicit
parentNode check
https://bugs.webkit.org/show_bug.cgi?id=195776

Attachment 364773: Patch

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




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

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

r=me with comments already discussed, and one new suggestion

>>> Source/WebCore/dom/Node.cpp:2533
>>> +	     m_refCount = 0; // Document needs m_refCount to be 0 here due to
m_referencingNodeCount.
>> 
>> I didn't fully understand this comment or the related ChangeLog explanation.
>> 
>> Can we make a nice API so that Document's use of m_refCount works without
this unique behavior?
>> 
>> We are calling Document::removedLastRef(). Is that a strong enough signal to
the Document that it really has no referencing nodes?
> 
> We can't because if m_refCount is not 0 here when m_referencingNodeCount is
not zero, Document::removedLastRef doesn't delete document. Instead, it gets
deleted when m_referencingNodeCount becomes 0 in decrementReferencingNodeCount.
But it deletes Document only when m_refCount is also zero. If we didn't
decrement m_refCount here, then we won't be able to tell in
decrementReferencingNodeCount whether m_refCount is actually 2, or it would
have been reached 0 but we simply didn't decrement.

Would it work to move this assignment of 0 into Document::removedLastRef?

Document::removedLastRef could include a comment that said
"Node::removedLastRef doesn't set refCount() to zero because it's not
observable. But we need to remember that our refCount reached zero in
subsequent calls to decrementReferencingNodeCount()."

How does that sound?