[webkit-reviews] review granted: [Bug 76693] ShadowRoot should inherit from DocumentFragment. : [Attachment 124903] another iteration

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Hayato Ito <hayato at chromium.org>'s
request for review:
Bug 76693: ShadowRoot should inherit from DocumentFragment.
https://bugs.webkit.org/show_bug.cgi?id=76693

Attachment 124903: another iteration
https://bugs.webkit.org/attachment.cgi?id=124903&action=review

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


> Source/WebCore/dom/Document.cpp:577
> +    if (hasRareData())
> +	   clearRareData();

This very mysterious bit of code needs a “why” comment. Since this is called in
Node, there must be a very good reason why it’s critical to call it earlier
here, and the comment should state that reason. It’s sort of obvious to me why,
but I don’t think it would be obvious to most programmers.

> Source/WebCore/dom/Document.cpp:661
> +	   if (element->shadowRoot())
> +	       buildAccessKeyMap(element->shadowRoot());

Should put this in a local variable so we don’t do it twice.

> Source/WebCore/dom/ShadowRoot.cpp:56
> +    if (hasRareData())
> +	   clearRareData();

Same comment about this as in Document.

> Source/WebCore/dom/TreeScope.h:41
> +// A class which inherits both Node and TreeScope must call clearRareData()
in its destructor.
> +// See http://trac.webkit.org/changeset/90378 for the reason.

This comment explains the calls to clearRareData, but it seems likely that
people reading the destructors won’t see the comment. I also think it’s
unnecessarily indirect to give a trac link instead of having a comment
somewhere in the code.