Remove MediaControls methods needed for the Cast button

Remove the explicit dependency between HTMLMediaElement and MediaControls
wrt the cast buttons.


OnRemotePlaybackAvailabilityChanged, OnRemotePlaybackConnecting,
and OnRemotePlaybackDisconnected are replaced by MediaControlsMediaEventListener
via an availability callback and listening for connect, connecting and disconnect
events on the RemotePlayback object off the MediaElement.

OnDisableRemotePlaybackAttributeChanged is replaced by a MutationObserver kept by
MediaControlsImpl.

RemotePlayback has methods that allow it to be used from C++ directly.

BUG=517102
TEST=existing and new tests + manual

Review-Url: https://codereview.chromium.org/2782373002
Cr-Commit-Position: refs/heads/master@{#468223}
Committed: https://chromium.googlesource.com/chromium/src/+/77db41b769602da7ccc0106c285490f149fe343d

[whywhat, 2017-03-30 01:54:17]

avayvod@chromium.org changed reviewers:
+ mlamouri@chromium.org

[whywhat, 2017-03-30 01:54:18]

PTaL

It's not ready to submit yet:
- doesn't build due to the dependencies -- I guess I should try to move the one
button out to modules/mediacontrols to fix it?
- doesn't watch availability -- I've little idea how to that in the
RemotePlaybackTest and thus no idea at all how to do it in non-test code; where
do I get the context and bindings and so on?
- since it doesn't even build, I haven't tested it much

So from the initial review I'd expect some high level feedback as well as
pointers how to fix the problems above :)

[mlamouri (slow - plz ping), 2017-03-30 20:45:51]

On 2017/03/30 at 01:54:18, avayvod wrote:
> PTaL
> 
> It's not ready to submit yet:
> - doesn't build due to the dependencies -- I guess I should try to move the
one button out to modules/mediacontrols to fix it?

Unfortunately, you can't just move to modules/mediacontrols/ as MediaControls
will need to create this object and it can't if it lives in modules/. I will see
if I can just move this class their in which case, you could do this.

> - doesn't watch availability -- I've little idea how to that in the
RemotePlaybackTest and thus no idea at all how to do it in non-test code; where
do I get the context and bindings and so on?

I'm not sure I follow what you mean here. Do you mean how to call
watchAvailability from the cast button? You could do
`ScriptState::forMainWorld(...)` but maybe you want a C++ friendly method that
can take a C++ callback instead of doing all the plumbing JS requires?

[whywhat, 2017-03-31 15:46:28]

On 2017/03/30 at 20:45:51, mlamouri wrote:
> On 2017/03/30 at 01:54:18, avayvod wrote:
> > PTaL
> > 
> > It's not ready to submit yet:
> > - doesn't build due to the dependencies -- I guess I should try to move the
one button out to modules/mediacontrols to fix it?
> 
> Unfortunately, you can't just move to modules/mediacontrols/ as MediaControls
will need to create this object and it can't if it lives in modules/. I will see
if I can just move this class their in which case, you could do this.

Well, there could be some transition Web* interface? I could also look into
moving MediaControls there as I might have more time :)

> 
> > - doesn't watch availability -- I've little idea how to that in the
RemotePlaybackTest and thus no idea at all how to do it in non-test code; where
do I get the context and bindings and so on?
> 
> I'm not sure I follow what you mean here. Do you mean how to call
watchAvailability from the cast button? You could do
`ScriptState::forMainWorld(...)` but maybe you want a C++ friendly method that
can take a C++ callback instead of doing all the plumbing JS requires?

If I have a C++ friendly method I'd have to duplicate most of the logic as the
JS implementation doesn't transition easily into pure C++ too (unless I have a
C++ callback for each JS callback or something). The main benefit of using the
JS harness would be to reuse the code and dogfood our logic for web developers.

[mlamouri (slow - plz ping), 2017-04-04 12:46:44]

