Cleanup RemotePlatformLayer when disconnecting RemoteFrame.

Cleanup RemotePlatformLayer when disconnecting RemoteFrame.

BUG=448507
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189994

[lfg, 2015-01-16 23:23:42]

lfg@chromium.org changed reviewers:
+ kenrb@chromium.org

[lfg, 2015-01-16 23:23:42]

PTAL.

[kenrb, 2015-01-19 17:06:19]

Are you able to move RemotePlatformLayer to RemoteFrame? It is on Frame for
legacy reasons (we used to have LocalFrames pretending to be RemoteFrames).

[lfg, 2015-01-19 23:27:18]

On 2015/01/19 17:06:19, kenrb wrote:
> Are you able to move RemotePlatformLayer to RemoteFrame? It is on Frame for
> legacy reasons (we used to have LocalFrames pretending to be RemoteFrames).

I tried looking into it, but I wasn't sure how to implement
isRemoteFrameTemporary():

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Any advice on that?

[kenrb, 2015-01-20 17:34:59]

On 2015/01/19 23:27:18, lfg wrote:
> On 2015/01/19 17:06:19, kenrb wrote:
> > Are you able to move RemotePlatformLayer to RemoteFrame? It is on Frame for
> > legacy reasons (we used to have LocalFrames pretending to be RemoteFrames).
> 
> I tried looking into it, but I wasn't sure how to implement
> isRemoteFrameTemporary():
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Any advice on that?

I think it can be removed.

[lfg, 2015-01-23 17:36:03]

On 2015/01/20 17:34:59, kenrb wrote:
> On 2015/01/19 23:27:18, lfg wrote:
> > On 2015/01/19 17:06:19, kenrb wrote:
> > > Are you able to move RemotePlatformLayer to RemoteFrame? It is on Frame
for
> > > legacy reasons (we used to have LocalFrames pretending to be
RemoteFrames).
> > 
> > I tried looking into it, but I wasn't sure how to implement
> > isRemoteFrameTemporary():
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Any advice on that?
> 
> I think it can be removed.

Done, PTAL.

[kenrb, 2015-01-23 18:31:02]

Good stuff, it will be nice to have this cleaned up.

I would speculate wildly that the failing unit tests are related to the
ASSERT_NOT_REACHED(), in which case it might be that some tests need to be
modified for this.

https://codereview.chromium.org/842033003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderPart.cpp (left):

https://codereview.chromium.org/842033003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderPart.cpp:170: if
(toFrameView(widget())->frame().isRemoteFrameTemporary())
I am surprised if this does not break input events when running with
--site-per-process. The comment is stale because RemoteFrames now have
RemoteFrameViews, but don't we need to handle the case where widget() is a
RemoteFrameView below? Line 173 should crash as an invalid downcast.

[lfg, 2015-02-03 03:43:46]

The failed tests were because of a uninitialized variable m_remotePlatformLayer
in RemoteFrame.cpp .

Please, take another look.

https://codereview.chromium.org/842033003/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderPart.cpp (left):

https://codereview.chromium.org/842033003/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderPart.cpp:170: if
(toFrameView(widget())->frame().isRemoteFrameTemporary())
On 2015/01/23 18:31:02, kenrb wrote:
> I am surprised if this does not break input events when running with
> --site-per-process. The comment is stale because RemoteFrames now have
> RemoteFrameViews, but don't we need to handle the case where widget() is a
> RemoteFrameView below? Line 173 should crash as an invalid downcast.

RemoteFrameViews return false to isFrameView(), so they should be handled in the
if statement above this.

[kenrb, 2015-02-03 18:12:47]

lgtm

You probably want dcheng and japhet for Blink owner reviewers.

[lfg, 2015-02-03 18:37:06]

lfg@chromium.org changed reviewers:
+ dcheng@chromium.org, japhet@chromium.org

[lfg, 2015-02-03 18:37:07]

dcheng@chromium.org: Please review changes in 

