Move Start/Stop Compositor into RenderWidget::SetSwappedOut().

As pointed out in https://crrev.com/c/1336218 it's easy to miss the
Start/Stop Compositor when changing the RenderWidget's swapped out
state, and it looks like they are separate when not done with a single
method call. For safety and clarity, move them into SetSwappedOut.

R=piman@chromium.org

Change-Id: Ic45d5d911255886835e283a5777a3923f63e1830
Bug: 896836
Reviewed-on: https://chromium-review.googlesource.com/c/1338492Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608580}

content/renderer/render_frame_impl.cc

@@ -2235,11 +2235,8 @@ void RenderFrameImpl::OnSwapOut(
   if (is_main_frame_) {
     // The RenderWidget isn't actually closed here because we might need to use
     // it again. It can't be destroyed and recreated later as it is part of
-    // the |render_view_|, which must be kept alive. So instead just stop the
-    // compositor.
-    // TODO(crbug.com/419087): The RenderWidget should be destroyed as a result
-    // of this (main) frame going away, then we won't have to do this.
-    render_view_->GetWidget()->StopCompositor();
+    // the |render_view_|, which must be kept alive. So instead mark the widget
+    // as swapped out.
     render_view_->GetWidget()->SetSwappedOut(true);
   }
 
@@ -5868,16 +5865,8 @@ bool RenderFrameImpl::SwapIn() {
   if (is_main_frame_) {
     CHECK(!render_view_->main_render_frame_);
     render_view_->main_render_frame_ = this;
-    if (render_view_->GetWidget()->is_swapped_out()) {
+    if (render_view_->GetWidget()->is_swapped_out())
       render_view_->GetWidget()->SetSwappedOut(false);
-      // In OnSwapOut() the RenderWidget's compositor was stopped instead of
-      // deleting the RenderWidget. So here we can start it again. If the
-      // |render_view_|'s RenderWidget started swapped in, it may already be
-      // swapped in here, so this does nothing.
-      // TODO(crbug.com/419087): The RenderWidget should be newly created here,
-      // then we won't have to do this.
-      render_view_->GetWidget()->StartCompositor();
-    }
     render_view_->UpdateWebViewWithDeviceScaleFactor();
   }
 

content/renderer/render_frame_proxy.cc

@@ -146,13 +146,8 @@ RenderFrameProxy* RenderFrameProxy::CreateFrameProxy(
     // be updated, as the OnSwapOut flow which normally does this won't happen
     // in that case.  See https://crbug.com/653746 and
     // https://crbug.com/651980.
-    if (!render_widget->is_swapped_out()) {
-      // TODO(crbug.com/419087): The RenderWidget should be destroyed as a
-      // result of the frame going away, previously. Then we won't have to do
-      // this.
-      render_widget->StopCompositor();
+    if (!render_widget->is_swapped_out())
       render_widget->SetSwappedOut(true);
-    }
   } else {
     // Create a frame under an existing parent. The parent is always expected
     // to be a RenderFrameProxy, because navigations initiated by local frames

content/renderer/render_widget.cc

@@ -1575,6 +1575,29 @@ LayerTreeView* RenderWidget::InitializeLayerTreeView() {
   return layer_tree_view_.get();
 }
 
+void RenderWidget::StartCompositor() {
+  if (!is_hidden_)
+    layer_tree_view_->SetVisible(true);
+}
+
+void RenderWidget::StopCompositor() {
+  layer_tree_view_->SetVisible(false);
+  // Drop all gpu resources, this makes SetVisible(true) more expensive/slower
+  // but we don't expect to use this RenderWidget again until some possible
+  // future navigation. This brings us a bit closer to emulating deleting the
+  // RenderWidget instead of just stopping the compositor.
+  layer_tree_view_->ReleaseLayerTreeFrameSink();
+}
+
+void RenderWidget::SetSwappedOut(bool is_swapped_out) {
+  DCHECK_NE(is_swapped_out, is_swapped_out_);
+  is_swapped_out_ = is_swapped_out;
+  if (is_swapped_out)
+    StopCompositor();
+  else
+    StartCompositor();
+}
+
 void RenderWidget::DoDeferredClose() {
   // Prevent compositor from setting up new IPC channels, since we know a
   // WidgetMsg_Close is coming.
@@ -2916,20 +2939,6 @@ cc::ManagedMemoryPolicy RenderWidget::GetGpuMemoryPolicy(
   return actual;
 }
 
-void RenderWidget::StartCompositor() {
-  if (!is_hidden_)
-    layer_tree_view_->SetVisible(true);
-}
-
-void RenderWidget::StopCompositor() {
-  layer_tree_view_->SetVisible(false);
-  // Drop all gpu resources, this makes SetVisible(true) more expensive/slower
-  // but we don't expect to use this RenderWidget again until some possible
-  // future navigation. This brings us a bit closer to emulating deleting the
-  // RenderWidget instead of just stopping the compositor.
-  layer_tree_view_->ReleaseLayerTreeFrameSink();
-}
-
 void RenderWidget::HasPointerRawMoveEventHandlers(bool has_handlers) {
   if (input_event_queue_)
     input_event_queue_->HasPointerRawMoveEventHandlers(has_handlers);

content/renderer/render_widget.h

@@ -226,7 +226,7 @@ class CONTENT_EXPORT RenderWidget
   // sent (only ACKs) and the process is free to exit when there are no other
   // active RenderWidgets. The RenderWidget is not used for compositing as there
   // is no WebWidget that should display content when swapped out.
-  void SetSwappedOut(bool is_swapped_out) { is_swapped_out_ = is_swapped_out; }
+  void SetSwappedOut(bool is_swapped_out);
   bool is_swapped_out() const { return is_swapped_out_; }
 
   // This is true once a Close IPC has been received. The actual action of
@@ -364,11 +364,6 @@ class CONTENT_EXPORT RenderWidget
       const gfx::Size& initial_screen_size,
       float initial_device_scale_factor);
 
-  // Initiates the compositor to set up IPC channels and begin its scheduler.
-  void StartCompositor();
-  // Pauses the compositor's scheduler and tears down its IPC channels.
-  void StopCompositor();
-
   LayerTreeView* layer_tree_view() const { return layer_tree_view_.get(); }
   WidgetInputHandlerManager* widget_input_handler_manager() {
     return widget_input_handler_manager_.get();
@@ -567,8 +562,16 @@ class CONTENT_EXPORT RenderWidget
 
   static scoped_refptr<base::SingleThreadTaskRunner> GetCleanupTaskRunner();
 
+  // Creates the compositor, but leaves it in a stopped state, where it will
+  // not set up IPC channels or begin trying to produce frames until started
+  // via StartCompositor().
   LayerTreeView* InitializeLayerTreeView();
 
+  // Initiates the compositor to set up IPC channels and begin its scheduler.
+  void StartCompositor();
+  // Pauses the compositor's scheduler and tears down its IPC channels.
+  void StopCompositor();
+
   void DoDeferredClose();
   void NotifyOnClose();