On 2017/03/31 at 15:46:28, avayvod wrote:
> On 2017/03/30 at 20:45:51, mlamouri wrote:
> > On 2017/03/30 at 01:54:18, avayvod wrote:
> > > PTaL
> > > 
> > > It's not ready to submit yet:
> > > - doesn't build due to the dependencies -- I guess I should try to move
the one button out to modules/mediacontrols to fix it?
> > 
> > Unfortunately, you can't just move to modules/mediacontrols/ as
MediaControls will need to create this object and it can't if it lives in
modules/. I will see if I can just move this class their in which case, you
could do this.
> 
> Well, there could be some transition Web* interface? I could also look into
moving MediaControls there as I might have more time :)

I've sent https://codereview.chromium.org/2795783004 which will be the first
step to move the individual buttons to modules/.

> > > - doesn't watch availability -- I've little idea how to that in the
RemotePlaybackTest and thus no idea at all how to do it in non-test code; where
do I get the context and bindings and so on?
> > 
> > I'm not sure I follow what you mean here. Do you mean how to call
watchAvailability from the cast button? You could do
`ScriptState::forMainWorld(...)` but maybe you want a C++ friendly method that
can take a C++ callback instead of doing all the plumbing JS requires?
> 
> If I have a C++ friendly method I'd have to duplicate most of the logic as the
JS implementation doesn't transition easily into pure C++ too (unless I have a
C++ callback for each JS callback or something). The main benefit of using the
JS harness would be to reuse the code and dogfood our logic for web developers.

If ScriptState::forMainWorld() is correct (bindings ppl will have to confirm), I
guess it's mostly about boilerplate so why not.

[whywhat, 2017-04-17 22:40:04]

Rebased

[whywhat, 2017-04-17 22:48:45]

Kentaro, any thoughts on reusing the IDL interface from C++?

I don't think there's FromMainWorld() available anymore.
I chose to go the Internal() way but now I have a problem figuring out how to
store callbacks:

1. JS will keep passing RemotePlaybackAvailabilityCallback* objects to
watchAvailability()
2. CastButtonElement would rather pass a WTF::Closure
3. Now I don't want to have two maps so I want to store something that can be
either a Closure or RPACallback.

Closure seems to be non-Heap friendly, it's usually kept as a unique_ptr<>
everywhere in Blink. So creating a Closure that would keep RPACallback seems not
to work.

Can I create a custom RPACallback that would call the Closure when called?

Should I have a new GarbaseCollectedFinalized class that would either keep
TraceMemberWrapper for the RPACallback or unique_ptr<> for WTF::Closure?

[whywhat, 2017-04-17 22:49:03]

avayvod@chromium.org changed reviewers:
+ haraken@chromium.org

[whywhat, 2017-04-17 22:49:03]

avayvod@chromium.org changed required reviewers:
+ mlamouri@chromium.org

[haraken, 2017-04-18 07:55:14]

On 2017/04/17 22:48:45, whywhat wrote:
> Kentaro, any thoughts on reusing the IDL interface from C++?
> 
> I don't think there's FromMainWorld() available anymore.
> I chose to go the Internal() way but now I have a problem figuring out how to
> store callbacks:
> 
> 1. JS will keep passing RemotePlaybackAvailabilityCallback* objects to
> watchAvailability()
> 2. CastButtonElement would rather pass a WTF::Closure
> 3. Now I don't want to have two maps so I want to store something that can be
> either a Closure or RPACallback.
> 
> Closure seems to be non-Heap friendly, it's usually kept as a unique_ptr<>
> everywhere in Blink. So creating a Closure that would keep RPACallback seems
not
> to work.
> 
> Can I create a custom RPACallback that would call the Closure when called?
> 
> Should I have a new GarbaseCollectedFinalized class that would either keep
> TraceMemberWrapper for the RPACallback or unique_ptr<> for WTF::Closure?