Source/core/*

japhet@chromium.org: Please review changes in 

Source/web/WebLocalFrameImpl.cpp

[dcheng, 2015-02-03 22:08:38]

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
Source/core/frame/RemoteFrame.cpp:89: if (owner() && owner()->isLocal())
Why do we do this work here instead of, say, RemoteFrame::detach()? Is there a
corresponding cleanup step for LocalFrames that we can mirror here?

Also, would it make more sense for this check to be something like:

if (m_remotePlatformLayer)
    setRemotePlatformLayer(nullptr);

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
Source/core/frame/RemoteFrame.h:28: bool isRemoteFrame() const override { return
true; }
In the future, let's separate cleanups like this into a separate patch.

[lfg, 2015-02-03 22:54:50]

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
Source/core/frame/RemoteFrame.cpp:89: if (owner() && owner()->isLocal())
On 2015/02/03 22:08:37, dcheng wrote:
> Why do we do this work here instead of, say, RemoteFrame::detach()? Is there a
> corresponding cleanup step for LocalFrames that we can mirror here?

It needs to run on both WebFrame::swap(), as well as detach(). It doesn't look
like there's any other place that would fit. (LocalFrame also have a
disconnectOwnerElement() override to do some cleanups).

> Also, would it make more sense for this check to be something like:
> 
> if (m_remotePlatformLayer)
>     setRemotePlatformLayer(nullptr);

This makes sense, changed.

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
File Source/core/frame/RemoteFrame.h (right):

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
Source/core/frame/RemoteFrame.h:28: bool isRemoteFrame() const override { return
true; }
On 2015/02/03 22:08:37, dcheng wrote:
> In the future, let's separate cleanups like this into a separate patch.

Acknowledged.

[dcheng, 2015-02-04 00:02:23]

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
Source/core/frame/RemoteFrame.cpp:89: if (owner() && owner()->isLocal())
On 2015/02/03 22:54:50, lfg wrote:
> On 2015/02/03 22:08:37, dcheng wrote:
> > Why do we do this work here instead of, say, RemoteFrame::detach()? Is there
a
> > corresponding cleanup step for LocalFrames that we can mirror here?
> 
> It needs to run on both WebFrame::swap(), as well as detach(). It doesn't look
> like there's any other place that would fit. (LocalFrame also have a
> disconnectOwnerElement() override to do some cleanups).

Can we add a comment to this effect? It's not immediately obvious to the reader
that that's the requirement for this logic belonging here. In addition, I think
there is a high chance that WebFrame::swap() will end up calling detach() on the
frames (but obviously that's still in flux).

[lfg, 2015-02-04 00:22:03]

On 2015/02/04 00:02:23, dcheng wrote:
>
https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
> File Source/core/frame/RemoteFrame.cpp (right):
> 
>
https://codereview.chromium.org/842033003/diff/80001/Source/core/frame/Remote...
> Source/core/frame/RemoteFrame.cpp:89: if (owner() && owner()->isLocal())
> On 2015/02/03 22:54:50, lfg wrote:
> > On 2015/02/03 22:08:37, dcheng wrote:
> > > Why do we do this work here instead of, say, RemoteFrame::detach()? Is
there
> a
> > > corresponding cleanup step for LocalFrames that we can mirror here?
> > 
> > It needs to run on both WebFrame::swap(), as well as detach(). It doesn't
look
> > like there's any other place that would fit. (LocalFrame also have a
> > disconnectOwnerElement() override to do some cleanups).
> 
> Can we add a comment to this effect? It's not immediately obvious to the
reader
> that that's the requirement for this logic belonging here. In addition, I
think
> there is a high chance that WebFrame::swap() will end up calling detach() on
the
> frames (but obviously that's still in flux).

Done.

[dcheng, 2015-02-04 01:14:31]

lgtm for Source/core changes.

[lfg, 2015-02-05 19:31:16]

New patchsets have been uploaded after l-g-t-m from dcheng@chromium.org

[Nate Chapin, 2015-02-10 23:30:52]

lgtm

[lfg, 2015-02-11 20:16:41]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2015-02-11 20:17:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/842033003/160001

[commit-bot: I haz the power, 2015-02-11 22:08:45]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189994