[webkit-reviews] review denied: [Bug 101821] Track subframe count to avoid traversing the tree when there's no subframes : [Attachment 174365] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Elliott Sprehn
<esprehn at chromium.org>'s request for review:
Bug 101821: Track subframe count to avoid traversing the tree when there's no
subframes
https://bugs.webkit.org/show_bug.cgi?id=101821

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174365&action=review


> Source/WebCore/dom/ContainerNodeAlgorithms.h:274
> +    void disconnectChildren();
>      void disconnect();

I would kind of prefer just having one method here that takes ShouldIncludeRoot
as an argument. It took me a while of looking around the code to understand why
we needed both of these methods. WDYT?

> Source/WebCore/dom/ContainerNodeAlgorithms.h:279
> +    void collectFrameOwners(Node* root);
> +    void collectFrameOwners(ElementShadow*);
> +    void disconnectCollectedFrameOwners();

These names are much more clear!

> Source/WebCore/dom/ContainerNodeAlgorithms.h:301
> +inline void ChildFrameDisconnector::collectFrameOwners(Node* root)

Not sure if we care that this used to be iterative and is now recursive.

> Source/WebCore/dom/ContainerNodeAlgorithms.h:306
> +    if (root->isElementNode() && root->isFrameOwnerElement())

Lets keep the hasCustomCallbacks in here for now and remove it in a followup
patch. That way if it still does somehow affect performance, we can identify it
separately from the performance impact of the rest of the change and not have
to revert the whole change.

> Source/WebCore/dom/NodeRareData.h:349
> +COMPILE_ASSERT(Page::maxNumberOfFrames < 1024,
Frame_limit_should_fit_in_rare_data_count);

<3 this assert.

> Source/WebCore/html/HTMLFrameOwnerElement.cpp:70
> +    // This causes an unload event in the subframe so it cannot be a part of

> +    // removedFrom() because the unload handler in a same domain frame must
be
> +    // able to reach upward into the owner document.

Thanks for making this more clear.