Sorry, would you elaborate on what you're trying to achieve?

IDL callbacks are not expected to be called from Blink C++.

[mlamouri (slow - plz ping), 2017-04-18 13:02:40]

Regarding the callback, maybe you could do something similar to
IntersectionObserver?

See:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Inter...
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Inter...
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Inter...
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Inter...

And its usage in ElementVisibilityObserver:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme...

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp
(right):

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp:84:
UpdateDisplayType();
I think you should stop listening to those when the media controls are no longer
in the tree. You should maybe introduce an attach()/detach() method and call it
in the MediaControlsImpl callbacks.

I've added some tests for this in
core/html/HTMLMediaElementEventListenersTest.cpp maybe you could do add similar
tests with cast on?

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp:223:
return remote_playback_->state() != "disconnected";
Can't use add `constexpr` to the header? Or even have a method returning the
enum?

Actually, what about renaming `state()` to `stateForBindings()` and have a
`GetState()` returning the enum? Same could be done for `prompt()`.

[whywhat, 2017-04-18 17:05:38]

On 2017/04/18 at 07:55:14, haraken wrote:
> On 2017/04/17 22:48:45, whywhat wrote:
> > Kentaro, any thoughts on reusing the IDL interface from C++?
> > 
> > I don't think there's FromMainWorld() available anymore.
> > I chose to go the Internal() way but now I have a problem figuring out how
to
> > store callbacks:
> > 
> > 1. JS will keep passing RemotePlaybackAvailabilityCallback* objects to
> > watchAvailability()
> > 2. CastButtonElement would rather pass a WTF::Closure
> > 3. Now I don't want to have two maps so I want to store something that can
be
> > either a Closure or RPACallback.
> > 
> > Closure seems to be non-Heap friendly, it's usually kept as a unique_ptr<>
> > everywhere in Blink. So creating a Closure that would keep RPACallback seems
not
> > to work.
> > 
> > Can I create a custom RPACallback that would call the Closure when called?
> > 
> > Should I have a new GarbaseCollectedFinalized class that would either keep
> > TraceMemberWrapper for the RPACallback or unique_ptr<> for WTF::Closure?
> 
> Sorry, would you elaborate on what you're trying to achieve?
> 
> IDL callbacks are not expected to be called from Blink C++.

Well, I want to reuse code that adds/removes/runs callbacks in
RemotePlayback.cpp. The callbacks can be either used via IDL from JavaScript or
now from the media controls code, which is C++.

Seems like Mounir's suggestion should work. I'd change
RemotePlaybackAvailabilityCallback to [Custom], declare an interface in C++ and
add two implementations, one that acts as a proxy to WTF::Closure and one in
v8/bindings for JS.

I remember I wanted to avoid having a [Custom] implementation of the callback in
the first place though.

[whywhat, 2017-04-19 02:25:02]

Unified watch availability code.

[whywhat, 2017-04-19 02:27:57]

PTAL

Some stuff doesn't work yet as the button can't currently cause all the controls
to relayout when devices become available.

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp
(right):

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp:84:
UpdateDisplayType();
On 2017/04/18 at 13:02:40, mlamouri wrote:
> I think you should stop listening to those when the media controls are no
longer in the tree. You should maybe introduce an attach()/detach() method and
call it in the MediaControlsImpl callbacks.
> 
> I've added some tests for this in
core/html/HTMLMediaElementEventListenersTest.cpp maybe you could do add similar
tests with cast on?

Okay. Seems like availability callbacks should be handled by MediaControlsImpl
unless we want to allow the cast button to call ComputeWhichControlsFit() when
the device is detected as available.

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp:223:
return remote_playback_->state() != "disconnected";
On 2017/04/18 at 13:02:40, mlamouri wrote:
> Can't use add `constexpr` to the header? Or even have a method returning the
enum?
> 
> Actually, what about renaming `state()` to `stateForBindings()` and have a
`GetState()` returning the enum? Same could be done for `prompt()`.

