Bug 147802 - Media Session: notify the UI process when media controls are enabled/disabled
Summary: Media Session: notify the UI process when media controls are enabled/disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 145411
  Show dependency treegraph
 
Reported: 2015-08-07 18:03 PDT by Matt Rajca
Modified: 2015-08-12 12:25 PDT (History)
4 users (show)

See Also:


Attachments
Patch (12.65 KB, patch)
2015-08-07 18:22 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (12.81 KB, patch)
2015-08-10 13:19 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2015-08-10 19:02 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (13.46 KB, patch)
2015-08-10 19:07 PDT, Matt Rajca
no flags Details | Formatted Diff | Diff
Patch (13.85 KB, patch)
2015-08-10 19:22 PDT, Matt Rajca
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Rajca 2015-08-07 18:03:09 PDT
UIs such as lock screens should have knowledge about which media controls are enabled/disabled.
Comment 1 Matt Rajca 2015-08-07 18:22:26 PDT
Created attachment 258550 [details]
Patch
Comment 2 Darin Adler 2015-08-09 18:15:29 PDT
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?
Comment 3 Matt Rajca 2015-08-10 13:06:10 PDT
(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.
Comment 4 Matt Rajca 2015-08-10 13:19:10 PDT
Created attachment 258643 [details]
Patch
Comment 5 Simon Fraser (smfr) 2015-08-10 18:29:16 PDT
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?
Comment 6 Matt Rajca 2015-08-10 19:00:56 PDT
(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.
Comment 7 Matt Rajca 2015-08-10 19:02:28 PDT
Created attachment 258681 [details]
Patch
Comment 8 Matt Rajca 2015-08-10 19:07:41 PDT
Created attachment 258682 [details]
Patch
Comment 9 Simon Fraser (smfr) 2015-08-10 19:15:36 PDT
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.
Comment 10 Matt Rajca 2015-08-10 19:22:49 PDT
Created attachment 258684 [details]
Patch
Comment 11 Matt Rajca 2015-08-11 11:04:52 PDT
(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.
Comment 12 Simon Fraser (smfr) 2015-08-12 10:00:19 PDT
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)?
Comment 13 Matt Rajca 2015-08-12 10:18:03 PDT
(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.
Comment 14 Eric Carlson 2015-08-12 11:39:54 PDT
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.
Comment 15 Matt Rajca 2015-08-12 12:07:28 PDT
(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!
Comment 16 Matt Rajca 2015-08-12 12:25:32 PDT
Committed r188345: <http://trac.webkit.org/changeset/188345>