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

RESOLVED FIXED147802

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

https://bugs.webkit.org/show_bug.cgi?id=147802

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

Matt Rajca

Reported 2015-08-07 18:03:09 PDT

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

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Matt Rajca

Comment 1 2015-08-07 18:22:26 PDT

Created attachment 258550 [details] Patch

Darin Adler

Comment 2 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?

Matt Rajca

Comment 3 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.

Matt Rajca

Comment 4 2015-08-10 13:19:10 PDT

Created attachment 258643 [details] Patch

Simon Fraser (smfr)

Comment 5 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?

Matt Rajca

Comment 6 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.

Matt Rajca

Comment 7 2015-08-10 19:02:28 PDT

Created attachment 258681 [details] Patch

Matt Rajca

Comment 8 2015-08-10 19:07:41 PDT

Created attachment 258682 [details] Patch

Simon Fraser (smfr)

Comment 9 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.

Matt Rajca

Comment 10 2015-08-10 19:22:49 PDT

Created attachment 258684 [details] Patch

Matt Rajca

Comment 11 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.

Simon Fraser (smfr)

Comment 12 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)?

Matt Rajca

Comment 13 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.

Eric Carlson

Comment 14 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.

Matt Rajca

Comment 15 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!

Matt Rajca

Comment 16 2015-08-12 12:25:32 PDT

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component Media

Assignee

Nobody

Reported

2015-08-07 18:03 PDT

Modified

2015-08-12 12:25 PDT History

CC List

4 users Show

URL

Keywords

Depends on

Blocks

145411

Dependencies

tree graph