With GetState() I don't really need ...ForBindings() suffix, do I?
Went with GetState() and *Internal methods for the rest.

[mlamouri (slow - plz ping), 2017-04-19 16:35:36]

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp
(right):

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp:223:
return remote_playback_->state() != "disconnected";
On 2017/04/19 at 02:27:57, whywhat wrote:
> On 2017/04/18 at 13:02:40, mlamouri wrote:
> > Can't use add `constexpr` to the header? Or even have a method returning the
enum?
> > 
> > Actually, what about renaming `state()` to `stateForBindings()` and have a
`GetState()` returning the enum? Same could be done for `prompt()`.
> 
> With GetState() I don't really need ...ForBindings() suffix, do I?
> Went with GetState() and *Internal methods for the rest.

I was suggesting to have stateForBindings() instead of state() to make it clear
for anyone reading the code that stateForBindings() isn't meant to be used
internally. Given that the coding style it's different, it's slightly clear so
no strong opinion.

https://codereview.chromium.org/2782373002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp
(right):

https://codereview.chromium.org/2782373002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp:121:
SetDisplayType(kMediaOverlayCastOffButton);
A bit surprising: why do we have this? Worth filing a follow-up bug?

[mlamouri (slow - plz ping), 2017-04-19 16:36:12]

I meant to add a top comment but didn't: I wasn't able to finish this review
because I had a couple of urgent ones (and not done with them). Will have
another look tomorrow.

[whywhat, 2017-04-19 22:40:48]

https://codereview.chromium.org/2782373002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp
(right):

https://codereview.chromium.org/2782373002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp:121:
SetDisplayType(kMediaOverlayCastOffButton);
On 2017/04/19 at 16:35:36, mlamouri wrote:
> A bit surprising: why do we have this? Worth filing a follow-up bug?

WDYM? That MediaCastOff and MediaOverlayCastOff are different resources?

We have Off and On to reflect "disconnected" (the Cast rectangle is empty) and
"connected/ing" (the rect is filled) states if that's what surprised you.

[whywhat, 2017-04-19 22:43:11]

On 2017/04/19 at 16:36:12, mlamouri wrote:
> I meant to add a top comment but didn't: I wasn't able to finish this review
because I had a couple of urgent ones (and not done with them). Will have
another look tomorrow.

That's fine. As I  mentioned, the relayout of the controls panel is not working
yet as well as the event handling w.r.t being detached from the document and
tests are missing.

[mlamouri (slow - plz ping), 2017-04-20 13:26:58]

On 2017/04/19 at 22:43:11, avayvod wrote:
> On 2017/04/19 at 16:36:12, mlamouri wrote:
> > I meant to add a top comment but didn't: I wasn't able to finish this review
because I had a couple of urgent ones (and not done with them). Will have
another look tomorrow.
> 
> That's fine. As I  mentioned, the relayout of the controls panel is not
working yet as well as the event handling w.r.t being detached from the document
and tests are missing.

Sounds good. Can you ping this CL when it's ready to be reviewed?

[whywhat, 2017-04-23 22:09:27]

Moved the events to MediaControlsMediaEventListener

[whywhat, 2017-04-23 22:09:50]

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

[commit-bot: I haz the power, 2017-04-23 22:09:58]

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-04-23 22:43:04]

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

[commit-bot: I haz the power, 2017-04-23 22:43:05]

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

[whywhat, 2017-04-23 23:42:09]

Handle double Attach()

[whywhat, 2017-04-23 23:43:23]

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

[commit-bot: I haz the power, 2017-04-23 23:43:33]

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-04-24 00:34:14]

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

[commit-bot: I haz the power, 2017-04-24 00:34:15]

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

[mlamouri (slow - plz ping), 2017-04-24 10:52:00]

Is this ready to be reviewed?

[whywhat, 2017-04-24 14:16:38]

