cc: Fix lost context handling when using impl-side painting.

cc: Fix lost context handling when using impl-side painting.

Make sure all tile references are dropped from active and pending
tree before the tile manager is destroyed. Also call
CheckForCompletedSetPixels() in tile manager destructor to ensure
all tiles with pending set pixels are deleted.

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

[enne (OOO), 2013-01-15 03:36:25]

How hard would it be to add PictureLayerImpl to the lost context unit tests?

https://codereview.chromium.org/11883030/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11883030/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:974: m_pendingTree.reset();
You should apply the if statement above on line 969 to both the active and
pending tree.  The pending tree can still have non-resource data coming from the
main thread that shouldn't be thrown away.

https://codereview.chromium.org/11883030/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1007:
m_client->onHasPendingTreeStateChanged(pendingTree());
This can go away too.

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

https://codereview.chromium.org/11883030/diff/1/cc/tile_manager.cc#newcode93
cc/tile_manager.cc:93: DCHECK(tiles_.size() == 0);
Can you DCHECK(tiles_with_pending_set_pixels_.size() == 0) here too? I know it's
obvious now, but somebody might change this later.

[reveman, 2013-01-15 04:05:13]

I'll look into adding a unit test tomorrow.

https://codereview.chromium.org/11883030/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11883030/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:974: m_pendingTree.reset();
On 2013/01/15 03:36:25, enne wrote:
> You should apply the if statement above on line 969 to both the active and
> pending tree.  The pending tree can still have non-resource data coming from
the
> main thread that shouldn't be thrown away.

Done.

https://codereview.chromium.org/11883030/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:1007:
m_client->onHasPendingTreeStateChanged(pendingTree());
On 2013/01/15 03:36:25, enne wrote:
> This can go away too.

Done.

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

https://codereview.chromium.org/11883030/diff/1/cc/tile_manager.cc#newcode93
cc/tile_manager.cc:93: DCHECK(tiles_.size() == 0);
On 2013/01/15 03:36:25, enne wrote:
> Can you DCHECK(tiles_with_pending_set_pixels_.size() == 0) here too? I know
it's
> obvious now, but somebody might change this later.

Done.

[reveman, 2013-01-15 22:31:17]

Added a unit test to latest patch. PTAL.

[enne (OOO), 2013-01-15 22:35:41]

https://codereview.chromium.org/11883030/diff/7001/cc/layer_tree_host_unittes...
File cc/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/11883030/diff/7001/cc/layer_tree_host_unittes...
cc/layer_tree_host_unittest_context.cc:816: class
ImplSidePaintingLayerTreeHostContextTest
How would this test fail before your patch?

I was wondering if this could go in
LayerTreeHostContextTestDontUseLostResources.

[reveman, 2013-01-15 23:16:25]

https://codereview.chromium.org/11883030/diff/7001/cc/layer_tree_host_unittes...
File cc/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/11883030/diff/7001/cc/layer_tree_host_unittes...
cc/layer_tree_host_unittest_context.cc:816: class
ImplSidePaintingLayerTreeHostContextTest
On 2013/01/15 22:35:41, enne wrote:
> How would this test fail before your patch?

The DCHECK(tiles_.size() == 0) in the tile manager the destructor fails.

> 
> I was wondering if this could go in
> LayerTreeHostContextTestDontUseLostResources.

The only reason I didn't just include a picture layer in that test is that it's
run both in single and multi threaded mode. I could make some changes that
allows me to subclass and reuse that test for impl side painting in mult thread
mode only if you prefer that over adding a new test like my current patch is
doing?

[enne (OOO), 2013-01-15 23:17:43]

Thanks for the explanation.  Both of those make a lot of sense.

lgtm

[commit-bot: I haz the power, 2013-01-15 23:37:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11883030/7001

[commit-bot: I haz the power, 2013-01-16 02:19:42]

Change committed as 177056