cc: Clean up tilings in UpdateTiles as well as in AppendQuads.

cc/layers/picture_layer_impl.cc

Index: cc/layers/picture_layer_impl.cc
diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
index 813bc47877c1f41cd803f2ab5ea4b5c9c45ec01d..760d210640ba5219c424238b5e166249422fd3f2 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -84,7 +84,8 @@ PictureLayerImpl::PictureLayerImpl(LayerTreeImpl* tree_impl,
       was_screen_space_transform_animating_(false),
       only_used_low_res_last_append_quads_(false),
       is_mask_(is_mask),
-      nearest_neighbor_(false) {
+      nearest_neighbor_(false),
+      need_tiling_cleanup_(false) {
   layer_tree_impl()->RegisterPictureLayerImpl(this);
 }
 
@@ -276,7 +277,7 @@ void PictureLayerImpl::AppendQuads(RenderPass* render_pass,
 
   // Keep track of the tilings that were used so that tilings that are
   // unused can be considered for removal.
-  std::vector<PictureLayerTiling*> seen_tilings;
+  last_append_quads_tilings_.clear();
 
   // Ignore missing tiles outside of viewport for tile priority. This is
   // normally the same as draw viewport but can be independently overridden by
@@ -403,8 +404,10 @@ void PictureLayerImpl::AppendQuads(RenderPass* render_pass,
     if (iter.resolution() != LOW_RESOLUTION)
       only_used_low_res_last_append_quads_ = false;
 
-    if (seen_tilings.empty() || seen_tilings.back() != iter.CurrentTiling())
-      seen_tilings.push_back(iter.CurrentTiling());
+    if (last_append_quads_tilings_.empty() ||
+        last_append_quads_tilings_.back() != iter.CurrentTiling()) {
+      last_append_quads_tilings_.push_back(iter.CurrentTiling());
+    }
   }
 
   if (missing_tile_count) {
@@ -421,7 +424,7 @@ void PictureLayerImpl::AppendQuads(RenderPass* render_pass,
   // that this is at the expense of doing cause more frequent re-painting. A
   // better scheme would be to maintain a tighter visible_content_rect for the
   // finer tilings.
-  CleanUpTilingsOnActiveLayer(seen_tilings);
+  CleanUpTilingsOnActiveLayer(last_append_quads_tilings_);
 }
 
 bool PictureLayerImpl::UpdateTiles(const Occlusion& occlusion_in_content_space,
@@ -449,6 +452,14 @@ bool PictureLayerImpl::UpdateTiles(const Occlusion& occlusion_in_content_space,
     AddTilingsForRasterScale();
   }
 
+  // Remove any non-ideal tilings that were not used last time we generated
+  // quads to save memory and processing time. Note that pending tree should
+  // only have one or two tilings (high and low res), so only clean up the
+  // active layer. Additionally, only clean up if AppendQuads didn't have
+  // a chance to do so, as inidicated by |need_tiling_cleanup_|.

[danakj, 2015/01/27 21:56:59]

can you say why you only clean up if AppendQuads didn't?

this would make us cleanup if we added a new tiling above this in
AddTilingsForRasterScale immediately, before append quads got a chance to clean
up. maybe that's okay? it doesn't seem intentional/obvious from the way the code
is organized tho..

[vmpstr, 2015/01/27 22:53:58]

You're right that it's okay, but you're also right that this was unintentional.
I moved it above the adding tiling and expanded the comment a bit.

+  if (need_tiling_cleanup_ && GetTree() == ACTIVE_TREE)
+    CleanUpTilingsOnActiveLayer(last_append_quads_tilings_);
+
   DCHECK(raster_page_scale_);
   DCHECK(raster_device_scale_);
   DCHECK(raster_source_scale_);
@@ -460,7 +471,6 @@ bool PictureLayerImpl::UpdateTiles(const Occlusion& occlusion_in_content_space,
 
   if (draw_transform_is_animating())
     raster_source_->SetShouldAttemptToUseDistanceFieldText();
-
   return UpdateTilePriorities(occlusion_in_content_space);
 }
 
@@ -779,6 +789,7 @@ PictureLayerTiling* PictureLayerImpl::AddTiling(float contents_scale) {
   DCHECK_GE(contents_scale, MinimumContentsScale());
   DCHECK_LE(contents_scale, MaximumContentsScale());
   DCHECK(raster_source_->HasRecordings());
+  need_tiling_cleanup_ = true;
   return tilings_->AddTiling(contents_scale, raster_source_);
 }
 
@@ -992,7 +1003,7 @@ void PictureLayerImpl::RecalculateRasterScales() {
 }
 
 void PictureLayerImpl::CleanUpTilingsOnActiveLayer(
-    std::vector<PictureLayerTiling*> used_tilings) {
+    const std::vector<PictureLayerTiling*>& used_tilings) {

[danakj, 2015/01/27 21:56:59]

why bother passing it in if it's a member on the class anyways?

[vmpstr, 2015/01/27 22:53:59]

It makes it far easier to test this function.. I can work to change it, but I
would prefer to keep it this way. Wdyt?

   DCHECK(layer_tree_impl()->IsActiveTree());
   if (tilings_->num_tilings() == 0)
     return;
@@ -1024,6 +1035,7 @@ void PictureLayerImpl::CleanUpTilingsOnActiveLayer(
 
   if (recycled_twin_set && recycled_twin_set->num_tilings() == 0)
     recycled_twin->ResetRasterScale();
+  need_tiling_cleanup_ = false;
 
   DCHECK_GT(tilings_->num_tilings(), 0u);
   SanityCheckTilingState();