I might need to fix the tests I've broken but basically, yes.

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp
(right):

https://codereview.chromium.org/2782373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/media_controls/elements/MediaControlCastButtonElement.cpp:84:
UpdateDisplayType();
On 2017/04/19 at 02:27:56, whywhat wrote:
> On 2017/04/18 at 13:02:40, mlamouri wrote:
> > I think you should stop listening to those when the media controls are no
longer in the tree. You should maybe introduce an attach()/detach() method and
call it in the MediaControlsImpl callbacks.
> > 
> > I've added some tests for this in
core/html/HTMLMediaElementEventListenersTest.cpp maybe you could do add similar
tests with cast on?
> 
> Okay. Seems like availability callbacks should be handled by MediaControlsImpl
unless we want to allow the cast button to call ComputeWhichControlsFit() when
the device is detected as available.

RE: tests - I'd have to add them in RemotePlaybackTest or next to it though as
core/html won't be able to use modules/remoteplayback IIUC.

[whywhat, 2017-04-24 15:57:14]

Added a test for event listeners

[whywhat, 2017-04-24 16:03:01]

Added checks for callbacks

[whywhat, 2017-04-24 17:24:30]

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

[commit-bot: I haz the power, 2017-04-24 17:25:08]

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-04-24 18:24:51]

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

[commit-bot: I haz the power, 2017-04-24 18:24:52]

Dry run: 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_...)

[mlamouri (slow - plz ping), 2017-04-24 18:40:00]

https://codereview.chromium.org/2782373002/diff/120001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/media_controls/MediaControlsMediaEventListener.cpp
(right):

https://codereview.chromium.org/2782373002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/media_controls/MediaControlsMediaEventListener.cpp:109:
return *HTMLMediaElementRemotePlayback::remote(GetMediaElement());
I think your crash on Windows is because you assume that RemotePlayback can't be
null while it can. Maybe return a pointer and null-check?

[whywhat, 2017-04-24 21:39:51]

Fixed Cast* MediaControlsImpl tests

[whywhat, 2017-04-24 21:41:52]

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

[commit-bot: I haz the power, 2017-04-24 21:42:51]

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-04-24 22:45:42]

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

[commit-bot: I haz the power, 2017-04-24 22:45:43]

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

[whywhat, 2017-04-24 23:04:20]

Removed unused from the header

[whywhat, 2017-04-24 23:59:29]

https://codereview.chromium.org/2782373002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp
(right):

https://codereview.chromium.org/2782373002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:310:
EXPECT_EQ(nullptr, weak_persistent_video);
Kentaro, this fails (and a similar test in existing
HTMLMediaElementEventListenersTest.cpp).

As I understand it, the reference chain is the following:

HTMLMediaElement -> Member<MediaControls> ->
Member<MediaControlsMediaEventListeners> -> Member<RemotePlayback> ->
HeapHashMap<int, Member<AvailabilityCallback>> -> unique_ptr<WTF::Closure> ->
WrapPersistent<MediaControlsMediaEventListeners> -> Member<MediaControls> etc.

If I don't register the WTF::Closure with RemotePlayback, the tests pass.
How should I handle the WTF::Closure correctly so that GC works?

I assumed that once the Member<AvailabilityCallback> is removed from the
HeapHashMap, it is unreachable and should be GCed. However it doesn't seem to
free the unique_ptr to WTF::Closure or something and that keeps a persistent
reference to the whole graph of traceable objects.

I tried changing it to WrapWeakPersistent() or even call Cancel() on
AvailabilityCallback that would reset unique_ptr but it didn't help :/

[whywhat, 2017-04-25 00:35:57]

