[webkit-reviews] review denied: [Bug 97807] [Chromium] Fix the find-in-page implementation for detaching frames. : [Attachment 166039] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Leandro Graciá Gil
<leandrogracia at chromium.org>'s request for review:
Bug 97807: [Chromium] Fix the find-in-page implementation for detaching frames.
https://bugs.webkit.org/show_bug.cgi?id=97807

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166039&action=review


This is great.	A few minor comments.  The main issue is that the unit tests
should go through the API rather than manually poking at WebCore internals and
potentially putting WebCore into an inconsistent state.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:581
> +    virtual void willDetachPage()

willDetachPage should always occur before frameDestroyed, so m_webFrameImpl
will always be non-0 here.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1658
> +    // Detached frame case. Return no results.

We don't need this comment.  It just says what the code does, not why it does
it.

> Source/WebKit/chromium/src/WebFrameImpl.cpp:1935
> +    // Do nothing on detached frames.

ditto

> Source/WebKit/chromium/src/WebFrameImpl.cpp:2726
> +    // Frame already detached.
> +    if (!frame() || !frame()->page())
> +	   return;

Is this possible?  Also this comment is redundant.

> Source/WebKit/chromium/src/WebFrameImpl.h:427
> +    // Observes for any destruction / page detachment events that might
occur
> +    // to the WebCore frame.
> +    OwnPtr<FrameDestructionHelper> m_frameDestructionHelper;

Why not just make WebFrameImpl inherit from FrameDestructionObserver?

> Source/WebKit/chromium/src/WebFrameImpl.h:471
> +    int m_findRequestIdentifier;

Should we initialize this value in the constructor?

> Source/WebKit/chromium/tests/WebFrameTest.cpp:1016
> +    // Detach the frame before finding.
> +    WebFrameImpl* secondFrame =
static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
> +    secondFrame->frame()->willDetachPage();
> +    secondFrame->frame()->detachFromPage();

Rather than doing this manually, we should do this by manipulating the DOM via
the API or via JavaScript.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:1061
> +    // Detach the frame between finding and scoping.
> +    WebFrameImpl* secondFrame =
static_cast<WebFrameImpl*>(mainFrame->traverseNext(false));
> +    secondFrame->frame()->willDetachPage();
> +    secondFrame->frame()->detachFromPage();

ditto