cc: Fix tile priority inversion in picture layer tiling.

cc: Fix tile priority inversion in picture layer tiling.

This patch addresses the case where we can have a tile that is required
for activation but is not in the NOW bin. This can happen since the
check for whether the tile is required uses the tile's content_rect,
which includes border pixels. However, the priority calculation for
the tile uses TileBounds without the border pixels.

This patch also moves the code to calculate required tile state from
all of the different iterator call sites to MakePrioritizedTile, with
an additional check to ensure that we never make a required tile outside
of the now bin.

R=danakj@chromium.org
BUG=668892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2651413004
Cr-Commit-Position: refs/heads/master@{#447293}
Committed: https://chromium.googlesource.com/chromium/src/+/a062f034b9c367a0b3481e0390bcc0aca830b707

[vmpstr, 2017-01-27 23:33:52]

Description was changed from

==========
cc: Fix tile priority inversion in picture layer tiling.

This patch addresses the case where we can have a tile that is required
for activation but is not in the NOW bin. This can happen since the
check for whether the tile is required uses the tile's content_rect,
which includes border pixels. However, the priority calculation for
the tile uses TileBounds without the border pixels.

This patch also moves the code to calculate required tile state from
all of the different iterator call sites to MakePrioritizedTile, with
an additional check to ensure that we never make a required tile outside
of the now bin.

R=danakj@chromium.org
BUG=668892
==========

to

==========
cc: Fix tile priority inversion in picture layer tiling.

This patch addresses the case where we can have a tile that is required
for activation but is not in the NOW bin. This can happen since the
check for whether the tile is required uses the tile's content_rect,
which includes border pixels. However, the priority calculation for
the tile uses TileBounds without the border pixels.

This patch also moves the code to calculate required tile state from
all of the different iterator call sites to MakePrioritizedTile, with
an additional check to ensure that we never make a required tile outside
of the now bin.

R=danakj@chromium.org
BUG=668892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[vmpstr, 2017-01-27 23:40:31]

Please take a look.

[danakj, 2017-01-30 16:55:18]

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.cc:819:
tile->set_required_for_activation(is_now_bin &&
I don't see a test checking this code, which appears to be changing to include
the NOW comparison?

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.h (right):

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.h:294: void UpdateRequiredStatesOnTile(Tile* tile)
const;
remove this

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:974: gfx::Rect visible_rect =
gfx::Rect(0, 0, 99, 99);
Can you also test what happens when the tile is 1px inside the rect?

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:980: Tile* tile =
pending_tiling->TileAt(1, 0);
Can you do some expectations to prove that the tile is on the edge of the
visible rect and border overlaps it?

[vmpstr, 2017-01-30 19:38:21]

Please take another look.

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.cc:819:
tile->set_required_for_activation(is_now_bin &&
On 2017/01/30 16:55:18, danakj (slow) wrote:
> I don't see a test checking this code, which appears to be changing to include
> the NOW comparison?

This was just a safeguard that we never have required tiles when we're not in
NOW bin. This shouldn't change behavior, and is tested in iterator tests. I
changed this to be a DCHECK instead.

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling.h (right):

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling.h:294: void UpdateRequiredStatesOnTile(Tile* tile)
const;
On 2017/01/30 16:55:18, danakj (slow) wrote:
> remove this

Done.

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
File cc/tiles/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:974: gfx::Rect visible_rect =
gfx::Rect(0, 0, 99, 99);
On 2017/01/30 16:55:18, danakj (slow) wrote:
> Can you also test what happens when the tile is 1px inside the rect?

Done.

https://codereview.chromium.org/2651413004/diff/1/cc/tiles/picture_layer_tili...
cc/tiles/picture_layer_tiling_unittest.cc:980: Tile* tile =
pending_tiling->TileAt(1, 0);
On 2017/01/30 16:55:18, danakj (slow) wrote:
> Can you do some expectations to prove that the tile is on the edge of the
> visible rect and border overlaps it?

Done.

[danakj, 2017-01-30 19:42:21]

LGTM

https://codereview.chromium.org/2651413004/diff/40001/cc/tiles/picture_layer_...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/2651413004/diff/40001/cc/tiles/picture_layer_...
cc/tiles/picture_layer_tiling.cc:822: (tile_priority.priority_bin ==
TilePriority::NOW ||
nit: this is nesting || against ||, you don't need the ()s

https://codereview.chromium.org/2651413004/diff/40001/cc/tiles/picture_layer_...
File cc/tiles/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/2651413004/diff/40001/cc/tiles/picture_layer_...
cc/tiles/picture_layer_tiling_unittest.cc:974: // Tile tile at (1, 0) should be
touching the visible rect, but not
Tile tile

[vmpstr, 2017-01-30 19:44:49]

I've readded UpdateRequiredStatesOnTile (and am using it now), since
https://codereview.chromium.org/2555743004/diff/460001/cc/tiles/tile_manager....
(which is in review) relies on it.

Please take another look if that's fine.

https://codereview.chromium.org/2651413004/diff/40001/cc/tiles/picture_layer_...
File cc/tiles/picture_layer_tiling.cc (right):

https://codereview.chromium.org/2651413004/diff/40001/cc/tiles/picture_layer_...
cc/tiles/picture_layer_tiling.cc:822: (tile_priority.priority_bin ==
TilePriority::NOW ||
On 2017/01/30 19:42:21, danakj (slow) wrote:
> nit: this is nesting || against ||, you don't need the ()s

Done.

https://codereview.chromium.org/2651413004/diff/40001/cc/tiles/picture_layer_...
File cc/tiles/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/2651413004/diff/40001/cc/tiles/picture_layer_...
cc/tiles/picture_layer_tiling_unittest.cc:974: // Tile tile at (1, 0) should be
touching the visible rect, but not
On 2017/01/30 19:42:21, danakj (slow) wrote:
> Tile tile

Done.

[danakj, 2017-01-30 19:46:52]

That's fine sure LGTM

[vmpstr, 2017-01-30 19:47:07]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-01-30 19:47:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-30 20:33:42]

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

[commit-bot: I haz the power, 2017-01-30 20:33:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vmpstr, 2017-01-30 21:40:01]

The CQ bit was checked by vmpstr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-30 21:40:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-01-30 22:42:17]

The CQ bit was unchecked by vmpstr@chromium.org

[vmpstr, 2017-01-30 22:42:18]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-01-30 22:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2651413004/#ps100001
(title: "whyyoucrash: update")

[commit-bot: I haz the power, 2017-01-30 22:42:26]

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

[commit-bot: I haz the power, 2017-01-30 22:42:27]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-01-31 19:15:22]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2017-01-31 19:15:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 19:22:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485890122302000,
"parent_rev": "34feb47fca547bae72a4c864d1e31bee25ef4cee", "commit_rev":
"a062f034b9c367a0b3481e0390bcc0aca830b707"}

[commit-bot: I haz the power, 2017-01-31 19:22:34]

Description was changed from

==========
cc: Fix tile priority inversion in picture layer tiling.

This patch addresses the case where we can have a tile that is required
for activation but is not in the NOW bin. This can happen since the
check for whether the tile is required uses the tile's content_rect,
which includes border pixels. However, the priority calculation for
the tile uses TileBounds without the border pixels.

This patch also moves the code to calculate required tile state from
all of the different iterator call sites to MakePrioritizedTile, with
an additional check to ensure that we never make a required tile outside
of the now bin.

R=danakj@chromium.org
BUG=668892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Fix tile priority inversion in picture layer tiling.

This patch addresses the case where we can have a tile that is required
for activation but is not in the NOW bin. This can happen since the
check for whether the tile is required uses the tile's content_rect,
which includes border pixels. However, the priority calculation for
the tile uses TileBounds without the border pixels.

This patch also moves the code to calculate required tile state from
all of the different iterator call sites to MakePrioritizedTile, with
an additional check to ensure that we never make a required tile outside
of the now bin.

R=danakj@chromium.org
BUG=668892
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2651413004
Cr-Commit-Position: refs/heads/master@{#447293}
Committed:
https://chromium.googlesource.com/chromium/src/+/a062f034b9c367a0b3481e0390bc...
==========

[commit-bot: I haz the power, 2017-01-31 19:22:35]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a062f034b9c367a0b3481e0390bc...