On 2017/04/24 at 23:59:29, whywhat wrote:
>
https://codereview.chromium.org/2782373002/diff/160001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp
(right):
> 
>
https://codereview.chromium.org/2782373002/diff/160001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:310:
EXPECT_EQ(nullptr, weak_persistent_video);
> Kentaro, this fails (and a similar test in existing
HTMLMediaElementEventListenersTest.cpp).
> 
> As I understand it, the reference chain is the following:
> 
> HTMLMediaElement -> Member<MediaControls> ->
Member<MediaControlsMediaEventListeners> -> Member<RemotePlayback> ->
HeapHashMap<int, Member<AvailabilityCallback>> -> unique_ptr<WTF::Closure> ->
WrapPersistent<MediaControlsMediaEventListeners> -> Member<MediaControls> etc.
> 
> If I don't register the WTF::Closure with RemotePlayback, the tests pass.
> How should I handle the WTF::Closure correctly so that GC works?
> 
> I assumed that once the Member<AvailabilityCallback> is removed from the
HeapHashMap, it is unreachable and should be GCed. However it doesn't seem to
free the unique_ptr to WTF::Closure or something and that keeps a persistent
reference to the whole graph of traceable objects.
> 
> I tried changing it to WrapWeakPersistent() or even call Cancel() on
AvailabilityCallback that would reset unique_ptr but it didn't help :/

Hm, I think the reason is the async task run to notify the callback about the
initial availability - it holds a persistent link to the RemotePlayback object.
If the unit test runs pending tasks, it passes.

[whywhat, 2017-04-25 01:12:48]

Fixed webkit_unit_tests

[whywhat, 2017-04-25 01:14:31]

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

[commit-bot: I haz the power, 2017-04-25 01:14:54]

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-04-25 02:11:24]

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

[commit-bot: I haz the power, 2017-04-25 02:11:25]

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_...)

[whywhat, 2017-04-25 02:32:33]

Fixed more tests

[whywhat, 2017-04-25 02:38:57]

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

[commit-bot: I haz the power, 2017-04-25 02:39:33]

Dry run: CQ is trying da patch.

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

[whywhat, 2017-04-25 02:42:28]

This should be ready for another round of review now.

[commit-bot: I haz the power, 2017-04-25 03:48:28]

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

[commit-bot: I haz the power, 2017-04-25 03:48:29]

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

[mlamouri (slow - plz ping), 2017-04-25 12:19:21]

