Always use the WebFrameWidget when attaching the root graphics

third_party/WebKit/Source/web/ChromeClientImpl.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 768 matching lines @@
     WebLocalFrameImpl* webframe = WebLocalFrameImpl::fromFrame(obj->getDocument()->axObjectCacheOwner().frame());
     if (webframe && webframe->client())
         webframe->client()->postAccessibilityEvent(WebAXObject(obj), toWebAXEvent(notification));
 }
 
 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)
 {
     // TODO(jbroman): This doesn't handle OOPIF correctly. We probably need a
     // branch for WebFrameWidget, like attachRootGraphicsLayer.
     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)
 {
     m_webView->enterFullScreenForElement(element);
 }
 
 void ChromeClientImpl::exitFullScreenForElement(Element* element)
 {
     m_webView->exitFullScreenForElement(element);
@@ skipping 251 matching lines @@
 {
     return adoptPtr(m_webView->scheduler()->createFrameScheduler(blameContext).release());
 }
 
 double ChromeClientImpl::lastFrameTimeMonotonic() const
 {
     return m_webView->lastFrameTimeMonotonic();
 }
 
 } // namespace blink