Bug 147802

Summary: Media Session: notify the UI process when media controls are enabled/disabled
Product: WebKit Reporter: Matt Rajca <mrajca>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: conrad_shultz, eric.carlson, mrajca, webkit-bug-importer
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 145411    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch eric.carlson: review+

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
DetailsFormatted DiffDiff
Patch (12.81 KB, patch)
2015-08-10 13:19 PDT, Matt Rajca 		no flags
DetailsFormatted DiffDiff
Patch (13.34 KB, patch)
2015-08-10 19:02 PDT, Matt Rajca 		no flags
DetailsFormatted DiffDiff
Patch (13.46 KB, patch)
2015-08-10 19:07 PDT, Matt Rajca 		no flags
DetailsFormatted DiffDiff
Patch (13.85 KB, patch)
2015-08-10 19:22 PDT, Matt Rajca 		eric.carlson: review+
DetailsFormatted DiffDiff
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.