Track occlusion per tree on Tile.

Track occlusion per tree on Tile.

Currently, the Tile class has one is_occluded bool. Give Tile two
occluded bools, one for each tree, so that the same tile can have
different occlusion on each tree.

This patch is an alternative to
https://codereview.chromium.org/343463004/ which also tracks occlusion
per tree, but stores occlusion info on TilePriority rather than Tile.

BUG=178971
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281788

[jbedley, 2014-07-07 19:12:25]

PTAL

[danakj, 2014-07-07 19:26:29]

Thanks! Couple things

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

https://codereview.chromium.org/374653003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:886: WhichTree tree =
This method should only be called on the pending tree. There's a DCHECK at the
top of PictureLayerImpl::MarkVisibleResourcesAsRequired as such, so you can
hard-code PENDING_TREE here.

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

https://codereview.chromium.org/374653003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3159: // Unshared tiles should have the
default TilePriority of being
fix up comments to not mention TilePriority

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

https://codereview.chromium.org/374653003/diff/1/cc/resources/tile.h#newcode139
cc/resources/tile.h:139: default:
don't use default: for enums, then we don't get compiler warnings when we add
another value to the enum. Instead, make each case above return, and just
NOTREACHED() below the switch.

https://codereview.chromium.org/374653003/diff/1/cc/resources/tile.h#newcode152
cc/resources/tile.h:152: NOTREACHED();
remove default: move this below the switch

or alternatively you could do is_occluded_[NUM_TREES] and have set/get just do
an array index with the tree variable. (like priority_)

[enne (OOO), 2014-07-07 19:49:31]

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

https://codereview.chromium.org/374653003/diff/1/cc/resources/tile.h#newcode148
cc/resources/tile.h:148: return is_occluded_on_active_tree_;
Should is_occluded just go on TilePriority so you could say
priority_[ACTIVE_TREE].is_occluded?

[danakj, 2014-07-07 20:08:04]

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

https://codereview.chromium.org/374653003/diff/1/cc/resources/tile.h#newcode148
cc/resources/tile.h:148: return is_occluded_on_active_tree_;
On 2014/07/07 19:49:31, enne wrote:
> Should is_occluded just go on TilePriority so you could say
> priority_[ACTIVE_TREE].is_occluded?

FWIW, she did that originally but with vlad's new work this seems a little up in
the air. So in the interest of working parallelly, this adds a second bool while
deferring the decision about priority or not.

[jbedley, 2014-07-07 20:25:14]

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

https://codereview.chromium.org/374653003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:886: WhichTree tree =
On 2014/07/07 19:26:28, danakj wrote:
> This method should only be called on the pending tree. There's a DCHECK at the
> top of PictureLayerImpl::MarkVisibleResourcesAsRequired as such, so you can
> hard-code PENDING_TREE here.

Done.

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

https://codereview.chromium.org/374653003/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl_unittest.cc:3159: // Unshared tiles should have the
default TilePriority of being
On 2014/07/07 19:26:29, danakj wrote:
> fix up comments to not mention TilePriority

Done.

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

https://codereview.chromium.org/374653003/diff/1/cc/resources/tile.h#newcode152
cc/resources/tile.h:152: NOTREACHED();
On 2014/07/07 19:26:29, danakj wrote:
> remove default: move this below the switch
> 
> or alternatively you could do is_occluded_[NUM_TREES] and have set/get just do
> an array index with the tree variable. (like priority_)

Done.

[danakj, 2014-07-07 20:26:56]

LGTM if enne@ is okay with doing this as an incremental step

https://codereview.chromium.org/374653003/diff/20001/cc/resources/tile.cc
File cc/resources/tile.cc (right):

https://codereview.chromium.org/374653003/diff/20001/cc/resources/tile.cc#new...
cc/resources/tile.cc:38: for (int i = 0; i < NUM_TREES; i++)
signed vs unsigned... maybe this works, but you might need size_t to make it
build on windows, i'm not sure. bots will tell us.

[enne (OOO), 2014-07-07 20:29:47]

That's fine as an incremental step.  I was just thinking about what the end
state should look like.

[danakj, 2014-07-07 20:30:41]

On 2014/07/07 20:29:47, enne wrote:
> That's fine as an incremental step.  I was just thinking about what the end
> state should look like.

Ok cool, we need quantum bits to see that right now I think..

[danakj, 2014-07-07 20:39:17]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-07-07 20:40:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbedley@chromium.org/374653003/20001

[commit-bot: I haz the power, 2014-07-08 02:06:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[commit-bot: I haz the power, 2014-07-08 02:30:53]

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

[commit-bot: I haz the power, 2014-07-08 02:30:54]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)

[jbedley, 2014-07-08 15:09:07]

The CQ bit was checked by jbedley@chromium.org

[commit-bot: I haz the power, 2014-07-08 15:11:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbedley@chromium.org/374653003/40001

[commit-bot: I haz the power, 2014-07-08 19:19:33]

Change committed as 281788