cc: Don't activate rasterize on demand when we have 0 memory.

cc: Don't activate rasterize on demand when we have 0 memory.

This patch partially fixes checkerboard on rapid tab switch problem.
When we become invisible we mark the active tree as requiring high
res content to draw, and manage tiles to evict resources. However,
manage tiles will end up scheduling an activation with rasterize on
demand since we couldn't schedule anything. This will end up activating
and clearing the require high res flag. This fixes it by not activating
or scheduling activation when we have no memory.

R=reveman
BUG=415760
Committed: https://crrev.com/23da7a6a68df0c0a0676b763cc84451d94cd9028
Cr-Commit-Position: refs/heads/master@{#296315}

[vmpstr, 2014-09-22 23:49:07]

Please take a look. We could also just return if we have 0 memory, but I think
we are still good to activate if for instance all tiles are solid (and are ready
to draw), since they use no memory.

[reveman, 2014-09-23 02:38:54]

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

https://codereview.chromium.org/594703002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:515: // invisible), if we have tiles that aren't
ready, then we shouldn't
Why is 0 the magic number here? Seems wrong to assume that memory limit is 0
when invisible. What if it's 1 byte or the size of 5 tiles?

Would it be better to not allow rasterize-on-demand unless visible?

[vmpstr, 2014-09-23 15:29:23]

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

https://codereview.chromium.org/594703002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:515: // invisible), if we have tiles that aren't
ready, then we shouldn't
On 2014/09/23 02:38:54, reveman wrote:
> Why is 0 the magic number here? Seems wrong to assume that memory limit is 0
> when invisible. What if it's 1 byte or the size of 5 tiles?
> 
> Would it be better to not allow rasterize-on-demand unless visible?

Good point. Do you think we should make it a part of global_state_ or pass it to
ManageTiles as a separate variable? I don't really think adding another client
function is appropriate here since we can just directly pass the value.

[reveman, 2014-09-23 16:39:11]

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

https://codereview.chromium.org/594703002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:515: // invisible), if we have tiles that aren't
ready, then we shouldn't
On 2014/09/23 15:29:22, vmpstr wrote:
> On 2014/09/23 02:38:54, reveman wrote:
> > Why is 0 the magic number here? Seems wrong to assume that memory limit is 0
> > when invisible. What if it's 1 byte or the size of 5 tiles?
> > 
> > Would it be better to not allow rasterize-on-demand unless visible?
> 
> Good point. Do you think we should make it a part of global_state_ or pass it
to
> ManageTiles as a separate variable? I don't really think adding another client
> function is appropriate here since we can just directly pass the value.

Maybe it's fine to just use "global_state_.memory_limit_policy != ALLOW_NOTHING"
here if we just update the description in tile_priority.h to make it more
explicit that this policy is always used when invisible. Wdyt?

[vmpstr, 2014-09-23 17:25:32]

PTAL. I'll work on a test.

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

https://codereview.chromium.org/594703002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:515: // invisible), if we have tiles that aren't
ready, then we shouldn't
On 2014/09/23 16:39:11, reveman wrote:
> On 2014/09/23 15:29:22, vmpstr wrote:
> > On 2014/09/23 02:38:54, reveman wrote:
> > > Why is 0 the magic number here? Seems wrong to assume that memory limit is
0
> > > when invisible. What if it's 1 byte or the size of 5 tiles?
> > > 
> > > Would it be better to not allow rasterize-on-demand unless visible?
> > 
> > Good point. Do you think we should make it a part of global_state_ or pass
it
> to
> > ManageTiles as a separate variable? I don't really think adding another
client
> > function is appropriate here since we can just directly pass the value.
> 
> Maybe it's fine to just use "global_state_.memory_limit_policy !=
ALLOW_NOTHING"
> here if we just update the description in tile_priority.h to make it more
> explicit that this policy is always used when invisible. Wdyt?

Done.

https://codereview.chromium.org/594703002/diff/20001/cc/resources/tile_priori...
File cc/resources/tile_priority.h (left):

https://codereview.chromium.org/594703002/diff/20001/cc/resources/tile_priori...
cc/resources/tile_priority.h:124: 
FYI, this is a git cl format change.

[reveman, 2014-09-23 18:47:24]

lgtm after adding a test

[danakj, 2014-09-23 20:55:28]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-09-23 20:55:29]

