Bug 217335 – Promote WKWebView media playback SPI to API

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

Various WKWebView SPI related to playing/pausing video would be useful as API instead

[Kate Cheney]

<rdar://problem/63406100>

[Kate Cheney]

Created attachment 410557 [details]
Patch

[Alex Christensen]

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?

[youenn fablet]

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.

[Kate Cheney]

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

[Alex Christensen]

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

[Eric Carlson]

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

[Kate Cheney]

Created attachment 410696 [details]
Patch

[Kate Cheney]

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

[Kate Cheney]

The new patch has the additional APIs to check for media playback existence, and whether the state is suspended or paused, plus testing.

[Sam Weinig]

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?

[Kate Cheney]

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

[Sam Weinig]

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

[Kate Cheney]

Created attachment 410778 [details]
Patch

[Alex Christensen]

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.

[Kate Cheney]

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

[Kate Cheney]

Created attachment 410805 [details]
Patch

[Kate Cheney]

Created attachment 410807 [details]
Patch

[Jer Noble]

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.

[Kate Cheney]

Created attachment 410945 [details]
Patch for landing

[Kate Cheney]

Thanks Jer! (And Alex, Youenn, Eric and Sam)

[EWS]

Committed r268268: <https://trac.webkit.org/changeset/268268>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410945 [details].

[Ryan Haddad]

Follow up fix for Catalyst build: https://trac.webkit.org/changeset/268290/webkit