cc: Don't swap PictureLayerTilingSet on activate.

cc/resources/picture_layer_tiling_set.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/resources/picture_layer_tiling_set.h"
 
 #include <limits>
 #include <set>
 #include <vector>
 
@@ skipping 22 matching lines @@
   return make_scoped_ptr(new PictureLayerTilingSet(client));
 }
 
 PictureLayerTilingSet::PictureLayerTilingSet(PictureLayerTilingClient* client)
     : client_(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(
     float min_acceptable_high_res_scale,
     float max_acceptable_high_res_scale,
     const std::vector<PictureLayerTiling*>& needed_tilings,
     bool should_have_low_res,
     PictureLayerTilingSet* twin_set,
     PictureLayerTilingSet* recycled_twin_set) {
   float twin_low_res_scale = 0.f;
@@ skipping 22 matching lines @@
     // Don't remove tilings that are required.
     if (std::find(needed_tilings.begin(), needed_tilings.end(), tiling) !=
         needed_tilings.end()) {
       continue;
     }
 
     to_remove.push_back(tiling);
   }
 
   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())
             : nullptr;
     // Remove the tiling from the recycle tree. Note that we ignore resolution,
     // since we don't need to maintain high/low res on the recycle set.
     if (recycled_twin_tiling)
       recycled_twin_set->Remove(recycled_twin_tiling);
 
     DCHECK_NE(HIGH_RESOLUTION, tiling->resolution());
     Remove(tiling);
   }
 }
 
+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);
 }
 
 bool PictureLayerTilingSet::SyncTilings(const PictureLayerTilingSet& other,
                                         const gfx::Size& new_layer_bounds,
                                         const Region& layer_invalidation,
                                         float minimum_contents_scale,
                                         RasterSource* raster_source) {
@@ skipping 392 matching lines @@
     case LOWER_THAN_LOW_RES:
       range = TilingRange(low_res_range.end, tilings_.size());
       break;
   }
 
   DCHECK_LE(range.start, range.end);
   return range;
 }
 
 }  // namespace cc