Always use the WebFrameWidget when attaching the root graphics

third_party/WebKit/Source/web/ChromeClientImpl.cpp

Index: third_party/WebKit/Source/web/ChromeClientImpl.cpp
diff --git a/third_party/WebKit/Source/web/ChromeClientImpl.cpp b/third_party/WebKit/Source/web/ChromeClientImpl.cpp
index d415d339f26e756b3c382f56434f2413ff7f6d18..9a9b1593949e665df07132fe2374ddbb71ecfa67 100644
--- a/third_party/WebKit/Source/web/ChromeClientImpl.cpp
+++ b/third_party/WebKit/Source/web/ChromeClientImpl.cpp
@@ -786,24 +786,12 @@ String ChromeClientImpl::acceptLanguages()
     return m_webView->client()->acceptLanguages();
 }
 
-void ChromeClientImpl::attachRootGraphicsLayer(GraphicsLayer* rootLayer, LocalFrame* localRoot)
+void ChromeClientImpl::attachRootGraphicsLayer(GraphicsLayer* rootLayer, LocalFrame* localFrame)
 {
-    // FIXME: For top-level frames we still use the WebView as a WebWidget. This
-    // special case will be removed when top-level frames get WebFrameWidgets.
-    if (localRoot->isMainFrame()) {
-        m_webView->setRootGraphicsLayer(rootLayer);
-    } else {
-        WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(localRoot);
-        // FIXME: The following conditional is only needed for staging until the
-        // Chromium patch lands that instantiates a WebFrameWidget.
-        if (!webFrame->frameWidget()) {
-            m_webView->setRootGraphicsLayer(rootLayer);
-            return;
-        }
-        DCHECK(webFrame);
-        DCHECK(webFrame->frameWidget());
-        toWebFrameWidgetImpl(webFrame->frameWidget())->setRootGraphicsLayer(rootLayer);
-    }
+    WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(localFrame)->localRoot();
+    DCHECK(webFrame->frameWidget() || !rootLayer);

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

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?

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

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.

[dcheng, 2016/06/09 04:43:51]

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.

+    if (webFrame->frameWidget())
+        webFrame->frameWidget()->setRootGraphicsLayer(rootLayer);
 }
 
 void ChromeClientImpl::didPaint(const PaintArtifact& paintArtifact)
@@ -813,44 +801,17 @@ void ChromeClientImpl::didPaint(const PaintArtifact& paintArtifact)
     m_webView->getPaintArtifactCompositor().update(paintArtifact);
 }
 
-void ChromeClientImpl::attachCompositorAnimationTimeline(CompositorAnimationTimeline* compositorTimeline, LocalFrame* localRoot)
+void ChromeClientImpl::attachCompositorAnimationTimeline(CompositorAnimationTimeline* compositorTimeline, LocalFrame* localFrame)
 {
-    // FIXME: For top-level frames we still use the WebView as a WebWidget. This
-    // special case will be removed when top-level frames get WebFrameWidgets.
-    if (localRoot->isMainFrame()) {
-        m_webView->attachCompositorAnimationTimeline(compositorTimeline);
-    } else {
-        WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(localRoot);
-        // FIXME: The following conditional is only needed for staging until the
-        // Chromium patch lands that instantiates a WebFrameWidget.
-        if (!webFrame->frameWidget()) {
-            m_webView->attachCompositorAnimationTimeline(compositorTimeline);
-            return;
-        }
-        DCHECK(webFrame);
-        DCHECK(webFrame->frameWidget());
-        toWebFrameWidgetImpl(webFrame->frameWidget())->attachCompositorAnimationTimeline(compositorTimeline);
-    }
+    WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(localFrame)->localRoot();
+    webFrame->frameWidget()->attachCompositorAnimationTimeline(compositorTimeline);
 }
 
-void ChromeClientImpl::detachCompositorAnimationTimeline(CompositorAnimationTimeline* compositorTimeline, LocalFrame* localRoot)
+void ChromeClientImpl::detachCompositorAnimationTimeline(CompositorAnimationTimeline* compositorTimeline, LocalFrame* localFrame)
 {
-    // FIXME: For top-level frames we still use the WebView as a WebWidget. This
-    // special case will be removed when top-level frames get WebFrameWidgets.
-    if (localRoot->isMainFrame()) {
-        m_webView->detachCompositorAnimationTimeline(compositorTimeline);
-    } else {
-        WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(localRoot);
-        // FIXME: The following conditional is only needed for staging until the
-        // Chromium patch lands that instantiates a WebFrameWidget.
-        if (!webFrame->frameWidget()) {
-            m_webView->detachCompositorAnimationTimeline(compositorTimeline);
-            return;
-        }
-        DCHECK(webFrame);
-        DCHECK(webFrame->frameWidget());
-        toWebFrameWidgetImpl(webFrame->frameWidget())->detachCompositorAnimationTimeline(compositorTimeline);
-    }
+    WebLocalFrameImpl* webFrame = WebLocalFrameImpl::fromFrame(localFrame)->localRoot();
+    if (webFrame->frameWidget())

[dcheng, 2016/06/09 04:43:51]

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.

+        webFrame->frameWidget()->detachCompositorAnimationTimeline(compositorTimeline);
 }
 
 void ChromeClientImpl::enterFullScreenForElement(Element* element)