lgtm with comments applied and tests passing ;)

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right):

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:78: friend
class MediaControlsMediaEventListener;
Do you need these for PromptInternal() and GetState()? If yes, maybe make the
methods public? I would prefer to avoid such dependencies.

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:85: : public
GarbageCollectedFinalized<AvailabilityCallback> {
Do you need to declare the class here? I mean, apart from the name.

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp
(right):

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:280:
TEST_F(RemotePlaybackTest, RemovingFromDocumentRemovesListenersAndCallbacks) {
This should be a media_controls/ test, shouldn't it?

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:315:
TEST_F(RemotePlaybackTest, ReInsertingInDocumentRestoresListenersAndCallbacks) {
ditto

[whywhat, 2017-04-25 15:45:37]

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right):

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:78: friend
class MediaControlsMediaEventListener;
On 2017/04/25 at 12:19:21, mlamouri wrote:
> Do you need these for PromptInternal() and GetState()? If yes, maybe make the
methods public? I would prefer to avoid such dependencies.

Prompt and State are used by the button.
Watch/CancelAvailabilityInternal are used by the MediaEventListeners.
Making them public sgtm.

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:85: : public
GarbageCollectedFinalized<AvailabilityCallback> {
On 2017/04/25 at 12:19:21, mlamouri wrote:
> Do you need to declare the class here? I mean, apart from the name.

Would you prefer it to be in a separate header/cpp file?

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp
(right):

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlaybackTest.cpp:280:
TEST_F(RemotePlaybackTest, RemovingFromDocumentRemovesListenersAndCallbacks) {
On 2017/04/25 at 12:19:21, mlamouri wrote:
> This should be a media_controls/ test, shouldn't it?

I guess so :)

[whywhat, 2017-04-25 21:26:18]

avayvod@chromium.org changed reviewers:
+ mlippautz@chromium.org
- haraken@chromium.org

[whywhat, 2017-04-25 21:26:18]

avayvod@chromium.org changed required reviewers:
+ mlippautz@chromium.org

[whywhat, 2017-04-25 21:26:19]

+Michael to review how I'm using TraceWrappers (apparently incorrectly, hence
the try bots failures).

[whywhat, 2017-04-25 22:31:23]

Hm, I think I figured the problem with GC.

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right):

https://codereview.chromium.org/2782373002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:85: : public
GarbageCollectedFinalized<AvailabilityCallback> {
On 2017/04/25 at 15:45:37, whywhat wrote:
> On 2017/04/25 at 12:19:21, mlamouri wrote:
> > Do you need to declare the class here? I mean, apart from the name.
> 
> Would you prefer it to be in a separate header/cpp file?

It's needed to be in the header since it's used by C++ clients of
RemotePlayback.

[whywhat, 2017-04-25 23:11:18]

Fixed comments and the GC test

[whywhat, 2017-04-25 23:19:16]

avayvod@chromium.org changed reviewers:
- mlippautz@chromium.org

[whywhat, 2017-04-25 23:19:16]

avayvod@chromium.org changed required reviewers:
- mlippautz@chromium.org

[whywhat, 2017-04-25 23:27:19]

Description was changed from

==========
Make the cast button depend on RemotePlayback

BUG=None
==========

to

==========
Remove the explicit dependency between HTMLMediaElement and the Cast button.

MediaControlsMediaEventListeners now listens to RemotePlayback state change
events and sets an availability callback.
MediaControlCastButtonElement uses RemotePlayback to check if and how it should
be shown and to trigger the picker/controller UI.
RemotePlayback now supports being used from C++ directly without extra JS
bindings craft.

BUG=517102
TEST=existing and new tests + manual
==========

[whywhat, 2017-04-26 00:07:52]

Rebase

[whywhat, 2017-04-26 00:08:08]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-04-26 00:08:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2782373002/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-26 00:08:39]

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-04-26 00:13:43]

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

[commit-bot: I haz the power, 2017-04-26 00:13:45]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[whywhat, 2017-04-26 00:17:53]

Added missing files

[whywhat, 2017-04-26 00:19:41]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-04-26 00:19:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2782373002/#ps260001
(title: "Added missing files")

[commit-bot: I haz the power, 2017-04-26 00:20:26]

CQ is trying da patch.

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

[whywhat, 2017-04-26 00:28:42]

The CQ bit was unchecked by avayvod@chromium.org

[whywhat, 2017-04-26 01:44:11]

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

[commit-bot: I haz the power, 2017-04-26 01:45:05]

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-04-26 03:35:56]

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

[commit-bot: I haz the power, 2017-04-26 03:35:57]

Dry run: 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_...)

[whywhat, 2017-04-26 15:49:24]

No DCHECK in Detach

[whywhat, 2017-04-26 15:54:00]

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

[commit-bot: I haz the power, 2017-04-26 15:54:32]

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-04-26 17:49:52]

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

[commit-bot: I haz the power, 2017-04-26 17:49:53]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-04-28 00:40:31]

Minimized logic changes

[whywhat, 2017-04-28 15:42:28]

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

[commit-bot: I haz the power, 2017-04-28 15:43:27]

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-04-28 17:00:46]

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

[commit-bot: I haz the power, 2017-04-28 17:00:48]

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

[whywhat, 2017-04-28 17:23:17]

Use MutationObserver for disableRemotePlayback

[whywhat, 2017-04-28 17:29:02]

Description was changed from

==========
Remove the explicit dependency between HTMLMediaElement and the Cast button.

