cc: Don't swap PictureLayerTilingSet on activate.

cc: Don't swap PictureLayerTilingSet on activate.

When activating, create new tilings on the active tree and make new
shared tiles. This way we don't have tilings on the recycle tree from
two frames ago, and the path which data travels is much more clear.

BUG=387116
Committed: https://crrev.com/a4ed6a25c69c34c986f36f01ecb5645babbdeed7
Cr-Commit-Position: refs/heads/master@{#307816}

[danakj, 2014-10-24 21:32:18]

Patchset #3 (id:40001) has been deleted

[danakj, 2014-10-27 18:40:02]

Patchset #5 (id:100001) has been deleted

[danakj, 2014-11-30 19:12:51]

Patchset #1 (id:1) has been deleted

[danakj, 2014-11-30 19:12:59]

Patchset #1 (id:20001) has been deleted

[danakj, 2014-11-30 19:13:05]

Patchset #1 (id:60001) has been deleted

[danakj, 2014-11-30 19:13:12]

Patchset #1 (id:80001) has been deleted

[danakj, 2014-11-30 19:13:17]

Patchset #1 (id:120001) has been deleted

[danakj, 2014-11-30 19:13:20]

Patchset #1 (id:140001) has been deleted

[danakj, 2014-11-30 19:21:00]

So this works and passes tests.

There's a tonne of TODOs left to address (not sure how many would be appropriate
to do so in this CL though), and I need to give some thought to some of the
PLT(Set) APIs.

Also PLTS::SyncTilings still exists and is being tested by unit tests. I need to
remove it and change those tests to use UpdateTilingsFromPending or something.

[enne (OOO), 2014-12-01 22:08:15]

