cc: Switch eviction iterators to consider combined priority.

cc: Switch eviction iterators to consider combined priority.

When processing eviction, we should be considering combined priority as
the determining factor in which tile goes next. The reason for this is
that even in situations where a tree priority would imply that a
particular tile is of low priority, we need to be mindful of where it
appears on the other tree as well.

This is captured in the fact that tile manager is using combined
priority to schedule tiles, but the eviction iterator itself does not
reflect this.

BUG=457977
R=vmiura, danakj

Committed: https://crrev.com/198f1cd771751f113641219e05d6ecf9f22b2bc2
Cr-Commit-Position: refs/heads/master@{#316124}

[vmpstr, 2015-02-12 19:51:57]

Please take a look.

[danakj, 2015-02-12 21:32:57]

https://codereview.chromium.org/924613002/diff/1/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/924613002/diff/1/cc/resources/tile.h#newcode74
cc/resources/tile.h:74: bool is_occluded_for_tree_priority(TreePriority
tree_priority) const {
no longer used? how come raster iterator doesn't use it?

[vmiura, 2015-02-12 22:18:41]

This seems to have addressed the original bug, so looks good overall.

https://codereview.chromium.org/924613002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/924613002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:535: (current_priority.resolution !=
NON_IDEAL_RESOLUTION) &&
What are the implications of these additional checks?

[vmpstr, 2015-02-12 23:38:19]

PTAL

https://codereview.chromium.org/924613002/diff/1/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/924613002/diff/1/cc/resources/tile.h#newcode74
cc/resources/tile.h:74: bool is_occluded_for_tree_priority(TreePriority
tree_priority) const {
On 2015/02/12 21:32:56, danakj wrote:
> no longer used? how come raster iterator doesn't use it?

In raster, occlusion is done via PLT::IsTileOccluded (occluded tiles are skipped
on a per tree basis).

https://codereview.chromium.org/924613002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/924613002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:535: (current_priority.resolution !=
NON_IDEAL_RESOLUTION) &&
On 2015/02/12 22:18:41, vmiura wrote:
> What are the implications of these additional checks?

This is just making us be able to have more memory for rasterization. Without
this a tile of a non ideal scale can block rasterization of a high res tile for
instance, because we don't have memory... This is kind of an optional change,
but I think it makes it a bit more robust. Let me know if you prefer I did this
separately (or didn't do it at all :) ) (I also changed the condition a bit and
added comments).

[danakj, 2015-02-12 23:48:47]

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

https://codereview.chromium.org/924613002/diff/40001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:542: (eviction_priority.resolution !=
NON_IDEAL_RESOLUTION) &&
I think you should do this separately with a test to demonstrate it.

[danakj, 2015-02-12 23:49:02]

LGTM otherwise

[vmpstr, 2015-02-12 23:51:12]

New patchsets have been uploaded after l-g-t-m from danakj@chromium.org

[vmpstr, 2015-02-12 23:52:34]

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

https://codereview.chromium.org/924613002/diff/40001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:542: (eviction_priority.resolution !=
NON_IDEAL_RESOLUTION) &&
On 2015/02/12 23:48:46, danakj wrote:
> I think you should do this separately with a test to demonstrate it.

OK. Removed from this CL.

[vmiura, 2015-02-12 23:54:50]

LGTM too.

[vmpstr, 2015-02-12 23:55:18]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2015-02-12 23:56:40]

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

[commit-bot: I haz the power, 2015-02-13 01:52:16]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-13 01:52:58]

Patchset 4 (id:??) landed as
https://crrev.com/198f1cd771751f113641219e05d6ecf9f22b2bc2
Cr-Commit-Position: refs/heads/master@{#316124}