cc: Push valid property values when CalcDrawProps skips layer.

If CalcDropProps does not run on a layer for any reason, then we
do not SavePaintProperties() on the layer. When we push the layer's
properties, we can not push the paint properties since they are not
valid. We should instead push the raw value on the layer.

This is safe because if we didn't SavePaintProperties() we will also
not call Update() on the layer, so there is no chance for painting
to change any property values. This is enforced by a new DCHECK()
in Layer::Update() and we have all the subclasses of Layer call up
to the Update() method.

R=boliu, enne
BUG=267574

Review URL: https://chromiumcodereview.appspot.com/21839004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@215512 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 187 insertions, 47 deletions

diff --git a/cc/layers/contents_scaling_layer.cc b/cc/layers/contents_scaling_layer.cc
index 03a790b..44a3f37 100644
--- a/cc/layers/contents_scaling_layer.cc
+++ b/cc/layers/contents_scaling_layer.cc
@@ -38,15 +38,17 @@ void ContentsScalingLayer::CalculateContentsScale(
 bool ContentsScalingLayer::Update(
     ResourceUpdateQueue* queue,
     const OcclusionTracker* occlusion) {
+  bool updated = Layer::Update(queue, occlusion);
+
   if (draw_properties().contents_scale_x == last_update_contents_scale_x_ &&
       draw_properties().contents_scale_y == last_update_contents_scale_y_)
-    return false;
+    return updated;
 
   last_update_contents_scale_x_ = draw_properties().contents_scale_x;
   last_update_contents_scale_y_ = draw_properties().contents_scale_y;
   // Invalidate the whole layer if scale changed.
   SetNeedsDisplayRect(gfx::Rect(paint_properties().bounds));
-  return false;
+  return updated;
 }
 
 }  // namespace cc
diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index 1079aec..aa4a002 100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -730,10 +730,16 @@ static void PostCopyCallbackToMainThread(
 void Layer::PushPropertiesTo(LayerImpl* layer) {
   DCHECK(layer_tree_host_);
 
+  // If we did not SavePaintProperties() for the layer this frame, then push the
+  // real property values, not the paint property values.
+  bool use_paint_properties = paint_properties_.source_frame_number ==
+                              layer_tree_host_->source_frame_number();
+
   layer->SetAnchorPoint(anchor_point_);
   layer->SetAnchorPointZ(anchor_point_z_);
   layer->SetBackgroundColor(background_color_);
-  layer->SetBounds(paint_properties_.bounds);
+  layer->SetBounds(use_paint_properties ? paint_properties_.bounds
+                                        : bounds_);
   layer->SetContentBounds(content_bounds());
   layer->SetContentsScale(contents_scale_x(), contents_scale_y());
   layer->SetDebugName(debug_name_);
@@ -845,11 +851,16 @@ void Layer::SavePaintProperties() {
   // TODO(reveman): Save all layer properties that we depend on not
   // changing until PushProperties() has been called. crbug.com/231016
   paint_properties_.bounds = bounds_;
+  paint_properties_.source_frame_number =
+      layer_tree_host_->source_frame_number();
 }
 
 bool Layer::Update(ResourceUpdateQueue* queue,
                    const OcclusionTracker* occlusion) {
   DCHECK(layer_tree_host_);
+  DCHECK_EQ(layer_tree_host_->source_frame_number(),
+            paint_properties_.source_frame_number) <<
+      "SavePaintProperties must be called for any layer that is painted.";
   return false;
 }
 
diff --git a/cc/layers/nine_patch_layer.cc b/cc/layers/nine_patch_layer.cc
index e88a95a..e32f6e0 100644
--- a/cc/layers/nine_patch_layer.cc
+++ b/cc/layers/nine_patch_layer.cc
@@ -56,6 +56,8 @@ void NinePatchLayer::SetBitmap(const SkBitmap& bitmap, gfx::Rect aperture) {
 
 bool NinePatchLayer::Update(ResourceUpdateQueue* queue,
                             const OcclusionTracker* occlusion) {
+  bool updated = Layer::Update(queue, occlusion);
+
   CreateUpdaterIfNeeded();
 
   if (resource_ &&
@@ -68,9 +70,9 @@ bool NinePatchLayer::Update(ResourceUpdateQueue* queue,
                                                    gfx::Vector2d());
     queue->AppendFullUpload(upload);
     bitmap_dirty_ = false;
-    return true;
+    updated = true;
   }
-  return false;
+  return updated;
 }
 
 void NinePatchLayer::CreateUpdaterIfNeeded() {
diff --git a/cc/layers/nine_patch_layer_unittest.cc b/cc/layers/nine_patch_layer_unittest.cc
index 73dd317..5268d02 100644
--- a/cc/layers/nine_patch_layer_unittest.cc
+++ b/cc/layers/nine_patch_layer_unittest.cc
@@ -71,6 +71,7 @@ TEST_F(NinePatchLayerTest, TriggerFullUploadOnceWhenChangingBitmap) {
   OcclusionTracker occlusion_tracker(gfx::Rect(), false);
 
   // No bitmap set should not trigger any uploads.
+  test_layer->SavePaintProperties();
   test_layer->SetTexturePriorities(calculator);
   test_layer->Update(&queue, &occlusion_tracker);
   EXPECT_EQ(0u, queue.FullUploadSize());
@@ -81,6 +82,7 @@ TEST_F(NinePatchLayerTest, TriggerFullUploadOnceWhenChangingBitmap) {
   bitmap.setConfig(SkBitmap::kARGB_8888_Config, 10, 10);
   bitmap.allocPixels();
   test_layer->SetBitmap(bitmap, gfx::Rect(5, 5, 1, 1));
+  test_layer->SavePaintProperties();
   test_layer->SetTexturePriorities(calculator);
   test_layer->Update(&queue, &occlusion_tracker);
   EXPECT_EQ(1u, queue.FullUploadSize());
@@ -105,6 +107,7 @@ TEST_F(NinePatchLayerTest, TriggerFullUploadOnceWhenChangingBitmap) {
   }
 
   // Nothing changed, so no repeated upload.
+  test_layer->SavePaintProperties();
   test_layer->SetTexturePriorities(calculator);
   test_layer->Update(&queue, &occlusion_tracker);
   EXPECT_EQ(0u, queue.FullUploadSize());
@@ -117,6 +120,7 @@ TEST_F(NinePatchLayerTest, TriggerFullUploadOnceWhenChangingBitmap) {
   }
 
   // Reupload after eviction
+  test_layer->SavePaintProperties();
   test_layer->SetTexturePriorities(calculator);
   test_layer->Update(&queue, &occlusion_tracker);
   EXPECT_EQ(1u, queue.FullUploadSize());
@@ -126,6 +130,7 @@ TEST_F(NinePatchLayerTest, TriggerFullUploadOnceWhenChangingBitmap) {
   layer_tree_host_->contents_texture_manager()->UnregisterTexture(
       params.texture);
   EXPECT_EQ(NULL, params.texture->resource_manager());
+  test_layer->SavePaintProperties();
   test_layer->SetTexturePriorities(calculator);
   ResourceUpdateQueue queue2;
   test_layer->Update(&queue2, &occlusion_tracker);
diff --git a/cc/layers/paint_properties.h b/cc/layers/paint_properties.h
index 9b93698..323e949 100644
--- a/cc/layers/paint_properties.h
+++ b/cc/layers/paint_properties.h
@@ -11,9 +11,11 @@ namespace cc {
 
 // Container for properties that layers need to save before they can be paint.
 struct CC_EXPORT PaintProperties {
-  PaintProperties() {}
+  PaintProperties() : source_frame_number(-1) {}
 
   gfx::Size bounds;
+
+  int source_frame_number;
 };
 
 }  // namespace cc
diff --git a/cc/layers/picture_layer.cc b/cc/layers/picture_layer.cc
index 2e7a90b..08a5817 100644
--- a/cc/layers/picture_layer.cc
+++ b/cc/layers/picture_layer.cc
@@ -69,8 +69,8 @@ void PictureLayer::SetNeedsDisplayRect(const gfx::RectF& layer_rect) {
   Layer::SetNeedsDisplayRect(layer_rect);
 }
 
-bool PictureLayer::Update(ResourceUpdateQueue*,
-                          const OcclusionTracker*) {
+bool PictureLayer::Update(ResourceUpdateQueue* queue,
+                          const OcclusionTracker* occlusion) {
   // Do not early-out of this function so that PicturePile::Update has a chance
   // to record pictures due to changing visibility of this layer.
 
@@ -79,6 +79,8 @@ bool PictureLayer::Update(ResourceUpdateQueue*,
                benchmark_instrumentation::kSourceFrameNumber,
                layer_tree_host()->source_frame_number());
 
+  bool updated = Layer::Update(queue, occlusion);
+
   pile_->Resize(paint_properties().bounds);
 
   // Calling paint in WebKit can sometimes cause invalidations, so save
@@ -95,12 +97,12 @@ bool PictureLayer::Update(ResourceUpdateQueue*,
   }
   devtools_instrumentation::ScopedLayerTask paint_layer(
       devtools_instrumentation::kPaintLayer, id());
-  bool updated = pile_->Update(client_,
-                               SafeOpaqueBackgroundColor(),
-                               contents_opaque(),
-                               pile_invalidation_,
-                               visible_layer_rect,
-                               rendering_stats_instrumentation());
+  updated |= pile_->Update(client_,
+                           SafeOpaqueBackgroundColor(),
+                           contents_opaque(),
+                           pile_invalidation_,
+                           visible_layer_rect,
+                           rendering_stats_instrumentation());
   if (updated) {
     SetNeedsPushProperties();
   } else {
diff --git a/cc/layers/scrollbar_layer.cc b/cc/layers/scrollbar_layer.cc
index 927ae7b..4a63835 100644
--- a/cc/layers/scrollbar_layer.cc
+++ b/cc/layers/scrollbar_layer.cc
@@ -302,10 +302,12 @@ bool ScrollbarLayer::Update(ResourceUpdateQueue* queue,
   if (layer_tree_host()->settings().solid_color_scrollbars)
     return false;
 
+  bool updated = false;
+
   {
     base::AutoReset<bool> ignore_set_needs_commit(&ignore_set_needs_commit_,
                                                   true);
-    ContentsScalingLayer::Update(queue, occlusion);
+    updated = ContentsScalingLayer::Update(queue, occlusion);
   }
 
   dirty_rect_.Union(update_rect_);
@@ -318,8 +320,8 @@ bool ScrollbarLayer::Update(ResourceUpdateQueue* queue,
 
   gfx::Rect content_rect = ScrollbarLayerRectToContentRect(
       gfx::Rect(scrollbar_->Location(), bounds()));
-  bool updated = UpdatePart(track_updater_.get(), track_.get(), content_rect,
-                            queue);
+  updated |= UpdatePart(track_updater_.get(), track_.get(), content_rect,
+                        queue);
 
   if (scrollbar_->HasThumb()) {
     thumb_thickness_ = scrollbar_->ThumbThickness();
diff --git a/cc/layers/scrollbar_layer_unittest.cc b/cc/layers/scrollbar_layer_unittest.cc
index f6c2dca..0c160c4 100644
--- a/cc/layers/scrollbar_layer_unittest.cc
+++ b/cc/layers/scrollbar_layer_unittest.cc
@@ -451,6 +451,7 @@ class ScrollbarLayerTestResourceCreation : public testing::Test {
     ResourceUpdateQueue queue;
     OcclusionTracker occlusion_tracker(gfx::Rect(), false);
 
+    scrollbar_layer->SavePaintProperties();
     scrollbar_layer->SetTexturePriorities(calculator);
     layer_tree_host_->contents_texture_manager()->PrioritizeTextures();
     scrollbar_layer->Update(&queue, &occlusion_tracker);
@@ -532,6 +533,7 @@ class ScaledScrollbarLayerTestResourceCreation : public testing::Test {
     ResourceUpdateQueue queue;
     OcclusionTracker occlusion_tracker(gfx::Rect(), false);
 
+    scrollbar_layer->SavePaintProperties();
     scrollbar_layer->SetTexturePriorities(calculator);
     layer_tree_host_->contents_texture_manager()->PrioritizeTextures();
     scrollbar_layer->Update(&queue, &occlusion_tracker);
diff --git a/cc/layers/texture_layer.cc b/cc/layers/texture_layer.cc
index 1a2e911..316042b 100644
--- a/cc/layers/texture_layer.cc
+++ b/cc/layers/texture_layer.cc
@@ -179,7 +179,7 @@ bool TextureLayer::DrawsContent() const {
 
 bool TextureLayer::Update(ResourceUpdateQueue* queue,
                           const OcclusionTracker* occlusion) {
-  bool updated = false;
+  bool updated = Layer::Update(queue, occlusion);
   if (client_) {
     if (uses_mailbox_) {
       TextureMailbox mailbox;
diff --git a/cc/layers/tiled_layer.cc b/cc/layers/tiled_layer.cc
index 753cdfa..bac12f0 100644
--- a/cc/layers/tiled_layer.cc
+++ b/cc/layers/tiled_layer.cc
@@ -327,7 +327,7 @@ bool TiledLayer::UpdateTiles(int left,
                              int bottom,
                              ResourceUpdateQueue* queue,
                              const OcclusionTracker* occlusion,
-                             bool* did_paint) {
+                             bool* updated) {
   CreateUpdaterIfNeeded();
 
   bool ignore_occlusions = !occlusion;
@@ -345,7 +345,7 @@ bool TiledLayer::UpdateTiles(int left,
   if (paint_rect.IsEmpty())
     return true;
 
-  *did_paint = true;
+  *updated = true;
   UpdateTileTextures(
       paint_rect, left, top, right, bottom, queue, occlusion);
   return true;
@@ -730,19 +730,20 @@ void TiledLayer::UpdateScrollPrediction() {
 bool TiledLayer::Update(ResourceUpdateQueue* queue,
                         const OcclusionTracker* occlusion) {
   DCHECK(!skips_draw_ && !failed_update_);  // Did ResetUpdateState get skipped?
+
+  bool updated = false;
+
   {
     base::AutoReset<bool> ignore_set_needs_commit(&ignore_set_needs_commit_,
                                                   true);
 
-    ContentsScalingLayer::Update(queue, occlusion);
+    updated |= ContentsScalingLayer::Update(queue, occlusion);
     UpdateBounds();
   }
 
   if (tiler_->has_empty_bounds() || !DrawsContent())
     return false;
 
-  bool did_paint = false;
-
   // Animation pre-paint. If the layer is small, try to paint it all
   // immediately whether or not it is occluded, to avoid paint/upload
   // hiccups while it is animating.
@@ -753,16 +754,16 @@ bool TiledLayer::Update(ResourceUpdateQueue* queue,
                                      &top,
                                      &right,
                                      &bottom);
-    UpdateTiles(left, top, right, bottom, queue, NULL, &did_paint);
-    if (did_paint)
-      return did_paint;
+    UpdateTiles(left, top, right, bottom, queue, NULL, &updated);
+    if (updated)
+      return updated;
     // This was an attempt to paint the entire layer so if we fail it's okay,
     // just fallback on painting visible etc. below.
     failed_update_ = false;
   }
 
   if (predicted_visible_rect_.IsEmpty())
-    return did_paint;
+    return updated;
 
   // Visible painting. First occlude visible tiles and paint the non-occluded
   // tiles.
@@ -771,22 +772,22 @@ bool TiledLayer::Update(ResourceUpdateQueue* queue,
       predicted_visible_rect_, &left, &top, &right, &bottom);
   MarkOcclusionsAndRequestTextures(left, top, right, bottom, occlusion);
   skips_draw_ = !UpdateTiles(
-      left, top, right, bottom, queue, occlusion, &did_paint);
+      left, top, right, bottom, queue, occlusion, &updated);
   if (skips_draw_)
     tiler_->reset();
-  if (skips_draw_ || did_paint)
+  if (skips_draw_ || updated)
     return true;
 
   // If we have already painting everything visible. Do some pre-painting while
   // idle.
   gfx::Rect idle_paint_content_rect = IdlePaintRect();
   if (idle_paint_content_rect.IsEmpty())
-    return did_paint;
+    return updated;
 
   // Prepaint anything that was occluded but inside the layer's visible region.
-  if (!UpdateTiles(left, top, right, bottom, queue, NULL, &did_paint) ||
-      did_paint)
-    return did_paint;
+  if (!UpdateTiles(left, top, right, bottom, queue, NULL, &updated) ||
+      updated)
+    return updated;
 
   int prepaint_left, prepaint_top, prepaint_right, prepaint_bottom;
   tiler_->ContentRectToTileIndices(idle_paint_content_rect,
@@ -814,40 +815,40 @@ bool TiledLayer::Update(ResourceUpdateQueue* queue,
       while (bottom < prepaint_bottom) {
         ++bottom;
         if (!UpdateTiles(
-                left, bottom, right, bottom, queue, NULL, &did_paint) ||
-            did_paint)
-          return did_paint;
+                left, bottom, right, bottom, queue, NULL, &updated) ||
+            updated)
+          return updated;
       }
     }
     if (deltas[i].y() < 0) {
       while (top > prepaint_top) {
         --top;
         if (!UpdateTiles(
-                left, top, right, top, queue, NULL, &did_paint) ||
-            did_paint)
-          return did_paint;
+                left, top, right, top, queue, NULL, &updated) ||
+            updated)
+          return updated;
       }
     }
     if (deltas[i].x() < 0) {
       while (left > prepaint_left) {
         --left;
         if (!UpdateTiles(
-                left, top, left, bottom, queue, NULL, &did_paint) ||
-            did_paint)
-          return did_paint;
+                left, top, left, bottom, queue, NULL, &updated) ||
+            updated)
+          return updated;
       }
     }
     if (deltas[i].x() > 0) {
       while (right < prepaint_right) {
         ++right;
         if (!UpdateTiles(
-                right, top, right, bottom, queue, NULL, &did_paint) ||
-            did_paint)
-          return did_paint;
+                right, top, right, bottom, queue, NULL, &updated) ||
+            updated)
+          return updated;
       }
     }
   }
-  return did_paint;
+  return updated;
 }
 
 bool TiledLayer::NeedsIdlePaint() {
diff --git a/cc/layers/video_layer.cc b/cc/layers/video_layer.cc
index 6cba239..8d728e0 100644
--- a/cc/layers/video_layer.cc
+++ b/cc/layers/video_layer.cc
@@ -24,12 +24,16 @@ scoped_ptr<LayerImpl> VideoLayer::CreateLayerImpl(LayerTreeImpl* tree_impl) {
 
 bool VideoLayer::Update(ResourceUpdateQueue* queue,
                         const OcclusionTracker* occlusion) {
+  bool updated = Layer::Update(queue, occlusion);
+
   // Video layer doesn't update any resources from the main thread side,
   // but repaint rects need to be sent to the VideoLayerImpl via commit.
   //
   // This is the inefficient legacy redraw path for videos.  It's better to
   // communicate this directly to the VideoLayerImpl.
-  return !update_rect_.IsEmpty();
+  updated |= !update_rect_.IsEmpty();
+
+  return updated;
 }
 
 }  // namespace cc
diff --git a/cc/trees/layer_tree_host_unittest.cc b/cc/trees/layer_tree_host_unittest.cc
index 9f9f653..ab0046c 100644
--- a/cc/trees/layer_tree_host_unittest.cc
+++ b/cc/trees/layer_tree_host_unittest.cc
@@ -16,6 +16,7 @@
 #include "cc/layers/layer_impl.h"
 #include "cc/layers/picture_layer.h"
 #include "cc/layers/scrollbar_layer.h"
+#include "cc/layers/solid_color_layer.h"
 #include "cc/layers/video_layer.h"
 #include "cc/output/begin_frame_args.h"
 #include "cc/output/copy_output_request.h"
@@ -4074,5 +4075,111 @@ class LayerTreeHostTestVideoLayerInvalidate : public LayerTreeHostTest {
 
 SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestVideoLayerInvalidate);
 
+class LayerTreeHostTestPushHiddenLayer : public LayerTreeHostTest {
+ protected:
+  virtual void SetupTree() OVERRIDE {
+    root_layer_ = Layer::Create();
+    root_layer_->SetAnchorPoint(gfx::PointF());
+    root_layer_->SetPosition(gfx::Point());
+    root_layer_->SetBounds(gfx::Size(10, 10));
+
+    parent_layer_ = SolidColorLayer::Create();
+    parent_layer_->SetAnchorPoint(gfx::PointF());
+    parent_layer_->SetPosition(gfx::Point());
+    parent_layer_->SetBounds(gfx::Size(10, 10));
+    parent_layer_->SetIsDrawable(true);
+    root_layer_->AddChild(parent_layer_);
+
+    child_layer_ = SolidColorLayer::Create();
+    child_layer_->SetAnchorPoint(gfx::PointF());
+    child_layer_->SetPosition(gfx::Point());
+    child_layer_->SetBounds(gfx::Size(10, 10));
+    child_layer_->SetIsDrawable(true);
+    parent_layer_->AddChild(child_layer_);
+
+    layer_tree_host()->SetRootLayer(root_layer_);
+    LayerTreeHostTest::SetupTree();
+  }
+
+  virtual void BeginTest() OVERRIDE { PostSetNeedsCommitToMainThread(); }
+
+  virtual void DidCommitAndDrawFrame() OVERRIDE {
+    switch (layer_tree_host()->source_frame_number()) {
+      case 1:
+        // The layer type used does not need to push properties every frame.
+        EXPECT_FALSE(child_layer_->needs_push_properties());
+
+        // Change the bounds of the child layer, but make it skipped
+        // by CalculateDrawProperties.
+        parent_layer_->SetOpacity(0.f);
+        child_layer_->SetBounds(gfx::Size(5, 5));
+        break;
+      case 2:
+        // The bounds of the child layer were pushed to the impl side.
+        EXPECT_FALSE(child_layer_->needs_push_properties());
+
+        EndTest();
+        break;
+    }
+  }
+
+  virtual void DidActivateTreeOnThread(LayerTreeHostImpl* impl) OVERRIDE {
+    LayerImpl* root = impl->active_tree()->root_layer();
+    LayerImpl* parent = root->children()[0];
+    LayerImpl* child = parent->children()[0];
+
+    switch (impl->active_tree()->source_frame_number()) {
+      case 1:
+        EXPECT_EQ(gfx::Size(5, 5).ToString(), child->bounds().ToString());
+        break;
+    }
+  }
+
+  virtual void AfterTest() OVERRIDE {}
+
+  scoped_refptr<Layer> root_layer_;
+  scoped_refptr<SolidColorLayer> parent_layer_;
+  scoped_refptr<SolidColorLayer> child_layer_;
+};
+
+SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestPushHiddenLayer);
+
+class LayerTreeHostTestUpdateLayerInEmptyViewport : public LayerTreeHostTest {
+ protected:
+  virtual void InitializeSettings(LayerTreeSettings* settings) OVERRIDE {
+    settings->impl_side_painting = true;
+  }
+
+  virtual void SetupTree() OVERRIDE {
+    root_layer_ = FakePictureLayer::Create(&client_);
+    root_layer_->SetAnchorPoint(gfx::PointF());
+    root_layer_->SetBounds(gfx::Size(10, 10));
+
+    layer_tree_host()->SetRootLayer(root_layer_);
+    LayerTreeHostTest::SetupTree();
+  }
+
+  virtual void BeginTest() OVERRIDE {
+    // The viewport is empty, but we still need to update layers on the main
+    // thread.
+    layer_tree_host()->SetViewportSize(gfx::Size(0, 0));
+    PostSetNeedsCommitToMainThread();
+  }
+
+  virtual void DidCommit() OVERRIDE {
+    // The layer should be updated even though the viewport is empty, so we
+    // are capable of drawing it on the impl tree.
+    EXPECT_GT(root_layer_->update_count(), 0u);
+    EndTest();
+  }
+
+  virtual void AfterTest() OVERRIDE {}
+
+  FakeContentLayerClient client_;
+  scoped_refptr<FakePictureLayer> root_layer_;
+};
+
+MULTI_THREAD_TEST_F(LayerTreeHostTestUpdateLayerInEmptyViewport);
+
 }  // namespace
 }  // namespace cc