cc/blimp: Eliminate Layer updates for common cases.

cc/blimp/layer_tree_host_remote.cc

Index: cc/blimp/layer_tree_host_remote.cc
diff --git a/cc/blimp/layer_tree_host_remote.cc b/cc/blimp/layer_tree_host_remote.cc
index b5d129f98fa96827229497c63be9b1e9ae1afbf5..e92685b7c28d1acd35e5496d63a1f7eb8971e263 100644
--- a/cc/blimp/layer_tree_host_remote.cc
+++ b/cc/blimp/layer_tree_host_remote.cc
@@ -29,6 +29,29 @@ namespace {
 base::TimeDelta kDefaultFrameInterval = base::TimeDelta::FromMilliseconds(16);
 
 static base::StaticAtomicSequenceNumber s_layer_tree_host_sequence_number;
+
+bool ShouldUpdateLayer(Layer* layer) {
+  // If the embedder has marked the layer as non-drawable.
+  if (!layer->DrawsContent())
+    return false;
+
+  // If the layer bounds are empty.
+  if (layer->bounds().IsEmpty())
+    return false;
+
+  // If the layer is transparent and has no background filters applied.
+  // See EffectTree::UpdateIsDrawn for the logic details.
+  // A few things have been ignored:
+  // 1) We don't support threaded animations at the moment, so the opacity can
+  //    not change on the client.
+  // 2) This does not account for layer hiding its subtree, but that is never
+  //    used by the renderer.

[ajuma, 2016/11/01 19:27:58]

It's true that the renderer never uses the HideLayerAndSubtree call, but there's
still the case of a layer with opacity 0 causing its subtree to get skipped.
Those layers won't get skipped with the logic below (but that might be fine --
the amount of work saved by skipping is small).

[Khushal, 2016/11/01 21:09:01]

You're right, thanks! I've added that in the comment. Will come back here later,
I'm going to leave the bug open for now.

Also, while writing the test, I ran into a problem because
CallFunctionForEveryLayer wouldn't run on a mask layer's subtree. Is that
expected? Is the mask layer not supposed to have any children?

[ajuma, 2016/11/01 21:37:07]

Right, a mask layer shouldn't have children, the mask layer itself is supposed
to represent the entire mask.

[Khushal, 2016/11/01 21:42:23]

Ohh, makes sense. Would it be a good idea to add a DCHECK in Layer::SetMaskLayer
for this? Makes it obvious to someone reading it.

[ajuma, 2016/11/01 21:47:20]

Yes, that's an excellent idea!

[Khushal, 2016/11/01 23:21:00]

Done. :)

+  if (layer->opacity() == 0.f && layer->background_filters().IsEmpty())
+    return false;
+
+  return true;
+}
+
 }  // namespace
 
 LayerTreeHostRemote::InitParams::InitParams() = default;
@@ -363,6 +386,8 @@ void LayerTreeHostRemote::BeginMainFrame() {
     // layers. See crbug.com/650885.
     LayerTreeHostCommon::CallFunctionForEveryLayer(
         layer_tree_.get(), [&layer_list](Layer* layer) {
+          if (!ShouldUpdateLayer(layer))
+            return;
           layer->SavePaintProperties();
           layer_list.push_back(layer);
         });