cc: Remove tile sharing from tilings.

cc: Remove tile sharing from tilings.

This patch removes sharing tiles from tilings. Previously, tiles
that were not invalidated were shared between the pending and
the active trees. With this patch, tiles that are exist on the
active tree and are not invalidated are not created on the pending
tree, resulting in fewer overall tiles. This improves performance
of updating and managing tiles.

Also, this patch opens up opportunities for a lot of code clean
up that can otherwise be confusing when two trees are involved.

R=danakj, enne

Committed: https://crrev.com/0061b75e810f088900a0a8fbdd29ba5595c73016
Cr-Commit-Position: refs/heads/master@{#326804}

Committed: https://crrev.com/a960b5558d7210bb5bfa67a832f800bb36e629a7
Cr-Commit-Position: refs/heads/master@{#327553}

[vmpstr, 2015-04-01 21:56:32]

vmpstr@chromium.org changed reviewers:
+ danakj@chromium.org, enne@chromium.org

[vmpstr, 2015-04-01 21:56:32]

Hi. This isn't done, but I'd appreciate an early review to see how people feel
about this approach.

This saves about 2% on total fast path thread times in tough compositor cases.

[danakj, 2015-04-02 21:39:16]

Why did you do a Pending/ActivePLT split classes like that? It seems like it'll
be harder to do commit-to-active that way, and you have to duplicate stuff.

[enne (OOO), 2015-04-02 21:45:34]

I have to admit I am not super convinced on the pending/active split.  There are
a lot of places where you still have to add conditionals, some extra code
duplication, etc.

I agree that PictureLayerTiling is a bit of a mess, but I am not convinced this
is the right split.  It's not the same as something like
PicturePile/PicturePileImpl where they both have considerably different APIs and
logic, even though they're the same data structure.

https://codereview.chromium.org/1051993002/diff/1/cc/resources/active_picture...
File cc/resources/active_picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/active_picture...
cc/resources/active_picture_layer_tiling.cc:189: // If we decide that the tile
is occluded, but the same tile would not be
Can you help me understand this logic in the land of not sharing tiles?

Isn't it possible that there is no pending tile in the same location (because no
invalidation), but that it is also not occluded (because of other changes)?

It seems like you want to grab the occlusion and viewport from the pending twin
and just do a second check, regardless of tile existence?

https://codereview.chromium.org/1051993002/diff/1/cc/resources/active_picture...
cc/resources/active_picture_layer_tiling.cc:234: bool
ActivePictureLayerTiling::IsTileOccludedOnActiveTree(
This looks a lot like PendingPictureLayerTiling::IsTileOccluded

https://codereview.chromium.org/1051993002/diff/1/cc/resources/pending_pictur...
File cc/resources/pending_picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/pending_pictur...
cc/resources/pending_picture_layer_tiling.cc:136: if
(!TilingMatchesTileIndecies(active_twin))
indices?

https://codereview.chromium.org/1051993002/diff/1/cc/resources/picture_layer_...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/picture_layer_...
cc/resources/picture_layer_tiling.h:70: virtual void SetRasterSourceOnTiles() =
0;
This function has some api smell for me.  It's weird that it doesn't take an
argument.  It's weird that it exists alongside SetRasterSourceAndResize.  Maybe
its work should be rolled into something else?

https://codereview.chromium.org/1051993002/diff/1/cc/resources/picture_layer_...
cc/resources/picture_layer_tiling.h:76: virtual bool
IsTileRequiredForActivation(const Tile* tile) const = 0;
This is weird to me to have base level functions that only one tiling really
implements?

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_all.cc:182:
tiling_->pending_visible_rect(),
pending and current are super confusing to me when reading code, because
sometimes they're the same and sometimes they're not.

I understand why you have the pending function, because it's super convenient.

In this call site, since this is the PendingVisibleTilingIterator, should these
both be current_visible_rect?

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
File cc/resources/tiling_set_raster_queue_all.h (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_all.h:151: VISIBLE_RECT,
Should this be ACTIVE_VISIBLE_RECT?

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_all.h:152: PENDING_VISIBLE_RECT,
This is nice!

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
File cc/resources/tiling_set_raster_queue_required.cc (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_required.cc:35: tiling->IsActiveTree()) {
This is a bit weird.  ActiveTiling knows it's active, but PictureLayerTiling has
to ask the client?

Also, aren't both branches of this if statement the same? If it's the active
tree, then you ask for the pending visible rect.  If it's pending, you ask for
the current visible rect.  Why not just ask for the pending visible rect in both
cases?

[vmpstr, 2015-04-02 22:33:27]

What I'm trying to accomplish is to have a tiling (active tree tiling) that is
complete and is capable of drawing itself all on its own. That's easy, since we
already have these tilings. The slightly more confusing part is I'm also trying
to have a tiling (pending tree tiling) that is sort of a patch on top of the
active tree tiling. When applied, it will result in a new active tree tiling.
This patch is incomplete (only has some tiles).

If that's implemented, the way I was thinking of doing commit to active is to
actually create a matching empty pending tiling, do the invalidation/raster
source updates, and apply it to the active tree in the same function. That is,
from the tiling perspective it wouldn't be commit to active, it would just be a
regular commit flow. From the tiling set, it will just do things synchronously. 

From my understanding, neither one of you likes this approach, so I will see
what else I can do. I don't want to add more complexity to PictureLayerTiling,
since I find that it's already too complicated for my taste. Maybe as a first
step, I should try to uncomplicate things that I find complicated.

Thanks for the review!

https://codereview.chromium.org/1051993002/diff/1/cc/resources/active_picture...
File cc/resources/active_picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/active_picture...
cc/resources/active_picture_layer_tiling.cc:189: // If we decide that the tile
is occluded, but the same tile would not be
On 2015/04/02 21:45:34, enne wrote:
> Can you help me understand this logic in the land of not sharing tiles?
> 
> Isn't it possible that there is no pending tile in the same location (because
no
> invalidation), but that it is also not occluded (because of other changes)?
> 
> It seems like you want to grab the occlusion and viewport from the pending
twin
> and just do a second check, regardless of tile existence?

This tile exists on the active tree and the question here is whether it's
occluded (ie we can skip it for raster). The condition to determine that is
two-fold:

1. If it's not occluded on the active tree, then it's not occluded (the initial
if)
2. If, upon activation, this tile will not be overriden (pending tile doesn't
have that tile but normally would (tile indices match)), but at the same time,
upon activation, this tile will be unoccluded, then it's considered not
occluded.

I guess the way I implement it here (past the initial if) is if we have a
pending tree and it doesn't have a tile at the index and indices match, then ask
the pending tree if it considers this tile to be occluded. Otherwise, it's
occluded.

https://codereview.chromium.org/1051993002/diff/1/cc/resources/active_picture...
cc/resources/active_picture_layer_tiling.cc:234: bool
ActivePictureLayerTiling::IsTileOccludedOnActiveTree(
On 2015/04/02 21:45:34, enne wrote:
> This looks a lot like PendingPictureLayerTiling::IsTileOccluded

Yep, I think this can be moved to
PictureLayerTiling::IsTileOccludedOnCurrentTree type of thing

https://codereview.chromium.org/1051993002/diff/1/cc/resources/pending_pictur...
File cc/resources/pending_picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/pending_pictur...
cc/resources/pending_picture_layer_tiling.cc:136: if
(!TilingMatchesTileIndecies(active_twin))
On 2015/04/02 21:45:34, enne wrote:
> indices?

Yup, poor spelling on my part.

https://codereview.chromium.org/1051993002/diff/1/cc/resources/picture_layer_...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/picture_layer_...
cc/resources/picture_layer_tiling.h:70: virtual void SetRasterSourceOnTiles() =
0;
On 2015/04/02 21:45:34, enne wrote:
> This function has some api smell for me.  It's weird that it doesn't take an
> argument.  It's weird that it exists alongside SetRasterSourceAndResize. 
Maybe
> its work should be rolled into something else?

With unshared tiles, I think this could be done as part of
SetRasterSourceAndResize.

https://codereview.chromium.org/1051993002/diff/1/cc/resources/picture_layer_...
cc/resources/picture_layer_tiling.h:76: virtual bool
IsTileRequiredForActivation(const Tile* tile) const = 0;
On 2015/04/02 21:45:34, enne wrote:
> This is weird to me to have base level functions that only one tiling really
> implements?

Well, both trees implement these (aside from TakeTilesAndPropertiesFrom).
IsTileRequiredForDraw for instance just happens to be always false on the
pending tree... Is that the part you mean? Or do you mean the couple of "nothing
to do here" functions? That's just from the fact that I'm trying to treat these
tilings as the same from the tiling set perspective.

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_all.cc:182:
tiling_->pending_visible_rect(),
On 2015/04/02 21:45:34, enne wrote:
> pending and current are super confusing to me when reading code, because
> sometimes they're the same and sometimes they're not.
> 
> I understand why you have the pending function, because it's super convenient.
> 
> In this call site, since this is the PendingVisibleTilingIterator, should
these
> both be current_visible_rect?

At this point we don't really know if the tiling set is coming from the pending
tree or the active tree... For the active tree, this does an actual difference
between what the pending visible rect is and what the currently set visible rect
is. For the pending tree, these are both the same and the iterator will be
empty.

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
File cc/resources/tiling_set_raster_queue_required.cc (right):

https://codereview.chromium.org/1051993002/diff/1/cc/resources/tiling_set_ras...
cc/resources/tiling_set_raster_queue_required.cc:35: tiling->IsActiveTree()) {
On 2015/04/02 21:45:34, enne wrote:
> This is a bit weird.  ActiveTiling knows it's active, but PictureLayerTiling
has
> to ask the client?
> 
> Also, aren't both branches of this if statement the same? If it's the active
> tree, then you ask for the pending visible rect.  If it's pending, you ask for
> the current visible rect.  Why not just ask for the pending visible rect in
both
> cases?

I can reduce it to just an if based on type. For required for draw, we want
active visible rect; for required for activation, we want the pending visible
rect. I guess it's a bit of a misnomer for me to say pending, since we reserve
that for pending tree/active tree. To clarify, pending visible rect means the
rect that will become the current visible rect upon activation (which in cases
of no pending tree is the current visible rect). Whereas the current visible
rect is the visible rect that is currently in effect on the given tiling (ie
it's visible rect on pending when asking the pending tree and it's visible rect
on active when asking the active tree).

[danakj, 2015-04-02 22:36:47]

On Thu, Apr 2, 2015 at 3:33 PM, <vmpstr@chromium.org> wrote:

> What I'm trying to accomplish is to have a tiling (active tree tiling)
> that is
> complete and is capable of drawing itself all on its own. That's easy,
> since we
> already have these tilings. The slightly more confusing part is I'm also
> trying
> to have a tiling (pending tree tiling) that is sort of a patch on top of
> the
> active tree tiling. When applied, it will result in a new active tree
> tiling.
> This patch is incomplete (only has some tiles).
>
> If that's implemented, the way I was thinking of doing commit to active is
> to
> actually create a matching empty pending tiling, do the invalidation/raster
> source updates, and apply it to the active tree in the same function. That
> is,
> from the tiling perspective it wouldn't be commit to active, it would just
> be a
> regular commit flow. From the tiling set, it will just do things
> synchronously.
>

This makes a lot of sense, thanks :)


>
> From my understanding, neither one of you likes this approach, so I will
> see
> what else I can do. I don't want to add more complexity to
> PictureLayerTiling,
> since I find that it's already too complicated for my taste. Maybe as a
> first
> step, I should try to uncomplicate things that I find complicated.
>

IMHO the split would be nicer if it wasnt virtual/overrides. The way it is
now they are acting like they implement the same thing but they are also
really different. It feels awkward to me. If other alternatives are more
awkward maybe we can reconsider :)


>
> Thanks for the review!
>
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/active_picture_layer_tiling.cc
> File cc/resources/active_picture_layer_tiling.cc (right):
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/active_picture_layer_tiling.cc#newcode189
> cc/resources/active_picture_layer_tiling.cc:189: // If we decide that
> the tile is occluded, but the same tile would not be
> On 2015/04/02 21:45:34, enne wrote:
>
>> Can you help me understand this logic in the land of not sharing
>>
> tiles?
>
>  Isn't it possible that there is no pending tile in the same location
>>
> (because no
>
>> invalidation), but that it is also not occluded (because of other
>>
> changes)?
>
>  It seems like you want to grab the occlusion and viewport from the
>>
> pending twin
>
>> and just do a second check, regardless of tile existence?
>>
>
> This tile exists on the active tree and the question here is whether
> it's occluded (ie we can skip it for raster). The condition to determine
> that is two-fold:
>
> 1. If it's not occluded on the active tree, then it's not occluded (the
> initial if)
> 2. If, upon activation, this tile will not be overriden (pending tile
> doesn't have that tile but normally would (tile indices match)), but at
> the same time, upon activation, this tile will be unoccluded, then it's
> considered not occluded.
>
> I guess the way I implement it here (past the initial if) is if we have
> a pending tree and it doesn't have a tile at the index and indices
> match, then ask the pending tree if it considers this tile to be
> occluded. Otherwise, it's occluded.
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/active_picture_layer_tiling.cc#newcode234
> cc/resources/active_picture_layer_tiling.cc:234: bool
> ActivePictureLayerTiling::IsTileOccludedOnActiveTree(
> On 2015/04/02 21:45:34, enne wrote:
>
>> This looks a lot like PendingPictureLayerTiling::IsTileOccluded
>>
>
> Yep, I think this can be moved to
> PictureLayerTiling::IsTileOccludedOnCurrentTree type of thing
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/pending_picture_layer_tiling.cc
> File cc/resources/pending_picture_layer_tiling.cc (right):
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/pending_picture_layer_tiling.cc#newcode136
> cc/resources/pending_picture_layer_tiling.cc:136: if
> (!TilingMatchesTileIndecies(active_twin))
> On 2015/04/02 21:45:34, enne wrote:
>
>> indices?
>>
>
> Yup, poor spelling on my part.
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/picture_layer_tiling.h
> File cc/resources/picture_layer_tiling.h (right):
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/picture_layer_tiling.h#newcode70
> cc/resources/picture_layer_tiling.h:70: virtual void
> SetRasterSourceOnTiles() = 0;
> On 2015/04/02 21:45:34, enne wrote:
>
>> This function has some api smell for me.  It's weird that it doesn't
>>
> take an
>
>> argument.  It's weird that it exists alongside
>>
> SetRasterSourceAndResize.  Maybe
>
>> its work should be rolled into something else?
>>
>
> With unshared tiles, I think this could be done as part of
> SetRasterSourceAndResize.
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/picture_layer_tiling.h#newcode76
> cc/resources/picture_layer_tiling.h:76: virtual bool
> IsTileRequiredForActivation(const Tile* tile) const = 0;
> On 2015/04/02 21:45:34, enne wrote:
>
>> This is weird to me to have base level functions that only one tiling
>>
> really
>
>> implements?
>>
>
> Well, both trees implement these (aside from
> TakeTilesAndPropertiesFrom). IsTileRequiredForDraw for instance just
> happens to be always false on the pending tree... Is that the part you
> mean? Or do you mean the couple of "nothing to do here" functions?
> That's just from the fact that I'm trying to treat these tilings as the
> same from the tiling set perspective.
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/tiling_set_raster_queue_all.cc
> File cc/resources/tiling_set_raster_queue_all.cc (right):
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/tiling_set_raster_queue_all.cc#newcode182
> cc/resources/tiling_set_raster_queue_all.cc:182:
> tiling_->pending_visible_rect(),
> On 2015/04/02 21:45:34, enne wrote:
>
>> pending and current are super confusing to me when reading code,
>>
> because
>
>> sometimes they're the same and sometimes they're not.
>>
>
>  I understand why you have the pending function, because it's super
>>
> convenient.
>
>  In this call site, since this is the PendingVisibleTilingIterator,
>>
> should these
>
>> both be current_visible_rect?
>>
>
> At this point we don't really know if the tiling set is coming from the
> pending tree or the active tree... For the active tree, this does an
> actual difference between what the pending visible rect is and what the
> currently set visible rect is. For the pending tree, these are both the
> same and the iterator will be empty.
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/tiling_set_raster_queue_required.cc
> File cc/resources/tiling_set_raster_queue_required.cc (right):
>
> https://codereview.chromium.org/1051993002/diff/1/cc/
> resources/tiling_set_raster_queue_required.cc#newcode35
> cc/resources/tiling_set_raster_queue_required.cc:35:
> tiling->IsActiveTree()) {
> On 2015/04/02 21:45:34, enne wrote:
>
>> This is a bit weird.  ActiveTiling knows it's active, but
>>
> PictureLayerTiling has
>
>> to ask the client?
>>
>
>  Also, aren't both branches of this if statement the same? If it's the
>>
> active
>
>> tree, then you ask for the pending visible rect.  If it's pending, you
>>
> ask for
>
>> the current visible rect.  Why not just ask for the pending visible
>>
> rect in both
>
>> cases?
>>
>
> I can reduce it to just an if based on type. For required for draw, we
> want active visible rect; for required for activation, we want the
> pending visible rect. I guess it's a bit of a misnomer for me to say
> pending, since we reserve that for pending tree/active tree. To clarify,
> pending visible rect means the rect that will become the current visible
> rect upon activation (which in cases of no pending tree is the current
> visible rect). Whereas the current visible rect is the visible rect that
> is currently in effect on the given tiling (ie it's visible rect on
> pending when asking the pending tree and it's visible rect on active
> when asking the active tree).
>
> https://codereview.chromium.org/1051993002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmpstr, 2015-04-07 00:28:37]

Ok, how's this? There's still a couple of eviction tests failing, but that's
because this is blocked on some eviction iterator changes.

[danakj, 2015-04-08 00:13:07]

This looks better so far.. I got as far as my comments are... and I'll come back
tomorrow.

https://codereview.chromium.org/1051993002/diff/100001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:685: if (*iter) {
can you add a comment explaining the expectations

https://codereview.chromium.org/1051993002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:1538: SetupTrees(pending_pile,
active_pile);
Would it keep these tests simpler to test on the pending tree? Maybe just
SetupPendingTree() instead of both trees?

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:138: if
(raster_source_->CoversRect(tile->content_rect(),
Doing scaling math for every tile here is a bit sad, but I guess the above TODO
covers it.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:151: for (const auto& tile_pair :
pending_twin->tiles_)
can you // this is copying the tiles_ from the pending twin or something, i was
like wat. or is there a way to make this more clear in the code?

how about insert(pending_twin->tiles_.begin(), pending_twin->tiles_.end())? But
then that does't replace does it. This operator[] per tile seems both hard to
read and performancely problematic, but maybe there is not a better way. What
about swap() and insert?

[enne (OOO), 2015-04-09 17:54:24]

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (left):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:111: // Check our twin for a valid tile.
I'm not 100% opposed to different classes for pending and active in the future,
but having the side by side diff makes this a lot easier to see this amazing
code deletion.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:134: // TODO(vmpstr): Figure out how to
pass "remove a tile" information from
Is this TODO an optimization, in that you think checking all these CoversRect
calls are inefficient? I guess in the past we issues invalidations for all of
the missing recordings and that would drop the tiles and force them to not be
recreated.

Now that https://codereview.chromium.org/1061203002 has landed, the recording
rect should be always correct, because there can no longer be holes in it.  You
should just be able to throw away anything outside of it.  Not sure if that's
more efficient or not.

Maybe SetRasterSourceAndResize could do this with the recorded region as well as
the layer bounds? *thinking out loud*

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:295: // Active tree should always create a
tile.
O_O

Can the active tree DCHECK that its raster source covers this rect?

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:731: // If we're not allowed to required
tiles for activation, then the tile is
*require

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:749: // Otherwise, the tile is required if
it's in the pending tree's visible
I think this comment (and a lot of other ones in this function and some other
longer functions with lots of conditionals like ShouldCreateTileAt) could maybe
benefit from fewer "describing what the code is doing" comments and more "why is
this doing this" comments.  I would honestly probably remove most of the
comments in this function.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:100: return (iter == tiles_.end()) ? nullptr
: iter->second.get();
unnecessary parens?

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:103: void UpdateTileAndTwinPriority(Tile*
tile) const;
Can you make these not all public if they don't need to be?

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/tiling_se...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:182:
tiling_->pending_visible_rect(),
This is still a little brain-breaking to me.  PendingVisibleTilingIterator is
about iterating through active tree tilings with the pending visible rect. 
However, it's still called on the pending tree, which happens to have two
identical rects here, so it eventually is a noop?  O_O

Can this be more obvious? If you have an iterator that only goes on one tree's
tilings, maybe just ask if the tiling is on the pending or active tree and skip
it if it's on the wrong tree?

What about calling this RequiredForActivationActiveTilingIterator? That way
there's no Pending in the name, so it's clear that it's on the active tree? 
That name is hella unwieldy though, but it's less confusing to me.

[vmpstr, 2015-04-10 20:25:14]

https://codereview.chromium.org/1051993002/diff/100001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:685: if (*iter) {
On 2015/04/08 00:13:06, danakj wrote:
> can you add a comment explaining the expectations

Done.

https://codereview.chromium.org/1051993002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:685: if (*iter) {
On 2015/04/08 00:13:06, danakj wrote:
> can you add a comment explaining the expectations

Done.

https://codereview.chromium.org/1051993002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:1538: SetupTrees(pending_pile,
active_pile);
On 2015/04/08 00:13:06, danakj wrote:
> Would it keep these tests simpler to test on the pending tree? Maybe just
> SetupPendingTree() instead of both trees?

Done.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (left):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:111: // Check our twin for a valid tile.
On 2015/04/09 17:54:23, enne wrote:
> I'm not 100% opposed to different classes for pending and active in the
future,
> but having the side by side diff makes this a lot easier to see this amazing
> code deletion.

Acknowledged.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:134: // TODO(vmpstr): Figure out how to
pass "remove a tile" information from
On 2015/04/09 17:54:24, enne wrote:
> Is this TODO an optimization, in that you think checking all these CoversRect
> calls are inefficient? I guess in the past we issues invalidations for all of
> the missing recordings and that would drop the tiles and force them to not be
> recreated.

This is both an optimization and a simplification imo. The reason is that when
we attempt to create a tile on the pending tree, if we fail to do so, we can
just either store something in tile_map or have a separate container that says
something along the lines of "this tile would exist if it was possible to create
it". When activating (in this function), we could iterate that container and
remove those tiles since with the new raster source we know they can't exist. 

The loop here does exactly the same thing, except it checks for every tile if it
is possible to create it with the new raster source. 

Another possible todo here is to not store raster_source on a tile, but on a
tiling and return it out of an iterator. If that happens, then this whole loop
(both if and else parts) can be removed.
> 
> Now that https://codereview.chromium.org/1061203002 has landed, the recording
> rect should be always correct, because there can no longer be holes in it. 
You
> should just be able to throw away anything outside of it.  Not sure if that's
> more efficient or not.
> 
> Maybe SetRasterSourceAndResize could do this with the recorded region as well
as
> the layer bounds? *thinking out loud*

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:138: if
(raster_source_->CoversRect(tile->content_rect(),
On 2015/04/08 00:13:06, danakj wrote:
> Doing scaling math for every tile here is a bit sad, but I guess the above
TODO
> covers it.

Yeep.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:151: for (const auto& tile_pair :
pending_twin->tiles_)
On 2015/04/08 00:13:06, danakj wrote:
> can you // this is copying the tiles_ from the pending twin or something, i
was
> like wat. or is there a way to make this more clear in the code?
> 
> how about insert(pending_twin->tiles_.begin(), pending_twin->tiles_.end())?
But
> then that does't replace does it. This operator[] per tile seems both hard to
> read and performancely problematic, but maybe there is not a better way. What
> about swap() and insert?

I'm not sure what is bad here? Any method (short of swap) would have to do an
equivalent of operator[] at least once, which this does. Swap/insert doesn't
really work for the reasons you mentioned: we have to both create new tiles and
override old tiles. It's also a hash_map, so I don't think it's that bad.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:295: // Active tree should always create a
tile.
On 2015/04/09 17:54:23, enne wrote:
> O_O
> 
> Can the active tree DCHECK that its raster source covers this rect?

So this is the reason I split the classes into two initially :) This function
wouldn't exist on the active tree at all, we'd just call CreateTile instead of
if (ShouldCreateTile()) CreateTile(). 

The raster source doesn't have to cover the tile, we're just saying that we
_should_ create a tile here. Whether CreateTile itself succeed or not is
different (that's the one that checks whether we can actually raster the tile).
I could move that check from CreateTile to ShouldCreateTile, but.. I'm a bit
more hesitant on that, since I have to verify that all calls to CreateTile are
protected.

Alternatively, I could move this ShouldCreate into Create and always call
Create, which would early out if !ShouldCreate...

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:749: // Otherwise, the tile is required if
it's in the pending tree's visible
On 2015/04/09 17:54:23, enne wrote:
> I think this comment (and a lot of other ones in this function and some other
> longer functions with lots of conditionals like ShouldCreateTileAt) could
maybe
> benefit from fewer "describing what the code is doing" comments and more "why
is
> this doing this" comments.  I would honestly probably remove most of the
> comments in this function.

Done.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:100: return (iter == tiles_.end()) ? nullptr
: iter->second.get();
On 2015/04/09 17:54:24, enne wrote:
> unnecessary parens?

Done.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:100: return (iter == tiles_.end()) ? nullptr
: iter->second.get();
On 2015/04/09 17:54:24, enne wrote:
> unnecessary parens?

Done.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:103: void UpdateTileAndTwinPriority(Tile*
tile) const;
On 2015/04/09 17:54:24, enne wrote:
> Can you make these not all public if they don't need to be?

Done.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/tiling_se...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:182:
tiling_->pending_visible_rect(),
On 2015/04/09 17:54:24, enne wrote:
> This is still a little brain-breaking to me.  PendingVisibleTilingIterator is
> about iterating through active tree tilings with the pending visible rect. 
> However, it's still called on the pending tree, which happens to have two
> identical rects here, so it eventually is a noop?  O_O

> 
> Can this be more obvious? If you have an iterator that only goes on one tree's
> tilings, maybe just ask if the tiling is on the pending or active tree and
skip
> it if it's on the wrong tree?
> 
> What about calling this RequiredForActivationActiveTilingIterator? That way
> there's no Pending in the name, so it's clear that it's on the active tree? 
> That name is hella unwieldy though, but it's less confusing to me.


Yep. The way I am meaning to write this is that a "PendingVisibleRect" is
another rect in our set of priority rects (along side of skewport, soon border
rect, visible rect). Perhaps the name is just not ideal. But given that it's
just a new rect called Foo, then both pending and active trees have a rect
called Foo. It just happens that in the pending tree case Foo == visible rect.
So this would be FooTilingIterator and would be a noop on the pending tree, and
do something on the active tree. 

I'm reluctant to start adding if (we're on one tree or the other) type of logic
here, since it can quickly become more confusing.

So I guess my question is: is there a name for Foo that would make this easier
to understand, or is your concern with the fact that this new region is only
"unique" on one of the trees?

[enne (OOO), 2015-04-10 21:16:28]

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:295: // Active tree should always create a
tile.
On 2015/04/10 20:25:13, vmpstr wrote:
> On 2015/04/09 17:54:23, enne wrote:
> > O_O
> > 
> > Can the active tree DCHECK that its raster source covers this rect?
> 
> So this is the reason I split the classes into two initially :) This function
> wouldn't exist on the active tree at all, we'd just call CreateTile instead of
> if (ShouldCreateTile()) CreateTile(). 
> 
> The raster source doesn't have to cover the tile, we're just saying that we
> _should_ create a tile here. Whether CreateTile itself succeed or not is
> different (that's the one that checks whether we can actually raster the
tile).
> I could move that check from CreateTile to ShouldCreateTile, but.. I'm a bit
> more hesitant on that, since I have to verify that all calls to CreateTile are
> protected.
> 
> Alternatively, I could move this ShouldCreate into Create and always call
> Create, which would early out if !ShouldCreate...

Maybe the answer here is just better comments about why the active tree should
always create a tile but the pending tree needs more checks?

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/tiling_se...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:182:
tiling_->pending_visible_rect(),
On 2015/04/10 20:25:14, vmpstr wrote:
> On 2015/04/09 17:54:24, enne wrote:
> > This is still a little brain-breaking to me.  PendingVisibleTilingIterator
is
> > about iterating through active tree tilings with the pending visible rect. 
> > However, it's still called on the pending tree, which happens to have two
> > identical rects here, so it eventually is a noop?  O_O
> 
> > 
> > Can this be more obvious? If you have an iterator that only goes on one
tree's
> > tilings, maybe just ask if the tiling is on the pending or active tree and
> skip
> > it if it's on the wrong tree?
> > 
> > What about calling this RequiredForActivationActiveTilingIterator? That way
> > there's no Pending in the name, so it's clear that it's on the active tree? 
> > That name is hella unwieldy though, but it's less confusing to me.
> 
> 
> Yep. The way I am meaning to write this is that a "PendingVisibleRect" is
> another rect in our set of priority rects (along side of skewport, soon border
> rect, visible rect). Perhaps the name is just not ideal. But given that it's
> just a new rect called Foo, then both pending and active trees have a rect
> called Foo. It just happens that in the pending tree case Foo == visible rect.
> So this would be FooTilingIterator and would be a noop on the pending tree,
and
> do something on the active tree. 
> 
> I'm reluctant to start adding if (we're on one tree or the other) type of
logic
> here, since it can quickly become more confusing.
> 
> So I guess my question is: is there a name for Foo that would make this easier
> to understand, or is your concern with the fact that this new region is only
> "unique" on one of the trees?

My concern is kind of both? Maybe a comment here or on the enum about when this
iterator does something real and when it subtly does nothing would help.

[vmpstr, 2015-04-10 22:24:29]

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:295: // Active tree should always create a
tile.
On 2015/04/10 21:16:28, enne wrote:
> On 2015/04/10 20:25:13, vmpstr wrote:
> > On 2015/04/09 17:54:23, enne wrote:
> > > O_O
> > > 
> > > Can the active tree DCHECK that its raster source covers this rect?
> > 
> > So this is the reason I split the classes into two initially :) This
function
> > wouldn't exist on the active tree at all, we'd just call CreateTile instead
of
> > if (ShouldCreateTile()) CreateTile(). 
> > 
> > The raster source doesn't have to cover the tile, we're just saying that we
> > _should_ create a tile here. Whether CreateTile itself succeed or not is
> > different (that's the one that checks whether we can actually raster the
> tile).
> > I could move that check from CreateTile to ShouldCreateTile, but.. I'm a bit
> > more hesitant on that, since I have to verify that all calls to CreateTile
are
> > protected.
> > 
> > Alternatively, I could move this ShouldCreate into Create and always call
> > Create, which would early out if !ShouldCreate...
> 
> Maybe the answer here is just better comments about why the active tree should
> always create a tile but the pending tree needs more checks?

Added a comment.

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/tiling_se...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/100001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:182:
tiling_->pending_visible_rect(),
On 2015/04/10 21:16:28, enne wrote:
> On 2015/04/10 20:25:14, vmpstr wrote:
> > On 2015/04/09 17:54:24, enne wrote:
> > > This is still a little brain-breaking to me.  PendingVisibleTilingIterator
> is
> > > about iterating through active tree tilings with the pending visible rect.

> > > However, it's still called on the pending tree, which happens to have two
> > > identical rects here, so it eventually is a noop?  O_O
> > 
> > > 
> > > Can this be more obvious? If you have an iterator that only goes on one
> tree's
> > > tilings, maybe just ask if the tiling is on the pending or active tree and
> > skip
> > > it if it's on the wrong tree?
> > > 
> > > What about calling this RequiredForActivationActiveTilingIterator? That
way
> > > there's no Pending in the name, so it's clear that it's on the active
tree? 
> > > That name is hella unwieldy though, but it's less confusing to me.
> > 
> > 
> > Yep. The way I am meaning to write this is that a "PendingVisibleRect" is
> > another rect in our set of priority rects (along side of skewport, soon
border
> > rect, visible rect). Perhaps the name is just not ideal. But given that it's
> > just a new rect called Foo, then both pending and active trees have a rect
> > called Foo. It just happens that in the pending tree case Foo == visible
rect.
> > So this would be FooTilingIterator and would be a noop on the pending tree,
> and
> > do something on the active tree. 
> > 
> > I'm reluctant to start adding if (we're on one tree or the other) type of
> logic
> > here, since it can quickly become more confusing.
> > 
> > So I guess my question is: is there a name for Foo that would make this
easier
> > to understand, or is your concern with the fact that this new region is only
> > "unique" on one of the trees?
> 
> My concern is kind of both? Maybe a comment here or on the enum about when
this
> iterator does something real and when it subtly does nothing would help.

I've added a comment (next to the enum in the header). Let me know if you know
of a good name for this new region and I'll change it throughout. 

In another attempt for defense here, we have a similar issue with other regions
like the skewport. It overlaps the visible rect, and sometimes it's exactly the
same as the visible rect. However, we don't have explicit logic here to handle
the case when they are the same. It just helps not to think about it, but verify
that it does the right thing where the regions are the same. (as an aside, the
pending visible rect will also be the same as the current visible rect in some
cases on the active tree).

It is a different case, because on the pending tree, in this implementation, the
pending visible rect will always be the same as the visible rect. Adding an if
statement though  kind of accomplishes a task of a comment by explicitly saying
"we know that on the pending tree they are the same, skip the case" instead of a
comment that says "this is a noop on the pending tree". I don't know if putting
an if-statement in this case is something that I like.

[enne (OOO), 2015-04-10 22:30:22]

Ok, thanks! What are the pieces left here? Testing?

[vmpstr, 2015-04-10 22:36:43]

On 2015/04/10 22:30:22, enne wrote:
> Ok, thanks! What are the pieces left here? Testing?

Still gotta do eviction iterator changes. They would be similar to the raster
iterator changes, that's why I wanted to make sure that it's looking more or
less OK first.

[timford35, 2015-04-10 22:43:21]

timford35@gmail.com changed reviewers:
+ timford35@gmail.com

[vmpstr, 2015-04-13 19:24:51]

PTAL. The latest patchset includes eviction changes and final unittest fixes
(ie, this is "land-able")

[enne (OOO), 2015-04-13 22:36:03]

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:2090: TEST_F(PictureLayerImplTest,
AllHighResRequiredEvenIfShared) {
AllHighResRequiredEvenIfAirquotesSharedAirquotes

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:2136: TEST_F(PictureLayerImplTest,
EverythingRequiredIfActiveMissingTiles) {
O_o?

This was an optimization to make it possible to instantly activate if the active
tree was entirely checkerboarded.

This is testing that we do extra work in this case.

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:2285: // Tile 0,0 should be shared, but
tile 1,1 should not be.
Can you change this comment (and other comments [and test titles]) to not
reference sharing?

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:302: // tree is represents content that we
draw on screen, which means that
"is represents"

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:288: const PictureLayerTiling* twin =
This needs a serious comment.  It's still hard to keep in my brain that
pending_visible_rect can be the active visible rect if there's not a pending
twin.  And then this just *happens* to work out that in both the case where
there's no pending twin and where it's the pending layer that iterating using a
difference rect of visible to pending visible is a no-op.

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/tile_mana...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/tile_mana...
cc/resources/tile_manager_unittest.cc:717: EXPECT_EQ(12, distance_decreasing);
What's up with this change?

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/tiling_se...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/tiling_se...
cc/resources/tiling_set_eviction_queue.cc:263: if
(!(*tilings_)[tiling_index_]->has_eventually_rect_tiles()) {
How about a temporary variable for (*tilings)[tiling_index_]?

https://codereview.chromium.org/1051993002/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_picture.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_picture.cc:308: // If active tree doesn't have
the layer, then pending tree should have
This is the first frame?

[vmpstr, 2015-04-13 23:52:13]

PTAL

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:2090: TEST_F(PictureLayerImplTest,
AllHighResRequiredEvenIfShared) {
On 2015/04/13 22:36:02, enne wrote:
> AllHighResRequiredEvenIfAirquotesSharedAirquotes

Done.

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:2136: TEST_F(PictureLayerImplTest,
EverythingRequiredIfActiveMissingTiles) {
On 2015/04/13 22:36:02, enne wrote:
> O_o?
> 
> This was an optimization to make it possible to instantly activate if the
active
> tree was entirely checkerboarded.
> 
> This is testing that we do extra work in this case.

I've changed this back. I'm not sure it's worth it though, since it triggers in
the case that:

- We don't "require high res to draw"
- The raster sources of this and twin are the same size
- The visible rect of this and twin are the same

So in other words, in situations where have the same viewport and active one has
a hole in the viewport's recording, then the tile would not be required for
activation. Do you know if this is a frequent situation? It costs us a couple of
extra function calls and a rect comparison per rfa tile in other situations.

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:2285: // Tile 0,0 should be shared, but
tile 1,1 should not be.
On 2015/04/13 22:36:02, enne wrote:
> Can you change this comment (and other comments [and test titles]) to not
> reference sharing?

Done.

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:302: // tree is represents content that we
draw on screen, which means that
On 2015/04/13 22:36:02, enne wrote:
> "is represents"

Done.

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:288: const PictureLayerTiling* twin =
On 2015/04/13 22:36:02, enne wrote:
> This needs a serious comment.  It's still hard to keep in my brain that
> pending_visible_rect can be the active visible rect if there's not a pending
> twin.  And then this just *happens* to work out that in both the case where
> there's no pending twin and where it's the pending layer that iterating using
a
> difference rect of visible to pending visible is a no-op.

Well... to be fair, it doesn't just happen, it was written that way. I've added
a comment. I'm also open to other suggestions of how to implement this. I don't
think adding comments like this makes anything any more clear.

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/tile_mana...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/tile_mana...
cc/resources/tile_manager_unittest.cc:717: EXPECT_EQ(12, distance_decreasing);
On 2015/04/13 22:36:02, enne wrote:
> What's up with this change?

Oops. This was a wrong fix for the test. Changed.

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/tiling_se...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/resources/tiling_se...
cc/resources/tiling_set_eviction_queue.cc:263: if
(!(*tilings_)[tiling_index_]->has_eventually_rect_tiles()) {
On 2015/04/13 22:36:03, enne wrote:
> How about a temporary variable for (*tilings)[tiling_index_]?

Are you OK with this being a follow-up (since I have to remove
"skip_shared_out_of_order_tiles" stuff anyway), or would you prefer I do this in
this patch?

https://codereview.chromium.org/1051993002/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_picture.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_picture.cc:308: // If active tree doesn't have
the layer, then pending tree should have
On 2015/04/13 22:36:03, enne wrote:
> This is the first frame?

Yes. Did you want me to change the comment to that or are you just clarifying?

[vmpstr, 2015-04-13 23:53:48]

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/200001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:2136: TEST_F(PictureLayerImplTest,
EverythingRequiredIfActiveMissingTiles) {
On 2015/04/13 23:52:12, vmpstr wrote:
> On 2015/04/13 22:36:02, enne wrote:
> > O_o?
> > 
> > This was an optimization to make it possible to instantly activate if the
> active
> > tree was entirely checkerboarded.
> > 
> > This is testing that we do extra work in this case.
> 
> I've changed this back. I'm not sure it's worth it though, since it triggers
in
> the case that:
> 
> - We don't "require high res to draw"
> - The raster sources of this and twin are the same size
> - The visible rect of this and twin are the same
> 
> So in other words, in situations where have the same viewport and active one
has
> a hole in the viewport's recording, then the tile would not be required for
> activation. Do you know if this is a frequent situation? It costs us a couple
of
> extra function calls and a rect comparison per rfa tile in other situations.

I guess in a fast scroll it can happen after a fling, nevermind.

[enne (OOO), 2015-04-14 23:27:17]

https://codereview.chromium.org/1051993002/diff/220001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1051993002/diff/220001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:287: // This returns "next" visible rect.
That is, when the pending tree (if one
Ok, fine.  I think this comment makes things more confusing.  Maybe it's not
worth having.

This function would be more clear in my head at least if it said:

if (!is active tree)
  return current_visible_rect;

Tree* twin = Getcetera()
if (!twin)
  return empty  // sanity
return twin->current_visible_rect

Would this still work in the DifferenceIterator without other changes?  "I'm the
active tree without a twin, so the pending visible rect is my own visible rect"
just continues to make no sense to me.

[vmpstr, 2015-04-15 19:16:36]

https://codereview.chromium.org/1051993002/diff/220001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/1051993002/diff/220001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.h:287: // This returns "next" visible rect.
That is, when the pending tree (if one
On 2015/04/14 23:27:16, enne wrote:
> Ok, fine.  I think this comment makes things more confusing.  Maybe it's not
> worth having.
> 
> This function would be more clear in my head at least if it said:
> 
> if (!is active tree)
>   return current_visible_rect;
> 
> Tree* twin = Getcetera()
> if (!twin)
>   return empty  // sanity
> return twin->current_visible_rect
> 
> Would this still work in the DifferenceIterator without other changes?  "I'm
the
> active tree without a twin, so the pending visible rect is my own visible
rect"
> just continues to make no sense to me.

Sure. Done.

[enne (OOO), 2015-04-15 20:13:51]

Thanks!  Sorry for all the nitpicking.  Your code is clever, but I just want to
be more obvious for future me and others.

lgtm

[vmpstr, 2015-04-17 16:53:11]

Thanks for the review. I reran this on perf locally and it's kind of all over
the place. A bunch of regressions in tough compositor cases, some improvements,
some no changes... I'm kind of at a loss. Would you be OK with submitting this
just for the sake of getting perf numbers from the bots, because I'm not very
confident in my local results anymore. We can revert shortly after the numbers
appear.

[vmpstr, 2015-04-17 16:53:49]

On 2015/04/17 16:53:11, vmpstr wrote:
> Thanks for the review. I reran this on perf locally and it's kind of all over
> the place. A bunch of regressions in tough compositor cases, some
improvements,
> some no changes... I'm kind of at a loss. Would you be OK with submitting this
> just for the sake of getting perf numbers from the bots, because I'm not very
> confident in my local results anymore. We can revert shortly after the numbers
> appear.

Also, I've added a new more cleanups that speed up the flow, but I can always
add more, I'd just like to see how this looks right now.

[vmpstr, 2015-04-20 20:06:29]

I had to change this quite a bit, PTAL

[enne (OOO), 2015-04-20 22:36:28]

https://codereview.chromium.org/1051993002/diff/300001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1051993002/diff/300001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:634: scoped_refptr<RasterSource>* raster_source)
{
Pointer to a refptr <_<

Is this something that needs to get landed in this patch? I think there are
other ways to remove the update raster source on activation loop.

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:104: failed_tile_indices_.push_back(key);
failed could be a better word.  What about remove_on_activation_indices?

It's a little out of the way to record tiles inside of CreateTile, as it depends
on calling code to call CreateTile for every tile that could exist on the active
tree, so that it'll get removed later.  It seems like it'd be better for calling
code to do this?  Although, maybe that's a double covers check unless you just
assume that CreateTile will always succeed.

Also, I think this could be smarter, as the recording rect doesn't have holes in
it anymore (although I guess we do hold onto uninvalidated recording grid cells
forever outside of it, but that shouldn't be applicable here).  It seems like we
could just find the rects that are no longer covered and drop tiles that
intersect them.

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
cc/resources/tiling_set_eviction_queue.cc:42: auto* tiling =
tiling_set->tiling_at(i);
style nit: don't use auto here.

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:36: PictureLayerTiling*
active_non_ideal_pending_high_res_tiling = nullptr;
I know why you need this, but wowwww this variable.  Can you maybe leave a
comment here so that it's more clear?

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:53: stages_.reserve(6);
Premature optimization enne says maybe base::StackVector would save you this
allocation over std::vector.

[vmpstr, 2015-04-22 18:38:58]

PTAL

https://codereview.chromium.org/1051993002/diff/300001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/1051993002/diff/300001/cc/layers/picture_laye...
cc/layers/picture_layer_impl.cc:634: scoped_refptr<RasterSource>* raster_source)
{
On 2015/04/20 22:36:28, enne wrote:
> Pointer to a refptr <_<
> 
> Is this something that needs to get landed in this patch? I think there are
> other ways to remove the update raster source on activation loop.

Done.

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:104: failed_tile_indices_.push_back(key);
On 2015/04/20 22:36:28, enne wrote:
> failed could be a better word.  What about remove_on_activation_indices?

Done.

> 
> It's a little out of the way to record tiles inside of CreateTile, as it
depends
> on calling code to call CreateTile for every tile that could exist on the
active
> tree, so that it'll get removed later.  It seems like it'd be better for
calling
> code to do this?  Although, maybe that's a double covers check unless you just
> assume that CreateTile will always succeed.
> 
> Also, I think this could be smarter, as the recording rect doesn't have holes
in
> it anymore (although I guess we do hold onto uninvalidated recording grid
cells
> forever outside of it, but that shouldn't be applicable here).  It seems like
we
> could just find the rects that are no longer covered and drop tiles that
> intersect them.

I'm not sure what you're proposing here. CreateTile is protected, so the caller
is only this class and friends (and tests, I guess). Or are you proposing
changing the raster source interface to expose the region/rect it has recorded?

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
cc/resources/tiling_set_eviction_queue.cc:42: auto* tiling =
tiling_set->tiling_at(i);
On 2015/04/20 22:36:28, enne wrote:
> style nit: don't use auto here.

Done.

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:36: PictureLayerTiling*
active_non_ideal_pending_high_res_tiling = nullptr;
On 2015/04/20 22:36:28, enne wrote:
> I know why you need this, but wowwww this variable.  Can you maybe leave a
> comment here so that it's more clear?

Done.

https://codereview.chromium.org/1051993002/diff/300001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:53: stages_.reserve(6);
On 2015/04/20 22:36:28, enne wrote:
> Premature optimization enne says maybe base::StackVector would save you this
> allocation over std::vector.

Done.

[enne (OOO), 2015-04-22 20:56:40]

On 2015/04/22 at 18:38:58, vmpstr wrote:

> > It's a little out of the way to record tiles inside of CreateTile, as it
depends
> > on calling code to call CreateTile for every tile that could exist on the
active
> > tree, so that it'll get removed later.  It seems like it'd be better for
calling
> > code to do this?  Although, maybe that's a double covers check unless you
just
> > assume that CreateTile will always succeed.
> > 
> > Also, I think this could be smarter, as the recording rect doesn't have
holes in
> > it anymore (although I guess we do hold onto uninvalidated recording grid
cells
> > forever outside of it, but that shouldn't be applicable here).  It seems
like we
> > could just find the rects that are no longer covered and drop tiles that
> > intersect them.
> 
> I'm not sure what you're proposing here. CreateTile is protected, so the
caller is only this class and friends (and tests, I guess). Or are you proposing
changing the raster source interface to expose the region/rect it has recorded?

No, no.  It is protected, but it just feels like a side effect that a bit too
subtle for my tastes.  I think I'd prefer if you tracked the invalid indices in
the calling code for CreateTile, just during the
CreateMissingTilesInLiveTilesRect call.

[vmpstr, 2015-04-22 21:46:21]

On 2015/04/22 20:56:40, enne wrote:
> On 2015/04/22 at 18:38:58, vmpstr wrote:
> 
> > > It's a little out of the way to record tiles inside of CreateTile, as it
> depends
> > > on calling code to call CreateTile for every tile that could exist on the
> active
> > > tree, so that it'll get removed later.  It seems like it'd be better for
> calling
> > > code to do this?  Although, maybe that's a double covers check unless you
> just
> > > assume that CreateTile will always succeed.
> > > 
> > > Also, I think this could be smarter, as the recording rect doesn't have
> holes in
> > > it anymore (although I guess we do hold onto uninvalidated recording grid
> cells
> > > forever outside of it, but that shouldn't be applicable here).  It seems
> like we
> > > could just find the rects that are no longer covered and drop tiles that
> > > intersect them.
> > 
> > I'm not sure what you're proposing here. CreateTile is protected, so the
> caller is only this class and friends (and tests, I guess). Or are you
proposing
> changing the raster source interface to expose the region/rect it has
recorded?
> 
> No, no.  It is protected, but it just feels like a side effect that a bit too
> subtle for my tastes.  I think I'd prefer if you tracked the invalid indices
in
> the calling code for CreateTile, just during the
> CreateMissingTilesInLiveTilesRect call.

CreateTile is used in 4 functions now (CreateMissingTilesInLiveTilesRect,
SetRasterSourceAndResize, Invalidate*, SetLiveTilesRect; * - this should only do
anything on the active tree, so it doesn't really count). I can change it so
that every time we call CreateTile and it returns null we record the index.
However, I would argue that that's more subtle, since any further changes that
introduce CreateTile calls (such as moving tile creation to the iterators, if
that ever becomes viable) would have to remember to also record a failed index.
That seems a bigger burden to remember than to call CreateTile instead of
manually creating a tile. Besides, even if CreateTile was public, then the
caller would also have to remember to save a failed index if it couldn't create
a tile... 

To put it differently, if we stored nullptr in tiles_, then this would also
solve the problem of identifying which tiles were attempted and failed. This
would also take place in CreateTiles though.

[enne (OOO), 2015-04-22 22:18:52]

On 2015/04/22 at 21:46:21, vmpstr wrote:

> CreateTile is used in 4 functions now (CreateMissingTilesInLiveTilesRect,
SetRasterSourceAndResize, Invalidate*, SetLiveTilesRect; * - this should only do
anything on the active tree, so it doesn't really count). I can change it so
that every time we call CreateTile and it returns null we record the index.
However, I would argue that that's more subtle, since any further changes that
introduce CreateTile calls (such as moving tile creation to the iterators, if
that ever becomes viable) would have to remember to also record a failed index.
That seems a bigger burden to remember than to call CreateTile instead of
manually creating a tile. Besides, even if CreateTile was public, then the
caller would also have to remember to save a failed index if it couldn't create
a tile... 

Which of those four functions create tiles that are not covered by the raster
source? It seems like Invalidate is the only one, and only because of the
synthetic invalidations from the PicturePile?

> To put it differently, if we stored nullptr in tiles_, then this would also
solve the problem of identifying which tiles were attempted and failed. This
would also take place in CreateTiles though.

I'm not sure I want to handle nullptr in tiles.  We've avoided this so far, and
I think there's some sanity to not null-checking tiles.  Storing null entries in
some other map is just failed indices with a different face.

[vmpstr, 2015-04-22 23:00:34]

On 2015/04/22 22:18:52, enne wrote:
> On 2015/04/22 at 21:46:21, vmpstr wrote:
> 
> > CreateTile is used in 4 functions now (CreateMissingTilesInLiveTilesRect,
> SetRasterSourceAndResize, Invalidate*, SetLiveTilesRect; * - this should only
do
> anything on the active tree, so it doesn't really count). I can change it so
> that every time we call CreateTile and it returns null we record the index.
> However, I would argue that that's more subtle, since any further changes that
> introduce CreateTile calls (such as moving tile creation to the iterators, if
> that ever becomes viable) would have to remember to also record a failed
index.
> That seems a bigger burden to remember than to call CreateTile instead of
> manually creating a tile. Besides, even if CreateTile was public, then the
> caller would also have to remember to save a failed index if it couldn't
create
> a tile... 
> 
> Which of those four functions create tiles that are not covered by the raster
> source? It seems like Invalidate is the only one, and only because of the
> synthetic invalidations from the PicturePile?

All of them? I'm not sure relying on what raster source guarantees in terms of
coverage is a good approach to solving this. That is, looking at code in, say,
SetRasterSourceAndResize, I'm not sure I can convince myself that a tile will
always be created there without digging through picture pile impl and display
list impl to see what CoversRect does.

> 
> > To put it differently, if we stored nullptr in tiles_, then this would also
> solve the problem of identifying which tiles were attempted and failed. This
> would also take place in CreateTiles though.
> 
> I'm not sure I want to handle nullptr in tiles.  We've avoided this so far,
and
> I think there's some sanity to not null-checking tiles.  Storing null entries
in
> some other map is just failed indices with a different face.

I completely agree that it's the same thing. I'm not suggesting storing nullptrs
in tiles_; my only point was that if we did store nullptrs, it would be done in
CreateTile not outside. 

To me, this seems like a minor part of this patch, so I'm going to make the
changes you're suggesting.

[vmpstr, 2015-04-22 23:38:21]

PTAL

[enne (OOO), 2015-04-23 00:40:44]

On 2015/04/22 at 23:00:34, vmpstr wrote:
> 
> All of them? I'm not sure relying on what raster source guarantees in terms of
coverage is a good approach to solving this. That is, looking at code in, say,
SetRasterSourceAndResize, I'm not sure I can convince myself that a tile will
always be created there without digging through picture pile impl and display
list impl to see what CoversRect does.

I want to be more explicit about removing tiles that are no longer part of the
raster source.  I think there's too much implicit behavior here.  I'm balking at
one bit of implicit behavior (that you call CreateTile for everything that could
be not covered anymore) and you're balking at other bits (that we invalidate
everything that's not covered anymore, although I would argue that's a guarantee
of PicturePile).

[vmpstr, 2015-04-23 00:57:12]

On 2015/04/23 00:40:44, enne wrote:
> On 2015/04/22 at 23:00:34, vmpstr wrote:
> > 
> > All of them? I'm not sure relying on what raster source guarantees in terms
of
> coverage is a good approach to solving this. That is, looking at code in, say,
> SetRasterSourceAndResize, I'm not sure I can convince myself that a tile will
> always be created there without digging through picture pile impl and display
> list impl to see what CoversRect does.
> 
> I want to be more explicit about removing tiles that are no longer part of the
> raster source.  I think there's too much implicit behavior here.  I'm balking
at
> one bit of implicit behavior (that you call CreateTile for everything that
could
> be not covered anymore) and you're balking at other bits (that we invalidate
> everything that's not covered anymore, although I would argue that's a
guarantee
> of PicturePile).

Ok, in that case we need to address that before this patch. The current behavior
is that we always try to create a tile on a pending tree, and sometimes we fail
and have nothing in the tile map key for that spot. When we activate, we always
destroy and recreate all unshared tiles (regardless of invalidation), so when we
go to recreate a tile on the active tree, we check the twin (which doesn't have
it), and then do another CoversRect check with the new raster source and figure
out that we also can't create it on the active. That's more implicit than the
behavior of this patch, which has a (partial) vector of these indices. The
problem this patch has where it can't reproduce previous behavior is that when a
tile is missing, it implicitly used to imply that it can't be created. With this
patch, where tiles are missing because they haven't changed, we need something
else to act as a signal that a tile can't be created.

[enne (OOO), 2015-04-23 01:07:55]

On 2015/04/23 at 00:57:12, vmpstr wrote:

> Ok, in that case we need to address that before this patch. The current
behavior is that we always try to create a tile on a pending tree, and sometimes
we fail and have nothing in the tile map key for that spot. When we activate, we
always destroy and recreate all unshared tiles (regardless of invalidation), so
when we go to recreate a tile on the active tree, we check the twin (which
doesn't have it), and then do another CoversRect check with the new raster
source and figure out that we also can't create it on the active. That's more
implicit than the behavior of this patch, which has a (partial) vector of these
indices. The problem this patch has where it can't reproduce previous behavior
is that when a tile is missing, it implicitly used to imply that it can't be
created. With this patch, where tiles are missing because they haven't changed,
we need something else to act as a signal that a tile can't be created.

I'd be ok with landing this as-is and cleaning it up as another step if you
wanted to do it in that order, as well.

[vmpstr, 2015-04-23 20:06:01]

As per offline discussion, I've changed this to remove invalidated tiles before
copying the pending tree. Another fun subtlety I discovered is that accessing
PendingInvalidation during activation doesn't produce right results, since the
invalidation was already moved to the active tree, so I just pass the
invalidation down to TakeTilesAndPropertiesFrom

[enne (OOO), 2015-04-23 20:18:28]

lgtm, thanks!

[enne (OOO), 2015-04-23 20:18:36]

https://codereview.chromium.org/1051993002/diff/360001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/360001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:3515: // Since there is no
invalidaiton, pending tree should have no tiles.
typo

[vmpstr, 2015-04-23 20:37:02]

https://codereview.chromium.org/1051993002/diff/360001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/360001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:3515: // Since there is no
invalidaiton, pending tree should have no tiles.
On 2015/04/23 20:18:36, enne wrote:
> typo

Done.

[vmpstr, 2015-04-24 15:32:40]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2015-04-24 15:32:41]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1051993002/#ps380001
(title: "typo")

[commit-bot: I haz the power, 2015-04-24 15:33:37]

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

[commit-bot: I haz the power, 2015-04-24 15:38:26]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-04-24 15:39:24]

Patchset 20 (id:??) landed as
https://crrev.com/0061b75e810f088900a0a8fbdd29ba5595c73016
Cr-Commit-Position: refs/heads/master@{#326804}

[vmpstr, 2015-04-27 17:03:35]

A revert of this CL (patchset #20 id:380001) has been created in
https://codereview.chromium.org/1108773003/ by vmpstr@chromium.org.

The reason for reverting is: Several failures happened as a result..

[vmpstr, 2015-04-28 19:06:04]

Please take a look.

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:140: bool create_missing_tiles = false;
This was added to fix crbug.com/481482

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:248: void
PictureLayerTiling::Invalidate(const Region& layer_invalidation) {
This function changed to fix crbug.com/481421

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/tiling_se...
File cc/resources/tiling_set_raster_queue_all.cc (right):

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/tiling_se...
cc/resources/tiling_set_raster_queue_all.cc:62: bool use_high_res_tiling =
high_res_tiling && high_res_tiling->has_tiles();
This is a fix for crbug.com/481509

[enne (OOO), 2015-04-28 21:53:40]

<_<

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:140: bool create_missing_tiles = false;
On 2015/04/28 19:06:04, vmpstr wrote:
> This was added to fix crbug.com/481482

How 'bout that unit test?

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:248: void
PictureLayerTiling::Invalidate(const Region& layer_invalidation) {
On 2015/04/28 19:06:03, vmpstr wrote:
> This function changed to fix crbug.com/481421

Do we not have an LCD unit test for this?

[vmpstr, 2015-04-29 00:38:44]

PTAL

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:140: bool create_missing_tiles = false;
On 2015/04/28 21:53:40, enne wrote:
> On 2015/04/28 19:06:04, vmpstr wrote:
> > This was added to fix crbug.com/481482
> 
> How 'bout that unit test?

Done.

https://codereview.chromium.org/1051993002/diff/400001/cc/resources/picture_l...
cc/resources/picture_layer_tiling.cc:248: void
PictureLayerTiling::Invalidate(const Region& layer_invalidation) {
On 2015/04/28 21:53:40, enne wrote:
> On 2015/04/28 19:06:03, vmpstr wrote:
> > This function changed to fix crbug.com/481421
> 
> Do we not have an LCD unit test for this?

Not that I saw (and none that failed on the first run). I've added one, but I
had to promote LayerTreeSettings to protected in lthi. Please look that over as
well.

[enne (OOO), 2015-04-29 02:18:49]

https://codereview.chromium.org/1051993002/diff/420001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/420001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:4886:
host_impl_.settings_.can_use_lcd_text = false;
Sorry, but I'm not super keen on this, as other folks might start changing
settings (but code doesn't have any way of being notified that they have
changed).  I should probably make LTHI::settings_ be const.

Can you change the raster source to be one that doesn't use lcd text and then
set that on the layer? That's the normal path for this.

[vmpstr, 2015-04-29 15:57:01]

https://codereview.chromium.org/1051993002/diff/420001/cc/layers/picture_laye...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/1051993002/diff/420001/cc/layers/picture_laye...
cc/layers/picture_layer_impl_unittest.cc:4886:
host_impl_.settings_.can_use_lcd_text = false;
On 2015/04/29 02:18:49, enne wrote:
> Sorry, but I'm not super keen on this, as other folks might start changing
> settings (but code doesn't have any way of being notified that they have
> changed).  I should probably make LTHI::settings_ be const.
> 
> Can you change the raster source to be one that doesn't use lcd text and then
> set that on the layer? That's the normal path for this.

That wouldn't exercise the code that this is testing, since this is testing
exactly that: code that changes the raster source to the one without LCD. 

I'll do this via LTH test then.

[vmpstr, 2015-04-29 17:53:30]

As per offline discussion, I changed layer draw_properties_ instead, but I still
had to promote that to protected. Please look that over as well.

[enne (OOO), 2015-04-29 18:01:24]

https://codereview.chromium.org/1051993002/diff/440001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/1051993002/diff/440001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:806: DrawProperties<LayerImpl> draw_properties_;
Could you just call draw_properties() that returns a non-const ref?

[vmpstr, 2015-04-29 18:23:31]

On 2015/04/29 18:01:24, enne wrote:
> https://codereview.chromium.org/1051993002/diff/440001/cc/layers/layer_impl.h
> File cc/layers/layer_impl.h (right):
> 
>
https://codereview.chromium.org/1051993002/diff/440001/cc/layers/layer_impl.h...
> cc/layers/layer_impl.h:806: DrawProperties<LayerImpl> draw_properties_;
> Could you just call draw_properties() that returns a non-const ref?

Oh I didn't realize it has a non const getter. Changed.

[enne (OOO), 2015-04-29 18:40:13]

lgtm

[vmpstr, 2015-04-29 20:42:35]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-04-29 20:44:11]

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

[commit-bot: I haz the power, 2015-04-29 21:25:40]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2015-04-29 21:26:37]

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