[Android] Stop hiding the RWHV layer subtree when hiding the widget

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index 41416bea1e8b63c40e88b5b15edfe1fe07983288..fd331520a79f283fc4c301f42849129da6f27817 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -596,23 +596,7 @@ void RenderWidgetHostViewAndroid::Show() {
     return;
 
   is_showing_ = true;
-  if (layer_.get())
-    layer_->SetHideLayerAndSubtree(false);
-
-  if (overscroll_controller_)
-    overscroll_controller_->Enable();
-
-  frame_evictor_->SetVisible(true);
-
-  if (!host_ || !host_->is_hidden())
-    return;
-
-  host_->WasShown(ui::LatencyInfo());
-
-  if (content_view_core_) {
-    StartObservingRootWindow();
-    RequestVSyncUpdate(BEGIN_FRAME);
-  }
+  ShowInternal();
 }
 
 void RenderWidgetHostViewAndroid::Hide() {
@@ -620,27 +604,10 @@ void RenderWidgetHostViewAndroid::Hide() {
     return;
 
   is_showing_ = false;
-  if (layer_.get() && locks_on_frame_count_ == 0)
-    layer_->SetHideLayerAndSubtree(true);
-
-  if (overscroll_controller_)
-    overscroll_controller_->Disable();
-
-  frame_evictor_->SetVisible(false);
-  // We don't know if we will ever get a frame if we are hiding the renderer, so
-  // we need to cancel all requests
-  AbortPendingReadbackRequests();
 
-  RunAckCallbacks(cc::SurfaceDrawStatus::DRAW_SKIPPED);
-
-  if (!host_ || host_->is_hidden())
-    return;
-
-  // Inform the renderer that we are being hidden so it can reduce its resource
-  // utilization.
-  host_->WasHidden();
-
-  StopObservingRootWindow();
+  bool hide_frontbuffer = true;
+  bool stop_observing_root_window = true;
+  HideInternal(hide_frontbuffer, stop_observing_root_window);
 }
 
 bool RenderWidgetHostViewAndroid::IsShowing() {
@@ -1487,6 +1454,61 @@ void RenderWidgetHostViewAndroid::AcceleratedSurfaceInitialized(int route_id) {
   accelerated_surface_route_id_ = route_id;
 }
 
+void RenderWidgetHostViewAndroid::ShowInternal() {
+  DCHECK(is_showing_);
+
+  bool was_showing_frontbuffer =
+      layer_.get() && !layer_->hide_layer_and_subtree();
+  if (!was_showing_frontbuffer) {
+    if (layer_.get())
+      layer_->SetHideLayerAndSubtree(false);
+    frame_evictor_->SetVisible(true);
+  }
+
+  if (overscroll_controller_)
+    overscroll_controller_->Enable();
+
+  if (!host_ || !host_->is_hidden())
+    return;

[no sievers, 2015/03/19 20:21:52]

Can we move this early-out to the top?

We should never drop the frontbuffer, while not telling the renderer to hide.
Then it should also avoid redundant frame_evictor_->SetVisible() calls, and
don't need the |was_showing_frontbuffer| check.

[jdduke (slow), 2015/03/20 14:59:29]

We can probably move this call up, but I'm not sure about the hide call. 

+
+  host_->WasShown(ui::LatencyInfo());
+
+  if (content_view_core_) {
+    StartObservingRootWindow();
+    RequestVSyncUpdate(BEGIN_FRAME);
+  }
+}
+
+void RenderWidgetHostViewAndroid::HideInternal(
+    bool hide_frontbuffer,
+    bool stop_observing_root_window) {
+  if (hide_frontbuffer) {
+    if (layer_.get() && locks_on_frame_count_ == 0)
+      layer_->SetHideLayerAndSubtree(true);
+
+    frame_evictor_->SetVisible(false);
+  }
+
+  if (stop_observing_root_window)
+    StopObservingRootWindow();
+
+  if (overscroll_controller_)
+    overscroll_controller_->Disable();
+
+  // We don't know if we will ever get a frame if we are hiding the renderer, so
+  // we need to cancel all requests
+  AbortPendingReadbackRequests();
+
+  RunAckCallbacks(cc::SurfaceDrawStatus::DRAW_SKIPPED);
+
+  if (!host_ || host_->is_hidden())

[no sievers, 2015/03/19 20:21:52]

Same here.

[jdduke (slow), 2015/03/20 14:59:29]

We can't move this call up because the the host may be hidden but the
frontbuffer is still showing.

+    return;
+
+  // Inform the renderer that we are being hidden so it can reduce its resource
+  // utilization.
+  host_->WasHidden();
+}
+
 void RenderWidgetHostViewAndroid::AttachLayers() {
   if (!content_view_core_)
     return;
@@ -1518,6 +1540,12 @@ void RenderWidgetHostViewAndroid::RequestVSyncUpdate(uint32 requests) {
 
   bool should_request_vsync = !outstanding_vsync_requests_ && requests;
   outstanding_vsync_requests_ |= requests;
+
+  // If the host has been hidden, defer vsync requests until it is shown
+  // again via |Show()|.
+  if (!host_ || host_->is_hidden())

[no sievers, 2015/03/18 21:04:48]

see question below

+    return;
+
   // Note that if we're not currently observing the root window, outstanding
   // vsync requests will be pushed if/when we resume observing in
   // |StartObservingRootWindow()|.
@@ -1527,6 +1555,7 @@ void RenderWidgetHostViewAndroid::RequestVSyncUpdate(uint32 requests) {
 
 void RenderWidgetHostViewAndroid::StartObservingRootWindow() {
   DCHECK(content_view_core_);
+  DCHECK(is_showing_);
   if (observing_root_window_)
     return;
 
@@ -1827,7 +1856,8 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
   if (!content_view_core_)
     return;
 
-  StartObservingRootWindow();
+  if (is_showing_)
+    StartObservingRootWindow();
 
   if (resize)
     WasResized();
@@ -1868,6 +1898,17 @@ void RenderWidgetHostViewAndroid::OnCompositingDidCommit() {
   RunAckCallbacks(cc::SurfaceDrawStatus::DRAWN);
 }
 
+void RenderWidgetHostViewAndroid::OnVisibilityChanged(bool visible) {
+  DCHECK(is_showing_);
+  if (visible) {
+    ShowInternal();
+  } else {
+    bool hide_frontbuffer = true;
+    bool stop_observing_root_window = false;
+    HideInternal(hide_frontbuffer, stop_observing_root_window);
+  }
+}
+
 void RenderWidgetHostViewAndroid::OnAttachCompositor() {
   DCHECK(content_view_core_);
   if (!overscroll_controller_)
@@ -1884,7 +1925,7 @@ void RenderWidgetHostViewAndroid::OnDetachCompositor() {
 void RenderWidgetHostViewAndroid::OnVSync(base::TimeTicks frame_time,
                                           base::TimeDelta vsync_period) {
   TRACE_EVENT0("cc,benchmark", "RenderWidgetHostViewAndroid::OnVSync");
-  if (!host_)
+  if (!host_ || host_->is_hidden())

[no sievers, 2015/03/18 21:04:48]

Doesn't the renderer turn beginFrame off when it gets hidden?  And why do we
need this check now and didn't before, is it because we now might keep observing
the root window while the window is not visible (and Hide() was not called)
while previously we relied on being completely hidden and cut off from
notifications?

Maybe it's good paranoia, but it does make it a bit more confusing on what the
vsync and beginFrame contract really is. We could also consider not ticking in
WindowAndroid if it's hidden.

[jdduke (slow), 2015/03/20 14:59:29]

Right, this preserves the original behavior which is having a hard cutoff of
BeginFrame dispatch. The compositor *should* unsubscribe from BeginFrame after
RenderWidget receives the hidden notification, but I kept the hidden check as a
precauation. I'd be OK with removing it, but I'm not sure there's a great
non-racy way to verify that we get the unsubscribe call.
Yeah, I'm fine restoring the original code, as our begin frame subscription
logic should in theory be enough to guard here. Either way.

     return;
 
   const uint32 current_vsync_requests = outstanding_vsync_requests_;
@@ -1907,6 +1948,18 @@ void RenderWidgetHostViewAndroid::OnAnimate(base::TimeTicks begin_frame_time) {
     SetNeedsAnimate();
 }
 
+void RenderWidgetHostViewAndroid::OnActivityResumed() {
+  DCHECK(is_showing_);
+  ShowInternal();
+}
+
+void RenderWidgetHostViewAndroid::OnActivityPaused() {
+  DCHECK(is_showing_);
+  bool hide_frontbuffer = false;
+  bool stop_observing_root_window = false;
+  HideInternal(hide_frontbuffer, stop_observing_root_window);
+}
+
 void RenderWidgetHostViewAndroid::OnLostResources() {
   ReleaseLocksOnSurface();
   if (layer_.get())