[webkit-reviews] review granted: [Bug 107302] Assert the connectedSubframeCount is consistent and fix over counting : [Attachment 184336] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Elliott Sprehn
<esprehn at chromium.org>'s request for review:
Bug 107302: Assert the connectedSubframeCount is consistent and fix over
counting
https://bugs.webkit.org/show_bug.cgi?id=107302

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=184336&action=review


I didn't review particularly deeply, but looks fine. It's a great feeling when
adding a check uncovers an actual bug.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:125
> +unsigned assertConnectedSubframeCountIsConsistent(Node* node)

I don't get what it means when an "assert" function returns a value. I don't
think that it should be named like this in this case.

> Source/WebCore/dom/ContainerNodeAlgorithms.cpp:145
> +    // If we overcount it's safe, but not optimal. 

It would be helpful to describe in more detail here what is non-optimal. Will
we leak? Or will we need to traverse frame tree an extra time somewhere?