Always use the WebFrameWidget when attaching the root graphics

Always use the WebFrameWidget when attaching the root graphics
layer or the compositor animation timeline in ChromeClientImpl.

Now that we always instantiate a WebFrameWidget for local
roots, we can remove the fallback code that goes directly
through WebView.

This change also fixes an issue where local roots were not
being passed to
ChromeClientImpl::attachCompositorAnimationTimeline which
caused the AnimationTimeline to be attached to the wrong
widget.

BUG=587025
Committed: https://crrev.com/af41ee000f480c570adf535fede679c90819d10f
Cr-Commit-Position: refs/heads/master@{#403781}

[lfg, 2016-06-03 21:03:15]

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

[lfg, 2016-06-03 21:03:16]

PTAL.

[lfg, 2016-06-03 21:03:17]

PTAL.

[dcheng, 2016-06-08 00:10:17]

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:792:
DCHECK(webFrame->frameWidget() || !rootLayer);
Why is it OK for rootLayer to be null in this case? Doesn't this mean we're
calling it too late or too many times?

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewFrameWidget.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewFrameWidget.cpp:270:
m_webView->setRootGraphicsLayer(layer);
Can we make these private in WebViewImpl and friend WebViewFrameWidget for
access? This seems like a good step towards helping pull more widget components
out of WebView.

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:314:
m_testWebViewClient->initializeLayerTreeView();
I don't think I understand why we need this forwarding client: we haven't
removed anything from the public interface.

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:129:
TestWebViewWidgetClient(TestWebViewClient* testWebViewClient)
Nit: explicit

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1609: WebWidget* widget =
WebFrameWidget::create(&widgetClient, localFrame);
I wonder if we can make this simpler for test code by storing the WebFrameWidget
on TestWebFrameClient? FrameTestHelpers::createLocalChild() can be responsible
for setting this up. Obviously we'll need something a bit different for the main
frame eventually, but that seems like it should make it easier for test code to
avoid duplicating so much boilerplate. WDYT?

[lfg, 2016-06-08 21:09:47]

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:792:
DCHECK(webFrame->frameWidget() || !rootLayer);
On 2016/06/08 00:10:17, dcheng wrote:
> Why is it OK for rootLayer to be null in this case? Doesn't this mean we're
> calling it too late or too many times?

We call PaintLayerCompositor::detachRootLayer() in Document::detach() (which
calls this with nullptr as the rootLayer), and detach() happens after the frame
widget is destroyed. The widget is destroyed in RenderWidget::CloseForFrame,
which is called from FrameLoaderClientImpl::detach, right before the document is
detached (See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc...).

Also on the same topic, at this point we already called
setRootGraphicsLayer(nullptr) in WebViewImpl::willCloseLayerTreeView, so the
call was a noop anyway.

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/WebViewFrameWidget.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/WebViewFrameWidget.cpp:270:
m_webView->setRootGraphicsLayer(layer);
On 2016/06/08 00:10:17, dcheng wrote:
> Can we make these private in WebViewImpl and friend WebViewFrameWidget for
> access? This seems like a good step towards helping pull more widget
components
> out of WebView.

Done.

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp:314:
m_testWebViewClient->initializeLayerTreeView();
On 2016/06/08 00:10:17, dcheng wrote:
> I don't think I understand why we need this forwarding client: we haven't
> removed anything from the public interface.

I switched TestWebWidgetClient to be a class instead of an empty implementation
(this was needed because I had to override allowsBrokenNullLayerTreeView() in
tests that needed a frame widget). After that, TestWebViewClient can't be casted
to TestWebWidgetClient anymore, so I added the forwarding code -- it's basically
the same issue we have with WebFrameWidgetImpl/WebViewFrameWidget/WebViewImpl.

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/FrameTestHelpers.h (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/FrameTestHelpers.h:129:
TestWebViewWidgetClient(TestWebViewClient* testWebViewClient)
On 2016/06/08 00:10:17, dcheng wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1609: WebWidget* widget =
WebFrameWidget::create(&widgetClient, localFrame);
On 2016/06/08 00:10:17, dcheng wrote:
> I wonder if we can make this simpler for test code by storing the
WebFrameWidget
> on TestWebFrameClient? FrameTestHelpers::createLocalChild() can be responsible
> for setting this up. Obviously we'll need something a bit different for the
main
> frame eventually, but that seems like it should make it easier for test code
to
> avoid duplicating so much boilerplate. WDYT?

I think that's doable, but there's one test that needs to override the client
(GestureEventTestWebWidgetClient). What do you think of having an optional
argument in createLocalChild for the client, with a default of nullptr which
would give you the default TestWebWidgetClient? We would also need to add code
to TestWebWidgetClient to destroy itself, since we won't be closing
widget->close(), but I think that can be done similarly to how RenderWidget
destroys itself.

[dcheng, 2016-06-09 04:43:52]

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:792:
DCHECK(webFrame->frameWidget() || !rootLayer);
On 2016/06/08 at 21:09:47, lfg (OOO until Jun 25) wrote:
> On 2016/06/08 00:10:17, dcheng wrote:
> > Why is it OK for rootLayer to be null in this case? Doesn't this mean we're
> > calling it too late or too many times?
> 
> We call PaintLayerCompositor::detachRootLayer() in Document::detach() (which
calls this with nullptr as the rootLayer), and detach() happens after the frame
widget is destroyed. The widget is destroyed in RenderWidget::CloseForFrame,
which is called from FrameLoaderClientImpl::detach, right before the document is
detached (See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc...).
> 
> Also on the same topic, at this point we already called
setRootGraphicsLayer(nullptr) in WebViewImpl::willCloseLayerTreeView, so the
call was a noop anyway.

That doesn't sound right: we should be calling Document::detach() before we call
into the embedder via FrameLoaderClientImpl::detached(). Let's chat about this
tomorrow: the timing sounds weird, and the fact that we still need this
conditional doesn't feel right.

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:813: if
(webFrame->frameWidget())
Similar comment to the attachRootGraphicsLayer comment here. At face value, it
looks like PaintLayerCompositor doesn't expect to call into here with a detached
frame, so having this check seems out of place.

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right):

https://codereview.chromium.org/2036403002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/WebFrameTest.cpp:1609: WebWidget* widget =
WebFrameWidget::create(&widgetClient, localFrame);
On 2016/06/08 at 21:09:47, lfg (OOO until Jun 25) wrote:
> On 2016/06/08 00:10:17, dcheng wrote:
> > I wonder if we can make this simpler for test code by storing the
WebFrameWidget
> > on TestWebFrameClient? FrameTestHelpers::createLocalChild() can be
responsible
> > for setting this up. Obviously we'll need something a bit different for the
main
> > frame eventually, but that seems like it should make it easier for test code
to
> > avoid duplicating so much boilerplate. WDYT?
> 
> I think that's doable, but there's one test that needs to override the client
(GestureEventTestWebWidgetClient). What do you think of having an optional
argument in createLocalChild for the client, with a default of nullptr which
would give you the default TestWebWidgetClient? We would also need to add code
to TestWebWidgetClient to destroy itself, since we won't be closing
widget->close(), but I think that can be done similarly to how RenderWidget
destroys itself.

That seems reasonable to me.

[lfg, 2016-06-29 12:54:31]

On 2016/06/09 04:43:52, dcheng wrote:
> > We call PaintLayerCompositor::detachRootLayer() in Document::detach() (which
> calls this with nullptr as the rootLayer), and detach() happens after the
frame
> widget is destroyed. The widget is destroyed in RenderWidget::CloseForFrame,
> which is called from FrameLoaderClientImpl::detach, right before the document
is
> detached (See
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Loc...).
> > 
> > Also on the same topic, at this point we already called
> setRootGraphicsLayer(nullptr) in WebViewImpl::willCloseLayerTreeView, so the
> call was a noop anyway.
> 
> That doesn't sound right: we should be calling Document::detach() before we
call
> into the embedder via FrameLoaderClientImpl::detached(). Let's chat about this
> tomorrow: the timing sounds weird, and the fact that we still need this
> conditional doesn't feel right.

The full callstack is RenderFrameImpl::OnDeleteFrame -> WebFrame::detach ->
LocalFrame::detach -> Document::detach. Looking in LocalFrame::detach, the call
to the embedder is in Frame::detach(), which is called almost at the end of the
function. There's a lot going on there, I'm a bit afraid of changing the order
here. Why do you think the embedder should be notified before the document is
detached?

[lfg, 2016-06-30 19:38:22]

PTAL

[dcheng, 2016-07-04 07:34:36]

https://codereview.chromium.org/2036403002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2036403002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:795: if
(webFrame->frameWidget())
Please add a comment why this method and the method on line 815 need null
checks.

https://codereview.chromium.org/2036403002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/2036403002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameWidget.h:84: virtual void
setRootGraphicsLayer(GraphicsLayer*) = 0;
Hmm, can we move the Blink-internal methods to their own section with a comment?
I'll clean it up in a followup CL.

[lfg, 2016-07-04 17:42:08]

Patchset #9 (id:160001) has been deleted

[lfg, 2016-07-04 17:46:13]

https://codereview.chromium.org/2036403002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/2036403002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:795: if
(webFrame->frameWidget())
On 2016/07/04 07:34:36, dcheng wrote:
> Please add a comment why this method and the method on line 815 need null
> checks.

Done.

https://codereview.chromium.org/2036403002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrameWidget.h (right):

https://codereview.chromium.org/2036403002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrameWidget.h:84: virtual void
setRootGraphicsLayer(GraphicsLayer*) = 0;
On 2016/07/04 07:34:36, dcheng wrote:
> Hmm, can we move the Blink-internal methods to their own section with a
comment?
> I'll clean it up in a followup CL.

Done.

[dcheng, 2016-07-05 02:04:41]

lgtm

[lfg, 2016-07-05 13:20:07]

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

[lfg, 2016-07-05 13:20:07]

jochen@chromium.org: Please review changes in 

content/, components/

[jochen (gone - plz use gerrit), 2016-07-05 14:20:34]

lgtm

[lfg, 2016-07-05 14:26:42]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2016-07-05 14:27:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-05 14:57:42]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-07-05 14:59:12]

