TextureLayer: clear texture id when clearing client

TextureLayer: clear texture id when clearing client.

This is to avoid having the impl side accessing a texture that's been deleted.

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

[danakj, 2013-03-28 00:04:52]

Can you explain the problem this fixes in the description?

[danakj, 2013-03-28 00:05:03]

Also, unit test?

[piman, 2013-03-28 01:18:03]

On 2013/03/28 00:05:03, danakj wrote:
> Also, unit test?

Done, and Done.

[danakj, 2013-03-28 01:44:43]

Thanks! LGTM with one question about the test.

https://codereview.chromium.org/13126002/diff/7001/cc/layers/texture_layer_un...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/13126002/diff/7001/cc/layers/texture_layer_un...
cc/layers/texture_layer_unittest.cc:532: // We want to delete the texture, so
that if the layer still uses it
The test looks good, but would it be more straightforward to check the number of
textures used by drawing? ie TestWGC3D::NumUsedTextures() == 0. Counting on a
dcheck without testing that the dcheck exists (of course not possible) seems a
bit less reliable.

[piman, 2013-03-28 03:11:35]

PTAL

https://codereview.chromium.org/13126002/diff/7001/cc/layers/texture_layer_un...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/13126002/diff/7001/cc/layers/texture_layer_un...
cc/layers/texture_layer_unittest.cc:532: // We want to delete the texture, so
that if the layer still uses it
On 2013/03/28 01:44:44, danakj wrote:
> The test looks good, but would it be more straightforward to check the number
of
> textures used by drawing? ie TestWGC3D::NumUsedTextures() == 0. Counting on a
> dcheck without testing that the dcheck exists (of course not possible) seems a
> bit less reliable.

Fair enough, I tried it on the new patch. It requires a bit more plumbing to
have the correct count passed through the commit/draw flow, but it seems fairly
reliable this way.

[danakj, 2013-03-28 03:57:07]

LGTM!

https://codereview.chromium.org/13126002/diff/15001/cc/layers/texture_layer_u...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/13126002/diff/15001/cc/layers/texture_layer_u...
cc/layers/texture_layer_unittest.cc:536: texture_layer_->ClearClient();
This should also damage the layer so that it will try to redraw, otherwise we
may not draw anything. Calling SetNeedsDisplay() on the layer should do the
trick!

[danakj, 2013-03-28 04:13:55]

https://codereview.chromium.org/13126002/diff/15001/cc/layers/texture_layer_u...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/13126002/diff/15001/cc/layers/texture_layer_u...
cc/layers/texture_layer_unittest.cc:565: bool result) OVERRIDE {
Alternatively, or additionally, EXPECT_TRUE(result) would help guard to make
sure we tried to draw. (And would fail with my CL to drop draw+swap when there's
no damage.)

[piman, 2013-03-28 21:16:19]

https://codereview.chromium.org/13126002/diff/15001/cc/layers/texture_layer_u...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/13126002/diff/15001/cc/layers/texture_layer_u...
cc/layers/texture_layer_unittest.cc:536: texture_layer_->ClearClient();
On 2013/03/28 03:57:07, danakj wrote:
> This should also damage the layer so that it will try to redraw, otherwise we
> may not draw anything. Calling SetNeedsDisplay() on the layer should do the
> trick!

Done.

https://codereview.chromium.org/13126002/diff/15001/cc/layers/texture_layer_u...
cc/layers/texture_layer_unittest.cc:565: bool result) OVERRIDE {
On 2013/03/28 04:13:55, danakj wrote:
> Alternatively, or additionally, EXPECT_TRUE(result) would help guard to make
> sure we tried to draw. (And would fail with my CL to drop draw+swap when
there's
> no damage.)

Done.

[commit-bot: I haz the power, 2013-03-28 21:19:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/13126002/27001

[commit-bot: I haz the power, 2013-03-29 02:27:55]

Change committed as 191268