Refactor client visibility handling

cc/trees/layer_tree_host_impl.cc

Index: cc/trees/layer_tree_host_impl.cc
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index d7d0f4c22d156bb9797bef7a43a6dd004a967c6f..2e4fb876f2700845dc67daeee3e7440521586b1b 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -157,35 +157,6 @@ void RecordCompositorSlowScrollMetric(InputHandler::ScrollInputType type,
   }
 }
 
-// Calls SetClientVisible on the provided |context_provider| and handles
-// additional cache cleanup.
-void UpdateVisibilityForContextProvider(int client_id,
-                                        ContextProvider* context_provider,
-                                        bool is_visible) {
-  if (!context_provider)
-    return;
-  gpu::ContextSupport* context_support = context_provider->ContextSupport();
-
-  context_support->SetClientVisible(client_id, is_visible);
-  bool aggressively_free_resources = !context_support->AnyClientsVisible();
-  if (aggressively_free_resources) {
-    context_provider->DeleteCachedResources();
-  }
-  context_support->SetAggressivelyFreeResources(aggressively_free_resources);
-}
-
-// Same as UpdateVisibilityForContextProvider, except that the
-// |context_provider| is locked before being used.
-void LockAndUpdateVisibilityForContextProvider(
-    int client_id,
-    ContextProvider* context_provider,
-    bool is_visible) {
-  if (!context_provider)
-    return;
-  ContextProvider::ScopedContextLock hold(context_provider);
-  UpdateVisibilityForContextProvider(client_id, context_provider, is_visible);
-}
-
 }  // namespace
 
 DEFINE_SCOPED_UMA_HISTOGRAM_TIMER(PendingTreeDurationHistogramTimer,
@@ -1296,11 +1267,7 @@ void LayerTreeHostImpl::UpdateTileManagerMemoryPolicy(
     // are visible. Notify the worker context here. We handle becoming
     // invisible in NotifyAllTileTasksComplete to avoid interrupting running
     // work.
-    if (output_surface_) {
-      LockAndUpdateVisibilityForContextProvider(
-          id_, output_surface_->worker_context_provider(),
-          true /* is_visible */);
-    }
+    SetWorkerContextVisibility(true /* is_visible */);

[danakj, 2016/08/26 23:49:09]

i think the name of this bool is in the function name already

[ericrk, 2016/08/29 22:43:22]

Done.

 
     // If |global_tile_state_.hard_memory_limit_in_bytes| is greater than 0, we
     // allow the image decode controller to retain resources. We handle the
@@ -1390,11 +1357,7 @@ void LayerTreeHostImpl::NotifyAllTileTasksCompleted() {
       image_decode_controller_->SetShouldAggressivelyFreeResources(
           true /* aggressively_free_resources */);
     }
-    if (output_surface_) {
-      LockAndUpdateVisibilityForContextProvider(
-          id_, output_surface_->worker_context_provider(),
-          false /* is_visible */);
-    }
+    SetWorkerContextVisibility(false /* is_visible */);

[danakj, 2016/08/26 23:49:09]

dittos

[ericrk, 2016/08/29 22:43:23]

Done.

   }
 }
 
@@ -2132,10 +2095,7 @@ void LayerTreeHostImpl::SetVisible(bool visible) {
   }
 
   // Update visibility for the compositor context provider.
-  if (output_surface_) {
-    UpdateVisibilityForContextProvider(id_, output_surface_->context_provider(),
-                                       visible);
-  }
+  SetMainContextVisibility(visible);
 }
 
 void LayerTreeHostImpl::SetNeedsOneBeginImplFrame() {
@@ -2350,6 +2310,10 @@ void LayerTreeHostImpl::ReleaseOutputSurface() {
   CleanUpTileManagerAndUIResources();
   resource_provider_ = nullptr;
 
+  // Release any context visibility before we destroy the OutputSurface.
+  SetMainContextVisibility(false /* is_visible */);

[danakj, 2016/08/26 23:49:09]

dittos

[ericrk, 2016/08/29 22:43:23]

Done.

+  SetWorkerContextVisibility(false /* is_visible */);
+
   // Detach from the old output surface and reset |output_surface_| pointer
   // as this surface is going to be destroyed independent of if binding the
   // new output surface succeeds or not.
@@ -4129,4 +4093,48 @@ bool LayerTreeHostImpl::CommitToActiveTree() const {
   return !task_runner_provider_->HasImplThread();
 }
 
+void LayerTreeHostImpl::SetMainContextVisibility(bool is_visible) {
+  if (is_visible == !!main_context_visibility_)

[danakj, 2016/08/26 23:49:09]

Is it actually possible to have this early out happen and its not a bug?

[ericrk, 2016/08/29 22:43:22]

Sure, only change I may need to make is not call it with false/true directly, so
I need to check at the call site... but this would help us catch any bugs, so
probably best to remove this and handle exceptions at the call site.

+    return;
+
+  if (!output_surface_)
+    return;
+
+  auto* main_context = output_surface_->context_provider();
+

[danakj, 2016/08/26 23:49:08]

nit: remove whitespace

[ericrk, 2016/08/29 22:43:23]

Done.

+  if (!main_context)
+    return;
+
+  if (is_visible) {
+    main_context_visibility_ =
+        main_context->CacheController()->ClientBecameVisible();
+  } else {
+    main_context->CacheController()->ClientBecameNotVisible(
+        std::move(main_context_visibility_));
+  }
+}
+
+void LayerTreeHostImpl::SetWorkerContextVisibility(bool is_visible) {
+  if (is_visible == !!worker_context_visibility_)

[danakj, 2016/08/26 23:49:08]

Same Q

[ericrk, 2016/08/29 22:43:23]

Worker context visibility is handled through both direct visibility changes and
changes to the managed memory policy (if memory limit is zeroed we set to
invisible)... I'm not too confident that the memory policy changes are as
controlled as the visibility changes...

We should look into this more - it seems pretty messy to conflate these two...
and I'm not sure when we'd need to use memory policy rather than direct
visibility.  I think Android webview may do this. Let's look at this as a
follow-up, and leave this check for now (in the worker context case).

+    return;
+
+  if (!output_surface_)
+    return;
+
+  auto* worker_context = output_surface_->worker_context_provider();
+

[danakj, 2016/08/26 23:49:09]

nit: remove whitespace

fwiw I try to think of empty lines separating paragraphs of code, not
sentences.. aka for topic changes

[ericrk, 2016/08/29 22:43:23]

Done.

+  if (!worker_context)
+    return;
+
+  ContextProvider::ScopedContextLock hold(worker_context);
+

[danakj, 2016/08/26 23:49:09]

nit: remove whitespace

[ericrk, 2016/08/29 22:43:22]

Done.

+  if (is_visible) {
+    worker_context_visibility_ =
+        worker_context->CacheController()->ClientBecameVisible();
+  } else {
+    worker_context->CacheController()->ClientBecameNotVisible(
+        std::move(worker_context_visibility_));
+  }
+}
+
 }  // namespace cc