Description was changed from

==========
Always use the WebFrameWidget when attaching the root graphics
layer or the compositor animation timeline in ChromeClientImpl.

Now that we always instantiate a WebFrameWidget for local
roots, we can remove the fallback code that goes directly
through WebView.

This change also fixes an issue where local roots were not
being passed to
ChromeClientImpl::attachCompositorAnimationTimeline which
caused the AnimationTimeline to be attached to the wrong
widget.

BUG=587025
==========

to

==========
Always use the WebFrameWidget when attaching the root graphics
layer or the compositor animation timeline in ChromeClientImpl.

Now that we always instantiate a WebFrameWidget for local
roots, we can remove the fallback code that goes directly
through WebView.

This change also fixes an issue where local roots were not
being passed to
ChromeClientImpl::attachCompositorAnimationTimeline which
caused the AnimationTimeline to be attached to the wrong
widget.

BUG=587025

Committed: https://crrev.com/af41ee000f480c570adf535fede679c90819d10f
Cr-Commit-Position: refs/heads/master@{#403781}
==========

[commit-bot: I haz the power, 2016-07-05 14:59:13]

Patchset 10 (id:??) landed as
https://crrev.com/af41ee000f480c570adf535fede679c90819d10f
Cr-Commit-Position: refs/heads/master@{#403781}

[Mostyn Bramley-Moore, 2016-07-05 15:33:27]

mostynb@opera.com changed reviewers:
+ mostynb@opera.com

[Mostyn Bramley-Moore, 2016-07-05 15:33:28]

https://codereview.chromium.org/2036403002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebViewImpl.h (right):

https://codereview.chromium.org/2036403002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebViewImpl.h:542: friend class
WebViewFrameWidget;
This is a duplicate, after https://codereview.chromium.org/1991273003 landed in
the meantime.  Queued a fix: https://codereview.chromium.org/2121953003/

[lfg, 2016-07-05 15:35:43]

On 2016/07/05 15:33:28, Mostyn Bramley-Moore wrote:
>
https://codereview.chromium.org/2036403002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/WebViewImpl.h (right):
> 
>
https://codereview.chromium.org/2036403002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/WebViewImpl.h:542: friend class
> WebViewFrameWidget;
> This is a duplicate, after https://codereview.chromium.org/1991273003 landed
in
> the meantime.  Queued a fix: https://codereview.chromium.org/2121953003/

Ah, sorry about that. Forgot about that and the bots didn't complain. Thanks for
fixing it.