cc: Separate the priority from the tile and put in new PrioritizedTile

cc: Separate the priority from the tile and put in new PrioritizedTile

Move priority and is_occluded from Tile to new PrioritizedTile class.

Logged https://code.google.com/p/chromium/issues/detail?id=485705 for the traceviewer change

BUG=483293
Committed: https://crrev.com/8cc26dde493418fdc6b532ad1641f55bfb65815e
Cr-Commit-Position: refs/heads/master@{#329318}

[hendrikw, 2015-05-07 20:24:09]

hendrikw@chromium.org changed reviewers:
+ vmpstr@chromium.org

[hendrikw, 2015-05-07 20:24:10]

PTAL, thanks!

[vmpstr, 2015-05-08 18:11:06]

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

https://codereview.chromium.org/1130123003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1760: PrioritizedTile prioritized_tile
= queue->Top();
minor (because this is test) nit: const ref

https://codereview.chromium.org/1130123003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2074:
pending_layer_->HighResTiling()->UpdateAndGetAllPrioritizedTilesForTesting();
I think what you're using here is required_for_activation and required_for_draw
flags on tile. Can you add a separate "void UpdateAllRequiredStateForTesting()"
function that would do this?

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:813: void
PictureLayerTiling::UpdateTilePriority(
If you do go the route of GetPrioritizedTile(tile), then I think you would also
want to have a separate call for UpdateRequiredStateForTile(tile) that changes
the required_for_* flags.

Yes, it's going to be two calls, but it's more explicit.

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:825: for (auto& key_tile_pair : tiles_) {
const auto&?

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/prioritize...
File cc/resources/prioritized_tile.h (right):

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/prioritize...
cc/resources/prioritized_tile.h:28: void Invalidate() { tile_ = nullptr; }
This is a bit of an overused name. Can we name this something else?

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

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:274: for (auto tile : released_tiles_)
nit: auto*

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:518: const TileDrawInfo& draw_info =
tile->draw_info();
Since this is only used in the dcheck below, can you just make the dcheck use
tile->draw_info() instead?

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tile_manag...
File cc/resources/tile_manager_perftest.cc (right):

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tile_manag...
cc/resources/tile_manager_perftest.cc:299: ASSERT_TRUE(queue->Top().tile() !=
nullptr);
nit: Just ASSERT_TRUE(queue->Top().tile());

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tiling_set...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tiling_set...
cc/resources/tiling_set_eviction_queue.cc:226:
prioritized_tile_.UpdateTile(tile, (*tilings_)[tiling_index_]);
I find the fact that prioritized_tile is a self modifying thing a bit awkward. I
think PrioritizedTile should be const for all intents and purposes. This line
should be something like:

prioritized_tile_ = (*tilings_)[tiling_index_]->GetPrioritizedTile(tile);

In fact, I think it's possible that prioritized_tile_ only exists on the top
level iterator here, and each of the piecewise iterators just acts on a tile.
The only change that needs to happen there is TileMatchesRequiredFlags, since
the priority isn't updated, we would have to use the tiling IsTileRequiredFor*
functionality.

Maybe that can be a follow up to ensure that we don't miss some case that is
handled right now.

[hendrikw, 2015-05-08 19:21:03]

PTAL

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

https://codereview.chromium.org/1130123003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:1760: PrioritizedTile prioritized_tile
= queue->Top();
On 2015/05/08 18:11:06, vmpstr wrote:
> minor (because this is test) nit: const ref

Done.

https://codereview.chromium.org/1130123003/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:2074:
pending_layer_->HighResTiling()->UpdateAndGetAllPrioritizedTilesForTesting();
On 2015/05/08 18:11:06, vmpstr wrote:
> I think what you're using here is required_for_activation and
required_for_draw
> flags on tile. Can you add a separate "void
UpdateAllRequiredStateForTesting()"
> function that would do this?

Done.

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:813: void
PictureLayerTiling::UpdateTilePriority(
On 2015/05/08 18:11:06, vmpstr wrote:
> If you do go the route of GetPrioritizedTile(tile), then I think you would
also
> want to have a separate call for UpdateRequiredStateForTile(tile) that changes
> the required_for_* flags.
> 
> Yes, it's going to be two calls, but it's more explicit.

messier, dangerous, but I like to live dangerously.  hopefully we can move the
required stuff within the prioritized tile.

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:825: for (auto& key_tile_pair : tiles_) {
On 2015/05/08 18:11:06, vmpstr wrote:
> const auto&?

Done.

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/prioritize...
File cc/resources/prioritized_tile.h (right):

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/prioritize...
cc/resources/prioritized_tile.h:28: void Invalidate() { tile_ = nullptr; }
On 2015/05/08 18:11:06, vmpstr wrote:
> This is a bit of an overused name. Can we name this something else?

Done.

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

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:274: for (auto tile : released_tiles_)
On 2015/05/08 18:11:06, vmpstr wrote:
> nit: auto*

Ugh, done.

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:518: const TileDrawInfo& draw_info =
tile->draw_info();
On 2015/05/08 18:11:06, vmpstr wrote:
> Since this is only used in the dcheck below, can you just make the dcheck use
> tile->draw_info() instead?

Done.

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tile_manag...
File cc/resources/tile_manager_perftest.cc (right):

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tile_manag...
cc/resources/tile_manager_perftest.cc:299: ASSERT_TRUE(queue->Top().tile() !=
nullptr);
On 2015/05/08 18:11:06, vmpstr wrote:
> nit: Just ASSERT_TRUE(queue->Top().tile()); 

Done.

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tiling_set...
File cc/resources/tiling_set_eviction_queue.cc (right):

https://codereview.chromium.org/1130123003/diff/40001/cc/resources/tiling_set...
cc/resources/tiling_set_eviction_queue.cc:226:
prioritized_tile_.UpdateTile(tile, (*tilings_)[tiling_index_]);
On 2015/05/08 18:11:06, vmpstr wrote:
> I find the fact that prioritized_tile is a self modifying thing a bit awkward.
I
> think PrioritizedTile should be const for all intents and purposes. This line
> should be something like:
> 
> prioritized_tile_ = (*tilings_)[tiling_index_]->GetPrioritizedTile(tile);
> 
> In fact, I think it's possible that prioritized_tile_ only exists on the top
> level iterator here, and each of the piecewise iterators just acts on a tile.
> The only change that needs to happen there is TileMatchesRequiredFlags, since
> the priority isn't updated, we would have to use the tiling IsTileRequiredFor*
> functionality.
> 
> Maybe that can be a follow up to ensure that we don't miss some case that is
> handled right now.

ok, MakePrioritizedTile (not Get, Get implies that it exists within the class
already :D)

I'll look into using tile within the nested classes in a followup.

[vmpstr, 2015-05-11 16:44:44]

https://codereview.chromium.org/1130123003/diff/60001/cc/debug/frame_viewer_i...
File cc/debug/frame_viewer_instrumentation.h (right):

https://codereview.chromium.org/1130123003/diff/60001/cc/debug/frame_viewer_i...
cc/debug/frame_viewer_instrumentation.h:10: #include
"cc/resources/tile_priority.h"
Is this for TileResolution?

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:814: if (tile) {
This should be a dcheck and checked by the caller instead

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/prioritize...
File cc/resources/prioritized_tile.h (right):

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/prioritize...
cc/resources/prioritized_tile.h:15: class CC_EXPORT PrioritizedTile {
Can you leave a brief comment here about the usage of this class?

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

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:692: tile->contents_scale(),
prioritized_tile.priority().resolution,
Can you see what the resolution is used for? Or whether it is used? I was hoping
to just pass Tile* to CreateRasterTask

[hendrikw, 2015-05-11 17:38:30]

https://codereview.chromium.org/1130123003/diff/60001/cc/debug/frame_viewer_i...
File cc/debug/frame_viewer_instrumentation.h (right):

https://codereview.chromium.org/1130123003/diff/60001/cc/debug/frame_viewer_i...
cc/debug/frame_viewer_instrumentation.h:10: #include
"cc/resources/tile_priority.h"
On 2015/05/11 16:44:44, vmpstr wrote:
> Is this for TileResolution?

yes

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:814: if (tile) {
On 2015/05/11 16:44:44, vmpstr wrote:
> This should be a dcheck and checked by the caller instead

Done.

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/prioritize...
File cc/resources/prioritized_tile.h (right):

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/prioritize...
cc/resources/prioritized_tile.h:15: class CC_EXPORT PrioritizedTile {
On 2015/05/11 16:44:44, vmpstr wrote:
> Can you leave a brief comment here about the usage of this class?

Done.

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

https://codereview.chromium.org/1130123003/diff/60001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:692: tile->contents_scale(),
prioritized_tile.priority().resolution,
On 2015/05/11 16:44:44, vmpstr wrote:
> Can you see what the resolution is used for? Or whether it is used? I was
hoping
> to just pass Tile* to CreateRasterTask

it's passed to Analyze and Raster, which passes it to ScopedAnalyzeTask and
ScopedRasterTask, when pass it to a trace event.

It can be removed, but you'll lose it from the trace (which currently reports
the tile id, resolution, source frame number and layer it.

[vmpstr, 2015-05-11 23:44:28]

lgtm (% some comments that we discussed offline)

https://codereview.chromium.org/1130123003/diff/80001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling.cc (right):

https://codereview.chromium.org/1130123003/diff/80001/cc/resources/picture_la...
cc/resources/picture_layer_tiling.cc:832:
result.insert(std::make_pair(prioritized_tile.tile(), prioritized_tile));
This doesn't really matter in test, but this incurs an extra copy of the pair.

[hendrikw, 2015-05-12 00:15:28]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-05-12 00:15:29]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1130123003/#ps100001
(title: "move comment etc.")

[commit-bot: I haz the power, 2015-05-12 00:20:18]

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

[commit-bot: I haz the power, 2015-05-12 00:28:37]

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

[commit-bot: I haz the power, 2015-05-12 00:28:38]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hendrikw, 2015-05-12 00:59:13]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-05-12 00:59:14]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1130123003/#ps140001
(title: "merge")

[commit-bot: I haz the power, 2015-05-12 01:01:31]

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

[commit-bot: I haz the power, 2015-05-12 02:12:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-05-12 02:14:07]

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