WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Refactored to use Page visibility hooks.
(18.70 KB, patch)
2016-09-22 23:49 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Remove gAllowsNowPlayingControls from MediaSessionManagerMac.mm
(18.92 KB, patch)
2016-09-23 10:01 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Patch for landing
(18.91 KB, patch)
2016-09-23 11:16 PDT
,
Wenson Hsieh
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Wenson Hsieh
Comment 1
2016-09-22 19:43:38 PDT
<
rdar://problem/28430615
>
Wenson Hsieh
Comment 2
2016-09-22 19:58:38 PDT
Created
attachment 289653
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug