Add multiple destruction callbacks to AndroidOverlay.

Add multiple destruction callbacks to AndroidOverlay.

Rather than specify a single destruction callback in the config,
this CL replace it with AndroidOverlay::AddDestructionCallback().
Multiple callbacks are called starting with the most recently
registered.  This allows multiple interested parties to find out
when an overlay is destroyed.

Additionally, AndroidVideoSurfaceChooser no longer provides a 'stop
using overlay immediately' callback, since it was just aliased to
a destruction callback anyway from the overlay.  Instead, the client
may AddDestructionCallback, if desired, to find out about overlay
destruction when an overlay is provided by the chooser.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2883913003
Cr-Commit-Position: refs/heads/master@{#472529}
Committed: https://chromium.googlesource.com/chromium/src/+/f989595632258d0cd667e7ef74ad5aec874329bc

[liberato (no reviews please), 2017-05-16 04:51:45]

Description was changed from

==========
Add multiple destruction callbacks to AndroidOverlay.

Rather than specify a single destruction callback in the config,
this CL replace it with AndroidOverlay::AddDestructionCallback().
Multiple callbacks are called starting with the most recently
registered.  This allows multiple interested parties to find out
when an overlay is destroyed.

Additionally, AndroidVideoSurfaceChooser no longer provides a 'stop
using overlay immediately' callback, since it was just aliased to
a destruction callback anyway from the overlay.  Instead, the client
may AddDestructionCallback, if desired, to find out about overlay
destruction when an overlay is provided by the chooser.

BUG=
==========

to

==========
Add multiple destruction callbacks to AndroidOverlay.

Rather than specify a single destruction callback in the config,
this CL replace it with AndroidOverlay::AddDestructionCallback().
Multiple callbacks are called starting with the most recently
registered.  This allows multiple interested parties to find out
when an overlay is destroyed.

Additionally, AndroidVideoSurfaceChooser no longer provides a 'stop
using overlay immediately' callback, since it was just aliased to
a destruction callback anyway from the overlay.  Instead, the client
may AddDestructionCallback, if desired, to find out about overlay
destruction when an overlay is provided by the chooser.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-05-16 05:24:46]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 05:25:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 05:27:56]

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

[commit-bot: I haz the power, 2017-05-16 05:27:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[liberato (no reviews please), 2017-05-16 05:37:34]

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 05:37:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 07:07:41]

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

[commit-bot: I haz the power, 2017-05-16 07:07:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[liberato (no reviews please), 2017-05-16 18:25:42]

Description was changed from

==========
Add multiple destruction callbacks to AndroidOverlay.

Rather than specify a single destruction callback in the config,
this CL replace it with AndroidOverlay::AddDestructionCallback().
Multiple callbacks are called starting with the most recently
registered.  This allows multiple interested parties to find out
when an overlay is destroyed.

Additionally, AndroidVideoSurfaceChooser no longer provides a 'stop
using overlay immediately' callback, since it was just aliased to
a destruction callback anyway from the overlay.  Instead, the client
may AddDestructionCallback, if desired, to find out about overlay
destruction when an overlay is provided by the chooser.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add multiple destruction callbacks to AndroidOverlay.

Rather than specify a single destruction callback in the config,
this CL replace it with AndroidOverlay::AddDestructionCallback().
Multiple callbacks are called starting with the most recently
registered.  This allows multiple interested parties to find out
when an overlay is destroyed.

Additionally, AndroidVideoSurfaceChooser no longer provides a 'stop
using overlay immediately' callback, since it was just aliased to
a destruction callback anyway from the overlay.  Instead, the client
may AddDestructionCallback, if desired, to find out about overlay
destruction when an overlay is provided by the chooser.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[liberato (no reviews please), 2017-05-16 18:26:10]

liberato@chromium.org changed reviewers:
+ tguilbert@chromium.org, watk@chromium.org

[liberato (no reviews please), 2017-05-16 18:28:51]

this turns out to be much simpler than what i had in mind yesterday.  there's no
need for the surface chooser to relay OnSurfaceDestroyed to a client; the client
can just register if it needs it.

this lets AVDASharedState hold a reference to the surface bundle and register
itself for surface destruction callbacks.  it will drop its reference to the
surface bundle if the overlay is destroyed.  that's part of a follow-up CL,
though.

thanks
-fl

[watk, 2017-05-16 19:10:29]

I like this more :) lgtm

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
File media/base/android/android_overlay.cc (right):

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
media/base/android/android_overlay.cc:18: if (!destruction_cbs_.size())
nit: More idiomatic to write .empty() I think

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
media/base/android/android_overlay.cc:31: // with an invalid |this| ptr.
nit: not sure I understand the rationale as written. Doesn't seem to be a
problem if |this| is deleted. i.e., it's conceivable that we would keep running
cbs.

IMO the reason is just that we provide the reasonable contract that
destruction_cbs will not be called after the overlay is deleted.

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
File media/base/android/android_overlay.h (right):

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
media/base/android/android_overlay.h:54: void
AddDestructionCallback(AndroidOverlayConfig::DestroyedCB cb);
Destruction makes it sound a lot like you're registering for AndroidOverlay's
destructor.

What about AddOverlayDestroyedCb()

