cc: Remove TextureLayer::SetTextureId and TextureLayer::WillModifyTexture

cc: Remove TextureLayer::SetTextureId and TextureLayer::WillModifyTexture

These are not used anymore except for clearing the client-provided texture, so
this removes 1 of 4 TextureLayer paths, and simplifies externally-exposed state.

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

[piman, 2014-04-01 05:06:28]

danakj: please review. This is essentially a rebase of
https://chromiumcodereview.appspot.com/19303003

enne: OWNERs for webkit/renderer/compositor_bindings

[danakj, 2014-04-01 15:03:36]

Yay, thanks!

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:60: } else {
else if (texture_id_) ?

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer.h
File cc/layers/texture_layer.h (right):

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer....
cc/layers/texture_layer.h:95: // client. After this call it is safe to destroy
the texture / mailbox.
This isn't quite true is it? You need to wait for the ReleaseCallback still or
you risk killing the texture just before it's drawn. e.g. it could be in flight
to the browser.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer....
cc/layers/texture_layer.h:100: // it is safe to destroy the texture / mailbox.
This isn't quite true is it? You need to wait for the ReleaseCallback still or
you risk killing the texture just before it's drawn.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:211:
Mock::VerifyAndClearExpectations(layer_tree_host_.get());
Can you verify before the Composite() call?

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:229:
Mock::VerifyAndClearExpectations(layer_tree_host_.get());
can you verify before Composite()?

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:235:
Mock::VerifyAndClearExpectations(layer_tree_host_.get());
and here?

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:273:
Mock::VerifyAndClearExpectations(layer_tree_host_.get());
and here? .. and the rest

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:281: // But only once.
It didn't sync already, so not sure what this case is for, at least the comment
needs update.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:294: // Commit will return mailbox1.
should we commit earlier to show that the ClearTexture() will return the mailbox
on next commit?

[enne (OOO), 2014-04-01 17:28:03]

webkit/compositor_bindings/ rslgtm

[piman, 2014-04-01 20:21:10]

Addressed review comments, but this CL isn't enough for Android which introduced
some more SetTextureId cases. So hold on for an update after I figure out
whether we should rip it out or go through either the mailbox or the
client+texture path.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer....
cc/layers/texture_layer.cc:60: } else {
On 2014/04/01 15:03:37, danakj wrote:
> else if (texture_id_) ?

Done.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer.h
File cc/layers/texture_layer.h (right):

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer....
cc/layers/texture_layer.h:95: // client. After this call it is safe to destroy
the texture / mailbox.
On 2014/04/01 15:03:37, danakj wrote:
> This isn't quite true is it? You need to wait for the ReleaseCallback still or
> you risk killing the texture just before it's drawn. e.g. it could be in
flight
> to the browser.

Yeah, we've stopped doing the AcquireLayerTextures for mailboxes a while ago.
Updated comments here and below.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer....
cc/layers/texture_layer.h:100: // it is safe to destroy the texture / mailbox.
On 2014/04/01 15:03:37, danakj wrote:
> This isn't quite true is it? You need to wait for the ReleaseCallback still or
> you risk killing the texture just before it's drawn.

Done.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
File cc/layers/texture_layer_unittest.cc (right):

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:211:
Mock::VerifyAndClearExpectations(layer_tree_host_.get());
On 2014/04/01 15:03:37, danakj wrote:
> Can you verify before the Composite() call?

Done.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:229:
Mock::VerifyAndClearExpectations(layer_tree_host_.get());
On 2014/04/01 15:03:37, danakj wrote:
> can you verify before Composite()?

Done.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:235:
Mock::VerifyAndClearExpectations(layer_tree_host_.get());
On 2014/04/01 15:03:37, danakj wrote:
> and here?

Done.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:273:
Mock::VerifyAndClearExpectations(layer_tree_host_.get());
On 2014/04/01 15:03:37, danakj wrote:
> and here? .. and the rest

Done.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:281: // But only once.
On 2014/04/01 15:03:37, danakj wrote:
> It didn't sync already, so not sure what this case is for, at least the
comment
> needs update.

Yeah, copy&paste and fixup didn't make sense here. Removed this case.

https://codereview.chromium.org/213743005/diff/20001/cc/layers/texture_layer_...
cc/layers/texture_layer_unittest.cc:294: // Commit will return mailbox1.
On 2014/04/01 15:03:37, danakj wrote:
> should we commit earlier to show that the ClearTexture() will return the
mailbox
> on next commit?

Done.

[piman, 2014-04-01 23:59:29]

We're removing the non-ubercomp path on Android in
https://codereview.chromium.org/219963015/, so rebased on top of that (also
fixed a bug in the test).

[danakj, 2014-04-02 14:47:37]

LGTM w/ 1 more thing

https://codereview.chromium.org/213743005/diff/60001/cc/layers/texture_layer.h
File cc/layers/texture_layer.h (right):

https://codereview.chromium.org/213743005/diff/60001/cc/layers/texture_layer....
cc/layers/texture_layer.h:95: // this call it is safe to destroy the texture.
Ah, I see what you changed here, but to me it reads I can destroy the texture I
am holding to back the mailbox, which is not correct. Can we clarify a bit
futher that "the texture" means for the texture id path not for mailbox path?

[piman, 2014-04-02 22:11:10]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-04-02 22:11:32]

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

[piman, 2014-04-02 22:11:36]

https://codereview.chromium.org/213743005/diff/60001/cc/layers/texture_layer.h
File cc/layers/texture_layer.h (right):

https://codereview.chromium.org/213743005/diff/60001/cc/layers/texture_layer....
cc/layers/texture_layer.h:95: // this call it is safe to destroy the texture.
On 2014/04/02 14:47:37, danakj wrote:
> Ah, I see what you changed here, but to me it reads I can destroy the texture
I
> am holding to back the mailbox, which is not correct. Can we clarify a bit
> futher that "the texture" means for the texture id path not for mailbox path?

Done.

[danakj, 2014-04-02 22:35:39]

On Wed, Apr 2, 2014 at 6:11 PM, <piman@chromium.org> wrote:

>
> https://codereview.chromium.org/213743005/diff/60001/cc/
> layers/texture_layer.h
> File cc/layers/texture_layer.h (right):
>
> https://codereview.chromium.org/213743005/diff/60001/cc/
> layers/texture_layer.h#newcode95
> cc/layers/texture_layer.h:95: // this call it is safe to destroy the
> texture.
> On 2014/04/02 14:47:37, danakj wrote:
>
>> Ah, I see what you changed here, but to me it reads I can destroy the
>>
> texture I
>
>> am holding to back the mailbox, which is not correct. Can we clarify a
>>
> bit
>
>> futher that "the texture" means for the texture id path not for
>>
> mailbox path?
>
> Done.
>

Thanks!


>
> https://codereview.chromium.org/213743005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-04-03 08:37:20]

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

[commit-bot: I haz the power, 2014-04-03 08:37:20]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[danakj, 2014-04-03 14:25:55]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-04-03 14:26:04]

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

[commit-bot: I haz the power, 2014-04-03 14:40:59]

Change committed as 261380