MediaSessionManagerMac::nowPlayingEligibleSession() needs to honor the main content heuristic
<rdar://problem/28430615>
Created attachment 289653 [details] Patch
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?
(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.
Created attachment 289666 [details] Refactored to use Page visibility hooks.
Comment on attachment 289666 [details] Refactored to use Page visibility hooks. This looks better for sure! I would still like Jer to review officially.
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.
Created attachment 289687 [details] Remove gAllowsNowPlayingControls from MediaSessionManagerMac.mm
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.
Created attachment 289693 [details] Patch for landing
Comment on attachment 289693 [details] Patch for landing Clearing flags on attachment: 289693 Committed r206315: <http://trac.webkit.org/changeset/206315>
All reviewed patches have been landed. Closing bug.
(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.