cc: Split tiling set raster queues into all and required.

cc: Split tiling set raster queues into all and required.

This patch does several things:
- Make TilingSetRasterQueue a virtual interface
- Rename the old queue to be TilingSetRasterQueueAll
- Add a TilingSetRasterQueueRequired that only returns either
  REQUIRED_FOR_DRAW or REQUIRED_FOR_ACTIVATION tiles.
- Moves the creation of TilingSet queues into RasterTilePriorityQueue

R=reveman, enne, danakj

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

[vmpstr, 2014-12-30 18:21:46]

Please take a look. I still would like to add more tests, but an early review
would be appreciated.

[vmpstr, 2015-01-08 21:06:08]

rebased, added some todos. PTAL.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:426: // TODO(hendrikw): This doesn't seem to do
anything, remove?
hendrikw, FYI.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:991: // TODO(vmpstr): Make this use
REQUIRED_FOR_ACTIVAITON queue.
I could just make the change now, but I'd prefer to land the bulk of the work
first, then "enable" it. Let me know if that's fine

[danakj, 2015-01-08 23:24:41]

I think this looks cool!

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

https://codereview.chromium.org/816453008/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2798: EXPECT_TRUE(queue->IsEmpty());
test where this would be false?

https://codereview.chromium.org/816453008/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2807:
EXPECT_FALSE(tile->IsReadyToDraw());
can you count them and comment explain where the correct count comes from?

maybe make a situation where IsReadyToDraw() is true for some tiles (verify they
get skipped?) and where required is false (same thing)?

https://codereview.chromium.org/816453008/diff/100001/cc/resources/raster_til...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/816453008/diff/100001/cc/resources/raster_til...
cc/resources/raster_tile_priority_queue.cc:137: if (!tiling_set)
dont think this can be null anymore can it?

https://codereview.chromium.org/816453008/diff/100001/cc/resources/raster_til...
cc/resources/raster_tile_priority_queue.cc:142: tiling_set, tree_priority ==
SMOOTHNESS_TAKES_PRIORITY));
can you use a temp var to give this bool a name related to what it means for the
RTPQ?

https://codereview.chromium.org/816453008/diff/100001/cc/resources/raster_til...
cc/resources/raster_tile_priority_queue.cc:207: break;
maybe dhceck that if has_both_layers then both queues are non-null?

https://codereview.chromium.org/816453008/diff/100001/cc/resources/raster_til...
File cc/resources/raster_tile_priority_queue.h (right):

