cc: Don't swap PictureLayerTilingSet on activate.

cc/resources/picture_layer_tiling_set.cc

Index: cc/resources/picture_layer_tiling_set.cc
diff --git a/cc/resources/picture_layer_tiling_set.cc b/cc/resources/picture_layer_tiling_set.cc
index 51896a5b5f7f29cfb287e970fb8b301aaa4b83a0..fbd1fcfc5b33126f0f9f279e6b418df586f21a4b 100644
--- a/cc/resources/picture_layer_tiling_set.cc
+++ b/cc/resources/picture_layer_tiling_set.cc
@@ -40,15 +40,109 @@ PictureLayerTilingSet::PictureLayerTilingSet(PictureLayerTilingClient* client)
 PictureLayerTilingSet::~PictureLayerTilingSet() {
 }
 
-void PictureLayerTilingSet::SetClient(PictureLayerTilingClient* client) {
-  client_ = client;
-  for (size_t i = 0; i < tilings_.size(); ++i)
-    tilings_[i]->SetClient(client_);
+void PictureLayerTilingSet::UpdateTilingsToCurrentRasterSource(
+    RasterSource* raster_source,
+    const gfx::Size& layer_bounds,
+    bool solid_color,
+    const Region& layer_invalidation,
+    float minimum_contents_scale) {
+  if (layer_bounds.IsEmpty() || solid_color) {

[vmpstr, 2014/12/01 22:38:04]

I'd like to at some point see this move to the caller (maybe a todo?). If PLTS
is to become a dumb cache at some point, I'd like for it not to care whether
things are solid... "Bounds are empty" might still make sense here, or maybe it
can be in the caller as well.

[danakj, 2014/12/05 22:30:44]

Done. CanHaveTilings() should actually already cover both of these cases. Added
a DCHECK over there to show it.

+    RemoveAllTilings();
+    return;
+  }
+
+  for (size_t i = 0; i < tilings_.size(); ++i) {

[vmpstr, 2014/12/01 22:38:03]

nit: would erase(remove_if) with a lambda work here?

[danakj, 2014/12/05 22:30:44]

remove_if does not work with ScopedPtrVector because the iterator does not let
you assign to it (for good reasons).

I added remove_if() to ScopedPtrVector to do what we need and used it here now.

+    float scale = tilings_[i]->contents_scale();
+    if (scale < minimum_contents_scale) {
+      tilings_.swap(tilings_.begin() + i, tilings_.end() - 1);
+      tilings_.pop_back();
+      --i;
+    }
+  }
+
+  for (PictureLayerTiling* tiling : tilings_) {
+    tiling->UpdateTilesToCurrentRasterSource(raster_source, layer_invalidation,

[vmpstr, 2014/12/01 22:38:04]

I think this function should have the word "invalidate" in it. maybe. I think
what this is saying is "here are your new bounds, here is the region where
whatever you have cached is no longer valid, and here is a raster source for any
new things you might wanna make"

All in all, the most important action out of those to me seems to be "your cache
in this region is no longer valid" -> Invalidate! :D Everything else is just
kind of tacked on and could be separate functionality. Maybe something along the
lines of "SetNewRasterSource, Resize, Invalidate"? I'm leaning towards more
smaller functions and trust the caller not to mess up...

[danakj, 2014/12/08 16:49:25]

To give my POV here.. to me "invalidate" is actually just updating to the raster
source too. It looks much less direct though right now. Invalidating makes us
recreate a tile. Making a tile goes through the PLTClient (PLImpl) where it uses
the current raster_source_.

I was kinda hoping to use the raster source passed into the function, and
eventually maybe remove CreateTile from the PLTClient interface. I'm not sure if
that's entirely reasonable though.

So anyhow, in my head update-to-raster-source is a superset of invalidate (it
also includes resizing/dropping tiles, and changing the non-invalidated tiles
sometimes).

At the same time, I don't disagree with anything you said either. Except that
splitting it up makes less sense to me, because I see this as one thing "use
this new raster source" which has all these implications.

+                                             layer_bounds);
+    tiling->CreateMissingTilesInLiveTilesRect();
+
+    DCHECK(tiling->tile_size() ==
+           client_->CalculateTileSize(tiling->tiling_size()))
+        << "tile_size: " << tiling->tile_size().ToString()
+        << " tiling_size: " << tiling->tiling_size().ToString()
+        << " CalculateTileSize: "
+        << client_->CalculateTileSize(tiling->tiling_size()).ToString();
+  }
 }
 
-void PictureLayerTilingSet::RemoveTilesInRegion(const Region& region) {
-  for (size_t i = 0; i < tilings_.size(); ++i)
-    tilings_[i]->RemoveTilesInRegion(region);
+void PictureLayerTilingSet::UpdateTilingsFromPending(
+    RasterSource* raster_source,
+    const PictureLayerTilingSet& other,
+    const gfx::Size& layer_bounds,
+    bool solid_color,
+    const Region& layer_invalidation,
+    float minimum_contents_scale) {
+  DCHECK_EQ(ACTIVE_TREE, client_->GetTree());
+
+  if (layer_bounds.IsEmpty() || solid_color) {
+    RemoveAllTilings();
+    return;
+  }
+
+  for (size_t i = 0; i < tilings_.size(); ++i) {

[vmpstr, 2014/12/01 22:38:04]

This can probably be a separate function

[danakj, 2014/12/05 22:30:44]

Done.

+    float scale = tilings_[i]->contents_scale();
+    if (scale < minimum_contents_scale) {
+      tilings_.swap(tilings_.begin() + i, tilings_.end() - 1);
+      tilings_.pop_back();
+      --i;
+    }
+  }
+
+  for (PictureLayerTiling* tiling : tilings_) {
+    // TODO(danakj): In the activate case can we do something simpler? Just copy

[enne (OOO), 2014/12/01 22:08:17]

That sounds nice, although I think you'd still keep around bits of non-ideal
tilings that weren't invalidated?

[vmpstr, 2014/12/01 22:38:03]

I agree... I think we should be able to "blit" pending tree over the active tree
in a given region and keep the rest or something like that.

I think that might require larger changes though, not sure.

+    // things from the pending tree and share all the tiles?
+    tiling->UpdateTilesToCurrentRasterSource(raster_source, layer_invalidation,
+                                             layer_bounds);
+    tiling->CreateMissingTilesInLiveTilesRect();
+    tiling->set_resolution(NON_IDEAL_RESOLUTION);
+
+    DCHECK(tiling->tile_size() ==
+           client_->CalculateTileSize(tiling->tiling_size()))
+        << "tile_size: " << tiling->tile_size().ToString()
+        << " tiling_size: " << tiling->tiling_size().ToString()
+        << " CalculateTileSize: "
+        << client_->CalculateTileSize(tiling->tiling_size()).ToString();
+  }
+
+  // Copy missing tilings, tiles and tiling resolutions from the pending tiling
+  // set.
+  for (size_t i = 0; i < other.tilings_.size(); ++i) {

[vmpstr, 2014/12/01 22:38:04]

nit: for (PLT* tiling : other.tilings)

[danakj, 2014/12/05 22:30:44]

Done.

+    float contents_scale = other.tilings_[i]->contents_scale();
+    DCHECK_GE(contents_scale, minimum_contents_scale);
+
+    PictureLayerTiling* this_tiling = FindTilingWithScale(contents_scale);
+    if (!this_tiling) {
+      scoped_ptr<PictureLayerTiling> new_tiling =
+          PictureLayerTiling::Create(contents_scale, layer_bounds, client_);
+      tilings_.push_back(new_tiling.Pass());
+      this_tiling = tilings_.back();
+    }
+    this_tiling->CreateSharedTilesFromPending(*other.tilings_[i]);

[vmpstr, 2014/12/01 22:38:03]

I personally like this style of functionality, modulo "Pending". Maybe
CreateSharedTilesFromOther? or just CreateSharedTiles(...) or ShareFromThis

[danakj, 2014/12/05 22:30:44]

Done. CloneTilesAndPropertiesFrom()

+    this_tiling->set_resolution(other.tilings_[i]->resolution());
+
+    DCHECK_EQ(other.tilings_[i]->live_tiles_rect().ToString(),
+              this_tiling->live_tiles_rect().ToString());
+  }
+
+  tilings_.sort(LargestToSmallestScaleFunctor());
+
+#if DCHECK_IS_ON
+  if (!tilings_.empty()) {
+    size_t num_high_res = 0;
+    for (const auto& t : tilings_)

[vmpstr, 2014/12/01 22:38:03]

count_if?

[danakj, 2014/12/05 22:30:44]

Oh, cool. Done.

+      num_high_res += t->resolution() == HIGH_RESOLUTION ? 1 : 0;
+    DCHECK_EQ(1u, num_high_res);
+  }
+#endif
 }
 
 void PictureLayerTilingSet::CleanUpTilings(
@@ -91,14 +185,6 @@ void PictureLayerTilingSet::CleanUpTilings(
   }
 
   for (auto* tiling : to_remove) {
-    PictureLayerTiling* twin_tiling =
-        twin_set ? twin_set->FindTilingWithScale(tiling->contents_scale())
-                 : nullptr;
-    // Only remove tilings from the twin layer if they have
-    // NON_IDEAL_RESOLUTION.
-    if (twin_tiling && twin_tiling->resolution() == NON_IDEAL_RESOLUTION)
-      twin_set->Remove(twin_tiling);
-
     PictureLayerTiling* recycled_twin_tiling =
         recycled_twin_set
             ? recycled_twin_set->FindTilingWithScale(tiling->contents_scale())
@@ -113,6 +199,16 @@ void PictureLayerTilingSet::CleanUpTilings(
   }
 }
 
+void PictureLayerTilingSet::RemoveNonIdealTilings() {
+  for (size_t i = 0; i < tilings_.size(); ++i) {

[vmpstr, 2014/12/01 22:38:04]

nit: I'd prefer erase/remove_if, but feel free to disagree.

[danakj, 2014/12/05 22:30:44]

Done.

+    PictureLayerTiling* tiling = tilings_[i];
+    if (tiling->resolution() == NON_IDEAL_RESOLUTION) {
+      tilings_.erase(tilings_.begin() + i);
+      --i;
+    }
+  }
+}
+
 void PictureLayerTilingSet::MarkAllTilingsNonIdeal() {
   for (auto* tiling : tilings_)
     tiling->set_resolution(NON_IDEAL_RESOLUTION);