Bug 217335

Summary: Promote WKWebView media playback SPI to API
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, cdumez, eric.carlson, esprehn+autocc, ews-watchlist, glenn, jer.noble, kangil.han, philipj, sam, sergio, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Kate Cheney 2020-10-05 13:55:33 PDT
Various WKWebView SPI related to playing/pausing video would be useful as API instead
Comment 1 Kate Cheney 2020-10-05 13:56:10 PDT
<rdar://problem/63406100>
Comment 2 Kate Cheney 2020-10-05 14:13:59 PDT
Created attachment 410557 [details]
Patch
Comment 3 Alex Christensen 2020-10-05 22:51:12 PDT
Comment on attachment 410557 [details]
Patch

I believe this needs to go through the internal API review channels, but I have some initial feedback:
It's not clear what the difference between suspendAllMediaPlayback and pauseAllMediaPlayback is.
Do we also want a way to query whether there is media playback or presentations, and if it is paused or resumed?
Comment 4 youenn fablet 2020-10-06 00:35:05 PDT
I agree with Alex, we probably want to expose whether there is media playback so that the app can react and decide to pause.
MacOS Safari is for instance displaying an audio icon when a page is playing, it would be a useful addition for other apps.
Comment 5 Kate Cheney 2020-10-06 08:26:26 PDT
(In reply to Alex Christensen from comment #3)
> Comment on attachment 410557 [details]
> Patch
> 
> I believe this needs to go through the internal API review channels, but I
> have some initial feedback:

I kicked off the internal API Review Process before uploading this, and it is my understanding that it is ok to start patch review while that is happening.

> It's not clear what the difference between suspendAllMediaPlayback and
> pauseAllMediaPlayback is.

Noted, I can clarify that in the abstract.

> Do we also want a way to query whether there is media playback or
> presentations, and if it is paused or resumed?

That is a good idea. This may be better in another patch. I filed rdar://70000262 to track this.
Comment 6 Alex Christensen 2020-10-06 10:21:14 PDT
(In reply to katherine_cheney from comment #5)
> (In reply to Alex Christensen from comment #3)
> > Comment on attachment 410557 [details]
> > Patch
> > 
> > I believe this needs to go through the internal API review channels, but I
> > have some initial feedback:
> 
> I kicked off the internal API Review Process before uploading this, and it
> is my understanding that it is ok to start patch review while that is
> happening.
Yep, that's fine.  I just didn't see that.

> > Do we also want a way to query whether there is media playback or
> > presentations, and if it is paused or resumed?
> 
> That is a good idea. This may be better in another patch. I filed
> rdar://70000262 to track this.
I think they should be done at the same time.  It is very strange to add an API to toggle something without being able to see what the toggle state is.
Comment 7 Eric Carlson 2020-10-06 13:40:55 PDT
Comment on attachment 410557 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410557&action=review

> Source/WebCore/ChangeLog:10
> +        Change stopAllMediaPlayback to be more aptly named
> +        pauseAllMediaPlayback.

I'm not sure about this. I think "pause" implies playback can be started again, but from the app's perspective it is a one-time operation.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:325
> + @discussion Includes picture-in-picture, video fullscreen, and element fullscreen.

I don't think we need a distinction between "video fullscreen" and "element fullscreen". 

I'd just say `Includes picture-in-picture and fullscreen`.
Comment 8 Kate Cheney 2020-10-06 14:55:42 PDT
Created attachment 410696 [details]
Patch
Comment 9 Kate Cheney 2020-10-06 14:56:31 PDT
(In reply to Eric Carlson from comment #7)
> Comment on attachment 410557 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410557&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Change stopAllMediaPlayback to be more aptly named
> > +        pauseAllMediaPlayback.
> 
> I'm not sure about this. I think "pause" implies playback can be started
> again, but from the app's perspective it is a one-time operation.
> 

We will have to loop in Jer on this, I made this change based on a conversation we had.

> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:325
> > + @discussion Includes picture-in-picture, video fullscreen, and element fullscreen.
> 
> I don't think we need a distinction between "video fullscreen" and "element
> fullscreen". 
> 
> I'd just say `Includes picture-in-picture and fullscreen`.

Fixed this.
Comment 10 Kate Cheney 2020-10-06 15:02:21 PDT
The new patch has the additional APIs to check for media playback existence, and whether the state is suspended or paused, plus testing.
Comment 11 Sam Weinig 2020-10-06 16:48:13 PDT
Comment on attachment 410696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410696&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:357
> +/*! @abstract Check if media playback exists in the current WKWebView.
> + @result true if media playback exists in the current WKWebView, otherwise false.
> + */
> +- (void)mediaPlaybackExists:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> +
> +/*! @abstract Check if media playback is suspended in the current WKWebView.
> + @result true if media playback in the current WKWebView is suspended, otherwise false.
> + */
> +- (void)mediaPlaybackIsSuspended:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> +
> +/*! @abstract Check if media playback is paused in the current WKWebView.
> + @result true if media playback in the current WKWebView is paused, otherwise false.
> + */
> +- (void)mediaPlaybackIsPaused:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Have you considered merging these into one -(void)mediaPlaybackState(void (^)(WKMediaPlaybackState, NSError *))completionHandler type callback?
Comment 12 Kate Cheney 2020-10-06 16:55:56 PDT
(In reply to Sam Weinig from comment #11)
> Comment on attachment 410696 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410696&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:357
> > +/*! @abstract Check if media playback exists in the current WKWebView.
> > + @result true if media playback exists in the current WKWebView, otherwise false.
> > + */
> > +- (void)mediaPlaybackExists:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > +
> > +/*! @abstract Check if media playback is suspended in the current WKWebView.
> > + @result true if media playback in the current WKWebView is suspended, otherwise false.
> > + */
> > +- (void)mediaPlaybackIsSuspended:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > +
> > +/*! @abstract Check if media playback is paused in the current WKWebView.
> > + @result true if media playback in the current WKWebView is paused, otherwise false.
> > + */
> > +- (void)mediaPlaybackIsPaused:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Have you considered merging these into one -(void)mediaPlaybackState(void
> (^)(WKMediaPlaybackState, NSError *))completionHandler type callback?

I did not consider it, but that is a very good idea. I'll upload again with that change.
Comment 13 Sam Weinig 2020-10-06 17:38:03 PDT
(In reply to katherine_cheney from comment #12)
> (In reply to Sam Weinig from comment #11)
> > Comment on attachment 410696 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=410696&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:357
> > > +/*! @abstract Check if media playback exists in the current WKWebView.
> > > + @result true if media playback exists in the current WKWebView, otherwise false.
> > > + */
> > > +- (void)mediaPlaybackExists:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > > +
> > > +/*! @abstract Check if media playback is suspended in the current WKWebView.
> > > + @result true if media playback in the current WKWebView is suspended, otherwise false.
> > > + */
> > > +- (void)mediaPlaybackIsSuspended:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > > +
> > > +/*! @abstract Check if media playback is paused in the current WKWebView.
> > > + @result true if media playback in the current WKWebView is paused, otherwise false.
> > > + */
> > > +- (void)mediaPlaybackIsPaused:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > Have you considered merging these into one -(void)mediaPlaybackState(void
> > (^)(WKMediaPlaybackState, NSError *))completionHandler type callback?
> 
> I did not consider it, but that is a very good idea. I'll upload again with
> that change.

:thumbs-up:
Comment 14 Kate Cheney 2020-10-07 14:25:09 PDT
Created attachment 410778 [details]
Patch
Comment 15 Alex Christensen 2020-10-07 14:42:55 PDT
Comment on attachment 410778 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410778&action=review

> Source/WebKit/ChangeLog:22
> +        * UIProcess/API/Cocoa/WKMediaPlaybackState.h: Added.

New API headers should be included in WebKit.h for modules to work well.

> Source/WebKit/UIProcess/API/Cocoa/WKError.h:67
> +    WKErrorNoMediaPlayback WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)),

I don't think we need this error at all.  Having no media playback is a valid state to be in.

> Source/WebKit/UIProcess/API/Cocoa/WKMediaPlaybackState.h:32
> +    WKNoMediaPlayback,

WKMediaPlaybackStateNone?
For some reason, all the enums like this I've seen have all their members begin with the name of the enum.  It probably looks better in swift, too.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:344
> +- (void)resumeAllMediaPlayback WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

These should probably all have _Nullable completion handlers so you can write an app that knows when the state is set, especially since it is not set right away.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:353
> +- (void)mediaPlaybackState:(void (^)(WKMediaPlaybackState, NSError *))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I'd remove the NSError *.
The name of this is a little strange because it indicates the argument should be a state, but it's a block that receives the state.
Comment 16 Kate Cheney 2020-10-07 15:14:47 PDT
(In reply to Alex Christensen from comment #15)
> Comment on attachment 410778 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410778&action=review
> 

Thanks, will fix all of these.

> > Source/WebKit/ChangeLog:22
> > +        * UIProcess/API/Cocoa/WKMediaPlaybackState.h: Added.
> 
> New API headers should be included in WebKit.h for modules to work well.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKError.h:67
> > +    WKErrorNoMediaPlayback WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA)),
> 
> I don't think we need this error at all.  Having no media playback is a
> valid state to be in.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKMediaPlaybackState.h:32
> > +    WKNoMediaPlayback,
> 
> WKMediaPlaybackStateNone?
> For some reason, all the enums like this I've seen have all their members
> begin with the name of the enum.  It probably looks better in swift, too.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:344
> > +- (void)resumeAllMediaPlayback WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> These should probably all have _Nullable completion handlers so you can
> write an app that knows when the state is set, especially since it is not
> set right away.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:353
> > +- (void)mediaPlaybackState:(void (^)(WKMediaPlaybackState, NSError *))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> I'd remove the NSError *.
> The name of this is a little strange because it indicates the argument
> should be a state, but it's a block that receives the state.

requestMediaPlaybackState?
Comment 17 Kate Cheney 2020-10-07 17:44:24 PDT
Created attachment 410805 [details]
Patch
Comment 18 Kate Cheney 2020-10-07 18:34:57 PDT
Created attachment 410807 [details]
Patch
Comment 19 Jer Noble 2020-10-09 09:47:55 PDT
Comment on attachment 410807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=410807&action=review

R=me with just a couple nits:

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:351
> + WKNoMediaPlayback and the callback will return the error WKErrorNoMediaPlayback.

This makes it sounds like an error will be returned if there’s no media playback.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:931
> +    return _page->requestMediaPlaybackState([completionHandler = makeBlockPtr(completionHandler)] (auto&& mediaPlaybackState) {

I think this needs a nil check on completionHandler.
Comment 20 Kate Cheney 2020-10-09 09:58:04 PDT
Created attachment 410945 [details]
Patch for landing
Comment 21 Kate Cheney 2020-10-09 09:58:49 PDT
Thanks Jer! (And Alex, Youenn, Eric and Sam)
Comment 22 EWS 2020-10-09 10:37:30 PDT
Committed r268268: <https://trac.webkit.org/changeset/268268>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410945 [details].
Comment 23 Ryan Haddad 2020-10-09 14:45:59 PDT
Follow up fix for Catalyst build: https://trac.webkit.org/changeset/268290/webkit