Clean up remaining TODOs for moving RenderWidget to RenderFrameImpl

This removes the remaining TODOs pointing to bug 419087, 583347, and
912193 when they are about RenderWidget and not RenderWidgetHost. TODOs
about the RenderWidgetHost are clarified to explain that RenderWidget is
destroyed now.

R=avi@chromium.org

Bug: 419087, 583347, 912193
Change-Id: Ia491d2644997bac9d93bd319fb2ca0133997d86e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2006209Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Auto-Submit: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737954}

content/browser/frame_host/render_frame_host_manager.cc

@@ -2511,9 +2511,13 @@ void RenderFrameHostManager::CommitPending(
     }
   }
 
-  // For top-level frames, the RenderWidget{Host} will not be destroyed when the
+  // For top-level frames, the RenderWidgetHost will not be destroyed when the
   // local frame is detached. https://crbug.com/419087
   //
+  // The RenderWidget in the renderer process is destroyed, but the
+  // RenderWidgetHost and RenderWidgetHostView are still kept alive for a remote
+  // main frame.
+  //
   // To work around that, we hide it here. Truly this is to hit all the hide
   // paths in the browser side, but has a side effect of also hiding the
   // renderer side RenderWidget, even though it will get frozen anyway in the
@@ -2524,15 +2528,15 @@ void RenderFrameHostManager::CommitPending(
   // RenderFrameHost crashed.
   //
   // TODO(crbug.com/419087): This is only done for the main frame, as for sub
-  // frames the RenderWidget and its view will be destroyed when the frame is
-  // detached, but for the main frame it is not. This call to Hide() can go away
-  // when the main frame's RenderWidget is destroyed on frame detach. Note that
-  // calling this on a subframe that is not a local root would be incorrect as
-  // it would hide an ancestor local root's RenderWidget when that frame is not
-  // necessarily navigating. Removing this Hide() has previously been attempted
-  // without success in r426913 (https://crbug.com/658688) and r438516 (broke
-  // assumptions about RenderWidgetHosts not changing RenderWidgetHostViews over
-  // time).
+  // frames the RenderWidgetHost and its view will be destroyed when the frame
+  // is detached, but for the main frame it is not. This call to Hide() can go
+  // away when the main frame's RenderWidgetHost is destroyed on frame detach.
+  // Note that calling this on a subframe that is not a local root would be
+  // incorrect as it would hide an ancestor local root's RenderWidget when that
+  // frame is not necessarily navigating. Removing this Hide() has previously
+  // been attempted without success in r426913 (https://crbug.com/658688) and
+  // r438516 (broke assumptions about RenderWidgetHosts not changing
+  // RenderWidgetHostViews over time).
   //
   // |old_rvh| and |new_rvh| can be the same when navigating same-site from a
   // crashed RenderFrameHost. When RenderDocument will be implemented, this will

content/browser/renderer_host/render_view_host_impl.cc

@@ -860,9 +860,6 @@ void RenderViewHostImpl::OnRouteCloseEvent() {
   // This is only used when the RenderViewHost is not active, to signal to
   // the active RenderViewHost that JS has requested the page to close.
   //
-  // TODO(https://crbug.com/419087): Move to RenderFrameHost or
-  // RenderFrameProxyHost.
-  //
   // The delegate will route the close request to the active RenderViewHost.
   delegate_->RouteCloseEvent(this);
 }

content/common/view_messages.h

@@ -185,9 +185,6 @@ IPC_MESSAGE_ROUTED1(ViewHostMsg_ShowFullscreenWidget,
 
 // Sent from an inactive renderer for the browser to route to the active
 // renderer, instructing it to close.
-//
-// TODO(http://crbug.com/419087): Move this thing to Frame as it's a signal
-// from a swapped out frame to the mainframe of the frame tree.
 IPC_MESSAGE_ROUTED0(ViewHostMsg_RouteCloseEvent)
 
 // Indicates that the current page has been closed, after a ClosePage

content/renderer/compositor/layer_tree_view.cc

@@ -228,26 +228,10 @@ void LayerTreeView::RequestNewLayerTreeFrameSink() {
   if (!delegate_)
     return;
   // When the compositor is not visible it would not request a
-  // LayerTreeFrameSink so this is a race where it requested one then became
-  // not-visible. In that case, we can wait for it to become visible again
-  // before replying.
-  //
-  // This deals with an insidious race when the RenderWidget is swapped-out
-  // after this task was posted from the compositor. When swapped-out the
-  // compositor is stopped by making it not visible, and the RenderWidget (our
-  // delegate) is a zombie which can not be used (https://crbug.com/894899).
-  // Eventually the RenderWidget/LayerTreeView will not exist at all in this
-  // case (https://crbug.com/419087). So this handles the case for now since the
-  // compositor is not visible, and we can avoid using the RenderWidget until
-  // it marks the compositor visible again, indicating that it is valid to use
-  // the RenderWidget again.
-  //
-  // If there is no compositor thread, then this is a test-only path where
-  // composite is controlled directly by blink, and visibility is not
-  // considered. We don't expect blink to ever try composite on a swapped-out
-  // RenderWidget, which would be a bug, but the race condition can't happen
-  // in the single-thread case since this isn't a posted task.
-  if (!layer_tree_host_->IsVisible() && !!compositor_thread_) {
+  // LayerTreeFrameSink so this is a race where it requested one on the
+  // compositor thread while becoming non-visible on the main thread. In that
+  // case, we can wait for it to become visible again before replying.
+  if (!layer_tree_host_->IsVisible()) {
     layer_tree_frame_sink_request_failed_while_invisible_ = true;
     return;
   }
@@ -261,6 +245,10 @@ void LayerTreeView::DidInitializeLayerTreeFrameSink() {}
 void LayerTreeView::DidFailToInitializeLayerTreeFrameSink() {
   if (!delegate_)
     return;
+  // When the RenderWidget is made hidden while an async request for a
+  // LayerTreeFrameSink is being processed, then if it fails we would arrive
+  // here. Since the compositor does not request a LayerTreeFrameSink while not
+  // visible, we can delay trying again until becoming visible again.
   if (!layer_tree_host_->IsVisible()) {
     layer_tree_frame_sink_request_failed_while_invisible_ = true;
     return;

content/renderer/render_view_impl.h

@@ -87,20 +87,9 @@ class CreateViewParams;
 // the owner of it. Thus a tab may have multiple RenderViewImpls, one for the
 // main frame, and one for each other frame tree generated.
 //
-// The RenderViewImpl manages a WebView object from blink, which hosts the
-// web page and a blink frame tree. If the main frame (root of the tree) is
-// a local frame for this view, then it also manages a RenderWidget for the
-// main frame.
-//
-// The main distinction between RenderView and RenderWidget is that the
-// RenderView holds synchronized state across all processes participating in the
-// frame tree, whereas the RenderWidget holds per-root-frame state.
-//
-// TODO(419087): Currently even though the RenderViewImpl "manages" the
-// RenderWidget, the RenderWidget owns the RenderViewImpl. This is due to
-// RenderViewImpl historically being a subclass of RenderWidget. Breaking
-// the ownership relation will require moving the RenderWidget to the main
-// frame and updating all the blink objects to understand the lifetime changes.
+// When the main frame is part of this RenderViewImpl's frame tree, then this
+// object acts as the RenderWidgetDelegate for that frame's RenderWidget. Other
+// RenderWidgets would have a null RenderWidgetDelegate.
 class CONTENT_EXPORT RenderViewImpl : public blink::WebViewClient,
                                       public IPC::Listener,
                                       public RenderWidgetDelegate,
@@ -302,11 +291,6 @@ class CONTENT_EXPORT RenderViewImpl : public blink::WebViewClient,
   void ApplyPageVisibilityState(PageVisibilityState visibility_state,
                                 bool initial_setting);
 
-  // Closes the main frame RenderWidget.
-  // TODO(crbug.com/419087): Move ownership of the RenderWidget to
-  // RenderFrameImpl.
-  void CloseMainFrameRenderWidget();
-
  private:
   // For unit tests.
   friend class DevToolsAgentTest;

content/renderer/render_widget_delegate.h

@@ -17,9 +17,8 @@ namespace content {
 //
 // RenderWidgetDelegate
 //
-//  An interface implemented by an object owning a RenderWidget. This is
-//  intended to be temporary until the RenderViewImpl and RenderWidget classes
-//  are disentangled; see https://crbug.com/583347 and https://crbug.com/478281.
+//  An interface to provide View-level (and/or Page-level) functionality to
+//  the main frame's RenderWidget.
 class CONTENT_EXPORT RenderWidgetDelegate {
  public:
   virtual ~RenderWidgetDelegate() = default;

content/renderer/text_input_client_observer.cc

@@ -55,20 +55,13 @@ bool TextInputClientObserver::OnMessageReceived(const IPC::Message& message) {
 }
 
 bool TextInputClientObserver::Send(IPC::Message* message) {
-  // This class is attached to the main frame RenderWidget, but sends and
-  // receives messages while the main frame is remote (and the RenderWidget is
-  // destroyed). The messages are not received on RenderWidgetHostImpl, so
-  // there's no need to send through RenderWidget or use its routing id. We
-  // avoid this problem then by sending directly through RenderThread instead of
-  // through RenderWidget::Send().
-  // TODO(crbug.com/669219): This class should not be used while the main frame
-  // is remote.
+  // This class is attached to the main frame RenderWidget, but its messages
+  // are not received on RenderWidgetHostImpl, so there's no need to send
+  // through RenderWidget or use its routing id.
   return RenderThread::Get()->Send(message);
 }
 
 blink::WebFrameWidget* TextInputClientObserver::GetWebFrameWidget() const {
-  if (!render_widget_)
-    return nullptr;
   return static_cast<blink::WebFrameWidget*>(render_widget_->GetWebWidget());
 }
 

content/renderer/text_input_client_observer.h

@@ -42,14 +42,7 @@ class TextInputClientObserver : public IPC::Listener, public IPC::Sender {
   bool Send(IPC::Message* message) override;
 
  private:
-  // The WebFrameWidget corresponding to this TextInputClientObserver. While the
-  // main frame is remote, the value returned from this method for a main frame
-  // RenderWidget is null.
-  // TODO(crbug.com/669219): The browser shouldn't be sending IPCs that land
-  // in this class when the main frame is remote.
-  // TODO(crbug.com/419087): The lifetime of the WebFrameWidget should
-  // eventually be tied to the lifetime of the RenderWidget so this would never
-  // return null.
+  // The WebFrameWidget corresponding to this TextInputClientObserver.
   blink::WebFrameWidget* GetWebFrameWidget() const;
 
   blink::WebLocalFrame* GetFocusedFrame() const;

third_party/blink/renderer/core/exported/web_view_impl.cc

@@ -1219,11 +1219,8 @@ void WebViewImpl::Close() {
   AsView().page->WillBeDestroyed();
 
   // The main frame being detached in WillBeDestroyed() will make use of this
-  // which happens in Page::WillBeDestroyed(). But since the RenderWidget lives
   // |animation_host_| through its WebFrameWidget, before causing the
-  // forever (https://crbug.com/419087), the WebWidget is not closed elsewhere.
   // WebWidgetClient and the AnimationHost to be destroyed. So this is nulled
-  // So we close it here but try to simulate the same ordering by closing it
   // out after detaching the main frame.
   animation_host_ = nullptr;
 
@@ -2043,10 +2040,6 @@ void WebViewImpl::DidAttachLocalMainFrame() {
 void WebViewImpl::DidDetachLocalMainFrame() {
   // The WebWidgetClient that generated the |scoped_defer_main_frame_update_|
   // for a local main frame is going away.
-  // TODO(crbug.com/419087): For now, the WebWidgetClient (aka RenderWidget)
-  // is not destroyed, so this comment is not true, but it will be in the
-  // future. All references between |this| and the WebWidgetClient should be
-  // dropped regardless.
   scoped_defer_main_frame_update_ = nullptr;
 }
 

third_party/blink/renderer/core/frame/frame_test_helpers.cc

@@ -648,6 +648,7 @@ content::LayerTreeView* LayerTreeViewFactory::Initialize(
       &fake_thread_scheduler_);
   layer_tree_view_->Initialize(settings,
                                std::make_unique<cc::TestUkmRecorderFactory>());
+  layer_tree_view_->SetVisible(true);
   return layer_tree_view_.get();
 }
 

third_party/blink/renderer/core/page/chrome_client_impl.cc

@@ -788,9 +788,7 @@ void ChromeClientImpl::AttachCompositorAnimationTimeline(
   DCHECK(Platform::Current()->IsThreadedAnimationEnabled());
   WebLocalFrameImpl* web_frame = WebLocalFrameImpl::FromFrame(local_frame);
   WebFrameWidgetBase* widget = web_frame->LocalRootFrameWidget();
-  // TODO(crbug.com/912193): This is called while a frame is attached so widget
-  // is never null, right?
-  CHECK(widget);
+  DCHECK(widget);
   widget->AnimationHost()->AddAnimationTimeline(
       compositor_timeline->GetAnimationTimeline());
 }
@@ -801,9 +799,7 @@ void ChromeClientImpl::DetachCompositorAnimationTimeline(
   DCHECK(Platform::Current()->IsThreadedAnimationEnabled());
   WebLocalFrameImpl* web_frame = WebLocalFrameImpl::FromFrame(local_frame);
   WebFrameWidgetBase* widget = web_frame->LocalRootFrameWidget();
-  // TODO(crbug.com/912193): This should not be called after Document::Shutdown,
-  // so widget is never null, right?
-  CHECK(widget);
+  DCHECK(widget);
   widget->AnimationHost()->RemoveAnimationTimeline(
       compositor_timeline->GetAnimationTimeline());
 }