cc: Remove and Create the correct tiles when resizing live tiles rect

cc/resources/picture_layer_tiling.cc

Index: cc/resources/picture_layer_tiling.cc
diff --git a/cc/resources/picture_layer_tiling.cc b/cc/resources/picture_layer_tiling.cc
index b302873c1a33cf311ab4d0e027d32ed2ea4b8568..57c45654a9da5d28463f44c8e1a988244cb2d205 100644
--- a/cc/resources/picture_layer_tiling.cc
+++ b/cc/resources/picture_layer_tiling.cc
@@ -10,6 +10,7 @@
 
 #include "base/debug/trace_event.h"
 #include "base/debug/trace_event_argument.h"
+#include "base/logging.h"
 #include "cc/base/math_util.h"
 #include "cc/resources/tile.h"
 #include "cc/resources/tile_priority.h"
@@ -146,7 +147,7 @@ Tile* PictureLayerTiling::CreateTile(int i,
 
 void PictureLayerTiling::CreateMissingTilesInLiveTilesRect() {
   const PictureLayerTiling* twin_tiling = client_->GetTwinTiling(this);
-  bool include_borders = true;
+  bool include_borders = false;
   for (TilingData::Iterator iter(
            &tiling_data_, live_tiles_rect_, include_borders);
        iter;
@@ -157,6 +158,8 @@ void PictureLayerTiling::CreateMissingTilesInLiveTilesRect() {
       continue;
     CreateTile(key.first, key.second, twin_tiling);
   }
+
+  VerifyLiveTilesRect();
 }
 
 void PictureLayerTiling::UpdateTilesToCurrentPile(
@@ -172,10 +175,54 @@ void PictureLayerTiling::UpdateTilesToCurrentPile(
   gfx::Size tile_size = tiling_data_.max_texture_size();
 
   if (layer_bounds_ != old_layer_bounds) {
-    // Drop tiles outside the new layer bounds if the layer shrank.
-    SetLiveTilesRect(
-        gfx::IntersectRects(live_tiles_rect_, gfx::Rect(content_bounds)));
+    // The SetLiveTilesRect() method would drop tiles outside the new bounds,
+    // but may do so incorrectly if resizing the tiling causes the number of
+    // tiles in the tiling_data_ to change.
+    gfx::Rect content_rect(content_bounds);
+    int before_left = tiling_data_.TileXIndexFromSrcCoord(live_tiles_rect_.x());
+    int before_top = tiling_data_.TileYIndexFromSrcCoord(live_tiles_rect_.y());
+    int before_right =
+        tiling_data_.TileXIndexFromSrcCoord(live_tiles_rect_.right() - 1);
+    int before_bottom =
+        tiling_data_.TileYIndexFromSrcCoord(live_tiles_rect_.bottom() - 1);
+
+    live_tiles_rect_.Intersect(content_rect);

[vmpstr, 2014/08/27 19:13:21]

nit: Put a comment here saying that this is the place where you actually shrink
live tiles rect/tiling data. Otherwise it's kind of easy to skim over it without
noticing

[danakj, 2014/08/27 19:27:01]

Done.

     tiling_data_.SetTilingSize(content_bounds);
+
+    int after_right = -1;
+    int after_bottom = -1;
+    if (!live_tiles_rect_.IsEmpty()) {
+      after_right =
+          tiling_data_.TileXIndexFromSrcCoord(live_tiles_rect_.right() - 1);
+      after_bottom =
+          tiling_data_.TileYIndexFromSrcCoord(live_tiles_rect_.bottom() - 1);
+    }
+
+    // There is no recycled twin since this is run on the pending tiling.

[vmpstr, 2014/08/27 19:13:21]

Please DCHECK the tree

[danakj, 2014/08/27 19:27:01]

Oh, GetTree, I missed that in the client. Done.

+    PictureLayerTiling* recycled_twin = NULL;
+    DCHECK_EQ(recycled_twin, client_->GetRecycledTwinTiling(this));
+
+    // Drop tiles outside the new layer bounds if the layer shrank.
+    for (int i = after_right + 1; i <= before_right; ++i) {
+      for (int j = before_top; j <= before_bottom; ++j)
+        RemoveTileAt(i, j, recycled_twin);
+    }
+    for (int i = before_left; i <= before_right; ++i) {

[vmpstr, 2014/08/27 19:13:21]

nit: this can be after_right, I think? Otherwise you're clearing the corner
twice? Maybe?

[danakj, 2014/08/27 19:27:01]

Oh, yah. I think so.

+      for (int j = after_bottom + 1; j <= before_bottom; ++j)
+        RemoveTileAt(i, j, recycled_twin);
+    }
+
+    // If the layer grew, new tiles may exist inside the same live tiles rect.
+    const PictureLayerTiling* twin_tiling = client_->GetTwinTiling(this);
+    for (int i = before_right + 1; i <= after_right; ++i) {

[vmpstr, 2014/08/27 19:13:21]

Can you please add a comment in the style of

// ------===
// |    |xx|
// |    |xx|
// ------===

or something where X denotes the spot that this loop touches, etc? I'm not sure
if ascii art is the way to go here, but I think it should clarify what is going
on. I'm just looking at the indecies and trying to visualize the area being
iterated.

[danakj, 2014/08/27 19:27:01]

I changed these from a double for-loop to an if statement + dcheck + single for
loop. Does that help? Do you still want a verbose comment?

+      for (int j = before_top; j <= after_bottom; ++j)
+        CreateTile(i, j, twin_tiling);
+    }
+    for (int i = before_left; i <= before_right; ++i) {
+      for (int j = before_bottom + 1; j <= after_bottom; ++j)
+        CreateTile(i, j, twin_tiling);
+    }
+
     tile_size = client_->CalculateTileSize(content_bounds);
   }
 
@@ -191,6 +238,7 @@ void PictureLayerTiling::UpdateTilesToCurrentPile(
   PicturePileImpl* pile = client_->GetPile();
   for (TileMap::const_iterator it = tiles_.begin(); it != tiles_.end(); ++it)
     it->second->set_picture_pile(pile);
+  VerifyLiveTilesRect();
 }
 
 void PictureLayerTiling::RemoveTilesInRegion(const Region& layer_region) {
@@ -207,31 +255,33 @@ void PictureLayerTiling::DoInvalidate(const Region& layer_region,
                                       bool recreate_invalidated_tiles) {
   std::vector<TileMapKey> new_tile_keys;
   gfx::Rect expanded_live_tiles_rect =
-      tiling_data_.ExpandRectIgnoringBordersToTileBoundsWithBorders(
-          live_tiles_rect_);
+      tiling_data_.ExpandRectIgnoringBordersToTileBounds(live_tiles_rect_);
   for (Region::Iterator iter(layer_region); iter.has_rect(); iter.next()) {
     gfx::Rect layer_rect = iter.rect();
     gfx::Rect content_rect =
         gfx::ScaleToEnclosingRect(layer_rect, contents_scale_);
+    // Consider tiles inside the live tiles rect even if only their border
+    // pixels intersect the invalidation. But don't consider tiles outside
+    // the live tiles rect with the same conditions, as they won't exist.
+    int border_pixels = tiling_data_.border_texels();
+    content_rect.Inset(-border_pixels, -border_pixels);
     // Avoid needless work by not bothering to invalidate where there aren't
     // tiles.
     content_rect.Intersect(expanded_live_tiles_rect);
     if (content_rect.IsEmpty())
       continue;
-    bool include_borders = true;
+    // Since the content_rect includes border pixels already, don't include
+    // borders when iterating to avoid double counting them.
+    bool include_borders = false;
     for (TilingData::Iterator iter(
              &tiling_data_, content_rect, include_borders);
          iter;
          ++iter) {
-      TileMapKey key(iter.index());
-      TileMap::iterator find = tiles_.find(key);
-      if (find == tiles_.end())
-        continue;
-
-      ReleaseTile(find->second.get(), client_->GetTree());
-
-      tiles_.erase(find);
-      new_tile_keys.push_back(key);
+      // There is no recycled twin since this is run on the pending tiling.

[vmpstr, 2014/08/27 19:13:21]

nit: I think we have a client_->GetTree in this version, can you sprinkle a few
dchecks where you expect this to be run on the pending tree only

[danakj, 2014/08/27 19:27:01]

Done.

+      PictureLayerTiling* recycled_twin = NULL;
+      DCHECK_EQ(recycled_twin, client_->GetRecycledTwinTiling(this));
+      if (RemoveTileAt(iter.index_x(), iter.index_y(), recycled_twin))
+        new_tile_keys.push_back(iter.index());
     }
   }
 
@@ -395,10 +445,29 @@ gfx::Size PictureLayerTiling::CoverageIterator::texture_size() const {
   return tiling_->tiling_data_.max_texture_size();
 }
 
+bool PictureLayerTiling::RemoveTileAt(int i,
+                                      int j,
+                                      PictureLayerTiling* recycled_twin) {
+  TileMap::iterator found = tiles_.find(TileMapKey(i, j));
+  if (found == tiles_.end())
+    return false;
+  ReleaseTile(found->second.get(), client_->GetTree());
+  tiles_.erase(found);
+  if (recycled_twin) {
+    // Recycled twin does not also have a recycled twin, so pass NULL.
+    recycled_twin->RemoveTileAt(i, j, NULL);
+  }
+  return true;
+}
+
 void PictureLayerTiling::Reset() {
   live_tiles_rect_ = gfx::Rect();
-  for (TileMap::const_iterator it = tiles_.begin(); it != tiles_.end(); ++it)
+  PictureLayerTiling* recycled_twin = client_->GetRecycledTwinTiling(this);
+  for (TileMap::const_iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
     ReleaseTile(it->second.get(), client_->GetTree());
+    if (recycled_twin)
+      recycled_twin->RemoveTileAt(it->first.first, it->first.second, NULL);
+  }
   tiles_.clear();
 }
 
@@ -505,7 +574,7 @@ void PictureLayerTiling::UpdateTilePriorities(
   float content_to_screen_scale = ideal_contents_scale / contents_scale_;
 
   // Assign now priority to all visible tiles.
-  bool include_borders = true;
+  bool include_borders = false;
   has_visible_rect_tiles_ = false;
   for (TilingData::Iterator iter(
            &tiling_data_, visible_rect_in_content_space, include_borders);
@@ -607,15 +676,6 @@ void PictureLayerTiling::UpdateTilePriorities(
   current_eventually_rect_ = eventually_rect;
 }
 
-void PictureLayerTiling::RemoveTileAt(int i, int j) {
-  TileMapKey key(i, j);
-  TileMap::iterator found = tiles_.find(key);
-  if (found == tiles_.end())
-    return;
-  ReleaseTile(found->second.get(), client_->GetTree());
-  tiles_.erase(found);
-}
-
 void PictureLayerTiling::SetLiveTilesRect(
     const gfx::Rect& new_live_tiles_rect) {
   DCHECK(new_live_tiles_rect.IsEmpty() ||
@@ -632,16 +692,7 @@ void PictureLayerTiling::SetLiveTilesRect(
                                            new_live_tiles_rect);
        iter;
        ++iter) {
-    TileMapKey key(iter.index());
-    TileMap::iterator found = tiles_.find(key);
-    // If the tile was outside of the recorded region, it won't exist even
-    // though it was in the live rect.
-    if (found != tiles_.end()) {
-      ReleaseTile(found->second.get(), client_->GetTree());
-      tiles_.erase(found);
-      if (recycled_twin)
-        recycled_twin->RemoveTileAt(iter.index_x(), iter.index_y());
-    }
+    RemoveTileAt(iter.index_x(), iter.index_y(), recycled_twin);
   }
 
   const PictureLayerTiling* twin_tiling = client_->GetTwinTiling(this);
@@ -657,6 +708,30 @@ void PictureLayerTiling::SetLiveTilesRect(
   }
 
   live_tiles_rect_ = new_live_tiles_rect;
+  VerifyLiveTilesRect();
+}
+
+void PictureLayerTiling::VerifyLiveTilesRect() {
+#if DCHECK_IS_ON
+  for (TileMap::iterator it = tiles_.begin(); it != tiles_.end(); ++it) {
+    if (!it->second.get())
+      continue;
+    DCHECK(it->first.first < tiling_data_.num_tiles_x())
+        << this << " " << it->first.first << "," << it->first.second
+        << " num_tiles_x " << tiling_data_.num_tiles_x() << " live_tiles_rect "
+        << live_tiles_rect_.ToString();
+    DCHECK(it->first.second < tiling_data_.num_tiles_y())
+        << this << " " << it->first.first << "," << it->first.second
+        << " num_tiles_y " << tiling_data_.num_tiles_y() << " live_tiles_rect "
+        << live_tiles_rect_.ToString();
+    DCHECK(tiling_data_.TileBounds(it->first.first, it->first.second)
+               .Intersects(live_tiles_rect_))
+        << this << " " << it->first.first << "," << it->first.second
+        << " tile bounds "
+        << tiling_data_.TileBounds(it->first.first, it->first.second).ToString()
+        << " live_tiles_rect " << live_tiles_rect_.ToString();
+  }
+#endif
 }
 
 void PictureLayerTiling::DidBecomeRecycled() {
@@ -951,7 +1026,7 @@ PictureLayerTiling::TilingRasterTileIterator::TilingRasterTileIterator(
 
   visible_iterator_ = TilingData::Iterator(&tiling_->tiling_data_,
                                            tiling_->current_visible_rect_,
-                                           true /* include_borders */);
+                                           false /* include_borders */);
   if (!visible_iterator_) {
     AdvancePhase();
     return;