Media Session: notify the UI process when media controls are enabled/disabled

• Component: Media
• Status: RESOLVED FIXED

[Matt Rajca]

UIs such as lock screens should have knowledge about which media controls are enabled/disabled.

[Matt Rajca]

Created attachment 258550 [details]
Patch

[Darin Adler]

Comment on attachment 258550 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258550&action=review

> Source/WebCore/Modules/mediasession/MediaRemoteControls.h:56
> -    MediaRemoteControls(ScriptExecutionContext&);
> +    MediaRemoteControls(ScriptExecutionContext&, MediaSession*);

Why is this constructor public instead of private?

[Matt Rajca]

(In reply to comment #2)
> Comment on attachment 258550 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258550&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaRemoteControls.h:56
> > -    MediaRemoteControls(ScriptExecutionContext&);
> > +    MediaRemoteControls(ScriptExecutionContext&, MediaSession*);
> 
> Why is this constructor public instead of private?

When Media Sessions of a "content" category kind are created, they get a new MediaRemoteControls object by default. MediaRemoteControls' constructor is public so we can call it from MediaSession.

[Matt Rajca]

Created attachment 258643 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 258643 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258643&action=review

> Source/WebCore/Modules/mediasession/MediaRemoteControls.cpp:50
> +    if (m_session)
> +        m_session->controlIsEnabledDidChange();

But maybe m_previousTrackEnabled didn't change?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:5947
> +    focusManager->playbackAttributeDidChange(this, sourceElementID, IsPlaying, state & MediaProducer::IsSourceElementPlaying);
> +    focusManager->playbackAttributeDidChange(this, sourceElementID, IsPreviousTrackControlEnabled, state & MediaProducer::IsPreviousTrackControlEnabled);
> +    focusManager->playbackAttributeDidChange(this, sourceElementID, IsNextTrackControlEnabled, state & MediaProducer::IsNextTrackControlEnabled);

Seems a bit weird to do it like this. Why not just pass  state and let focusManger figure things out?

[Matt Rajca]

(In reply to comment #5)
> Comment on attachment 258643 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258643&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaRemoteControls.cpp:50
> > +    if (m_session)
> > +        m_session->controlIsEnabledDidChange();
> 
> But maybe m_previousTrackEnabled didn't change?

This doesn't matter as all the media state will get sent over to the UI process anyway.

> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:5947
> > +    focusManager->playbackAttributeDidChange(this, sourceElementID, IsPlaying, state & MediaProducer::IsSourceElementPlaying);
> > +    focusManager->playbackAttributeDidChange(this, sourceElementID, IsPreviousTrackControlEnabled, state & MediaProducer::IsPreviousTrackControlEnabled);
> > +    focusManager->playbackAttributeDidChange(this, sourceElementID, IsNextTrackControlEnabled, state & MediaProducer::IsNextTrackControlEnabled);
> 
> Seems a bit weird to do it like this. Why not just pass  state and let
> focusManger figure things out?

Good idea. I'll send a revised patch.

[Matt Rajca]

Created attachment 258681 [details]
Patch

[Matt Rajca]

Created attachment 258682 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 258682 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258682&action=review

> Source/WebCore/Modules/mediasession/MediaRemoteControls.h:67
> +    MediaSession* m_session { nullptr };

You're storing a raw pointer to the MediaSession, but MediaRemoteControls is refCounted, so I see nothing that clears the pointer if MediaSession is destroyed before MediaRemoteControls.

[Matt Rajca]

Created attachment 258684 [details]
Patch

[Matt Rajca]

(In reply to comment #9)
> Comment on attachment 258682 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258682&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaRemoteControls.h:67
> > +    MediaSession* m_session { nullptr };
> 
> You're storing a raw pointer to the MediaSession, but MediaRemoteControls is
> refCounted, so I see nothing that clears the pointer if MediaSession is
> destroyed before MediaRemoteControls.

Good catch. Fixed.

[Simon Fraser (smfr)]

Comment on attachment 258684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258684&action=review

> Source/WebCore/Modules/mediasession/MediaSession.cpp:74
> -        m_controls = adoptRef(*new MediaRemoteControls(context));
> +        m_controls = adoptRef(*new MediaRemoteControls(context, this));

Why doesn't this use MediaRemoteControls::create() (which would need an extra parameter)?

[Matt Rajca]

(In reply to comment #12)
> Comment on attachment 258684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258684&action=review
> 
> > Source/WebCore/Modules/mediasession/MediaSession.cpp:74
> > -        m_controls = adoptRef(*new MediaRemoteControls(context));
> > +        m_controls = adoptRef(*new MediaRemoteControls(context, this));
> 
> Why doesn't this use MediaRemoteControls::create() (which would need an
> extra parameter)?

The bindings generator expects only one parameter (the ScriptExecutionContext) so I can't add another one. Since the only case where we need to store a backpointer to the media session is this one, I just create the object directly.

[Eric Carlson]

Comment on attachment 258684 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258684&action=review

>>> Source/WebCore/Modules/mediasession/MediaSession.cpp:74
>>> +        m_controls = adoptRef(*new MediaRemoteControls(context, this));
>> 
>> Why doesn't this use MediaRemoteControls::create() (which would need an extra parameter)?
> 
> The bindings generator expects only one parameter (the ScriptExecutionContext) so I can't add another one. Since the only case where we need to store a backpointer to the media session is this one, I just create the object directly.

Can't you make a second MediaRemoteControls::create method and use it here? That will also allow you to make the MediaRemoteControls constructor private, which is the preferred pattern when possible in WebKit.

[Matt Rajca]

(In reply to comment #14)
> Comment on attachment 258684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258684&action=review
> 
> >>> Source/WebCore/Modules/mediasession/MediaSession.cpp:74
> >>> +        m_controls = adoptRef(*new MediaRemoteControls(context, this));
> >> 
> >> Why doesn't this use MediaRemoteControls::create() (which would need an extra parameter)?
> > 
> > The bindings generator expects only one parameter (the ScriptExecutionContext) so I can't add another one. Since the only case where we need to store a backpointer to the media session is this one, I just create the object directly.
> 
> Can't you make a second MediaRemoteControls::create method and use it here?
> That will also allow you to make the MediaRemoteControls constructor
> private, which is the preferred pattern when possible in WebKit.

Actually, I can stick with one create method but give the MediaSession parameter a default value of null. Thanks!

[Matt Rajca]

Committed r188345: <http://trac.webkit.org/changeset/188345>