MediaControlsMediaEventListeners now listens to RemotePlayback state change
events and sets an availability callback.
MediaControlCastButtonElement uses RemotePlayback to check if and how it should
be shown and to trigger the picker/controller UI.
RemotePlayback now supports being used from C++ directly without extra JS
bindings craft.

BUG=517102
TEST=existing and new tests + manual
==========

to

==========
Remove the explicit dependency between HTMLMediaElement and MediaControls
wrt the cast buttons.


OnRemotePlaybackAvailabilityChanged, OnRemotePlaybackConnecting,
and OnRemotePlaybackDisconnected are replaced by MediaControlsMediaEventListener
via an availability callback and listening for connect, connecting and
disconnect
events on the RemotePlayback object off the MediaElement.

OnDisableRemotePlaybackAttributeChanged is replaced by a MutationObserver kept
by
MediaControlsImpl.

RemotePlayback has methods that allow it to be used from C++ directly.

BUG=517102
TEST=existing and new tests + manual
==========

[whywhat, 2017-04-28 17:29:11]

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

[commit-bot: I haz the power, 2017-04-28 17:29:57]

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-04-28 18:56:47]

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

[commit-bot: I haz the power, 2017-04-28 18:56:50]

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

[whywhat, 2017-04-28 21:22:14]

Fixed MediaControlsImplTest

[whywhat, 2017-04-28 21:26:08]

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

[commit-bot: I haz the power, 2017-04-28 21:26:47]

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-04-29 01:52:12]

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

[commit-bot: I haz the power, 2017-04-29 01:52:13]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-04-29 04:04:29]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-04-29 04:04:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2782373002/#ps340001
(title: "Fixed MediaControlsImplTest")

[commit-bot: I haz the power, 2017-04-29 04:04:37]

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-04-29 04:09:18]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1493438669854130,
"parent_rev": "6f9127a09a54711eed16c5e386ed02453309ed91", "commit_rev":
"77db41b769602da7ccc0106c285490f149fe343d"}

[commit-bot: I haz the power, 2017-04-29 04:10:48]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1493438669854130,
"parent_rev": "6f9127a09a54711eed16c5e386ed02453309ed91", "commit_rev":
"77db41b769602da7ccc0106c285490f149fe343d"}

[commit-bot: I haz the power, 2017-04-29 04:11:16]

Description was changed from

==========
Remove the explicit dependency between HTMLMediaElement and MediaControls
wrt the cast buttons.


OnRemotePlaybackAvailabilityChanged, OnRemotePlaybackConnecting,
and OnRemotePlaybackDisconnected are replaced by MediaControlsMediaEventListener
via an availability callback and listening for connect, connecting and
disconnect
events on the RemotePlayback object off the MediaElement.

OnDisableRemotePlaybackAttributeChanged is replaced by a MutationObserver kept
by
MediaControlsImpl.

RemotePlayback has methods that allow it to be used from C++ directly.

BUG=517102
TEST=existing and new tests + manual
==========

to

==========
Remove the explicit dependency between HTMLMediaElement and MediaControls
wrt the cast buttons.


OnRemotePlaybackAvailabilityChanged, OnRemotePlaybackConnecting,
and OnRemotePlaybackDisconnected are replaced by MediaControlsMediaEventListener
via an availability callback and listening for connect, connecting and
disconnect
events on the RemotePlayback object off the MediaElement.

OnDisableRemotePlaybackAttributeChanged is replaced by a MutationObserver kept
by
MediaControlsImpl.

RemotePlayback has methods that allow it to be used from C++ directly.

BUG=517102
TEST=existing and new tests + manual

Review-Url: https://codereview.chromium.org/2782373002
Cr-Commit-Position: refs/heads/master@{#468223}
Committed:
https://chromium.googlesource.com/chromium/src/+/77db41b769602da7ccc0106c2854...
==========

[commit-bot: I haz the power, 2017-04-29 04:11:18]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/77db41b769602da7ccc0106c2854...