Introduce WebFrameWidget to Blink

Source/web/ChromeClientImpl.cpp

Index: Source/web/ChromeClientImpl.cpp
diff --git a/Source/web/ChromeClientImpl.cpp b/Source/web/ChromeClientImpl.cpp
index 080d23627072e3a2a1c9b8e4e8638356c79de991..3f3f3785662240c5b7191e4b1f0be661e0ffd9c2 100644
--- a/Source/web/ChromeClientImpl.cpp
+++ b/Source/web/ChromeClientImpl.cpp
@@ -92,6 +92,7 @@
 #include "web/ExternalPopupMenu.h"
 #include "web/PopupMenuChromium.h"
 #include "web/WebFileChooserCompletionImpl.h"
+#include "web/WebFrameWidgetImpl.h"
 #include "web/WebInputEventConversion.h"
 #include "web/WebLocalFrameImpl.h"
 #include "web/WebPluginContainerImpl.h"
@@ -492,8 +493,13 @@ void ChromeClientImpl::scheduleAnimation()
 
 void ChromeClientImpl::scheduleAnimationForFrame(LocalFrame* localRoot)
 {
-    // FIXME: This will proxy to a WebWidget attached to the WebLocalFrameImpl.
-    scheduleAnimation();
+    ASSERT(WebLocalFrameImpl::fromFrame(localRoot));
+    // If the frame is still being created, it might not yet have a WebWidget.

[dcheng, 2014/12/10 21:00:58]

I guess I don't understand--why are we scheduling animations before all our
state is set up? That seems hard to reason about.

[kenrb, 2014/12/11 20:21:21]

It is called during attachment of the Document's renderer, which is part of
frame initialization. Not totally unreasonable imo.

[dcheng, 2014/12/11 23:31:45]

Is this going to be temporary code? I'm OK with this on a temporary basis, but
in the long run, local roots should have a frame widget before we call this
code.

[kenrb, 2014/12/16 18:51:00]

The call to ChromeClientImpl::scheduleAnimation() will eventually go away, but I
don't know what would be involved in trying to make this not get called during
Document creation. Why do you say this shouldn't happen?

[dcheng, 2014/12/16 20:13:06]

A WebView is a WebWidget, so you can never have one without the other. So
calling a WebWidget method on a WebView always works.

I'm OK with this if it's temporary. However, I would like us to use the same
model for WebFrameWidget. If a WebLocalFrame is a local root, it should always
have a WebFrameWidget. It should never be the case that we create a local root
WebLocalFrame and then attach the WebFrameWidget later.

This helps simplify the rest of the code; we don't need to null check frame
widget if we have a local root: we know it's there, by definition.

+    if (!WebLocalFrameImpl::fromFrame(localRoot)->frameWidget()) {

[Nate Chapin, 2014/12/10 19:19:03]

Style nit: {} not necessary on this if-else

[kenrb, 2014/12/11 20:21:21]

Done.

+        scheduleAnimation();
+    } else {
+        WebLocalFrameImpl::fromFrame(localRoot)->frameWidget()->scheduleAnimation();
+    }
 }
 
 IntRect ChromeClientImpl::rootViewToScreen(const IntRect& rect) const
@@ -695,8 +701,15 @@ GraphicsLayerFactory* ChromeClientImpl::graphicsLayerFactory() const
 
 void ChromeClientImpl::attachRootGraphicsLayer(GraphicsLayer* rootLayer, LocalFrame* localRoot)
 {
-    // FIXME: Add call to frame's widget for non-zero frames.
-    m_webView->setRootGraphicsLayer(rootLayer);
+    // 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);
+        ASSERT(webFrame && webFrame->frameWidget());

[ncarter (slow), 2014/12/10 20:16:06]

After patching in this change, this ASSERT seems to fire in one of the
browser_tests, causing a failure in the test. This was on Windows in a debug
build. frameWidget() returns null. You can repro by running (and attaching a
debugger to the child processes):

browser_tests.exe --single_process
--gtest_filter=TaskManagerOOPIFBrowserTest.KillSubframe/1
--wait-for-debugger-children

The test is failing roughly at this point in time.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tas...

Not sure if it's the b.com or c.com subframe process that's dying.

[kenrb, 2014/12/11 20:21:21]

This is an artifact of me not testing without the Chromium patch in place. :(

I have added a temporary condition to catch this.

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