cc: Fix layer raster tile iterator not to skip a tile erroneously.

cc: Fix layer raster tile iterator not to skip a tile erroneously.

Fix the constructor of the layer raster tile iterator to never advance
a tiling raster tile iterator. This fixes the iterator not to skip the
first tile returned by a tiling raster tile iterator even if there are
no NOW tiles but only SOON and/or EVENTUALLY tiles.

Committed: https://crrev.com/2451de626227435026a3dcd83c8035baa2bf55d2
Cr-Commit-Position: refs/heads/master@{#294188}

[USE eero AT chromium.org, 2014-09-09 17:07:09]

e.hakkinen@samsung.com changed reviewers:
+ reveman@chromium.org, vmpstr@chromium.org

[USE eero AT chromium.org, 2014-09-09 17:13:45]

PTAL. Here is a fix for a layer raster tile iterator and an addition to a layer
raster tile unit test. That additional test fails without this patch and
succeeds with it.

I'm not very familiar with coordinates handling in unit tests, so please advice
how to do that properly, if my way is not a proper one.

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2620: EXPECT_EQ(16u,
high_res_tile_count);
Without the fix, the iterator returns only 15 high resolution tiles.

[vmpstr, 2014-09-09 17:37:35]

Good catch! Thanks for fixing this. Overall looks good, I just have a few nits
below

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1618: ++current_stage_;
nit: DCHECK_LT(current_stage_, arraysize(stages_));

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2578:
host_impl_.SetExternalDrawConstraints(transform,
nit: Does host_impl_.SetViewportSize work here instead? If so, use that instead.

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2585:
pending_layer_->draw_properties().screen_space_transform = transform;
I don't think you need this?

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2620: EXPECT_EQ(16u,
high_res_tile_count);
On 2014/09/09 17:13:45, e_hakkinen wrote:
> Without the fix, the iterator returns only 15 high resolution tiles.

These are non-now ones, right?

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2629:
host_impl_.SetExternalDrawConstraints(transform,
Same comments here as above

[USE eero AT chromium.org, 2014-09-10 07:48:20]

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1618: ++current_stage_;
On 2014/09/09 17:37:35, vmpstr wrote:
> nit: DCHECK_LT(current_stage_, arraysize(stages_));

Done.

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2578:
host_impl_.SetExternalDrawConstraints(transform,
On 2014/09/09 17:37:35, vmpstr wrote:
> nit: Does host_impl_.SetViewportSize work here instead? If so, use that
instead.

I don't see how as it is not about size but position.
That is to say I have to somehow move the viewport out of layer bounds in order
to prevent NOW tiles from being created.

Or maybe an occlusion tracker could be used?

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2585:
pending_layer_->draw_properties().screen_space_transform = transform;
On 2014/09/09 17:37:35, vmpstr wrote:
> I don't think you need this?

I'll check.

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2620: EXPECT_EQ(16u,
high_res_tile_count);
On 2014/09/09 17:37:35, vmpstr wrote:
> On 2014/09/09 17:13:45, e_hakkinen wrote:
> > Without the fix, the iterator returns only 15 high resolution tiles.
> 
> These are non-now ones, right?

Yes, correct.

https://codereview.chromium.org/555183002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:2629:
host_impl_.SetExternalDrawConstraints(transform,
On 2014/09/09 17:37:35, vmpstr wrote:
> Same comments here as above

And the same reply.

[vmpstr, 2014-09-10 15:49:05]

lgtm

https://codereview.chromium.org/555183002/diff/20001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/555183002/diff/20001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl_unittest.cc:2576:
pending_layer_->draw_properties().visible_content_rect =
This works. I just wanted it to be a simple one line to do your intention. I
think SetViewportSize with SetScrollOffset would work as well, but I think this
is better.

[USE eero AT chromium.org, 2014-09-10 15:53:53]

The CQ bit was checked by e.hakkinen@samsung.com

[commit-bot: I haz the power, 2014-09-10 15:54:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/e.hakkinen@samsung.com/555183002/20001

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

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

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

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (None)

[vmpstr, 2014-09-10 17:49:59]

The CQ bit was checked by vmpstr@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/e.hakkinen@samsung.com/555183002/20001

[commit-bot: I haz the power, 2014-09-10 18:02:26]

Committed patchset #2 (id:20001) as 7024d820d87bbe1f31237f8d7398bd0e93cb6635

[commit-bot: I haz the power, 2014-09-10 18:14:05]

Patchset 2 (id:??) landed as
https://crrev.com/2451de626227435026a3dcd83c8035baa2bf55d2
Cr-Commit-Position: refs/heads/master@{#294188}