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 implifies externally-exposed state.

BUG=None

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

[piman, 2013-07-15 23:16:48]

danakj: please review
jamesr: webkit/renderer/compositor_bindings/web_external_texture_layer_impl.cc
sievers: content/browser/renderer_host/render_widget_host_view_android.cc

Needs https://codereview.chromium.org/18796008/ for the tests.

[jamesr, 2013-07-15 23:17:41]

lgtm

[danakj, 2013-07-15 23:25:05]

LGTM

https://codereview.chromium.org/19303003/diff/1/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/19303003/diff/1/cc/layers/texture_layer.cc#ne...
cc/layers/texture_layer.cc:46: }
Should we ClearClient() in here? Or DCHECK that it was already done?

[no sievers, 2013-07-15 23:28:42]

On 2013/07/15 23:16:48, piman wrote:
> danakj: please review
> jamesr: webkit/renderer/compositor_bindings/web_external_texture_layer_impl.cc
> sievers: content/browser/renderer_host/render_widget_host_view_android.cc
> 

lgtm

> Needs https://codereview.chromium.org/18796008/ for the tests.

[piman, 2013-07-16 00:03:11]

https://codereview.chromium.org/19303003/diff/1/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/19303003/diff/1/cc/layers/texture_layer.cc#ne...
cc/layers/texture_layer.cc:46: }
On 2013/07/15 23:25:05, danakj wrote:
> Should we ClearClient() in here? Or DCHECK that it was already done?

We don't need to, we know we've been removed from the tree, which means we'll
have called SetLayerTreeHost(NULL) (asserted in layer.cc) which will have sync'd
with the impl thread.

[danakj, 2013-07-16 00:03:53]

https://codereview.chromium.org/19303003/diff/1/cc/layers/texture_layer.cc
File cc/layers/texture_layer.cc (right):

https://codereview.chromium.org/19303003/diff/1/cc/layers/texture_layer.cc#ne...
cc/layers/texture_layer.cc:46: }
On 2013/07/16 00:03:12, piman wrote:
> On 2013/07/15 23:25:05, danakj wrote:
> > Should we ClearClient() in here? Or DCHECK that it was already done?
> 
> We don't need to, we know we've been removed from the tree, which means we'll
> have called SetLayerTreeHost(NULL) (asserted in layer.cc) which will have
sync'd
> with the impl thread.

Ah, okay cool thanks.

[commit-bot: I haz the power, 2013-07-17 01:16:25]

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

[commit-bot: I haz the power, 2013-07-17 03:26:35]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-17 04:13:56]

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

[commit-bot: I haz the power, 2013-07-17 09:24:32]

Change committed as 212008

[jdduke (slow), 2013-07-17 15:11:04]

[+clholgat, +aelias, +dtrainor]

Chrome for Android makes use of TextureLayer::SetTextureId for most of its
custom layer types.  crbug.com/239568 and crbug.com/235290 track movement away
from this dependency, but I'm not sure how much progress has been made? 
aelias@, what about creating an intermediate layer type that provides this
functionality, separate from TextureLayer? This is blocking all Chromium rolls
at the moment.

[clholgat_google.com, 2013-07-17 16:40:37]

The old way is deprecated. The new way is not yet ready.