https://codereview.chromium.org/816453008/diff/100001/cc/resources/raster_til...
cc/resources/raster_tile_priority_queue.h:27: struct PairedTilingSetQueue {
This class is not a struct (anymore?), it has non-trivial methods. It should be
changed to a class.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:991: // TODO(vmpstr): Make this use
REQUIRED_FOR_ACTIVAITON queue.
On 2015/01/08 21:06:08, vmpstr wrote:
> I could just make the change now, but I'd prefer to land the bulk of the work
> first, then "enable" it. Let me know if that's fine

Yes sounds good. Then you can add a test to show the change doing something
along with it, if possible maybe :)

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:203: EXPECT_EQ(all_tiles,
required_for_activation_tiles);
this test is just checking that all tiles are required for activation, which is
only testing the test setup?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:216: EXPECT_EQ(all_tiles,
required_for_draw_tiles);
ditto?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:336:
EXPECT_EQ(actual_required_for_activation_tiles,
Can you EXPECT_NE(required_for_activation, all_tiles) ?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:350:
EXPECT_EQ(actual_required_for_draw_tiles, required_for_draw_tiles);
Can you EXPECT_NE(required_for_draw, all_tiles) ?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:408: required_for_activation_tiles);
ditto?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:421:
EXPECT_EQ(actual_required_for_draw_tiles, required_for_draw_tiles);
moar ditto?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue.h (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue.h:12: class Tile;
move up a line

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue.h:13: class CC_EXPORT TilingSetRasterQueue
{
no longer needs to be exported, i think?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue.h:15: TilingSetRasterQueue() = default;
do you need a constructor? remind me why default is good here?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue.h:16: virtual ~TilingSetRasterQueue() =
default;
ditto why is default better than {}?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:19: DCHECK(type !=
RasterTilePriorityQueue::Type::ALL);
DCHECK_NE?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:20: DCHECK(tiling_set);
nit: not really doing anything, we'll crash when we deref it?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:22: PictureLayerTiling* tiling
=
can you leave a comment about why highres is enough? maybe point to other places
in the code that guarantee this assumption?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:62: : tiling_(NULL),
current_tile_(NULL) {
nullptr

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:68: : tiling_(tiling),
tiling_data_(tiling_data), current_tile_(NULL) {
nullptr

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:69: if
(!tiling_->has_visible_rect_tiles())
Should we have some kinda tests for this condition? Or a DCHECK in the early out
that verifies all the tiles are not required?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:101: } while (!current_tile_ ||
!TileNeedsRaster(current_tile_));
TileNeedsRaster uses occlusion to determine if it needs raster. Occlusion still
has this duality where it's able to be determined on its own, but its also part
of the tile priority. So it's not clear that this check here isn't using the
priority that gets set down below on line 104.

Now.. I believe this is right as it is, cuz this is in the iterator, and
iterators genreate priorities, and its using the method that doesn't use the
stored priority state about occlusion..

But what I wanna say is that this is a lot of assumptions/dependencies/something
between this code and PLT/Tile. It would be nice if this was more obvious.
Adding comments here to explain that would help (but the callsite to occlusion
is in the header and they wouldnt really make sense here or there in that case).
Maybe getting rid of the help and do the check inline here with comments? Maybe
do them with breaks and make the loop while (true) to let you comment?

I'm not sure exactly what to suggest, but I feel like I get this but other
people reading this would have no idea without having reviewed previous iterator
CLs.

I guess same thoughts about line80.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.h (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.h:38: operator bool() const {
return !!current_tile_; }
should this be an operator comparablethingthatisntabool to avoid conversions to
bool by accident? not sure if it matters here compared to a smart pointer.. but
wdyt?

[vmpstr, 2015-01-09 20:21:41]

PTAL. I think I've addressed more comments, and added another test. If I missed
something, please let me know.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:991: // TODO(vmpstr): Make this use
REQUIRED_FOR_ACTIVAITON queue.
On 2015/01/08 23:24:40, danakj wrote:
> On 2015/01/08 21:06:08, vmpstr wrote:
> > I could just make the change now, but I'd prefer to land the bulk of the
work
> > first, then "enable" it. Let me know if that's fine
> 
> Yes sounds good. Then you can add a test to show the change doing something
> along with it, if possible maybe :)

The change would just simply this code, but end up with no change in behavior.
But yes, I would verify that if this code isn't there something breaks :P

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:203: EXPECT_EQ(all_tiles,
required_for_activation_tiles);
On 2015/01/08 23:24:41, danakj wrote:
> this test is just checking that all tiles are required for activation, which
is
> only testing the test setup?

Well it's testing that these tiles are all shared (all are required for
activation and required for draw), but yes this is a "basic sanity" test that
just ensures we can get the tiles out of any of the iterators.

I'll add another more interesting test.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:216: EXPECT_EQ(all_tiles,
required_for_draw_tiles);
On 2015/01/08 23:24:40, danakj wrote:
> ditto?

Ditto!

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:336:
EXPECT_EQ(actual_required_for_activation_tiles,
On 2015/01/08 23:24:41, danakj wrote:
> Can you EXPECT_NE(required_for_activation, all_tiles) ?

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:350:
EXPECT_EQ(actual_required_for_draw_tiles, required_for_draw_tiles);
On 2015/01/08 23:24:41, danakj wrote:
> Can you EXPECT_NE(required_for_draw, all_tiles) ?

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:408: required_for_activation_tiles);
On 2015/01/08 23:24:40, danakj wrote:
> ditto?

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:421:
EXPECT_EQ(actual_required_for_draw_tiles, required_for_draw_tiles);
On 2015/01/08 23:24:40, danakj wrote:
> moar ditto?

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue.h (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue.h:12: class Tile;
On 2015/01/08 23:24:41, danakj wrote:
> move up a line

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue.h:13: class CC_EXPORT TilingSetRasterQueue
{
On 2015/01/08 23:24:41, danakj wrote:
> no longer needs to be exported, i think?

Maybe? dtor needs to be exported, no? I'll see if trybots complain.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue.h:15: TilingSetRasterQueue() = default;
On 2015/01/08 23:24:41, danakj wrote:
> do you need a constructor? remind me why default is good here?

Removed.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue.h:16: virtual ~TilingSetRasterQueue() =
default;
On 2015/01/08 23:24:41, danakj wrote:
> ditto why is default better than {}?

Personal preference: it specifies intent better. I changed this to be {}, since
I don't remember what the outcome of that discussion thread was (and for
consistency with other code we have).

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:19: DCHECK(type !=
RasterTilePriorityQueue::Type::ALL);
On 2015/01/08 23:24:41, danakj wrote:
> DCHECK_NE?

Since it's an enum class it would require a non-existent operator<<. I'd prefer
to keep this here, since the type itself doesn't matter it just has to be not
ALL :)

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:20: DCHECK(tiling_set);
On 2015/01/08 23:24:41, danakj wrote:
> nit: not really doing anything, we'll crash when we deref it?

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:22: PictureLayerTiling* tiling
=
On 2015/01/08 23:24:41, danakj wrote:
> can you leave a comment about why highres is enough? maybe point to other
places
> in the code that guarantee this assumption?

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:62: : tiling_(NULL),
current_tile_(NULL) {
On 2015/01/08 23:24:41, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:68: : tiling_(tiling),
tiling_data_(tiling_data), current_tile_(NULL) {
On 2015/01/08 23:24:41, danakj wrote:
> nullptr

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:69: if
(!tiling_->has_visible_rect_tiles())
On 2015/01/08 23:24:41, danakj wrote:
> Should we have some kinda tests for this condition? Or a DCHECK in the early
out
> that verifies all the tiles are not required?

I added a DCHECK, let me know if that's what you mean?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:101: } while (!current_tile_ ||
!TileNeedsRaster(current_tile_));
On 2015/01/08 23:24:41, danakj wrote:
> TileNeedsRaster uses occlusion to determine if it needs raster. Occlusion
still
> has this duality where it's able to be determined on its own, but its also
part
> of the tile priority. So it's not clear that this check here isn't using the
> priority that gets set down below on line 104.
> 
> Now.. I believe this is right as it is, cuz this is in the iterator, and
> iterators genreate priorities, and its using the method that doesn't use the
> stored priority state about occlusion..
> 
> But what I wanna say is that this is a lot of
assumptions/dependencies/something
> between this code and PLT/Tile. It would be nice if this was more obvious.
> Adding comments here to explain that would help (but the callsite to occlusion
> is in the header and they wouldnt really make sense here or there in that
case).
> Maybe getting rid of the help and do the check inline here with comments?
Maybe
> do them with breaks and make the loop while (true) to let you comment?
> 
> I'm not sure exactly what to suggest, but I feel like I get this but other
> people reading this would have no idea without having reviewed previous
iterator
> CLs.
> 
> I guess same thoughts about line80.

I understand. I've changed this code around a bit an sprinkled some comments.
Let me know if that reads better now.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.h (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.h:38: operator bool() const {
return !!current_tile_; }
On 2015/01/08 23:24:41, danakj wrote:
> should this be an operator comparablethingthatisntabool to avoid conversions
to
> bool by accident? not sure if it matters here compared to a smart pointer..
but
> wdyt?

Can you elaborate? I'm not sure I understand your suggestion :\ In either case
this is a private class so the use is limited, but if there's a safer way to do
this, I'm up for it. Alternatively, we can just not use iterator style class,
and do like IsDone() or something.

[danakj, 2015-01-09 20:28:22]

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:19: DCHECK(type !=
RasterTilePriorityQueue::Type::ALL);
On 2015/01/09 20:21:40, vmpstr wrote:
> On 2015/01/08 23:24:41, danakj wrote:
> > DCHECK_NE?
> 
> Since it's an enum class it would require a non-existent operator<<. I'd
prefer
> to keep this here, since the type itself doesn't matter it just has to be not
> ALL :)

Can you static cast them to ints? If that's terrible, can you DCHECK(...) <<
type?

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.h (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.h:38: operator bool() const {
return !!current_tile_; }
On 2015/01/09 20:21:41, vmpstr wrote:
> On 2015/01/08 23:24:41, danakj wrote:
> > should this be an operator comparablethingthatisntabool to avoid conversions
> to
> > bool by accident? not sure if it matters here compared to a smart pointer..
> but
> > wdyt?
> 
> Can you elaborate? I'm not sure I understand your suggestion :\ In either case
> this is a private class so the use is limited, but if there's a safer way to
do
> this, I'm up for it. Alternatively, we can just not use iterator style class,
> and do like IsDone() or something.

I mean like this
https://code.google.com/p/chromium/codesearch#chromium/src/skia/ext/refptr.h&...

[vmpstr, 2015-01-09 20:52:42]

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:19: DCHECK(type !=
RasterTilePriorityQueue::Type::ALL);
On 2015/01/09 20:28:22, danakj wrote:
> On 2015/01/09 20:21:40, vmpstr wrote:
> > On 2015/01/08 23:24:41, danakj wrote:
> > > DCHECK_NE?
> > 
> > Since it's an enum class it would require a non-existent operator<<. I'd
> prefer
> > to keep this here, since the type itself doesn't matter it just has to be
not
> > ALL :)
> 
> Can you static cast them to ints? If that's terrible, can you DCHECK(...) <<
> type?

Done.

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.h (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.h:38: operator bool() const {
return !!current_tile_; }
On 2015/01/09 20:28:22, danakj wrote:
> On 2015/01/09 20:21:41, vmpstr wrote:
> > On 2015/01/08 23:24:41, danakj wrote:
> > > should this be an operator comparablethingthatisntabool to avoid
conversions
> > to
> > > bool by accident? not sure if it matters here compared to a smart
pointer..
> > but
> > > wdyt?
> > 
> > Can you elaborate? I'm not sure I understand your suggestion :\ In either
case
> > this is a private class so the use is limited, but if there's a safer way to
> do
> > this, I'm up for it. Alternatively, we can just not use iterator style
class,
> > and do like IsDone() or something.
> 
> I mean like this
>
https://code.google.com/p/chromium/codesearch#chromium/src/skia/ext/refptr.h&...

Hmm. Not really a fan from the clean code perspective, but this is less error
prone, I agree. I kinda put my own flavour in, let me know if that's OK.

[vmpstr, 2015-01-10 01:44:56]

PTAL

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.h (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.h:38: operator bool() const {
return !!current_tile_; }
On 2015/01/09 20:52:42, vmpstr wrote:
> On 2015/01/09 20:28:22, danakj wrote:
> > On 2015/01/09 20:21:41, vmpstr wrote:
> > > On 2015/01/08 23:24:41, danakj wrote:
> > > > should this be an operator comparablethingthatisntabool to avoid
> conversions
> > > to
> > > > bool by accident? not sure if it matters here compared to a smart
> pointer..
> > > but
> > > > wdyt?
> > > 
> > > Can you elaborate? I'm not sure I understand your suggestion :\ In either
> case
> > > this is a private class so the use is limited, but if there's a safer way
to
> > do
> > > this, I'm up for it. Alternatively, we can just not use iterator style
> class,
> > > and do like IsDone() or something.
> > 
> > I mean like this
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/skia/ext/refptr.h&...
> 
> Hmm. Not really a fan from the clean code perspective, but this is less error
> prone, I agree. I kinda put my own flavour in, let me know if that's OK.

After some discussion, I've decided to just go with a bool done() const; :)

[danakj, 2015-01-12 18:23:13]

LGTM

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.cc (right):

https://codereview.chromium.org/816453008/diff/140001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.cc:101: } while (!current_tile_ ||
!TileNeedsRaster(current_tile_));
On 2015/01/09 20:21:40, vmpstr wrote:
> On 2015/01/08 23:24:41, danakj wrote:
> > TileNeedsRaster uses occlusion to determine if it needs raster. Occlusion
> still
> > has this duality where it's able to be determined on its own, but its also
> part
> > of the tile priority. So it's not clear that this check here isn't using the
> > priority that gets set down below on line 104.
> > 
> > Now.. I believe this is right as it is, cuz this is in the iterator, and
> > iterators genreate priorities, and its using the method that doesn't use the
> > stored priority state about occlusion..
> > 
> > But what I wanna say is that this is a lot of
> assumptions/dependencies/something
> > between this code and PLT/Tile. It would be nice if this was more obvious.
> > Adding comments here to explain that would help (but the callsite to
occlusion
> > is in the header and they wouldnt really make sense here or there in that
> case).
> > Maybe getting rid of the help and do the check inline here with comments?
> Maybe
> > do them with breaks and make the loop while (true) to let you comment?
> > 
> > I'm not sure exactly what to suggest, but I feel like I get this but other
> > people reading this would have no idea without having reviewed previous
> iterator
> > CLs.
> > 
> > I guess same thoughts about line80.
> 
> I understand. I've changed this code around a bit an sprinkled some comments.
> Let me know if that reads better now.

Ya this is great, thank you for nice comments.

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:428:
TEST_F(TileManagerTilePriorityQueueTest, RasterTilePriorityQueueInvalidation) {
Great test!

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:445:
ASSERT_TRUE(pending_layer_->HighResTiling()->TileAt(0, 0));
nit: i prefer s/ASSERT/EXPECT/ throughout, you dont break scoping (like
DidDrawAllLayers() not being called causes a crash for eg - not this test tho)
and don't hide more info later potentially.

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_all.h (right):

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_all.h:37: bool done() const { return
current_tile_ == nullptr; }
One less operator :)

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tiling_set...
File cc/resources/tiling_set_raster_queue_required.h (right):

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tiling_set...
cc/resources/tiling_set_raster_queue_required.h:31: // This iterator will return
all tiles that are in the NOW bin on the given
Yay class comment!

[vmpstr, 2015-01-12 19:17:26]

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tile_manag...
File cc/resources/tile_manager_unittest.cc (right):

https://codereview.chromium.org/816453008/diff/220001/cc/resources/tile_manag...
cc/resources/tile_manager_unittest.cc:445:
ASSERT_TRUE(pending_layer_->HighResTiling()->TileAt(0, 0));
On 2015/01/12 18:23:13, danakj wrote:
> nit: i prefer s/ASSERT/EXPECT/ throughout, you dont break scoping (like
> DidDrawAllLayers() not being called causes a crash for eg - not this test tho)
> and don't hide more info later potentially.

Done.

[vmpstr, 2015-01-12 19:17:32]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-01-12 19:18:22]

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

[commit-bot: I haz the power, 2015-01-12 19:32:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-12 19:32:12]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[vmpstr, 2015-01-12 19:57:21]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-01-12 19:58:37]

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

[commit-bot: I haz the power, 2015-01-12 20:51:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-12 20:51:51]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vmpstr, 2015-01-12 21:21:36]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-01-12 21:23:07]

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

[commit-bot: I haz the power, 2015-01-12 22:21:20]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-01-12 22:22:43]

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