RESOLVED FIXED 162480
MediaSessionManagerMac::nowPlayingEligibleSession() needs to honor the main content heuristic
https://bugs.webkit.org/show_bug.cgi?id=162480
Summary MediaSessionManagerMac::nowPlayingEligibleSession() needs to honor the main c...
Wenson Hsieh
Reported 2016-09-22 19:43:10 PDT
MediaSessionManagerMac::nowPlayingEligibleSession() needs to honor the main content heuristic
Attachments
Patch (22.97 KB, patch)
2016-09-22 19:58 PDT, Wenson Hsieh
no flags
Refactored to use Page visibility hooks. (18.70 KB, patch)
2016-09-22 23:49 PDT, Wenson Hsieh
no flags
Remove gAllowsNowPlayingControls from MediaSessionManagerMac.mm (18.92 KB, patch)
2016-09-23 10:01 PDT, Wenson Hsieh
no flags
Patch for landing (18.91 KB, patch)
2016-09-23 11:16 PDT, Wenson Hsieh
no flags
Wenson Hsieh
Comment 1 2016-09-22 19:43:38 PDT
Wenson Hsieh
Comment 2 2016-09-22 19:58:38 PDT
Beth Dakin
Comment 3 2016-09-22 21:01:47 PDT
Comment on attachment 289653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289653&action=review This looks good to me, but a media expert should review it. > Source/WebKit2/UIProcess/API/Cocoa/WKView.h:57 > +@property (nonatomic) BOOL allowsNowPlayingControls; Is this so clients can disable this feature?
Beth Dakin
Comment 4 2016-09-22 21:15:38 PDT
(In reply to comment #3) > Comment on attachment 289653 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289653&action=review > > This looks good to me, but a media expert should review it. > > > Source/WebKit2/UIProcess/API/Cocoa/WKView.h:57 > > +@property (nonatomic) BOOL allowsNowPlayingControls; > > Is this so clients can disable this feature? We talked about this on irc, and I think we found a simpler way to do this.
Wenson Hsieh
Comment 5 2016-09-22 23:49:02 PDT
Created attachment 289666 [details] Refactored to use Page visibility hooks.
Beth Dakin
Comment 6 2016-09-23 07:57:21 PDT
Comment on attachment 289666 [details] Refactored to use Page visibility hooks. This looks better for sure! I would still like Jer to review officially.
Jer Noble
Comment 7 2016-09-23 08:24:06 PDT
Comment on attachment 289666 [details] Refactored to use Page visibility hooks. View in context: https://bugs.webkit.org/attachment.cgi?id=289666&action=review > Source/WebCore/page/Page.cpp:1474 > +void Page::visibleAndActiveDidChange(bool isVisibleAndActive) > +{ > + PlatformMediaSessionManager::setAllowsNowPlayingControls(!isVisibleAndActive); > +} > + I think this will cause issues on WK1 clients. One page in the process may be going into the background, and another one becoming foreground, which will either result in the process-global gAllowsNowPlayingControls being true or false depending on the order of operations. This is the right direction though. I would recommend instead of having this process global that the PlatformMediaSession has a method called, e.g., "pageAllowsNowPlaying()" which queries the page's visible-and-active state. Then in updateNowPlayingInfo(), you'd do something like: --- a/Source/WebCore/platform/audio/mac/MediaSessionManagerMac.mm +++ b/Source/WebCore/platform/audio/mac/MediaSessionManagerMac.mm @@ -116,7 +116,7 @@ void MediaSessionManagerMac::updateNowPlayingInfo() LOG(Media, "MediaSessionManagerMac::updateNowPlayingInfo - currentSession = %p", currentSession); - if (!currentSession) { + if (!currentSession || !currentSession->pageAllowsNowPlaying(()) { if (m_nowPlayingActive) { MRMediaRemoteSetCanBeNowPlayingApplication(false); LOG(Media, "MediaSessionManagerMac::updateNowPlayingInfo - clearing now playing info"); You'd still need the mechanism to call updateNowPlayingInfo() when the page's visibility state changes.
Wenson Hsieh
Comment 8 2016-09-23 10:01:45 PDT
Created attachment 289687 [details] Remove gAllowsNowPlayingControls from MediaSessionManagerMac.mm
Jer Noble
Comment 9 2016-09-23 10:54:07 PDT
Comment on attachment 289687 [details] Remove gAllowsNowPlayingControls from MediaSessionManagerMac.mm This version looks much better. I think you can land this as-is, but it would be nice to address a nit or two in a follow-up: The page shouldn't have to know about PlatformMediaSessionManager. There's already a state change notification system in Page which HTMLMediaElement could register for, and notify it's session when the visible and active state changes. As you pointed out, you'd need a way to coalesce multiple updates within the same run loop to avoid doing a bunch of unnecessary work, but that should be possible.
Wenson Hsieh
Comment 10 2016-09-23 11:16:17 PDT
Created attachment 289693 [details] Patch for landing
WebKit Commit Bot
Comment 11 2016-09-23 11:48:10 PDT
Comment on attachment 289693 [details] Patch for landing Clearing flags on attachment: 289693 Committed r206315: <http://trac.webkit.org/changeset/206315>
WebKit Commit Bot
Comment 12 2016-09-23 11:48:14 PDT
All reviewed patches have been landed. Closing bug.
Wenson Hsieh
Comment 13 2016-09-23 11:48:56 PDT
(In reply to comment #9) > Comment on attachment 289687 [details] > Remove gAllowsNowPlayingControls from MediaSessionManagerMac.mm > > This version looks much better. I think you can land this as-is, but it > would be nice to address a nit or two in a follow-up: > > The page shouldn't have to know about PlatformMediaSessionManager. There's > already a state change notification system in Page which HTMLMediaElement > could register for, and notify it's session when the visible and active > state changes. As you pointed out, you'd need a way to coalesce multiple > updates within the same run loop to avoid doing a bunch of unnecessary work, > but that should be possible. Sounds good. As we discussed on IRC, due to time constraints, I will follow up with this in a future patch that will include the infrastructure needed to test these changes.
Note You need to log in before you can comment on or make changes to this bug.