cc: Ensure to skip the first tile that would be returned by twin.

cc: Ensure to skip the first tile that would be returned by twin.

Since we're using a spiral iterator, we run into a situations where
a shared tile is visited by an iterator, but it is meant to be returned
by the twin (since it has higher priority). We already skip these types
of tiles in the Pop function. However, it could be the case that
the very first tile is of this kind. The fix is to ensure we skip these
types of tiles in the constructor as well.

BUG=422178
R=reveman

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

[vmpstr, 2014-10-10 08:01:59]

PTAL

[reveman, 2014-10-10 14:27:23]

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
cc/resources/raster_tile_priority_queue.cc:201:
PictureLayerImpl::LayerRasterTileIterator* next_iterator = NULL;
I find the use of next_iterator confusing. it's incremented in the for loop
statement but then assigned a new value right after that. what's going on here?

I think |next_iterator| being used as part of the for statement but set to null
initially is a bit confusing. Maybe change this to a "do while" loop to make it
more clear that we'll never try to increment it when null.

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
File cc/resources/raster_tile_priority_queue.h (right):

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
cc/resources/raster_tile_priority_queue.h:31: void
SkipNextSharedTileReturnedByTwin(TreePriority tree_priority);
I'm a bit confused by this name. Sounds like it will only every skip one tile
and it does so unconditionally? Is that really the case?

[reveman, 2014-10-10 14:40:47]

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
cc/resources/raster_tile_priority_queue.cc:201:
PictureLayerImpl::LayerRasterTileIterator* next_iterator = NULL;
s/NULL/nullptr/

[vmpstr, 2014-10-10 15:24:05]

PTAL

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
File cc/resources/raster_tile_priority_queue.cc (right):

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
cc/resources/raster_tile_priority_queue.cc:201:
PictureLayerImpl::LayerRasterTileIterator* next_iterator = NULL;
On 2014/10/10 14:27:22, reveman wrote:
> I find the use of next_iterator confusing. it's incremented in the for loop
> statement but then assigned a new value right after that. what's going on
here?
> 
> I think |next_iterator| being used as part of the for statement but set to
null
> initially is a bit confusing. Maybe change this to a "do while" loop to make
it
> more clear that we'll never try to increment it when null.

I made this into a while loop. FWIW, I just moved the code from above into a
separate function :)

And changed to nullptr.

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
File cc/resources/raster_tile_priority_queue.h (right):

https://codereview.chromium.org/647683002/diff/1/cc/resources/raster_tile_pri...
cc/resources/raster_tile_priority_queue.h:31: void
SkipNextSharedTileReturnedByTwin(TreePriority tree_priority);
On 2014/10/10 14:27:23, reveman wrote:
> I'm a bit confused by this name. Sounds like it will only every skip one tile
> and it does so unconditionally? Is that really the case?

No, I renamed to SkipTilesReturnedByTwin... It just skips the next (possibly
several) tiles that have a priority better suited for the other iterator. If you
have a different name suggestion, let me know.

[reveman, 2014-10-10 15:30:40]

lgtm

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

https://codereview.chromium.org/647683002/diff/110001/cc/resources/raster_til...
cc/resources/raster_tile_priority_queue.cc:217: ++(*next_iterator);
ah, we're advancing the iterator we decided to use above. thanks, I think a
while loop made this easier to understand. at least for me..

[vmpstr, 2014-10-10 15:36:18]

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

https://codereview.chromium.org/647683002/diff/110001/cc/resources/raster_til...
cc/resources/raster_tile_priority_queue.cc:217: ++(*next_iterator);
On 2014/10/10 15:30:40, reveman wrote:
> ah, we're advancing the iterator we decided to use above. thanks, I think a
> while loop made this easier to understand. at least for me..

Same here :)

[vmpstr, 2014-10-10 15:36:58]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-10-10 15:37:33]

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

[commit-bot: I haz the power, 2014-10-10 17:47:19]

Committed patchset #3 (id:110001)

[commit-bot: I haz the power, 2014-10-10 17:48:09]

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