Fix RenderWidget's WebWidget references.

RenderWidget holds onto a WebWidget. It has two methods GetFrameWidget() and
GetWebWidget() which should both just reference the WebWidget. Instead,
RenderWidget will [inappropriately] call out to its delegate to get the
FrameWebWidget, which the WebWidget should already be. This induces confusion.

This CL fixes the RenderWidget's webwidget_internal_ to always be a
WebFrameWidget when the RenderWidget is associated with a RenderView. This
changes the semantics, as we will never pass a WebViewImpl as a
RenderWidget's webwidget_internal_. This has one main side effect -- a remote
main frame's RenderWidget will now have a null WebWidget.

Change-Id: I930a9c91a88ac05882289df795ed1c4008e146fd
Bug: 995981, 419087
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1766889Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690968}

content/renderer/render_view_impl.cc

@@ -520,7 +520,9 @@ void RenderViewImpl::Initialize(
     // only requires single ownership and adding scoped_refptr<RenderWidget>
     // muddies this unnecessarily -- especially since this RenderWidget should
     // ultimately be own by the main frame.
-    GetWidget()->Init(std::move(show_callback), webview_->MainFrameWidget());
+    // We intentionally pass in a null webwidget since it shouldn't be needed
+    // for remote frames.
+    GetWidget()->Init(std::move(show_callback), nullptr);
 
     RenderFrameProxy::CreateFrameProxy(params->proxy_routing_id, GetRoutingID(),
                                        opener_frame, MSG_ROUTING_NONE,
@@ -1063,10 +1065,6 @@ const blink::WebView* RenderViewImpl::webview() const {
 
 // RenderWidgetOwnerDelegate -----------------------------------------
 
-blink::WebWidget* RenderViewImpl::GetWebWidgetForWidget() const {
-  return frame_widget_;
-}
-
 bool RenderViewImpl::RenderWidgetWillHandleMouseEventForWidget(
     const blink::WebMouseEvent& event) {
   // If the mouse is locked, only the current owner of the mouse lock can
@@ -2077,9 +2075,9 @@ void RenderViewImpl::OnTextAutosizerPageInfoChanged(
 }
 
 void RenderViewImpl::SetFocus(bool enable) {
-  // This is not an IPC message, don't go through the IPC handler. This is used
-  // in cases where the IPC message should not happen.
-  GetWidget()->SetFocus(enable);
+  // This is only called from RenderFrameProxy.
+  CHECK(!webview()->MainFrame()->IsWebLocalFrame());
+  webview()->SetFocus(enable);
 }
 
 void RenderViewImpl::ZoomLimitsChanged(double minimum_level,

content/renderer/render_view_impl.h

@@ -383,7 +383,6 @@ class CONTENT_EXPORT RenderViewImpl : public blink::WebViewClient,
 
   // RenderWidgetDelegate implementation ----------------------------------
 
-  blink::WebWidget* GetWebWidgetForWidget() const override;
   bool RenderWidgetWillHandleMouseEventForWidget(
       const blink::WebMouseEvent& event) override;
   void SetActiveForWidget(bool active) override;

content/renderer/render_widget.cc

@@ -522,6 +522,11 @@ void RenderWidget::Init(ShowCallback show_callback, WebWidget* web_widget) {
   input_handler_ = std::make_unique<RenderWidgetInputHandler>(this, this);
 
   LayerTreeView* layer_tree_view = InitializeLayerTreeView();
+
+  // TODO(https://crbug.com/995981): This conditional is temporary logic to
+  // handle the case of remote main frame RenderWidgets [which shouldn't exist
+  // to begin with].
+  if (web_widget)
     web_widget->SetAnimationHost(layer_tree_view->animation_host());
 
   blink::scheduler::WebThreadScheduler* main_thread_scheduler = nullptr;
@@ -2002,20 +2007,7 @@ blink::WebFrameWidget* RenderWidget::GetFrameWidget() const {
   if (closing_)
     return nullptr;
 
-  blink::WebWidget* widget;
-  if (delegate()) {
-    // Main frame WebFrameWidgets are held by the delegate, the internal widget
-    // points directly to the WebView.
-    // TODO(ekaramad): We should drop IPCs when |is_frozen_| instead of
-    // handling them and finding a null here. However there is also the case
-    // of the frame being detached without the widget being frozen to be
-    // resolved (https://crbug.com/906340). So for now this can return null.
-    widget = delegate()->GetWebWidgetForWidget();
-  } else {
-    // Subframes always have a WebFrameWidget themselves.
-    widget = webwidget_internal_;
-  }
-  return static_cast<blink::WebFrameWidget*>(widget);
+  return static_cast<blink::WebFrameWidget*>(webwidget_internal_);
 }
 
 void RenderWidget::ScreenRectToEmulatedIfNeeded(WebRect* window_rect) const {
@@ -3716,11 +3708,6 @@ void RenderWidget::DidNavigate() {
 }
 
 blink::WebWidget* RenderWidget::GetWebWidget() const {
-  if (delegate()) {
-    blink::WebWidget* delegate_widget = delegate()->GetWebWidgetForWidget();
-    if (delegate_widget)
-      return delegate_widget;
-  }
   return webwidget_internal_;
 }
 
@@ -3890,6 +3877,11 @@ base::WeakPtr<RenderWidget> RenderWidget::AsWeakPtr() {
 }
 
 void RenderWidget::SetWebWidgetInternal(blink::WebWidget* web_widget) {
+  // TODO(https://crbug.com/995981): This method should not need to exist, since
+  // we should be creating and destroying a RenderWidget along with the
+  // WebWidget.
+  if (web_widget)
+    web_widget->SetAnimationHost(layer_tree_view_->animation_host());
   webwidget_internal_ = web_widget;
 }
 

content/renderer/render_widget_browsertest.cc

@@ -8,6 +8,7 @@
 #include "content/common/visual_properties.h"
 #include "content/public/renderer/render_frame_visitor.h"
 #include "content/public/test/render_view_test.h"
+#include "content/renderer/compositor/layer_tree_view.h"
 #include "content/renderer/render_frame_proxy.h"
 #include "content/renderer/render_thread_impl.h"
 #include "content/renderer/render_view_impl.h"
@@ -223,4 +224,23 @@ TEST_F(RenderWidgetTest, PageFocusIme) {
             GetInputMethodController()->TextInputInfo().value.Utf8());
 }
 
+// Tests that the value of VisualProperties::is_pinch_gesture_active is
+// not propagated to the LayerTreeHost when properties are synced for main
+// frame.
+TEST_F(RenderWidgetTest, ActivePinchGestureUpdatesLayerTreeHost) {
+  auto* layer_tree_host = widget()->layer_tree_view()->layer_tree_host();
+  EXPECT_FALSE(layer_tree_host->is_external_pinch_gesture_active_for_testing());
+  content::VisualProperties visual_properties;
+
+  // Sync visual properties on a mainframe RenderWidget.
+  visual_properties.is_pinch_gesture_active = true;
+  widget()->OnSynchronizeVisualProperties(visual_properties);
+  // We do not expect the |is_pinch_gesture_active| value to propagate to the
+  // LayerTreeHost for the main-frame. Since GesturePinch events are handled
+  // directly by the layer tree for the main frame, it already knows whether or
+  // not a pinch gesture is active, and so we shouldn't propagate this
+  // information to the layer tree for a main-frame's widget.
+  EXPECT_FALSE(layer_tree_host->is_external_pinch_gesture_active_for_testing());
+}
+
 }  // namespace content

content/renderer/render_widget_delegate.h

@@ -25,10 +25,6 @@ class CONTENT_EXPORT RenderWidgetDelegate {
  public:
   virtual ~RenderWidgetDelegate() = default;
 
-  // Returns the WebWidget if the delegate has one. Otherwise it returns null,
-  // and RenderWidget will fall back to its own WebWidget.
-  virtual blink::WebWidget* GetWebWidgetForWidget() const = 0;
-
   // As in RenderWidgetInputHandlerDelegate. Return true if the event was
   // handled.
   virtual bool RenderWidgetWillHandleMouseEventForWidget(

content/renderer/render_widget_unittest.cc

@@ -521,7 +521,6 @@ class RenderWidgetPopupUnittest : public testing::Test {
 
 class StubRenderWidgetDelegate : public RenderWidgetDelegate {
  public:
-  blink::WebWidget* GetWebWidgetForWidget() const override { return nullptr; }
   bool RenderWidgetWillHandleMouseEventForWidget(
       const blink::WebMouseEvent& event) override {
     return false;
@@ -552,9 +551,8 @@ class StubRenderWidgetDelegate : public RenderWidgetDelegate {
 };
 
 // Tests that the value of VisualProperties::is_pinch_gesture_active is
-// propagated to the LayerTreeHost when properties are synced, but only for
-// subframe widgets.
-TEST_F(RenderWidgetUnittest, ActivePinchGestureUpdatesLayerTreeHost) {
+// propagated to the LayerTreeHost when properties are synced for subframes.
+TEST_F(RenderWidgetUnittest, ActivePinchGestureUpdatesLayerTreeHostSubFrame) {
   auto* layer_tree_host = widget()->layer_tree_view()->layer_tree_host();
   EXPECT_FALSE(layer_tree_host->is_external_pinch_gesture_active_for_testing());
   content::VisualProperties visual_properties;
@@ -573,20 +571,6 @@ TEST_F(RenderWidgetUnittest, ActivePinchGestureUpdatesLayerTreeHost) {
   visual_properties.is_pinch_gesture_active = false;
   widget()->OnSynchronizeVisualProperties(visual_properties);
   EXPECT_FALSE(layer_tree_host->is_external_pinch_gesture_active_for_testing());
-
-  // Repeat with a 'mainframe' widget.
-  std::unique_ptr<StubRenderWidgetDelegate> delegate =
-      std::make_unique<StubRenderWidgetDelegate>();
-  widget()->set_delegate(delegate.get());
-  visual_properties.is_pinch_gesture_active = true;
-  widget()->OnSynchronizeVisualProperties(visual_properties);
-  // We do not expect the |is_pinch_gesture_active| value to propagate to the
-  // LayerTreeHost for the main-frame. Since GesturePinch events are handled
-  // directly by the layer tree for the main frame, it already knows whether or
-  // not a pinch gesture is active, and so we shouldn't propagate this
-  // information to the layer tree for a main-frame's widget.
-  EXPECT_FALSE(layer_tree_host->is_external_pinch_gesture_active_for_testing());
-  DestroyWidget();
 }
 
 TEST_F(RenderWidgetPopupUnittest, EmulatingPopupRect) {
@@ -616,11 +600,6 @@ TEST_F(RenderWidgetPopupUnittest, EmulatingPopupRect) {
   std::unique_ptr<PopupRenderWidget> parent_widget(
       new PopupRenderWidget(&compositor_deps_));
 
-  // Emulation only happens for RenderWidgets with a delegate.
-  std::unique_ptr<StubRenderWidgetDelegate> delegate =
-      std::make_unique<StubRenderWidgetDelegate>();
-  parent_widget->set_delegate(delegate.get());
-
   // Setup emulation on the |parent_widget|.
   parent_widget->OnSynchronizeVisualProperties(visual_properties);
   parent_widget->OnEnableDeviceEmulation(emulation_params);

third_party/blink/public/web/web_view.h

@@ -102,6 +102,9 @@ class WebView {
   // Destroys the WebView.
   virtual void Close() = 0;
 
+  // Sets whether the WebView is focused.
+  virtual void SetFocus(bool enable) = 0;
+
   // Called to inform WebViewImpl that a local main frame has been attached.
   // After this call MainFrameImpl() will return a valid frame until it is
   // detached.