I think there are two ways we can handle this, we can keep a copy of the
old TextureLayer in our tree or we can switch everything back to
ImageLayers temporarily until the new shared resource manager (
https://chromiumcodereview.appspot.com/18191020/) is ready.


On Wed, Jul 17, 2013 at 8:11 AM, <jdduke@chromium.org> wrote:

> [+clholgat, +aelias, +dtrainor]
>
> Chrome for Android makes use of TextureLayer::SetTextureId for most of its
> custom layer types.  crbug.com/239568 and crbug.com/235290 track movement
> away
> from this dependency, but I'm not sure how much progress has been made?
> aelias@, what about creating an intermediate layer type that provides this
> functionality, separate from TextureLayer? This is blocking all Chromium
> rolls
> at the moment.
>
>
https://chromiumcodereview.**appspot.com/19303003/<https://chromiumcodereview...
>

[danakj, 2013-07-17 16:45:01]

On Wed, Jul 17, 2013 at 9:40 AM, Carson Holgate <clholgat@google.com> wrote:

> The old way is deprecated. The new way is not yet ready.
>
> I think there are two ways we can handle this, we can keep a copy of the
> old TextureLayer in our tree or we can switch everything back to
> ImageLayers temporarily until the new shared resource manager (
> https://chromiumcodereview.appspot.com/18191020/) is ready.
>

Without knowing much about the private android code.. would it be possible
to write a simple TextureLayerClient that mailboxes and provides the
textures to the TextureLayer and put a SetTextureId() function on that
thing instead?


>
>
> On Wed, Jul 17, 2013 at 8:11 AM, <jdduke@chromium.org> wrote:
>
>> [+clholgat, +aelias, +dtrainor]
>>
>> Chrome for Android makes use of TextureLayer::SetTextureId for most of its
>> custom layer types.  crbug.com/239568 and crbug.com/235290 track
>> movement away
>> from this dependency, but I'm not sure how much progress has been made?
>> aelias@, what about creating an intermediate layer type that provides
>> this
>> functionality, separate from TextureLayer? This is blocking all Chromium
>> rolls
>> at the moment.
>>
>>
https://chromiumcodereview.**appspot.com/19303003/<https://chromiumcodereview...
>>
>
>

[dtrainor, 2013-07-17 16:48:11]

AFAIK ImageLayer doesn't work with compressed images and always keeps a
software copy (not at a computer now so I can't check). That is
unacceptable for our use case :(. I'd prefer keeping a TextureLayer
implementation in our tree until we get the new version working. There is a
lot of work downstream that has to happen to move us off TextureLayer as
well, but we need the alternative layer finished first so we can port to it.

-Dave
 On Jul 17, 2013 9:40 AM, "Carson Holgate" <clholgat@google.com> wrote:

> The old way is deprecated. The new way is not yet ready.
>
> I think there are two ways we can handle this, we can keep a copy of the
> old TextureLayer in our tree or we can switch everything back to
> ImageLayers temporarily until the new shared resource manager (
> https://chromiumcodereview.appspot.com/18191020/) is ready.
>
>
> On Wed, Jul 17, 2013 at 8:11 AM, <jdduke@chromium.org> wrote:
>
>> [+clholgat, +aelias, +dtrainor]
>>
>> Chrome for Android makes use of TextureLayer::SetTextureId for most of its
>> custom layer types.  crbug.com/239568 and crbug.com/235290 track
>> movement away
>> from this dependency, but I'm not sure how much progress has been made?
>> aelias@, what about creating an intermediate layer type that provides
>> this
>> functionality, separate from TextureLayer? This is blocking all Chromium
>> rolls
>> at the moment.
>>
>>
https://chromiumcodereview.**appspot.com/19303003/<https://chromiumcodereview...
>>
>
>

[dtrainor, 2013-07-17 16:48:45]

Ah sorry by compressed images I mean ETC1 texture compression.
On Jul 17, 2013 9:48 AM, "David Trainor" <dtrainor@google.com> wrote:

> AFAIK ImageLayer doesn't work with compressed images and always keeps a
> software copy (not at a computer now so I can't check). That is
> unacceptable for our use case :(. I'd prefer keeping a TextureLayer
> implementation in our tree until we get the new version working. There is a
> lot of work downstream that has to happen to move us off TextureLayer as
> well, but we need the alternative layer finished first so we can port to it.
>
> -Dave
>  On Jul 17, 2013 9:40 AM, "Carson Holgate" <clholgat@google.com> wrote:
>
>> The old way is deprecated. The new way is not yet ready.
>>
>> I think there are two ways we can handle this, we can keep a copy of the
>> old TextureLayer in our tree or we can switch everything back to
>> ImageLayers temporarily until the new shared resource manager (
>> https://chromiumcodereview.appspot.com/18191020/) is ready.
>>
>>
>> On Wed, Jul 17, 2013 at 8:11 AM, <jdduke@chromium.org> wrote:
>>
>>> [+clholgat, +aelias, +dtrainor]
>>>
>>> Chrome for Android makes use of TextureLayer::SetTextureId for most of
>>> its
>>> custom layer types.  crbug.com/239568 and crbug.com/235290 track
>>> movement away
>>> from this dependency, but I'm not sure how much progress has been made?
>>> aelias@, what about creating an intermediate layer type that provides
>>> this
>>> functionality, separate from TextureLayer? This is blocking all Chromium
>>> rolls
>>> at the moment.
>>>
>>>
https://chromiumcodereview.**appspot.com/19303003/<https://chromiumcodereview...
>>>
>>
>>

[no sievers, 2013-07-17 16:52:32]

TextureLayerClient::PrepareTexture() provides the same functionality as
TextureLayer::SetTextureId().
The trick is just to manage client lifetime for each layer.

[no sievers, 2013-07-17 17:20:55]

On 2013/07/17 16:52:32, Daniel Sievers wrote:
> TextureLayerClient::PrepareTexture() provides the same functionality as
> TextureLayer::SetTextureId().
> The trick is just to manage client lifetime for each layer.

Even better would be to use TextureLayer::SetTextureMailbox() - let's move the
discussion to http://crbug/261151

[jdduke (slow), 2013-07-17 17:26:04]

On 2013/07/17 16:52:32, Daniel Sievers wrote:
> TextureLayerClient::PrepareTexture() provides the same functionality as
> TextureLayer::SetTextureId().
> The trick is just to manage client lifetime for each layer.

Why can't we just have a FooTextureLayer that owns a custom TextureLayerClient
that it injects to the TextureLayer from which it is derived? 

We can then add "FooTextureLayer::SetTextureId()" that passes the id to the
client (owned by FooTextureLayer), which in turn provides that via
PrepareTexture()?

I'd hate to duplicate all of TextureLayer downstream; these forks have a way of
sticking around longer than desirable... =/

[aberent, 2013-07-18 11:03:56]

With agreement from piman@ I have reverted this CL for now, since it breaks
Chrome for Android. The revert is https://codereview.chromium.org/19463010, and
the bug is https://code.google.com/p/chromium/issues/detail?id=261151.