MediaSessionManagerMac::nowPlayingEligibleSession() needs to honor the main content heuristic

• Component: Media
• Status: RESOLVED FIXED

[Wenson Hsieh]

MediaSessionManagerMac::nowPlayingEligibleSession() needs to honor the main content heuristic

[Wenson Hsieh]

<rdar://problem/28430615>

[Wenson Hsieh]

Created attachment 289653 [details]
Patch

[Beth Dakin]

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]

(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]

Created attachment 289666 [details]
Refactored to use Page visibility hooks.

[Beth Dakin]

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 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]

Created attachment 289687 [details]
Remove gAllowsNowPlayingControls from MediaSessionManagerMac.mm

[Jer Noble]

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]

Created attachment 289693 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 289693 [details]
Patch for landing

Clearing flags on attachment: 289693

Committed r206315: <http://trac.webkit.org/changeset/206315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Wenson Hsieh]

(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.