Introduce WebFrameWidget to Blink

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 74 matching lines @@
 #include "public/web/WebUserGestureToken.h"
 #include "public/web/WebViewClient.h"
 #include "public/web/WebWindowFeatures.h"
 #include "web/ColorChooserPopupUIController.h"
 #include "web/ColorChooserUIController.h"
 #include "web/DateTimeChooserImpl.h"
 #include "web/ExternalDateTimeChooser.h"
 #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"
 #include "web/WebPopupMenuImpl.h"
 #include "web/WebSettingsImpl.h"
 #include "web/WebViewImpl.h"
 #include "wtf/text/CString.h"
 #include "wtf/text/StringBuilder.h"
 #include "wtf/text/StringConcatenate.h"
 #include "wtf/unicode/CharacterNames.h"
@@ skipping 380 matching lines @@
     invalidateContentsAndRootView(updateRect);
 }
 
 void ChromeClientImpl::scheduleAnimation()
 {
     m_webView->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
 {
     IntRect screenRect(rect);
 
     if (m_webView->client()) {
         WebRect windowRect = m_webView->client()->windowRect();
         screenRect.move(windowRect.x, windowRect.y);
     }
@@ skipping 181 matching lines @@
     return false;
 }
 
 GraphicsLayerFactory* ChromeClientImpl::graphicsLayerFactory() const
 {
     return m_webView->graphicsLayerFactory();
 }
 
 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)
 {
     m_webView->enterFullScreenForElement(element);
 }
 
 void ChromeClientImpl::exitFullScreenForElement(Element* element)
 {
     m_webView->exitFullScreenForElement(element);
@@ skipping 153 matching lines @@
 {
     // FIXME: remove. See http://crbug.com/425756
     if (m_webView->autofillClient())
         m_webView->autofillClient()->openTextDataListChooser(WebInputElement(&input));
     WebLocalFrameImpl* webframe = WebLocalFrameImpl::fromFrame(input.document().frame());
     if (webframe->autofillClient())
         webframe->autofillClient()->openTextDataListChooser(WebInputElement(&input));
 }
 
 } // namespace blink