cc: Don't swap PictureLayerTilingSet on activate.

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 1827bea13d8923746825029c691e5b731226c0c1..a6a54f8a70ae663a2958346b231c31887c07f8ce 100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -73,6 +73,8 @@ PictureLayerImpl::Pair::~Pair() {
 PictureLayerImpl::PictureLayerImpl(LayerTreeImpl* tree_impl, int id)
     : LayerImpl(tree_impl, id),
       twin_layer_(nullptr),
+      tilings_(PictureLayerTilingSet::Create(this)),
+      // TODO(danakj): Can this be null to start?

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

I think it could be.  PushPropertiesTo happens before anything real happens, so
you wouldn't need to check for it anywhere, maybe?

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

Cool. It crashes a lot of unit tests and I'm not sure why. I'll investigate
changing this independently.

[enne (OOO), 2014/12/05 23:02:02]

Sounds good to me.

       raster_source_(PicturePileImpl::Create()),
       ideal_page_scale_(0.f),
       ideal_device_scale_(0.f),
@@ -133,54 +135,37 @@ void PictureLayerImpl::PushPropertiesTo(LayerImpl* base_layer) {
   twin_layer_ = layer_impl;
   layer_impl->twin_layer_ = this;
 
-  layer_impl->set_is_mask(is_mask_);
-  layer_impl->UpdateRasterSource(raster_source_);
-
+  // Solid color layers have no tilings.
   DCHECK_IMPLIES(raster_source_->IsSolidColor(), tilings_->num_tilings() == 0);
-  // Tilings would be expensive to push, so we swap.
-  layer_impl->tilings_.swap(tilings_);
-  layer_impl->tilings_->SetClient(layer_impl);
-  if (tilings_)
-    tilings_->SetClient(this);
+  // The pending tree should only have a high res (and possibly low res) tiling.
+  DCHECK_LE(tilings_->num_tilings(),
+            layer_tree_impl()->create_low_res_tiling() ? 2u : 1u);
 
-  // Ensure that the recycle tree doesn't have any unshared tiles.
-  if (tilings_ && raster_source_->IsSolidColor())
-    tilings_->RemoveAllTilings();
+  layer_impl->UpdateRasterSource(raster_source_, &invalidation_, is_mask_,
+                                 tilings_.get());
+  DCHECK(invalidation_.IsEmpty());
 
-  // Remove invalidated tiles from what will become a recycle tree.
-  if (tilings_)
-    tilings_->RemoveTilesInRegion(invalidation_);
+  // After syncing a solid color layer, the active layer has no tilings.
+  DCHECK_IMPLIES(raster_source_->IsSolidColor(),
+                 !layer_impl->tilings_->num_tilings());
 
   layer_impl->raster_page_scale_ = raster_page_scale_;
   layer_impl->raster_device_scale_ = raster_device_scale_;
   layer_impl->raster_source_scale_ = raster_source_scale_;
   layer_impl->raster_contents_scale_ = raster_contents_scale_;
   layer_impl->low_res_raster_contents_scale_ = low_res_raster_contents_scale_;
-  layer_impl->needs_post_commit_initialization_ = false;
 
-  // The invalidation on this soon-to-be-recycled layer must be cleared to
-  // mirror clearing the invalidation in PictureLayer's version of this function
-  // in case push properties is skipped.
-  layer_impl->invalidation_.Swap(&invalidation_);
-  invalidation_.Clear();
+  layer_impl->SanityCheckTilingState();
+
+  layer_impl->needs_post_commit_initialization_ = false;
   needs_post_commit_initialization_ = true;
 
   // We always need to push properties.
   // See http://crbug.com/303943
+  // TODO(danakj): Stop always pushing properties since we don't swap tilings.

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

\o/

   needs_push_properties_ = true;
 }
 
-void PictureLayerImpl::UpdateRasterSource(
-    scoped_refptr<RasterSource> raster_source) {
-  bool could_have_tilings = CanHaveTilings();
-  raster_source_.swap(raster_source);
-
-  // Need to call UpdateTiles again if CanHaveTilings changed.
-  if (could_have_tilings != CanHaveTilings()) {
-    layer_tree_impl()->set_needs_update_draw_properties();
-  }
-}
-
 void PictureLayerImpl::AppendQuads(RenderPass* render_pass,
                                    const Occlusion& occlusion_in_content_space,
                                    AppendQuadsData* append_quads_data) {
@@ -484,8 +469,6 @@ void PictureLayerImpl::UpdateTiles(const Occlusion& occlusion_in_content_space,
 
   UpdateIdealScales();
 
-  DCHECK_IMPLIES(tilings_->num_tilings() == 0, raster_contents_scale_ == 0.f)
-      << "A layer with no tilings shouldn't have valid raster scales";
   if (!raster_contents_scale_ || ShouldAdjustRasterScale()) {
     RecalculateRasterScales();
     AddTilingsForRasterScale();
@@ -580,6 +563,55 @@ PictureLayerImpl* PictureLayerImpl::GetRecycledTwinLayer() const {
   return twin_layer_;
 }
 
+void PictureLayerImpl::UpdateRasterSource(

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

I'm handwaving a bit, but my perspective here is that this function is trying to
be too general.  Maybe a totally separate UpdateFromMainThread and
UpdateFromPending would make sense? I'm not 100% sold on those names, but maybe
having different top level functions would make this more clear than trying to
merge them.

Also, it seems like maybe the CanHaveTilings checks are somewhat redundant here
with what goes on in
UpdateTilingsToCurrentRasterSource/UpdateTilingsFromPending?

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

Yes, vlad said similar thing in PLTS and I've determined they are redundant.

+    scoped_refptr<RasterSource> raster_source,
+    Region* new_invalidation,
+    bool is_mask,
+    const PictureLayerTilingSet* pending_set) {
+  // The bounds and the pile size may differ if the pile wasn't updated (ie.
+  // PictureLayer::Update didn't happen). In that case the pile will be empty.
+  DCHECK_IMPLIES(!raster_source->GetSize().IsEmpty(),
+                 bounds() == raster_source->GetSize())
+      << " bounds " << bounds().ToString() << " pile "
+      << raster_source->GetSize().ToString();
+
+  bool could_have_tilings = CanHaveTilings();
+  raster_source_.swap(raster_source);
+
+  // The |new_invalidation| must be cleared before updating tilings since they

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

nit: It's unclear to me what this comment is saying. |new_invalidation| is a
local variable, so it won't be accessed outside of this? Or do you mean the
invalidation in the caller might be accessed? In that case it seems like a weird
dependency: maybe we should swap in the caller and pass it here as a const ref
and copy? Or is that too slow?

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

The |new_invalidation| is a pointer to the invalidation on the pending tree (if
this is the active layer). And that invalidation is accessed by
PLTClient::GetPendingInvalidation().

Copying regions is banned as it is slow.

I though perhaps splitting this method in 2 like enne suggests will help.. But
splitting this method is a lot of duplicated code, and sharing code makes the
new_invalidation clear even further from where it matters, which is more
confusing /o\.

+  // access the invalidation through the PictureLayerTilingClient interface.
+  invalidation_.Clear();

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

This dance is a bit more awkward when it has to be done in some other function,
but I don't see a better way to do it other than scoped_ptr-ing the Region so
you can pass it into the function.  This is where we Region&&, I guess...

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

ya..

+  invalidation_.Swap(new_invalidation);
+
+  is_mask_ = is_mask;
+
+  // Need to call UpdateTiles again if CanHaveTilings changed.
+  if (could_have_tilings != CanHaveTilings())

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

nit: save CanHaveTilings() into a local before this, since you access it twice

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

Done.

+    layer_tree_impl()->set_needs_update_draw_properties();
+
+  if (!CanHaveTilings()) {
+    RemoveAllTilings();
+    return;
+  }
+
+  // TODO(danakj): Should this happen after doing UpdateTiles so we can avoid

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

I'm a little torn.  Doing after does seem like it would save work, but then we
have yet another needs_update_tilings_to_current_raster_source deferred boolean,
which is kind of a mess.  I think in general, we won't have multiple tilings
except during pinch scenarios, so maybe it's ok to just turn this TODO into a
permanent comment that it'd be more efficient (but more complicated) to do it
after dropping tilings.

What do you think?

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

Sure.

+  // doing this for tilings that will just go away on the pending tree? (What
+  // about commit to active tree?)
+  // TODO(danakj): Just pass along the pile to this function instead of using

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

Maybe, yeah? The client API made a lot more sense in the stone age of impl-side
painting before it was clear how everything was going to shape up and it was
useful to test things a bit more abstractly.  I kind of thing that it makes less
and less sense as it's been obvious that these things are pretty concrete and
the data can should just flow more explicitly and more clearly down to the set
and tilings.

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

Isn't that what we're doing? maybe? Or am I missing stuff? :P

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

It's passing the size also, more or less. It was passing the mask and solid
color state before also.

I went ahead and stopped passing the layer_bounds around a bit, but then a bunch
of unit tests break. So I left it as is with TODO for now.

+  // the client API?
+  // TODO(danakj): What I mean here is IsSyncTree, for the commit to active
+  // case.
+  if (layer_tree_impl()->IsPendingTree()) {
+    tilings_->UpdateTilingsToCurrentRasterSource(
+        raster_source_.get(), raster_source_->GetSize(),
+        raster_source_->IsSolidColor(), invalidation_, MinimumContentsScale());
+  } else {
+    tilings_->UpdateTilingsFromPending(

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

Maybe ShareFromOther? I've been trying to make PLTS be more agnostic to the fact
that it lives on a layer that is a part of an active or pending tree, and make
it more to be just a caching mechanism that is told what scales are important
and which are not. It usually helps me when I think of what kind of an API would
I want a dumb(-but-can-share) cache to have, although I don't have any good
ideas at the moment :\

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

This function does a bit more than that.

For tilings that exist it invalidates them, similar to the pending tree does
with invalidatoin.
For tilings that are shared it (if needed, creates and) clones from the pending
tree's tiling set.

+        raster_source_.get(), *pending_set, raster_source_->GetSize(),
+        raster_source_->IsSolidColor(), invalidation_, MinimumContentsScale());
+  }
+}
+
 void PictureLayerImpl::NotifyTileStateChanged(const Tile* tile) {
   if (layer_tree_impl()->IsActiveTree()) {
     gfx::RectF layer_damage_rect =
@@ -651,9 +683,6 @@ const PictureLayerTiling* PictureLayerImpl::GetPendingOrActiveTwinTiling(
   PictureLayerImpl* twin_layer = GetPendingOrActiveTwinLayer();
   if (!twin_layer)
     return nullptr;
-  // TODO(danakj): Remove this when no longer swapping tilings.
-  if (!twin_layer->tilings_)
-    return nullptr;
   return twin_layer->tilings_->FindTilingWithScale(tiling->contents_scale());
 }
 
@@ -772,69 +801,15 @@ gfx::Size PictureLayerImpl::CalculateTileSize(
   return gfx::Size(tile_width, tile_height);
 }
 
-void PictureLayerImpl::SyncFromActiveLayer(const PictureLayerImpl* other) {
-  DCHECK(!other->needs_post_commit_initialization_);
-  DCHECK(other->tilings_);
-
-  if (!DrawsContent()) {
-    RemoveAllTilings();
-    return;
-  }
-
-  raster_page_scale_ = other->raster_page_scale_;
-  raster_device_scale_ = other->raster_device_scale_;
-  raster_source_scale_ = other->raster_source_scale_;
-  raster_contents_scale_ = other->raster_contents_scale_;
-  low_res_raster_contents_scale_ = other->low_res_raster_contents_scale_;
-
-  bool synced_high_res_tiling = false;
-  if (CanHaveTilings()) {
-    synced_high_res_tiling = tilings_->SyncTilings(
-        *other->tilings_, raster_source_->GetSize(), invalidation_,
-        MinimumContentsScale(), raster_source_.get());
-  } else {
-    RemoveAllTilings();
-  }
-
-  // If our MinimumContentsScale has changed to prevent the twin's high res
-  // tiling from being synced, we should reset the raster scale and let it be
-  // recalculated (1) again. This can happen if our bounds shrink to the point
-  // where min contents scale grows.
-  // (1) - TODO(vmpstr) Instead of hoping that this will be recalculated, we
-  // should refactor this code a little bit and actually recalculate this.
-  // However, this is a larger undertaking, so this will work for now.
-  if (!synced_high_res_tiling)
-    ResetRasterScale();
-  else
-    SanityCheckTilingState();
-}
-
-void PictureLayerImpl::SyncTiling(
-    const PictureLayerTiling* tiling) {
-  if (!tilings_)
-    return;
-  if (!CanHaveTilingWithScale(tiling->contents_scale()))
-    return;
-  tilings_->AddTiling(tiling->contents_scale(), raster_source_->GetSize());
-
-  // If this tree needs update draw properties, then the tiling will
-  // get updated prior to drawing or activation.  If this tree does not
-  // need update draw properties, then its transforms are up to date and
-  // we can create tiles for this tiling immediately.
-  if (!layer_tree_impl()->needs_update_draw_properties() &&
-      should_update_tile_priorities_) {
-    // TODO(danakj): Add a DCHECK() that we are not using occlusion tracking
-    // when we stop using the pending tree in the browser compositor. If we want
-    // to support occlusion tracking here, we need to dirty the draw properties
-    // or save occlusion as a draw property.
-    UpdateTilePriorities(Occlusion());
-  }
-}
-
 void PictureLayerImpl::GetContentsResourceId(
     ResourceProvider::ResourceId* resource_id,
     gfx::Size* resource_size) const {
-  DCHECK_EQ(bounds().ToString(), raster_source_->GetSize().ToString());
+  // The bounds and the pile size may differ if the pile wasn't updated (ie.
+  // PictureLayer::Update didn't happen). In that case the pile will be empty.
+  DCHECK_IMPLIES(!raster_source_->GetSize().IsEmpty(),
+                 bounds() == raster_source_->GetSize())
+      << " bounds " << bounds().ToString() << " pile "
+      << raster_source_->GetSize().ToString();
   gfx::Rect content_rect(bounds());
   PictureLayerTilingSet::CoverageIterator iter(
       tilings_.get(), 1.f, content_rect, ideal_contents_scale_);
@@ -862,20 +837,9 @@ void PictureLayerImpl::GetContentsResourceId(
 }
 
 void PictureLayerImpl::DoPostCommitInitialization() {
+  // TODO(danakj): Remove this.

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

\o/

   DCHECK(needs_post_commit_initialization_);
   DCHECK(layer_tree_impl()->IsPendingTree());
-
-  if (!tilings_)
-    tilings_ = PictureLayerTilingSet::Create(this);
-
-  PictureLayerImpl* twin_layer = GetPendingOrActiveTwinLayer();
-  if (twin_layer) {
-    // If the twin has never been pushed to, do not sync from it.
-    // This can happen if this function is called during activation.
-    if (!twin_layer->needs_post_commit_initialization_)
-      SyncFromActiveLayer(twin_layer);
-  }
-
   needs_post_commit_initialization_ = false;
 }
 
@@ -888,9 +852,6 @@ PictureLayerTiling* PictureLayerImpl::AddTiling(float contents_scale) {
 
   DCHECK(raster_source_->HasRecordings());
 
-  if (PictureLayerImpl* twin_layer = GetPendingOrActiveTwinLayer())
-    twin_layer->SyncTiling(tiling);

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

Just to make sure I understand, this is no longer needed because both pending
and active can create the same tiling and will share tiles due to mechanisms in
PictureLayerTiling?

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

This is no longer needed because when the active tree adds a new tiling at a
different scale, the pending tree will also do so when it UpdateTiles and will
have to raster it in order activate. Said another way:
- Active tilings don't matter to the pending tree, only what's needed to
activate matter.
- Pending tilings don't matter to the active tree, when we activate we'll create
the tiling and share all the tiles.

-
   return tiling;
 }
 
@@ -936,6 +897,12 @@ void PictureLayerImpl::AddTilingsForRasterScale() {
   // Make sure we always have one high-res (even if high == low).
   high_res->set_resolution(HIGH_RESOLUTION);
 
+  if (layer_tree_impl()->IsPendingTree()) {
+    // On the pending tree, drop any tilings that are non-ideal since we don't
+    // need them to activate anyway.
+    tilings_->RemoveNonIdealTilings();
+  }
+
   SanityCheckTilingState();
 }
 
@@ -1116,9 +1083,6 @@ void PictureLayerImpl::CleanUpTilingsOnActiveLayer(
                            layer_tree_impl()->create_low_res_tiling(), twin_set,
                            recycled_twin_set);
 
-  if (twin_set && twin_set->num_tilings() == 0)
-    twin->ResetRasterScale();
-
   if (recycled_twin_set && recycled_twin_set->num_tilings() == 0)
     recycled_twin->ResetRasterScale();