enne@chromium.org changed reviewers:
+ enne@chromium.org

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

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (left):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:892: twin_layer->SyncTiling(tiling);
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?

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:77: // TODO(danakj): Can this be null to start?
I think it could be.  PushPropertiesTo happens before anything real happens, so
you wouldn't need to check for it anywhere, maybe?

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:165: // TODO(danakj): Stop always pushing
properties since we don't swap tilings.
\o/

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:566: void PictureLayerImpl::UpdateRasterSource(
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?

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:583: invalidation_.Clear();
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...

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:597: // TODO(danakj): Should this happen after
doing UpdateTiles so we can avoid
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?

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:600: // TODO(danakj): Just pass along the pile
to this function instead of using
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.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:840: // TODO(danakj): Remove this.
\o/

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:150: bool is_mask,
I'm a little not sure about this mask parameter in UpdateRasterSource, because
it makes it appear like being a mask can change.  (The previous API wasn't much
better.)  Layers don't change from being masks to not masks.  PictureLayerImpl
at least should never change.  PictureLayer just gets SetIsMask(true) via
SetMaskLayer and then can't ever change back.  Should is_mask be a constructor
parameter to PictureLayerImpl?

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:101: // TODO(danakj): In the activate
case can we do something simpler? Just copy
That sounds nice, although I think you'd still keep around bits of non-ideal
tilings that weren't invalidated?

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.h:60: bool SyncTilings(const
PictureLayerTilingSet& other,
SyncTilingsForTesting in the meantime?

https://codereview.chromium.org/640063010/diff/180001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer_impl.h (right):

https://codereview.chromium.org/640063010/diff/180001/cc/test/fake_picture_la...
cc/test/fake_picture_layer_impl.h:21: static scoped_ptr<FakePictureLayerImpl>
CreateWithRasterSource(
I'd slightly prefer CreateMaskWithRasterSource and CreateWithRasterSource and no
boolean parameter?

[vmpstr, 2014-12-01 22:38:02]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

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

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:581: // The |new_invalidation| must be cleared
before updating tilings since they
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?

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:589: if (could_have_tilings != CanHaveTilings())
nit: save CanHaveTilings() into a local before this, since you access it twice

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:600: // TODO(danakj): Just pass along the pile
to this function instead of using
Isn't that what we're doing? maybe? Or am I missing stuff? :P

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:609: tilings_->UpdateTilingsFromPending(
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 :\

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:49: if (layer_bounds.IsEmpty() ||
solid_color) {
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.

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:54: for (size_t i = 0; i <
tilings_.size(); ++i) {
nit: would erase(remove_if) with a lambda work here?

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:64:
tiling->UpdateTilesToCurrentRasterSource(raster_source, layer_invalidation,
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...

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:91: for (size_t i = 0; i <
tilings_.size(); ++i) {
This can probably be a separate function

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:101: // TODO(danakj): In the activate
case can we do something simpler? Just copy
On 2014/12/01 22:08:17, enne wrote:
> That sounds nice, although I think you'd still keep around bits of non-ideal
> tilings that weren't invalidated?

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.

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:118: for (size_t i = 0; i <
other.tilings_.size(); ++i) {
nit: for (PLT* tiling : other.tilings)

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:129:
this_tiling->CreateSharedTilesFromPending(*other.tilings_[i]);
I personally like this style of functionality, modulo "Pending". Maybe
CreateSharedTilesFromOther? or just CreateSharedTiles(...) or ShareFromThis

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:141: for (const auto& t : tilings_)
count_if?

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:203: for (size_t i = 0; i <
tilings_.size(); ++i) {
nit: I'd prefer erase/remove_if, but feel free to disagree.

[danakj, 2014-12-04 21:11:53]

Patchset #4 (id:220001) has been deleted

[danakj, 2014-12-05 22:30:45]

I've addressed feedback that was easy to address, and fixed the crashes on the
bots . The PictureLayerImpl interface is a little uh... awkward still. But I
gotta run for now.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (left):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:892: twin_layer->SyncTiling(tiling);
On 2014/12/01 22:08:17, enne wrote:
> 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?

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.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:77: // TODO(danakj): Can this be null to start?
On 2014/12/01 22:08:17, enne wrote:
> I think it could be.  PushPropertiesTo happens before anything real happens,
so
> you wouldn't need to check for it anywhere, maybe?

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

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:566: void PictureLayerImpl::UpdateRasterSource(
On 2014/12/01 22:08:17, enne wrote:
> 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?

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

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:581: // The |new_invalidation| must be cleared
before updating tilings since they
On 2014/12/01 22:38:03, vmpstr wrote:
> 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?

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\.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:583: invalidation_.Clear();
On 2014/12/01 22:08:17, enne wrote:
> 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...

ya..

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:589: if (could_have_tilings != CanHaveTilings())
On 2014/12/01 22:38:03, vmpstr wrote:
> nit: save CanHaveTilings() into a local before this, since you access it twice

Done.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:597: // TODO(danakj): Should this happen after
doing UpdateTiles so we can avoid
On 2014/12/01 22:08:17, enne wrote:
> 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?

Sure.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:600: // TODO(danakj): Just pass along the pile
to this function instead of using
On 2014/12/01 22:38:03, vmpstr wrote:
> Isn't that what we're doing? maybe? Or am I missing stuff? :P

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.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:609: tilings_->UpdateTilingsFromPending(
On 2014/12/01 22:38:03, vmpstr wrote:
> 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 :\

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.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:150: bool is_mask,
On 2014/12/01 22:08:17, enne wrote:
> SetMaskLayer and then can't ever change back.  Should is_mask be a constructor
> parameter to PictureLayerImpl?

Yes, that would be ideal if blink can actually do that. I will add a TODO.

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:49: if (layer_bounds.IsEmpty() ||
solid_color) {
On 2014/12/01 22:38:04, vmpstr wrote:
> 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.

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

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:54: for (size_t i = 0; i <
tilings_.size(); ++i) {
On 2014/12/01 22:38:03, vmpstr wrote:
> nit: would erase(remove_if) with a lambda work here?

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.

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:91: for (size_t i = 0; i <
tilings_.size(); ++i) {
On 2014/12/01 22:38:04, vmpstr wrote:
> This can probably be a separate function

Done.

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:118: for (size_t i = 0; i <
other.tilings_.size(); ++i) {
On 2014/12/01 22:38:04, vmpstr wrote:
> nit: for (PLT* tiling : other.tilings)

Done.

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:129:
this_tiling->CreateSharedTilesFromPending(*other.tilings_[i]);
On 2014/12/01 22:38:03, vmpstr wrote:
> I personally like this style of functionality, modulo "Pending". Maybe
> CreateSharedTilesFromOther? or just CreateSharedTiles(...) or ShareFromThis

Done. CloneTilesAndPropertiesFrom()

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:141: for (const auto& t : tilings_)
On 2014/12/01 22:38:03, vmpstr wrote:
> count_if?

Oh, cool. Done.

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:203: for (size_t i = 0; i <
tilings_.size(); ++i) {
On 2014/12/01 22:38:04, vmpstr wrote:
> nit: I'd prefer erase/remove_if, but feel free to disagree.

Done.

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.h:60: bool SyncTilings(const
PictureLayerTilingSet& other,
On 2014/12/01 22:08:17, enne wrote:
> SyncTilingsForTesting in the meantime?

Done.

https://codereview.chromium.org/640063010/diff/180001/cc/test/fake_picture_la...
File cc/test/fake_picture_layer_impl.h (right):

https://codereview.chromium.org/640063010/diff/180001/cc/test/fake_picture_la...
cc/test/fake_picture_layer_impl.h:21: static scoped_ptr<FakePictureLayerImpl>
CreateWithRasterSource(
On 2014/12/01 22:08:17, enne wrote:
> I'd slightly prefer CreateMaskWithRasterSource and CreateWithRasterSource and
no
> boolean parameter?

Done.

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

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:77: // TODO(danakj): Can this be null to start?
On 2014/12/05 22:30:44, danakj wrote:
> On 2014/12/01 22:08:17, enne wrote:
> > I think it could be.  PushPropertiesTo happens before anything real happens,
> so
> > you wouldn't need to check for it anywhere, maybe?
> 
> Cool. It crashes a lot of unit tests and I'm not sure why. I'll investigate
> changing this independently.

Sounds good to me.

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:150: bool is_mask,
On 2014/12/05 22:30:44, danakj wrote:
> On 2014/12/01 22:08:17, enne wrote:
> > SetMaskLayer and then can't ever change back.  Should is_mask be a
constructor
> > parameter to PictureLayerImpl?
> 
> Yes, that would be ideal if blink can actually do that. I will add a TODO.

Sorry.  It'd be ideal if Blink could do that for PictureLayer.  However, you can
make it a constructor parameter for PictureLayerImpl now?

https://codereview.chromium.org/640063010/diff/280001/cc/base/scoped_ptr_vect...
File cc/base/scoped_ptr_vector.h (right):

https://codereview.chromium.org/640063010/diff/280001/cc/base/scoped_ptr_vect...
cc/base/scoped_ptr_vector.h:167: iterator remove_if(Predicate predicate) {
I assume you didn't mean to include this in this patch?

https://codereview.chromium.org/640063010/diff/280001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_perftest.cc (right):

https://codereview.chromium.org/640063010/diff/280001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_perftest.cc:62: false);
I thought you got rid of this boolean parameter?

[danakj, 2014-12-06 17:14:08]

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.h (right):

https://codereview.chromium.org/640063010/diff/180001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.h:150: bool is_mask,
On 2014/12/05 23:02:02, enne wrote:
> On 2014/12/05 22:30:44, danakj wrote:
> > On 2014/12/01 22:08:17, enne wrote:
> > > SetMaskLayer and then can't ever change back.  Should is_mask be a
> constructor
> > > parameter to PictureLayerImpl?
> > 
> > Yes, that would be ideal if blink can actually do that. I will add a TODO.
> 
> Sorry.  It'd be ideal if Blink could do that for PictureLayer.  However, you
can
> make it a constructor parameter for PictureLayerImpl now?

Yah I guess so. I'll just dcheck in picture layer if it changes then

https://codereview.chromium.org/640063010/diff/280001/cc/base/scoped_ptr_vect...
File cc/base/scoped_ptr_vector.h (right):

https://codereview.chromium.org/640063010/diff/280001/cc/base/scoped_ptr_vect...
cc/base/scoped_ptr_vector.h:167: iterator remove_if(Predicate predicate) {
On 2014/12/05 23:02:02, enne wrote:
> I assume you didn't mean to include this in this patch?

This will re base away, but need it to run bots.

https://codereview.chromium.org/640063010/diff/280001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_perftest.cc (right):

https://codereview.chromium.org/640063010/diff/280001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_perftest.cc:62: false);
On 2014/12/05 23:02:02, enne wrote:
> I thought you got rid of this boolean parameter?

I always forget to try build perf tests *^^*

[danakj, 2014-12-06 17:46:09]

ok fixed those things! (perftests and is_mask via constructor)

[danakj, 2014-12-08 16:49:26]

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/640063010/diff/180001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:64:
tiling->UpdateTilesToCurrentRasterSource(raster_source, layer_invalidation,
On 2014/12/01 22:38:04, vmpstr wrote:
> 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...

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.

[enne (OOO), 2014-12-08 21:02:38]

lgtm.  I think there's a bunch of cleanup to be had after this, but this seems
like a good step to land.

Also, I agree that change source + invalidate + resize seems like an atomic
operation right now, because these operations are not separable or reorderable. 
vmpstr convinced me that it might be possible to make them separable/reorderable
without adding complexity, and thus simplifying the API into three more easy to
understand steps.  I think that approach could work too, if it ends up not
adding complexity to make those steps more robust.

Removing CreateTile seems vaguely possible.  It used to be that it did more
checking about whether or not it could create a tile, but that's all much more
cleanly on the raster source these days.  The only tricky part is that you need
to reach out to the TileManager to create tiles.  You can create tiles at
different times too, but maybe we can restrict that some.  We could certainly
change CreateTile in the short term to not be able to return a nullptr, and that
might be a good cleanup.

https://codereview.chromium.org/640063010/diff/320001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/640063010/diff/320001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.h:68: // TODO(danakj): DONT NEED LAYER
BOUNDSS WE HAVE THE RASTER SOURCE
BOUNDSS typo.  Is there a reason to punt this TODO?

[danakj, 2014-12-08 22:27:08]

Everything is now more awesome. I have a single method on PLTS, which removes
the awkward branch in PLI. And I split the PLT steps (Resize, Invalidate,
SetRasterSource) so I could reuse some of them when cloning but not all of them.

I also added some unit test coverage to show when we activate and there's a
non-shared tiling, the tiles on it are updated to use the new active raster
source (instead of keeping an old raster source alive).

https://codereview.chromium.org/640063010/diff/320001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/640063010/diff/320001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.h:68: // TODO(danakj): DONT NEED LAYER
BOUNDSS WE HAVE THE RASTER SOURCE
On 2014/12/08 21:02:38, enne wrote:
> BOUNDSS typo.  Is there a reason to punt this TODO?

Ya... I tried to do this a couple times, but it's a pretty annoying change for
unit tests, so I would like to do it separately. I should not capslock though
*^^*

[enne (OOO), 2014-12-08 23:22:38]

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:139: LOG(ERROR) << "Create unshared tile";
<_<

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:170: void
PictureLayerTiling::CloneTilesAndPropertiesFrom(
Maybe to be consistent with other "push" functions, call this
PushPropertiesAndTilesTo?

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:195: RemoveTileAt(key.first, key.second,
recycled_twin);
It's a little awkward to call this recycled_twin, but it's always nullptr. 
Maybe your DCHECK should just always check against nullptr and this shouldn't be
named?

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:328: // There is no recycled twin for the
peding tree during commit, or for the
typo

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:344: twin_tiling =
client_->GetPendingOrActiveTwinTiling(this);
Is this required? It seems like for the activation scenario, you either have a
shared tiling or an unshared tiling.  If this is an unshared tiling, there's no
twin.  If this is a shared tiling, this create tile logic should already have
happened in the CloneTilesAndPropertiesFrom loop?

[vmpstr, 2014-12-09 00:10:19]

https://codereview.chromium.org/640063010/diff/410001/cc/base/scoped_ptr_vect...
File cc/base/scoped_ptr_vector.h (right):

https://codereview.chromium.org/640063010/diff/410001/cc/base/scoped_ptr_vect...
cc/base/scoped_ptr_vector.h:167: iterator remove_if(Predicate predicate) {
Gone after rebase?

https://codereview.chromium.org/640063010/diff/410001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/640063010/diff/410001/cc/layers/picture_layer...
cc/layers/picture_layer_impl.cc:161: !layer_impl->tilings_->num_tilings());
nit: Above you do blah == 0, and here you do !blah. I think we should be
consistent (I prefer == 0, but either way works)

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:184: for (auto tile_map_pair : tiles_) {
nit: const auto&

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:190: for (auto key : to_remove) {
nit: const auto&

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:237: if (!resized)
Can this be done early on? Or is the tile size check important even if the
bounds remain the same? I see that it was that way before, so this is fine...
I'm just wondering what the reason is.

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:54: for (PictureLayerTiling*
twin_tiling : twin_set->tilings_) {
I feel like we could do better in terms of speed.. Like compute all three sets
ahead of time (in both, in twin only, in this only) in one pass, and then
update/create/invalidate in the other pass. I also think this is fine to leave
for later :)

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:164: auto to_remove =
tilings_.remove_if([](PictureLayerTiling* t) -> bool {
-> bool is optional here? Or are we requiring it?

[danakj, 2014-12-09 18:20:23]

PTAL

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:139: LOG(ERROR) << "Create unshared tile";
On 2014/12/08 23:22:37, enne wrote:
> <_<

Removed

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:170: void
PictureLayerTiling::CloneTilesAndPropertiesFrom(
On 2014/12/08 23:22:37, enne wrote:
> Maybe to be consistent with other "push" functions, call this
> PushPropertiesAndTilesTo?

This is called from PLTS::UpdateToCurrentRasterSource, so reversing the "update
this" to a "update that" when calling the tilings seemed awkward.

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:195: RemoveTileAt(key.first, key.second,
recycled_twin);
On 2014/12/08 23:22:37, enne wrote:
> It's a little awkward to call this recycled_twin, but it's always nullptr. 
> Maybe your DCHECK should just always check against nullptr and this shouldn't
be
> named?

Just trying to give the nullptr a name, otherwise it's like "What is that
nullptr for?" The def'n of recycled_twin did get quite far from this, I'll move
it closer?

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:328: // There is no recycled twin for the
peding tree during commit, or for the
On 2014/12/08 23:22:37, enne wrote:
> typo

Done.

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:344: twin_tiling =
client_->GetPendingOrActiveTwinTiling(this);
On 2014/12/08 23:22:37, enne wrote:
> Is this required? It seems like for the activation scenario, you either have a
> shared tiling or an unshared tiling.  If this is an unshared tiling, there's
no
> twin.  If this is a shared tiling, this create tile logic should already have
> happened in the CloneTilesAndPropertiesFrom loop?

Ya! Thanks this is no longer needed with the new stuff in PLTS.

https://codereview.chromium.org/640063010/diff/410001/cc/base/scoped_ptr_vect...
File cc/base/scoped_ptr_vector.h (right):

https://codereview.chromium.org/640063010/diff/410001/cc/base/scoped_ptr_vect...
cc/base/scoped_ptr_vector.h:167: iterator remove_if(Predicate predicate) {
On 2014/12/09 00:10:19, vmpstr wrote:
> Gone after rebase?

Yep

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:184: for (auto tile_map_pair : tiles_) {
On 2014/12/09 00:10:19, vmpstr wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:190: for (auto key : to_remove) {
On 2014/12/09 00:10:19, vmpstr wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:237: if (!resized)
On 2014/12/09 00:10:19, vmpstr wrote:
> Can this be done early on? Or is the tile size check important even if the
> bounds remain the same? I see that it was that way before, so this is fine...
> I'm just wondering what the reason is.

Right, if the tile size changes we have to do that (ie when the mask layer gets
too big and can't have a tile anymore). There are tests for this.

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:54: for (PictureLayerTiling*
twin_tiling : twin_set->tilings_) {
On 2014/12/09 00:10:19, vmpstr wrote:
> I feel like we could do better in terms of speed.. Like compute all three sets
> ahead of time (in both, in twin only, in this only) in one pass, and then
> update/create/invalidate in the other pass. I also think this is fine to leave
> for later :)

Writing something much faster here would be very complicated code in comparison
(like using the knowledge it's sorted by scale and then traversing both this and
other tiling lists kinda like a merge sort.. I'm not convinced it will matter
esp since the FindTilingWithScale is looking at <= 2 (or actually 1 on desktop)
tilings.

https://codereview.chromium.org/640063010/diff/410001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.cc:164: auto to_remove =
tilings_.remove_if([](PictureLayerTiling* t) -> bool {
On 2014/12/09 00:10:19, vmpstr wrote:
> -> bool is optional here? Or are we requiring it?

"However, if a lambda has one statement and that statement is a return statement
(and it returns an expression), the compiler can deduce the return type from the
type of that one returned expression. You have multiple statements in your
lambda, so it doesn't deduce the type."

Woa.

[enne (OOO), 2014-12-09 18:23:40]

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:195: RemoveTileAt(key.first, key.second,
recycled_twin);
On 2014/12/09 18:20:22, danakj wrote:
> On 2014/12/08 23:22:37, enne wrote:
> > It's a little awkward to call this recycled_twin, but it's always nullptr. 
> > Maybe your DCHECK should just always check against nullptr and this
shouldn't
> be
> > named?
> 
> Just trying to give the nullptr a name, otherwise it's like "What is that
> nullptr for?" The def'n of recycled_twin did get quite far from this, I'll
move
> it closer?

I think naming it only makes it more confusing.  Maybe if you called it
no_recycling_twin or null_twin or added overloads that didn't take a third
parameter?

[danakj, 2014-12-09 18:26:08]

PTAL

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/380001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:195: RemoveTileAt(key.first, key.second,
recycled_twin);
On 2014/12/09 18:23:40, enne wrote:
> On 2014/12/09 18:20:22, danakj wrote:
> > On 2014/12/08 23:22:37, enne wrote:
> > > It's a little awkward to call this recycled_twin, but it's always nullptr.

> > > Maybe your DCHECK should just always check against nullptr and this
> shouldn't
> > be
> > > named?
> > 
> > Just trying to give the nullptr a name, otherwise it's like "What is that
> > nullptr for?" The def'n of recycled_twin did get quite far from this, I'll
> move
> > it closer?
> 
> I think naming it only makes it more confusing.  Maybe if you called it
> no_recycling_twin or null_twin or added overloads that didn't take a third
> parameter?

I think an overload would make it too easy to not pass a recycled twin, whereas
this code is documenting (and proving) that there is none. Sure I can call it
null/no something!

[enne (OOO), 2014-12-09 18:30:37]

lgtm again, thanks.

https://codereview.chromium.org/640063010/diff/450001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/450001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:198: for (auto tile_map_pair : tiles_)
const auto& here too?

[danakj, 2014-12-09 18:31:15]

https://codereview.chromium.org/640063010/diff/450001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/450001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:198: for (auto tile_map_pair : tiles_)
On 2014/12/09 18:30:37, enne wrote:
> const auto& here too?

Thanks, done.

[vmpstr, 2014-12-09 18:36:06]

lgtm as well

https://codereview.chromium.org/640063010/diff/450001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/450001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:327: PictureLayerTiling* recycled_twin =
NULL;
nit: null_recycled_twin = nullptr, to be consistent with the other place you do
this.

[danakj, 2014-12-09 18:36:45]

https://codereview.chromium.org/640063010/diff/450001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/640063010/diff/450001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:327: PictureLayerTiling* recycled_twin =
NULL;
On 2014/12/09 18:36:05, vmpstr wrote:
> nit: null_recycled_twin = nullptr, to be consistent with the other place you
do
> this.

Done.

[danakj, 2014-12-09 18:37:31]

I'm concerned this seems to cause some content shell tests to time out on
android..

http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...

So I'm going to try repro locally.

[danakj, 2014-12-10 21:45:55]

Patchset #19 (id:530001) has been deleted

[danakj, 2014-12-10 21:46:02]

Patchset #18 (id:510001) has been deleted

[danakj, 2014-12-10 23:23:00]

ok this should be good to land now.

[danakj, 2014-12-10 23:24:35]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-12-10 23:26:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640063010/570001

[danakj, 2014-12-10 23:54:24]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-12-11 00:11:18]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-12-11 00:12:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640063010/570001

[commit-bot: I haz the power, 2014-12-11 01:09:47]

Committed patchset #19 (id:570001)

[commit-bot: I haz the power, 2014-12-11 01:10:28]

Patchset 19 (id:??) landed as
https://crrev.com/a4ed6a25c69c34c986f36f01ecb5645babbdeed7
Cr-Commit-Position: refs/heads/master@{#307816}