cc: Added tile bundles

cc: Added tile bundles

This patch adds TileBundles to cc. Currently, in impl
side painting we work with Tiles. A PictureLayerTiling
creates enough tiles to cover its rect (and registers
them with the tile manager), then it updates the
priorities of each of the tile, and finally informs
TileManager to do its work. TileManager sorts and assigns
memory to tiles, and begins rasterization.

With TileBundles, the tiling creates TileBundles (initially
2x2 tiles) to contain tiles. Update tile priorities now
only happens on a per bundle basis. In the initial patch,
TileManager still pulls out each individual tile for
processing.

**** Note to Perf Sheriffs ****
This patch affects performance of several of cc_perftests:
- PictureLayerTilingPerfTest.Invalidate
   This test is expected to regress slightly, but not
   too much.
- PictureLayerTilingPerfTest.UpdateTilePriorities
   This test's performance should improve greatly.
- TileManagerPerfTest.ManageTiles
   This test _should_ remain roughly the same, but there
   might be some up or down as the result of the patch.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239074

[vmpstr, 2013-11-20 00:00:21]

I think this still needs some work. In particular, I still have to fix
tile_manager, picture_layer_tiling, and prioritized_tile_set unittests, as well
as the perftests. Currently they don't compile, since tile no longer has a
priority for active/pending trees.

However, please take a look as this is the type of approach that we discussed
earlier. The idea is that BundleSetIterator for each iteration returns a
SharedTileBundle -- a container that has priorities for active and pending trees
as well as a set of tiles for which those priorities apply. I would recommend
looking in the tile_manager first for the usage.

I honestly don't know how it performs, since I have to fix perftests. From
observation, I had 180 tiles or so, which resulted in ~60 shared tile bundles
that reproduced all 180 tiles. However, the work to go from bundles to shared
bundles isn't trivial so we have to measure this carefully.

[enne (OOO), 2013-11-20 00:14:34]

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:483: tiles->InsertTile(tile, priority_bin);
Curious why tiles are in bins and not bundles, since there's still a sorting
step after this.

[vmpstr, 2013-11-20 00:54:08]

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:483: tiles->InsertTile(tile, priority_bin);
On 2013/11/20 00:14:34, enne wrote:
> Curious why tiles are in bins and not bundles, since there's still a sorting
> step after this.

There's a few cases (namely required for activation, is ready to draw, and is
active) that are still very much a per-tile states. I'm weary of just making a
bundle priority based on one particular tile's is-ready-to-draw state, for
instance. I'd like to fix the perftests to see if that has to be necessarily in
this patch. If so, I'll see what's the best way to fit a bundle into the set (or
I'll just think about it in general :P)

[reveman, 2013-11-20 01:34:37]

Looks like a good first step to me if it doesn't cause too much of a performance
regression.

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:968: if (tile->is_visible() == 0)
is_visible() returning 0 means tile is visible?

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
cc/resources/tile_manager.h:164: typedef base::hash_map<TileBundle::Id,
TileBundle*> TileBundleMap;
nit: make location of this typedef consistent with TileMap by moving this below
or moving TileMap up here.

[enne (OOO), 2013-11-20 21:20:29]

https://codereview.chromium.org/62283012/diff/120001/cc/resources/shared_tile...
File cc/resources/shared_tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/120001/cc/resources/shared_tile...
cc/resources/shared_tile_bundle.h:16: class SharedTileBundle {
I'm not quite sure I'm buying the SharedTileBundle idea.  Previously, a
PictureLayerTiling had a set of tiles, some of which were shared with its twin
layer.  Now, a PictureLayerTiling has a set of bundles (which have tiles).  The
tiles are shared with the twin but the bundles are not.

Now, I think I understand why you don't share the bundles, since invalidations
only affect tiles, and so you could share tiles but not share the entire bundle.
 But then if you don't share, you have to find and combine bundles later every
time you iterate through them, which seems like a lot of extra work and
complication.  Lots of asking for 

What about sharing bundles like you share tiles, but have active and pending
tile pointers and active and pending priorities.  Some bookkeeping will have to
be done during activation/recycling, but I think that's less than what has to be
done to generate SharedTileBundles.  If you do this, then you don't need to know
which bundle is active or pending because it will be explicit which tiles are
which, and then you can ditch the client pointer on bundles too.

https://codereview.chromium.org/62283012/diff/120001/cc/resources/shared_tile...
cc/resources/shared_tile_bundle.h:80: eBothPriorities,
BOTH_PRIORITIES

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:107: TileMap tiles_;
I wonder if there's a way to do this generally but not have to go all the way to
a hash map.  You've already baked in the tile shape elsewhere in constants, so
you could just have a fixed array of scoped_refptr here, indexed by local index.

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:108: int width_;
Do bundles need their width/height/offset explicitly if you have a bundle tiling
data on PictureLayerTiling?

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:114: PictureLayerTilingClient* client_;
I wish this wasn't necessary.

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/62283012/diff/120001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:405: active_bin =
kBinReadyToDrawMap[tile_is_ready_to_draw][active_bin];
Ugh, all this logic is so complicated.  Maybe after bundle v1 we can try to make
more work per-bundle instead of per-tile.

[vmpstr, 2013-11-21 23:37:47]

The latest approach is more promising in terms of performance.

Invalidation perftests increase in performance, but I suspect it's because of
the test itself (it now allocates less tiles because the "twin" tiles all
exist), I'll fix that up.

UpdateTilePriorities perftests still increase by over 100%

ManageTiles perftests increases slightly in performance (5-10%), but again I
think that's because of the test itself (ie, I have to update less things --
bundles -- when updating priorities).

I'll be fixing all of the test issues, but I would also appreciate feedback on
the latest approach.

[enne (OOO), 2013-11-25 22:17:11]

What did you change in the perf test? I'm not sure I understand what the issue
is, in terms of it not testing the right thing.

https://codereview.chromium.org/62283012/diff/350001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:482: scoped_refptr<TileBundle>
PictureLayerImpl::CreateTileBundle(int width,
Are all these parameters just a gfx::Rect (in...content space?)

https://codereview.chromium.org/62283012/diff/350001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:510: bool PictureLayerImpl::IsActive() const {
I think PictureLayerTiling is guaranteed to be created as pending and then
activated via PictureLayerTiling::DidBecomeActive and then recycled via
PictureLayerTiling::DidBecomeRecycled?  You could just store this on
PictureLayerTiling rather than calling out to the client?

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:708: // TODO(vmpstr): Since the old active
tree became the recycled tree,
For what it's worth, the old pending tree becomes the recycled tree after it
pushes from pending -> active.

Can you help me understand what the problem is more here? I feel like the
invalidation region and creating new tiles already handles this, or that there
isn't a fundamental difference between tiles and bundles wrt
recycle/active/pending tree sharing.

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:761: client_->UpdatePile(*tile_it);
Previously DidBecomeActive and UpdateTileToCurrentPile touched both shared and
unshared tiles.  This work was really only needed for the shared tiles, since
unshared tiles already had the right pile.  If tiles are shared, there is no
invalidation, and therefore the new pile is also correct.

I am not sure these checks are needed, and all tiles should be updated to the
new pile both here and below.

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:75: tiles_[tree][index_y][index_x] = tile;
What about DCHECKing spatial relationships here, that the tile's content rect
fits in the bundle's rect?

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:78: bool SetTile(WhichTree tree);
Is there a better name for this function?

https://codereview.chromium.org/62283012/diff/350001/cc/test/fake_picture_lay...
File cc/test/fake_picture_layer_tiling_client.h (right):

https://codereview.chromium.org/62283012/diff/350001/cc/test/fake_picture_lay...
cc/test/fake_picture_layer_tiling_client.h:74: TileManager* set_tile_manager_;
This is a little bit sketchy.  Maybe it's wrong for the tile manager to live in
the FakePictureLayerTilingClient and we need something like a
FakePictureLayerTilingClientClient (haha) or to just have every
FakePictureLayerTilingClient take in a tile manager in its ctor with lifetime
guarantees provided by the caller.

[vmpstr, 2013-11-27 00:03:14]

PTAL.

https://codereview.chromium.org/62283012/diff/350001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:482: scoped_refptr<TileBundle>
PictureLayerImpl::CreateTileBundle(int width,
On 2013/11/25 22:17:11, enne wrote:
> Are all these parameters just a gfx::Rect (in...content space?)

Yup, changed.

https://codereview.chromium.org/62283012/diff/350001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:510: bool PictureLayerImpl::IsActive() const {
On 2013/11/25 22:17:11, enne wrote:
> I think PictureLayerTiling is guaranteed to be created as pending and then
> activated via PictureLayerTiling::DidBecomeActive and then recycled via
> PictureLayerTiling::DidBecomeRecycled?  You could just store this on
> PictureLayerTiling rather than calling out to the client?

The one state change that's missing is going from recycled to pending. Are you
proposing adding a callback for that or are you proposing treating recycled as
pending? The latter should work since we don't call any functions on the
recycled tilings, but it also makes it difficult to DCHECK that we don't.

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:708: // TODO(vmpstr): Since the old active
tree became the recycled tree,
On 2013/11/25 22:17:11, enne wrote:
> For what it's worth, the old pending tree becomes the recycled tree after it
> pushes from pending -> active.
> 
> Can you help me understand what the problem is more here? I feel like the
> invalidation region and creating new tiles already handles this, or that there
> isn't a fundamental difference between tiles and bundles wrt
> recycle/active/pending tree sharing.

I've changed this... This wasn't the correct fix for the problem I was seeing.

The problem is the following:

We can call UpdateTilePriorities on the active tree, which can create a new tile
bundle (that is not on the twin) that is now in the new live tiles rect. This
populates the tiles in the tile bundle active_tree bucket.

The active tiling then later on becomes the pending tiling and the pending one
becomes the active.

However, the only way we swap active and pending tiles in a bundle is via did
become active (which is called, obviously, on the pending tree). The result is
that this newly created bundle is inaccessible via the pending tree and the
tiles are never swapped.

Now in the new active tiling (old pending tiling, if this still makes any
sense), we call UpdateTilePriorities again and this time we get the twin tiling
bundle that we created earlier. However, because swap was never called on this
tiling, we now have a tiling that already has active_tree bucket tiles filled
in, and so we dcheck in TileBundle::AddTileAt.


I considered always populating the tiling on both trees (ie, when we create a
new one on this, add it to the twin immediately), but that doesn't work in all
situations since we might not have a twin yet (ie, it's currently recycled). So
the fix I came up with is to keep track on the bundle whether the tiles need to
be swapped and then doing a lightweight check to see if tiles need swapping
every time we grab a bundle. I don't necessarily think it's the best solution,
but I think it works properly. If we add a "did become pending" callback, then
we can do this swap check during that callback.

The order of the callbacks will then be important then:
- DidBecomeRecycled first called on the current active tree (this sets the needs
swapping state)
- DidBecomeActive must be called next on the current pending tree (this swaps
everything that it can visit and resets the needs swapping state)
- DidBecomePending is then called some time later when we create a new pending
tree (this would only swap whatever needs swapping as indicated by the state).

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:761: client_->UpdatePile(*tile_it);
On 2013/11/25 22:17:11, enne wrote:
> Previously DidBecomeActive and UpdateTileToCurrentPile touched both shared and
> unshared tiles.  This work was really only needed for the shared tiles, since
> unshared tiles already had the right pile.  If tiles are shared, there is no
> invalidation, and therefore the new pile is also correct.
> 
> I am not sure these checks are needed, and all tiles should be updated to the
> new pile both here and below.

I changed this to be only updated for shared tiles. The reason I need the check
is that the iterator gets the tiles from both active and pending tilings (which
is a departure from the way it was done before) and we can run into a situation
where the current client's pile can't raster into a tile that is only on the
twin tiling (due to different recording region; ie the tile bundle may exist on
both trees, but the bottom row of tiles in the bundle might only be covered by a
recording from only one of the trees)

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:75: tiles_[tree][index_y][index_x] = tile;
On 2013/11/25 22:17:11, enne wrote:
> What about DCHECKing spatial relationships here, that the tile's content rect
> fits in the bundle's rect?

A similar thing is dchecked in UpdateToLocalIndex already. The bundle doesn't
really have a content rect (at least, currently), so the dcheck only checks that
the index of the tile falls within the area managed by this bundle.

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:78: bool SetTile(WhichTree tree);
On 2013/11/25 22:17:11, enne wrote:
> Is there a better name for this function?

Renamed it to InitializeNewTileForTree to better reflect what it does. I'm open
to other suggestions.

https://codereview.chromium.org/62283012/diff/350001/cc/test/fake_picture_lay...
File cc/test/fake_picture_layer_tiling_client.h (right):

https://codereview.chromium.org/62283012/diff/350001/cc/test/fake_picture_lay...
cc/test/fake_picture_layer_tiling_client.h:74: TileManager* set_tile_manager_;
On 2013/11/25 22:17:11, enne wrote:
> This is a little bit sketchy.  Maybe it's wrong for the tile manager to live
in
> the FakePictureLayerTilingClient and we need something like a
> FakePictureLayerTilingClientClient (haha) or to just have every
> FakePictureLayerTilingClient take in a tile manager in its ctor with lifetime
> guarantees provided by the caller.

Fair enough. I think I'll make tile manager be a constructor argument, since we
do need the same tile manager for multiple tiling clients (if they are to get
the same bundles and whatnot).

https://codereview.chromium.org/62283012/diff/380001/cc/resources/tile_manage...
File cc/resources/tile_manager_perftest.cc (right):

https://codereview.chromium.org/62283012/diff/380001/cc/resources/tile_manage...
cc/resources/tile_manager_perftest.cc:160: i < bundle_count;
I tried running this loop 4 times, but it made no significant difference. Either
this approach makes ManageTiles slightly faster, or the compiler is good at
optimizing my 4x loop :\

https://codereview.chromium.org/62283012/diff/380001/cc/test/fake_picture_lay...
File cc/test/fake_picture_layer_tiling_client.h (right):

https://codereview.chromium.org/62283012/diff/380001/cc/test/fake_picture_lay...
cc/test/fake_picture_layer_tiling_client.h:66: set_tile_manager_ = tile_manager;
This is still a todo that I'm working on.

[enne (OOO), 2013-11-27 01:38:05]

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:708: // TODO(vmpstr): Since the old active
tree became the recycled tree,
Re: DidBecomeRecycled/Active/Pending with swapping state, that sgtm as a
short/medium-term solution.  I do agree with what you said offline that the
longer solution here is to just sharing tilings.  This essentially lets you do
everything in one pass and doesn't have this "no twin when recycled problem".

Even if this is a short term solution, could you at least add one unit test that
runs through a scenario where you have some shared and some unshared tiles
during this activation? Maybe in picture_layer_impl_unittest with different live
tiles rects?

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:761: client_->UpdatePile(*tile_it);
On 2013/11/27 00:03:15, vmpstr wrote:
> On 2013/11/25 22:17:11, enne wrote:
> > Previously DidBecomeActive and UpdateTileToCurrentPile touched both shared
and
> > unshared tiles.  This work was really only needed for the shared tiles,
since
> > unshared tiles already had the right pile.  If tiles are shared, there is no
> > invalidation, and therefore the new pile is also correct.
> > 
> > I am not sure these checks are needed, and all tiles should be updated to
the
> > new pile both here and below.
> 
> I changed this to be only updated for shared tiles. The reason I need the
check
> is that the iterator gets the tiles from both active and pending tilings
(which
> is a departure from the way it was done before) and we can run into a
situation
> where the current client's pile can't raster into a tile that is only on the
> twin tiling (due to different recording region; ie the tile bundle may exist
on
> both trees, but the bottom row of tiles in the bundle might only be covered by
a
> recording from only one of the trees)

Hmm.  In the case that the pile can't raster onto those tiles, then that tile
will not exist.  Maybe the iterator just needs a tree parameter so that you
don't iterate over unshared, pending-only tiles?

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:75: tiles_[tree][index_y][index_x] = tile;
On 2013/11/27 00:03:15, vmpstr wrote:
> On 2013/11/25 22:17:11, enne wrote:
> > What about DCHECKing spatial relationships here, that the tile's content
rect
> > fits in the bundle's rect?
> 
> A similar thing is dchecked in UpdateToLocalIndex already. The bundle doesn't
> really have a content rect (at least, currently), so the dcheck only checks
that
> the index of the tile falls within the area managed by this bundle.

Oh, I see.  I guess I misunderstood what was being passed to the bundle.

https://codereview.chromium.org/62283012/diff/380001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/62283012/diff/380001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:507: bool PictureLayerImpl::IsActive() const {
What did you think about my suggestion to remove these?

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

https://codereview.chromium.org/62283012/diff/380001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:196: for (TileBundle::Iterator
tile_it(it->second); tile_it; ++tile_it)
Hmm, this now updates both the pending and active tiles even if just the pending
tree has an LCD change.  Maybe also a good reason for TileBundle::Iterator to
take a tree parameter?

https://codereview.chromium.org/62283012/diff/380001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/380001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:101: *index_x -= bundle_rect_.x();
Is bundle_rect just a collection of tile indices or is it a content rect? If
indices, those should stay integers, to be consistent with the rest of the code.
 It's not really a rectangle of space but a collection of indices.

Sorry, I think I misunderstood what this rect was here.

https://codereview.chromium.org/62283012/diff/380001/cc/resources/tile_manage...
File cc/resources/tile_manager_perftest.cc (right):

https://codereview.chromium.org/62283012/diff/380001/cc/resources/tile_manage...
cc/resources/tile_manager_perftest.cc:160: i < bundle_count;
On 2013/11/27 00:03:15, vmpstr wrote:
> I tried running this loop 4 times, but it made no significant difference.
Either
> this approach makes ManageTiles slightly faster, or the compiler is good at
> optimizing my 4x loop :\

Thanks for trying!

[vmpstr, 2013-11-27 21:09:35]

Please take a look. I'm somewhat deliberately punting changing unittests and
writing new ones towards the end of this review. I plan to add several tests for
the new functionality as a part of this review, but I just don't want to add
them until we solidify the rest of the code.

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:708: // TODO(vmpstr): Since the old active
tree became the recycled tree,
On 2013/11/27 01:38:05, enne wrote:
> Re: DidBecomeRecycled/Active/Pending with swapping state, that sgtm as a
> short/medium-term solution.  I do agree with what you said offline that the
> longer solution here is to just sharing tilings.  This essentially lets you do
> everything in one pass and doesn't have this "no twin when recycled problem".
> 
> Even if this is a short term solution, could you at least add one unit test
that
> runs through a scenario where you have some shared and some unshared tiles
> during this activation? Maybe in picture_layer_impl_unittest with different
live
> tiles rects?

Of course. I plan to add more than one. :) I just want to get the review in more
or less the shape that it will take, so that I don't spend too much time
changing newly written tests. I hope that's OK.

https://codereview.chromium.org/62283012/diff/350001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:761: client_->UpdatePile(*tile_it);
On 2013/11/27 01:38:05, enne wrote:
> On 2013/11/27 00:03:15, vmpstr wrote:
> > On 2013/11/25 22:17:11, enne wrote:
> > > Previously DidBecomeActive and UpdateTileToCurrentPile touched both shared
> and
> > > unshared tiles.  This work was really only needed for the shared tiles,
> since
> > > unshared tiles already had the right pile.  If tiles are shared, there is
no
> > > invalidation, and therefore the new pile is also correct.
> > > 
> > > I am not sure these checks are needed, and all tiles should be updated to
> the
> > > new pile both here and below.
> > 
> > I changed this to be only updated for shared tiles. The reason I need the
> check
> > is that the iterator gets the tiles from both active and pending tilings
> (which
> > is a departure from the way it was done before) and we can run into a
> situation
> > where the current client's pile can't raster into a tile that is only on the
> > twin tiling (due to different recording region; ie the tile bundle may exist
> on
> > both trees, but the bottom row of tiles in the bundle might only be covered
by
> a
> > recording from only one of the trees)
> 
> Hmm.  In the case that the pile can't raster onto those tiles, then that tile
> will not exist.  Maybe the iterator just needs a tree parameter so that you
> don't iterate over unshared, pending-only tiles?

Done.

https://codereview.chromium.org/62283012/diff/380001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/62283012/diff/380001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:507: bool PictureLayerImpl::IsActive() const {
On 2013/11/27 01:38:05, enne wrote:
> What did you think about my suggestion to remove these?

Sorry, I thought I replied to this one. I don't mind doing it, but I think that
will come at a cost of adding DidBecomePending callback. Or am I
misunderstanding something?

My comment was basically that we'll have to keep state and make sure it's up to
date, which seems like an optimization for a price of possible inconsistent
state? I don't know :P I can change it if you feel that it will be better.

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

https://codereview.chromium.org/62283012/diff/380001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:196: for (TileBundle::Iterator
tile_it(it->second); tile_it; ++tile_it)
On 2013/11/27 01:38:05, enne wrote:
> Hmm, this now updates both the pending and active tiles even if just the
pending
> tree has an LCD change.  Maybe also a good reason for TileBundle::Iterator to
> take a tree parameter?

Good point. I changed this. This function is run on both of the trees though (at
different times), so I added a which tree here.

https://codereview.chromium.org/62283012/diff/380001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/380001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:101: *index_x -= bundle_rect_.x();
On 2013/11/27 01:38:05, enne wrote:
> Is bundle_rect just a collection of tile indices or is it a content rect? If
> indices, those should stay integers, to be consistent with the rest of the
code.
>  It's not really a rectangle of space but a collection of indices.
> 
> Sorry, I think I misunderstood what this rect was here.

Done.

[enne (OOO), 2013-11-27 22:21:14]

This feels like it's converging.  My biggest balk is at the IsActive stuff, but
I have some smaller comments too.

https://codereview.chromium.org/62283012/diff/380001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/62283012/diff/380001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:507: bool PictureLayerImpl::IsActive() const {
On 2013/11/27 21:09:36, vmpstr wrote:
> On 2013/11/27 01:38:05, enne wrote:
> > What did you think about my suggestion to remove these?
> 
> Sorry, I thought I replied to this one. I don't mind doing it, but I think
that
> will come at a cost of adding DidBecomePending callback. Or am I
> misunderstanding something?
> 
> My comment was basically that we'll have to keep state and make sure it's up
to
> date, which seems like an optimization for a price of possible inconsistent
> state? I don't know :P I can change it if you feel that it will be better.

You could set the tiling to be pending when it becomes recycled and that would
save having the callback.

This is a wishywashy reason, but I feel like it's cleaner to have the tiling
sort of go through clear states of pending -> active -> recycled -> pending.  If
it's a client callback, you could just be like "I'm pending now" or "I'm active
now" at arbitrary points in time in a test, which is kind of not real and breaks
assumptions of how it should be used.  It's also a bit redundant.  Either the
layer tells the tiling what state it's in or the tiling asks the layer what
state it's in, but maybe not both?

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

https://codereview.chromium.org/62283012/diff/400001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:684: WhichTree tree, gfx::Rect
new_live_tiles_rect) {
Shouldn't this only set the live tiles rect for the current tree? I guess I
don't quite understand why this needs to be a parameter.

https://codereview.chromium.org/62283012/diff/400001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:736: for (TileBundle::Iterator
tile_it(it->second.get(), ACTIVE_TREE);
Thanks for this.  It makes the loop a lot easier to read.  :)

https://codereview.chromium.org/62283012/diff/400001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/62283012/diff/400001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:280: TileBundleMap tile_bundles_;  // It is
not legal to have a NULL tile in the
Comment is wrong.  Also, can you put this on its own line? I hate these
multiline indented comments.

https://codereview.chromium.org/62283012/diff/400001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/400001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:75: bool active_tree_only_tile() {
This is unused now, yeah?

https://codereview.chromium.org/62283012/diff/400001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:87: TilePriority* priority_[NUM_TREES];
Why does this need to be stored? Can you just ask for bundle_->priority(tree)?

[vmpstr, 2013-11-27 23:31:56]

Please take a look. I'll start writing unittests if you feel that this is more
or less in shape.

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

https://codereview.chromium.org/62283012/diff/400001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:684: WhichTree tree, gfx::Rect
new_live_tiles_rect) {
On 2013/11/27 22:21:15, enne wrote:
> Shouldn't this only set the live tiles rect for the current tree? I guess I
> don't quite understand why this needs to be a parameter.

This gets set in UpdateTilePriorities, which takes the tree as a parameter. I
added some code to dcheck/initialize it in UpdateTilePriorities though and this
now doesn't need the parameter.

https://codereview.chromium.org/62283012/diff/400001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/62283012/diff/400001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:280: TileBundleMap tile_bundles_;  // It is
not legal to have a NULL tile in the
On 2013/11/27 22:21:15, enne wrote:
> Comment is wrong.  Also, can you put this on its own line? I hate these
> multiline indented comments.

Done.

https://codereview.chromium.org/62283012/diff/400001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/400001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:75: bool active_tree_only_tile() {
On 2013/11/27 22:21:15, enne wrote:
> This is unused now, yeah?

Oh yeah! Done.

https://codereview.chromium.org/62283012/diff/400001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:87: TilePriority* priority_[NUM_TREES];
On 2013/11/27 22:21:15, enne wrote:
> Why does this need to be stored? Can you just ask for bundle_->priority(tree)?

I think that's a leftover from some previous iteration, removed.

[enne (OOO), 2013-11-27 23:43:09]

On 2013/11/27 23:31:56, vmpstr wrote:
> Please take a look. I'll start writing unittests if you feel that this is more
> or less in shape.

Yeah, thanks for putting up with all the review back and forth.  I'm really
happy with how this has shaped up.

[enne (OOO), 2013-11-27 23:43:18]

https://codereview.chromium.org/62283012/diff/420001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/62283012/diff/420001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:236: bundle_tiling_data_.SetMaxTextureSize(
Re: unit tests.  Can you also make sure to add some tiling index unit tests
related to verify off-by-one errors around internal/external tile edges and
borders? I'm not 100% confident that this math is right here.

Basically I want to verify that an iterator over tiling_data_ with a rect vs
bundle_tiling_data_ will do the right thing.  My worry is where (when comparing
iterating over the same rect on both) you get a bundle from bundle_tiling_data_
that contains no tiles you would have gotten from tiling_data_, or alternatively
a tile from tiling_data_ but not the bundle it's in.

https://codereview.chromium.org/62283012/diff/420001/cc/test/fake_picture_lay...
File cc/test/fake_picture_layer_tiling_client.h (right):

https://codereview.chromium.org/62283012/diff/420001/cc/test/fake_picture_lay...
cc/test/fake_picture_layer_tiling_client.h:42: virtual bool IsActive() const
OVERRIDE {
Not needed anymore.

[reveman, 2013-12-02 01:46:08]

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:252: void TileManager::DidChangeTilePriority(Tile*
bundle) {
s/bundle/tile/ ?

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
cc/resources/tile_manager.h:143: // Overriden from RefCountedManager<Tile>:
Mind fixing the spelling error here while at it? Overridden.

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
cc/resources/tile_manager.h:147: virtual void Release(TileBundle* tile)
OVERRIDE;
s/tile/bundle/ ?

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
cc/resources/tile_manager.h:149: // Overriden from RasterWorkerPoolClient:
and here.

[vmpstr, 2013-12-02 23:08:00]

Please take a look. I've added some unittests.

https://codereview.chromium.org/62283012/diff/420001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/62283012/diff/420001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:236: bundle_tiling_data_.SetMaxTextureSize(
On 2013/11/27 23:43:19, enne wrote:
> Re: unit tests.  Can you also make sure to add some tiling index unit tests
> related to verify off-by-one errors around internal/external tile edges and
> borders? I'm not 100% confident that this math is right here.
> 
> Basically I want to verify that an iterator over tiling_data_ with a rect vs
> bundle_tiling_data_ will do the right thing.  My worry is where (when
comparing
> iterating over the same rect on both) you get a bundle from
bundle_tiling_data_
> that contains no tiles you would have gotten from tiling_data_, or
alternatively
> a tile from tiling_data_ but not the bundle it's in.

This is done. I ended up changing the bundle_tiling_data max texture size
calculation. I initially misunderstood how the max texture size interacts with
tiling data and border pixels. This should now be correct (and unittested)

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:252: void TileManager::DidChangeTilePriority(Tile*
bundle) {
On 2013/12/02 01:46:08, David Reveman wrote:
> s/bundle/tile/ ?

Done.

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
cc/resources/tile_manager.h:143: // Overriden from RefCountedManager<Tile>:
On 2013/12/02 01:46:08, David Reveman wrote:
> Mind fixing the spelling error here while at it? Overridden.

Done.

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
cc/resources/tile_manager.h:147: virtual void Release(TileBundle* tile)
OVERRIDE;
On 2013/12/02 01:46:08, David Reveman wrote:
> s/tile/bundle/ ?

Done.

https://codereview.chromium.org/62283012/diff/420001/cc/resources/tile_manage...
cc/resources/tile_manager.h:149: // Overriden from RasterWorkerPoolClient:
On 2013/12/02 01:46:08, David Reveman wrote:
> and here.

Done.

https://codereview.chromium.org/62283012/diff/420001/cc/test/fake_picture_lay...
File cc/test/fake_picture_layer_tiling_client.h (right):

https://codereview.chromium.org/62283012/diff/420001/cc/test/fake_picture_lay...
cc/test/fake_picture_layer_tiling_client.h:42: virtual bool IsActive() const
OVERRIDE {
On 2013/11/27 23:43:19, enne wrote:
> Not needed anymore.

Done.

[enne (OOO), 2013-12-03 02:28:37]

Looking good.

Do you think there needs to be any testing around edge cases of half-filled
bundles? Like bundles where one layer ends its recorded region (or layer size)
halfway through a bundle, so it is not entirely filled with tiles, but its twin
does have all of the tiles?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:29: gfx::Size
ComputeBundleTextureSize(gfx::Size tile_size,
Yeah, this is exactly the kind of function I expected you might need.  :)

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:34: int bundle_width = inner_tile_width *
kTileBundleWidth + 2 * border_texels;
Is the 2 here kTileBundleWidth?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_set_unittest.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_set_unittest.cc:179:
~PictureLayerTilingSetSyncTest() {
Could you just order these variables differently in the class to get the
implicit destruction order you need?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:299: for (int tile_y = 0; tile_y <
2; ++tile_y) {
Is it possible to not make this depend on the magic bundle constants?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:832: success = false;
Why do you need this success variable with EXPECT_EQ on the previous line?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:98: int offset_x_;
Can you rename these four members in a way that makes it clear that they are
index offsets and index ranges and not spatial offsets and widths?

[vmpstr, 2013-12-03 18:44:42]

Please take a look.

With regards to testing edge cases that you described, that's really already
handled by tile bundle tests. I mean, when one layer is shorter than the other,
they will still share a bundle, except one of ACTIVE or PENDING trees tiles will
have nulls past the layer border (because the tiling won't create tiles for it).
Everything else should work exactly the same. I mean did become active will
cause a swap, but that's correct since the tilings will switch as well.

I can add tests if you think it's still worth it, though. Let me know.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.cc:34: int bundle_width = inner_tile_width *
kTileBundleWidth + 2 * border_texels;
On 2013/12/03 02:28:37, enne wrote:
> Is the 2 here kTileBundleWidth?

No, that's 2 for left and right borders. It's similar to how inner_tile_width is
calculated right above: we need to add those back in.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_set_unittest.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_set_unittest.cc:179:
~PictureLayerTilingSetSyncTest() {
On 2013/12/03 02:28:37, enne wrote:
> Could you just order these variables differently in the class to get the
> implicit destruction order you need?

Done.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:299: for (int tile_y = 0; tile_y <
2; ++tile_y) {
On 2013/12/03 02:28:37, enne wrote:
> Is it possible to not make this depend on the magic bundle constants?

I changed this to be kTileBundle{Width,Height} which is already defined here.
It's hard not to rely on some particular constant, since I'm testing
tiling->BundleAt()->TileAt() vs tiling->TileAt() here, which means I have to
figure out what the BundleAt and TileAt indexes are independently.

I could expose BundleAt index_offset stuff, but I would prefer not to, since
it's then testing more of consistency instead of correctness (ie, it's
consistent that if bundle offset is this, then it has tiles with those
offsets... but whether those tiles actually belong in there is not known). I
don't know if I'm making any sense :P

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:832: success = false;
On 2013/12/03 02:28:37, enne wrote:
> Why do you need this success variable with EXPECT_EQ on the previous line?

Oh this is just so it doesn't spam a lot of lines if _everything_ breaks. I can
remove this if that's better?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:98: int offset_x_;
On 2013/12/03 02:28:37, enne wrote:
> Can you rename these four members in a way that makes it clear that they are
> index offsets and index ranges and not spatial offsets and widths?

Done. Please check if it's better.

[reveman, 2013-12-03 19:58:18]

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:245: void
RemoveBundleIfEmptyContainingTileAt(int i, int j);
RemoveBundleContainingTileAtIfEmpty?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:155: InitializeNewTileForTree(trees_to_iterate_[1]))
Could this be integrated in the loop below instead? Maybe by doing
++current_tree_ in a way that's consistent with how we advance index_x_..

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:167:
!InitializeNewTileForTree(trees_to_iterate_[1])));
I find this code a bit hard to understand. Could some of this be moved into the
loop? does the "index_y_ < bundle_->height_" check need to be done for every
iteration or just when index_y_ is incremented?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:173: if (!tile || tile == current_tile_)
is the "tile == current_tile_" check needed? would it be cleaner to handle that
in the function above instead?

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:78: std::vector<WhichTree> trees_to_iterate_;
As tree types can be used for indexing, could this be replaced with "WhichTree
min_tree_; WhichTree max_tree_;"?

[enne (OOO), 2013-12-03 20:49:00]

On 2013/12/03 18:44:42, vmpstr wrote:

> With regards to testing edge cases that you described, that's really already
> handled by tile bundle tests. I mean, when one layer is shorter than the
other,
> they will still share a bundle, except one of ACTIVE or PENDING trees tiles
will
> have nulls past the layer border (because the tiling won't create tiles for
it).
> Everything else should work exactly the same. I mean did become active will
> cause a swap, but that's correct since the tilings will switch as well.

Yeah, mostly I was worried about null handling and making sure there's not an
assumption that every tile in a bundle is filled, but it's a tiny edge case.  If
you feel like it's already well-handled by existing tests, then no need to
specifically test it.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:832: success = false;
On 2013/12/03 18:44:43, vmpstr wrote:
> On 2013/12/03 02:28:37, enne wrote:
> > Why do you need this success variable with EXPECT_EQ on the previous line?
> 
> Oh this is just so it doesn't spam a lot of lines if _everything_ breaks. I
can
> remove this if that's better?

You could make them ASSERT instead of EXPECT and it'll abort?

[vmpstr, 2013-12-03 22:01:35]

PTAL.

Re: "Yeah, mostly I was worried about null handling and making sure there's not
an
assumption that every tile in a bundle is filled, but it's a tiny edge case.  If
you feel like it's already well-handled by existing tests, then no need to
specifically test it."

Yeah, the whole time I was writing this patch, I was always working on the
assumption that a bundle might not be filled. So, I think it is handled. I'll
admit that I should probably do more tests though...

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:245: void
RemoveBundleIfEmptyContainingTileAt(int i, int j);
On 2013/12/03 19:58:19, David Reveman wrote:
> RemoveBundleContainingTileAtIfEmpty?

Yeah, I think any way you slice it, the naming is kind of awkward :) Done.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:832: success = false;
On 2013/12/03 20:49:01, enne wrote:
> On 2013/12/03 18:44:43, vmpstr wrote:
> > On 2013/12/03 02:28:37, enne wrote:
> > > Why do you need this success variable with EXPECT_EQ on the previous line?
> > 
> > Oh this is just so it doesn't spam a lot of lines if _everything_ breaks. I
> can
> > remove this if that's better?
> 
> You could make them ASSERT instead of EXPECT and it'll abort?

Done.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:155: InitializeNewTileForTree(trees_to_iterate_[1]))
On 2013/12/03 19:58:19, David Reveman wrote:
> Could this be integrated in the loop below instead? Maybe by doing
> ++current_tree_ in a way that's consistent with how we advance index_x_..

Done. I rewrote this a bit.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:167:
!InitializeNewTileForTree(trees_to_iterate_[1])));
On 2013/12/03 19:58:19, David Reveman wrote:
> I find this code a bit hard to understand. Could some of this be moved into
the
> loop? does the "index_y_ < bundle_->height_" check need to be done for every
> iteration or just when index_y_ is incremented?

Done.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:173: if (!tile || tile == current_tile_)
On 2013/12/03 19:58:19, David Reveman wrote:
> is the "tile == current_tile_" check needed? would it be cleaner to handle
that
> in the function above instead?

We need this to make sure that the iterator doesn't return the same (shared)
tile twice. I added a comment to clear it up. I prefer to keep this in the
function so that I don't have to do the check in other spots.

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:78: std::vector<WhichTree> trees_to_iterate_;
On 2013/12/03 19:58:19, David Reveman wrote:
> As tree types can be used for indexing, could this be replaced with "WhichTree
> min_tree_; WhichTree max_tree_;"?

Done.

[reveman, 2013-12-03 23:13:10]

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/460001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:173: if (!tile || tile == current_tile_)
On 2013/12/03 22:01:36, vmpstr wrote:
> On 2013/12/03 19:58:19, David Reveman wrote:
> > is the "tile == current_tile_" check needed? would it be cleaner to handle
> that
> > in the function above instead?
> 
> We need this to make sure that the iterator doesn't return the same (shared)
> tile twice. I added a comment to clear it up. I prefer to keep this in the
> function so that I don't have to do the check in other spots.

Got it. Thanks for adding the comment.

https://codereview.chromium.org/62283012/diff/500001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/500001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:154: current_tree_ =
static_cast<WhichTree>(current_tree_ + 1);
should min/max/current_tree_ be unsigned instead?

https://codereview.chromium.org/62283012/diff/500001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:171: current_tree_ = min_tree_;
I like to think of the tree as just another index dimension. Would something
like this work and be cleaner?

do {
  current_tree_++;
  if (current_tree_ > max_tree_) {
    current_tree_ = min_tree_;
    ++index_x_;
    if (index_x_ >= bundle_->index_count_x_) {
       index_x_ = 0;
       ++index_y_;
       if (index_y_ >= bundle_->index_count_y_)
         break;
    }
  }
} while (!InitializeNewTile());

[enne (OOO), 2013-12-03 23:14:30]

https://codereview.chromium.org/62283012/diff/500001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/500001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:154: current_tree_ =
static_cast<WhichTree>(current_tree_ + 1);
On 2013/12/03 23:13:10, David Reveman wrote:
> should min/max/current_tree_ be unsigned instead?

unsigned would be a style violation, I think.

[vmpstr, 2013-12-03 23:41:33]

https://codereview.chromium.org/62283012/diff/500001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/500001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:154: current_tree_ =
static_cast<WhichTree>(current_tree_ + 1);
On 2013/12/03 23:14:30, enne wrote:
> On 2013/12/03 23:13:10, David Reveman wrote:
> > should min/max/current_tree_ be unsigned instead?
> 
> unsigned would be a style violation, I think.

I kind of prefer to keep it as WhichTree to be a bit explicit on what it means.

https://codereview.chromium.org/62283012/diff/500001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:171: current_tree_ = min_tree_;
On 2013/12/03 23:13:10, David Reveman wrote:
> I like to think of the tree as just another index dimension. Would something
> like this work and be cleaner?
> 
> do {
>   current_tree_++;
>   if (current_tree_ > max_tree_) {
>     current_tree_ = min_tree_;
>     ++index_x_;
>     if (index_x_ >= bundle_->index_count_x_) {
>        index_x_ = 0;
>        ++index_y_;
>        if (index_y_ >= bundle_->index_count_y_)
>          break;
>     }
>   }
> } while (!InitializeNewTile());

Oh I see what you mean. That's really nice! I was worried about current_tree_ >
max_tree_ type of check since that might be not exactly defined (ie no value >
max_tree in the enum), but then I remembered that we have NUM_TREES, which works
nicely as a sentinel. 

I think it looks better now

[reveman, 2013-12-04 00:14:24]

cc/resources/tile* changes lgtm

[enne (OOO), 2013-12-04 01:01:32]

https://codereview.chromium.org/62283012/diff/520001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/62283012/diff/520001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:269: friend class CoverageIterator;
Not your fault, but can you move this friend to be with the other friend?

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:87: ++conservative_count_;
Do you need a conditional before incrementing this variable too like you do for
RemoveTileAt?  What stops somebody from calling AddTileAt at the same x y and
bumping up the count?

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:20: class CC_EXPORT TileBundle : public
RefCountedManaged<TileBundle> {
Can you separate this from the forward declarations with a blank line?

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:34: inline bool IsEmpty() const { return
conservative_count_ == 0; }
No need for inline on member functions.

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:105: size_t conservative_count_;
Can you rename this to tile_count to differentiate from index_count?

What makes it "conservative"?

size_t => int

[vmpstr, 2013-12-04 01:11:56]

PTAL.

https://codereview.chromium.org/62283012/diff/520001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling.h (right):

https://codereview.chromium.org/62283012/diff/520001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:269: friend class CoverageIterator;
On 2013/12/04 01:01:38, enne wrote:
> Not your fault, but can you move this friend to be with the other friend?

Done.

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle.cc
File cc/resources/tile_bundle.cc (right):

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle...
cc/resources/tile_bundle.cc:87: ++conservative_count_;
On 2013/12/04 01:01:38, enne wrote:
> Do you need a conditional before incrementing this variable too like you do
for
> RemoveTileAt?  What stops somebody from calling AddTileAt at the same x y and
> bumping up the count?

Same x y and different tree does bump up the count, that's why it's
conservative. Same x y and tree is DCHECKed a couple of lines up.

RemoveTileAt is allowed to be called multiple times on the same thing and
returns true when it actually does something. This is to avoid "Tile* tile =
bundle->TileAt(...); if (tile) bundle->RemoveTileAt(...);" type of code

On the flip-side, AddTileAt for the same coordinates as an existing tile is
always wrong

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle.h
File cc/resources/tile_bundle.h (right):

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:20: class CC_EXPORT TileBundle : public
RefCountedManaged<TileBundle> {
On 2013/12/04 01:01:38, enne wrote:
> Can you separate this from the forward declarations with a blank line?

Done.

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:34: inline bool IsEmpty() const { return
conservative_count_ == 0; }
On 2013/12/04 01:01:38, enne wrote:
> No need for inline on member functions.

Done.

https://codereview.chromium.org/62283012/diff/520001/cc/resources/tile_bundle...
cc/resources/tile_bundle.h:105: size_t conservative_count_;
On 2013/12/04 01:01:38, enne wrote:
> Can you rename this to tile_count to differentiate from index_count?
> 

Done.

> What makes it "conservative"?
> 

If there's only one tile, but it's on both trees, the count will be 2. That is,
the only thing you can really say is that if it's 0, then the bundle is empty.
When it's non zero then it could be larger than the amount of tiles actually
stored in it (due to them appearing on two trees).

> size_t => int

Done.

[enne (OOO), 2013-12-04 01:18:34]

lgtm; let's land this puppy.

[commit-bot: I haz the power, 2013-12-04 23:45:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/62283012/470028

[commit-bot: I haz the power, 2013-12-05 01:27:10]

Failed to apply patch for cc/layers/picture_layer_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layers/picture_layer_impl.cc
  Hunk #2 succeeded at 491 with fuzz 1 (offset 12 lines).
  Hunk #3 succeeded at 732 (offset 20 lines).
  Hunk #4 FAILED at 733.
  1 out of 4 hunks FAILED -- saving rejects to file
cc/layers/picture_layer_impl.cc.rej

Patch:       cc/layers/picture_layer_impl.cc
Index: cc/layers/picture_layer_impl.cc
diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
index
a8fecb981f81025c3e393fca8296cf1375d1ddb9..32df431773af592c45d9a097b99643769e620f03
100644
--- a/cc/layers/picture_layer_impl.cc
+++ b/cc/layers/picture_layer_impl.cc
@@ -179,10 +179,10 @@ void PictureLayerImpl::AppendQuads(QuadSink* quad_sink,
         } else if (mode == ManagedTileState::TileVersion::PICTURE_PILE_MODE) {
           color = DebugColors::PictureTileBorderColor();
           width = DebugColors::PictureTileBorderWidth(layer_tree_impl());
-        } else if (iter->priority(ACTIVE_TREE).resolution == HIGH_RESOLUTION) {
+        } else if (iter.priority().resolution == HIGH_RESOLUTION) {
           color = DebugColors::HighResTileBorderColor();
           width = DebugColors::HighResTileBorderWidth(layer_tree_impl());
-        } else if (iter->priority(ACTIVE_TREE).resolution == LOW_RESOLUTION) {
+        } else if (iter.priority().resolution == LOW_RESOLUTION) {
           color = DebugColors::LowResTileBorderColor();
           width = DebugColors::LowResTileBorderWidth(layer_tree_impl());
         } else if (iter->contents_scale() > contents_scale_x()) {
@@ -479,6 +479,14 @@ scoped_refptr<Tile>
PictureLayerImpl::CreateTile(PictureLayerTiling* tiling,
       is_using_lcd_text_);
 }
 
+scoped_refptr<TileBundle> PictureLayerImpl::CreateTileBundle(int offset_x,
+                                                             int offset_y,
+                                                             int width,
+                                                             int height) {
+  return layer_tree_impl()->tile_manager()->CreateTileBundle(
+      offset_x, offset_y, width, height);
+}
+
 void PictureLayerImpl::UpdatePile(Tile* tile) {
   tile->set_picture_pile(pile_);
 }
@@ -704,7 +712,7 @@ void PictureLayerImpl::MarkVisibleResourcesAsRequired()
const {
       // This iteration is over the visible content rect which is potentially
       // less conservative than projecting the viewport into the layer.
       // Ignore tiles that are know to be outside the viewport.
-      if (iter->priority(PENDING_TREE).distance_to_visible_in_pixels != 0)
+      if (iter.priority().distance_to_visible_in_pixels != 0)
         continue;
 
       missing_region.Subtract(iter.geometry_rect());
@@ -725,7 +733,7 @@ void PictureLayerImpl::MarkVisibleResourcesAsRequired()
const {
     // This iteration is over the visible content rect which is potentially
     // less conservative than projecting the viewport into the layer.
     // Ignore tiles that are know to be outside the viewport.
-    if (iter->priority(PENDING_TREE).distance_to_visible_in_pixels != 0)
+    if (iter.priority().distance_to_visible_in_pixels != 0)
       continue;
 
     // If the missing region doesn't cover it, this tile is fully

[commit-bot: I haz the power, 2013-12-05 18:49:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vmpstr@chromium.org/62283012/590001

[commit-bot: I haz the power, 2013-12-06 00:01:57]

Change committed as 239074

[epenner, 2013-12-11 20:38:21]

On 2013/12/06 00:01:57, I haz the power (commit-bot) wrote:
> Change committed as 239074

Hi. I'd like someone from on Android to discuss such changes at a high level
first. Can we have a bugs for future issues like this? And then can we CC at
least aelias, klobag or myself?

This change is okay I suppose, but there are major trade-offs being made that I
think should warrant at least a bit of discussion with the relevant
stake-holders.

[epenner, 2013-12-11 20:59:34]

> Hi. I'd like someone from on Android to discuss such changes at a high level
> first. Can we have a bugs for future issues like this? And then can we CC at
> least aelias, klobag or myself?
> 
> This change is okay I suppose, but there are major trade-offs being made that
I
> think should warrant at least a bit of discussion with the relevant
> stake-holders.

I think my biggest concern is regarding visible TileBundles. Do we just call
those all visible now?

[enne (OOO), 2013-12-11 21:12:14]

On 2013/12/11 20:59:34, epenner wrote:
> > Hi. I'd like someone from on Android to discuss such changes at a high level
> > first. Can we have a bugs for future issues like this? And then can we CC at
> > least aelias, klobag or myself?

We do have a bug for this in http://crbug.com/318440.  Sorry for not catching
that it was missing here.

> > This change is okay I suppose, but there are major trade-offs being made
that
> I
> > think should warrant at least a bit of discussion with the relevant
> > stake-holders.

Major trade-offs such as...?

[enne (OOO), 2013-12-11 21:14:10]

On 2013/12/11 21:12:14, enne wrote:

> Major trade-offs such as...?

Sorry, better said: could you file bugs if you have concerns?

[vmpstr, 2013-12-11 21:41:06]

On 2013/12/11 20:38:21, epenner wrote:
> On 2013/12/06 00:01:57, I haz the power (commit-bot) wrote:
> > Change committed as 239074
> 
> Hi. I'd like someone from on Android to discuss such changes at a high level
> first. Can we have a bugs for future issues like this? And then can we CC at
> least aelias, klobag or myself?
> 
> This change is okay I suppose, but there are major trade-offs being made that
I
> think should warrant at least a bit of discussion with the relevant
> stake-holders.

Sounds good. Sorry about sneaking this one in. I'll try to include the people
you mentioned on future bugs that are somewhat major. And, as enne said, please
feel free to open new bugs if there are concerns about this change.

[epennerAtGoogle, 2013-12-11 22:19:41]

I'm sorry forget my original post (although a bug is always probably good). No
need to CC anyone in particular. I don't mean to say there is some android
police or something :P I suppose I would just like to be more involved with the
future designs in CC for some areas I've been working on in the past.

The main trade-off I was concerned about is whether this increases the number of
'visible' tiles. I didn't see special logic for that, but perhaps it's in here?
If we still have the same visible tile set there's nothing wrong, except I am a
bit concerned that we have moved on from micro-optimizations and approximations
and into bigger designs just for the sake of micro-optimization. In general, I
find that once you go there, it becomes harder to understand and change.  The
other bug I've seen we create 20 tilings in some cases, so those seem like lower
hanging fruit to improve our tile processing times.

[vmpstr, 2013-12-11 22:35:41]

On 2013/12/11 22:19:41, epennerAtGoogle wrote:
> I'm sorry forget my original post (although a bug is always probably good). No
> need to CC anyone in particular. I don't mean to say there is some android
> police or something :P I suppose I would just like to be more involved with
the
> future designs in CC for some areas I've been working on in the past.
> 
> The main trade-off I was concerned about is whether this increases the number
of
> 'visible' tiles. I didn't see special logic for that, but perhaps it's in
here?
> If we still have the same visible tile set there's nothing wrong, except I am
a
> bit concerned that we have moved on from micro-optimizations and
approximations
> and into bigger designs just for the sake of micro-optimization. In general, I
> find that once you go there, it becomes harder to understand and change.  The
> other bug I've seen we create 20 tilings in some cases, so those seem like
lower
> hanging fruit to improve our tile processing times.

This patch does have the potential to increase the number of tiles that are
marked visible. It assigns a single priority (distance/time to visible) for a
group of neighbouring tiles, so if a couple of those tiles are truly visible and
a couple are not, then the priority will have time and distance to visible as 0,
thus making all four "visible" (ie NOW_BIN). (let's continue the discussion on
how and whether to fix that on a bug that doesn't exist yet? :P)

I personally don't think this is a change for the sake of micro optimization. I
mean, technically you could claim that about most design changes, so I don't
know (I'm obviously biased in this particular example...). We've changed the
amount of information that we carry through to certain parts of the system (ie,
tile manager can figure out which tiles are next to each other in small groups),
and at the same time moved towards unifying tilings (skipping sync tilings and
such). I agree that there's no new big "feature" here, but I think that
sometimes restructuring the code is a worthwhile exercise as it exposes new
avenues of optimization.

[enne (OOO), 2013-12-11 22:52:03]

On 2013/12/11 22:35:41, vmpstr wrote:

> This patch does have the potential to increase the number of tiles that are
> marked visible.

On the other hand, this *doesn't* increase the number of tiles marked as
required for activation.  I'm less concerned with prioritization being a little
bit more sloppy in exchange for it taking a lot less time to do.  Sure, some
tiles may be considered more important than they used to be, but they were
already close to the viewport, but this doesn't seem too bad to me.

> I personally don't think this is a change for the sake of micro optimization.

I would also argue that this optizimation is not a micro, in that it decouples
our prioritization/sorting unit from our rasterization/upload unit.  It also
paves the way for a bunch of other future optimizations:

(1) Spatially grouping tiles in the tile manager allows rastering multiples
tiles together at once, which helps with clipping costs in Skia.
(2) Tile bundles helps move towards sharing PictureLayerTiling(Set) across
trees, which will help further in decreasing prioritization costs.

[epenner, 2013-12-11 23:26:51]

Okay. Thanks both, consider me convinced.