[tguilbert, 2017-05-16 19:11:51]

LGTM

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
File media/base/android/android_overlay.h (right):

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
media/base/android/android_overlay.h:54: void
AddDestructionCallback(AndroidOverlayConfig::DestroyedCB cb);
Can you document the guarantees of the contract? It seems as though there is no
guarantee the callbacks will actually be called back, if |this| is destroyed
during RunDestructionCallbacks().

[liberato (no reviews please), 2017-05-16 20:04:02]

thanks for the feedback.

-fl

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
File media/base/android/android_overlay.cc (right):

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
media/base/android/android_overlay.cc:18: if (!destruction_cbs_.size())
On 2017/05/16 19:10:28, watk wrote:
> nit: More idiomatic to write .empty() I think

Done.

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
media/base/android/android_overlay.cc:31: // with an invalid |this| ptr.
On 2017/05/16 19:10:28, watk wrote:
> nit: not sure I understand the rationale as written. Doesn't seem to be a
> problem if |this| is deleted. i.e., it's conceivable that we would keep
running
> cbs.
> 
> IMO the reason is just that we provide the reasonable contract that
> destruction_cbs will not be called after the overlay is deleted.

if we call with |this| after it's deleted, then it's unclear what the caller
would do with it.  it might be aliased to some other overlay by that point.

i'll just shorted the comment to the first sentence, and avoid the issue :)

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
File media/base/android/android_overlay.h (right):

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
media/base/android/android_overlay.h:54: void
AddDestructionCallback(AndroidOverlayConfig::DestroyedCB cb);
On 2017/05/16 19:10:29, watk wrote:
> Destruction makes it sound a lot like you're registering for AndroidOverlay's
> destructor.
> 
> What about AddOverlayDestroyedCb()

AddSurfaceDestroyedCb()?

https://codereview.chromium.org/2883913003/diff/40001/media/base/android/andr...
media/base/android/android_overlay.h:54: void
AddDestructionCallback(AndroidOverlayConfig::DestroyedCB cb);
On 2017/05/16 19:11:51, tguilbert wrote:
> Can you document the guarantees of the contract? It seems as though there is
no
> guarantee the callbacks will actually be called back, if |this| is destroyed
> during RunDestructionCallbacks().

there is a guarantee that it won't be called if |this| is destroyed by an
earlier callback.  i've updated the docs to make this clear.

[liberato (no reviews please), 2017-05-16 20:04:13]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-05-16 20:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
tguilbert@chromium.org
Link to the patchset: https://codereview.chromium.org/2883913003/#ps60001
(title: "renamed methods, updated comments")

[commit-bot: I haz the power, 2017-05-16 20:05:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[liberato (no reviews please), 2017-05-16 21:02:39]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-05-16 21:02:41]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org,
tguilbert@chromium.org
Link to the patchset: https://codereview.chromium.org/2883913003/#ps80001
(title: "rebased.... though i got not conflicts?")

[commit-bot: I haz the power, 2017-05-16 21:05:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 23:11:47]

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

[commit-bot: I haz the power, 2017-05-16 23:11:49]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[liberato (no reviews please), 2017-05-17 16:05:39]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2017-05-17 16:06:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 19:26:16]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495037139530530,
"parent_rev": "bbc51dc1d4a8e3a927183c36bd0f4f3723a6854f", "commit_rev":
"786f139d62c324e16e466afb5bdfb25494a3c128"}

[commit-bot: I haz the power, 2017-05-17 19:26:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495037139530530,
"parent_rev": "aac85c0cf8dd4ae61c5a946167ef59d3aae635cc", "commit_rev":
"f989595632258d0cd667e7ef74ad5aec874329bc"}

[commit-bot: I haz the power, 2017-05-17 19:26:55]

Description was changed from

==========
Add multiple destruction callbacks to AndroidOverlay.

Rather than specify a single destruction callback in the config,
this CL replace it with AndroidOverlay::AddDestructionCallback().
Multiple callbacks are called starting with the most recently
registered.  This allows multiple interested parties to find out
when an overlay is destroyed.

Additionally, AndroidVideoSurfaceChooser no longer provides a 'stop
using overlay immediately' callback, since it was just aliased to
a destruction callback anyway from the overlay.  Instead, the client
may AddDestructionCallback, if desired, to find out about overlay
destruction when an overlay is provided by the chooser.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Add multiple destruction callbacks to AndroidOverlay.

Rather than specify a single destruction callback in the config,
this CL replace it with AndroidOverlay::AddDestructionCallback().
Multiple callbacks are called starting with the most recently
registered.  This allows multiple interested parties to find out
when an overlay is destroyed.

Additionally, AndroidVideoSurfaceChooser no longer provides a 'stop
using overlay immediately' callback, since it was just aliased to
a destruction callback anyway from the overlay.  Instead, the client
may AddDestructionCallback, if desired, to find out about overlay
destruction when an overlay is provided by the chooser.

BUG=722933
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2883913003
Cr-Commit-Position: refs/heads/master@{#472529}
Committed:
https://chromium.googlesource.com/chromium/src/+/f989595632258d0cd667e7ef74ad...
==========

[commit-bot: I haz the power, 2017-05-17 19:26:56]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f989595632258d0cd667e7ef74ad...