Thanks for the test! It looks great, with a couple comments.

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5047:
layer_tree_host()->SetViewportSize(gfx::Size(1000, 1000));
the base class does this for you based on the root layer size and the DSF

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5059: PostSetVisibleToMainThread(false);
you're on the main thread, you can layer_tree_host()->SetVisible(false) to
remove some indirection and a post-task, if that doesn't break the test i think
it's preferable

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5074:
EXPECT_NE(ManagedTileState::TileVersion::PICTURE_PILE_MODE,
can you EXPECT that some tiles are RESOURCE mode (or all tiles, maybe by
changing how "fill with non solid color" works) cuz all-solid is able to
activate differently.

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5078: // We can finish the test now.
remove, this doesn't explain anything

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5101:
MULTI_THREAD_TEST_F(LayerTreeHostTestInvisibleDoesntActivate);
Can you TODO to enable with single thread impl-side painting?

[vmpstr, 2014-09-23 21:39:32]

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5047:
layer_tree_host()->SetViewportSize(gfx::Size(1000, 1000));
On 2014/09/23 20:55:29, danakj wrote:
> the base class does this for you based on the root layer size and the DSF

Done.

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5059: PostSetVisibleToMainThread(false);
On 2014/09/23 20:55:29, danakj wrote:
> you're on the main thread, you can layer_tree_host()->SetVisible(false) to
> remove some indirection and a post-task, if that doesn't break the test i
think
> it's preferable

Done.

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5074:
EXPECT_NE(ManagedTileState::TileVersion::PICTURE_PILE_MODE,
On 2014/09/23 20:55:28, danakj wrote:
> can you EXPECT that some tiles are RESOURCE mode (or all tiles, maybe by
> changing how "fill with non solid color" works) cuz all-solid is able to
> activate differently.

Done.

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5078: // We can finish the test now.
On 2014/09/23 20:55:28, danakj wrote:
> remove, this doesn't explain anything

Done.

https://codereview.chromium.org/594703002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5101:
MULTI_THREAD_TEST_F(LayerTreeHostTestInvisibleDoesntActivate);
On 2014/09/23 20:55:28, danakj wrote:
> Can you TODO to enable with single thread impl-side painting?

Done.

[danakj, 2014-09-23 21:42:10]

https://codereview.chromium.org/594703002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/594703002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5055: virtual void DidCommit() OVERRIDE {
layer_tree_host()->SetVisible(false); }
i really liked your comment explaining about the pending tree being created here
but not activated yet.

would it help to EXPECT the presence of the pending tree to make things clear?
might that be racey with DidCommit?

[vmpstr, 2014-09-23 22:31:50]

ptal

https://codereview.chromium.org/594703002/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/594703002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5055: virtual void DidCommit() OVERRIDE {
layer_tree_host()->SetVisible(false); }
On 2014/09/23 21:42:10, danakj wrote:
> i really liked your comment explaining about the pending tree being created
here
> but not activated yet.
> 
> would it help to EXPECT the presence of the pending tree to make things clear?
> might that be racey with DidCommit?

There shouldn't be races here.. However, I did a block activation until we
become invisible to be sure.

[danakj, 2014-09-23 22:45:10]

LGTM

https://codereview.chromium.org/594703002/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/594703002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5055: virtual void DidCommit() OVERRIDE {
layer_tree_host()->SetVisible(false); }
nit: move this below BeginCommitOnThread since it happens later in the test

https://codereview.chromium.org/594703002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5085: virtual void
DidSetVisibleOnImplTree(LayerTreeHostImpl* host_impl,
nit: move this above DidActivate

[vmpstr, 2014-09-23 22:53:33]

https://codereview.chromium.org/594703002/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/594703002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5055: virtual void DidCommit() OVERRIDE {
layer_tree_host()->SetVisible(false); }
On 2014/09/23 22:45:10, danakj wrote:
> nit: move this below BeginCommitOnThread since it happens later in the test

Done.

https://codereview.chromium.org/594703002/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:5085: virtual void
DidSetVisibleOnImplTree(LayerTreeHostImpl* host_impl,
On 2014/09/23 22:45:10, danakj wrote:
> nit: move this above DidActivate

Done.

[vmpstr, 2014-09-23 22:53:39]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-09-23 22:54:19]

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

[commit-bot: I haz the power, 2014-09-24 00:24:55]

Committed patchset #7 (id:120001) as fc1338fa29fb678d141777235886faa45d737320

[commit-bot: I haz the power, 2014-09-24 00:25:28]

Patchset 7 (id:??) landed as
https://crrev.com/23da7a6a68df0c0a0676b763cc84451d94cd9028
Cr-Commit-Position: refs/heads/master@{#296315}