Bug 225696 – Resurrect WKWebView media controls API removed in https://bugs.webkit.org/show_bug.cgi?id=221929

• Component: Media
• Status: RESOLVED FIXED

[youenn fablet]

<rdar://77863194>

[youenn fablet]

Created attachment 428370 [details]
Patch

[youenn fablet]

Created attachment 428372 [details]
Patch

[Kate Cheney]

We intentionally removed the deprecated versions of these in https://bugs.webkit.org/show_bug.cgi?id=223704 not sure we should add them back.

[Tim Horton]

Pretty sure the old names were swift-incompatible, right? You best check why that `#ifndef __swift__` was there, I think it was for good (and unfortunate) reason.

[Kate Cheney]

(In reply to Tim Horton from comment #4)
> Pretty sure the old names were swift-incompatible, right? You best check why
> that `#ifndef __swift__` was there, I think it was for good (and
> unfortunate) reason.

Yes, if we add these back we should add #ifndef __swift__ like in https://bugs.webkit.org/attachment.cgi?id=423389&action=review. The deprecated methods  added the nil value for the completion handler, but both the new and deprecated values have an overloaded interface of "func closeAllMediaPresentations()", etc. So it will cause a duplicate interface error.

[Tim Horton]

Comment on attachment 428372 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:346
> +- (void)closeAllMediaPresentations:(void (^_Nullable)(void))completionHandler WK_API_DEPRECATED_WITH_REPLACEMENT("closeAllMediaPresentationsWithCompletionHandler:", macos(WK_MAC_TBA, WK_MAC_TBA), ios(WK_IOS_TBA, WK_IOS_TBA));

Also, all of these deprecation macros should say the *actual* build that they were introduced in (and TBA for the removal side).

[Tim Horton]

Comment on attachment 428372 [details]
Patch

r- for now since this will break the module build

[youenn fablet]

(In reply to Tim Horton from comment #6)
> Comment on attachment 428372 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428372&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:346
> > +- (void)closeAllMediaPresentations:(void (^_Nullable)(void))completionHandler WK_API_DEPRECATED_WITH_REPLACEMENT("closeAllMediaPresentationsWithCompletionHandler:", macos(WK_MAC_TBA, WK_MAC_TBA), ios(WK_IOS_TBA, WK_IOS_TBA));
> 
> Also, all of these deprecation macros should say the *actual* build that
> they were introduced in (and TBA for the removal side).

I followed what was done in other parts of the headers, like loadSimulatedRequest.
I can change it to exact values though.

[youenn fablet]

Created attachment 428814 [details]
Patch

[Tim Horton]

(In reply to youenn fablet from comment #8)
> (In reply to Tim Horton from comment #6)
> > Comment on attachment 428372 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=428372&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:346
> > > +- (void)closeAllMediaPresentations:(void (^_Nullable)(void))completionHandler WK_API_DEPRECATED_WITH_REPLACEMENT("closeAllMediaPresentationsWithCompletionHandler:", macos(WK_MAC_TBA, WK_MAC_TBA), ios(WK_IOS_TBA, WK_IOS_TBA));
> > 
> > Also, all of these deprecation macros should say the *actual* build that
> > they were introduced in (and TBA for the removal side).
> 
> I followed what was done in other parts of the headers, like
> loadSimulatedRequest.
> I can change it to exact values though.

The situation for loadSimulatedRequest is not equivalent, since it has not been included in a shipped SDK.

[Kate Cheney]

Comment on attachment 428814 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:978
> +    [self setAllMediaPlaybackSuspended:FALSE completionHandler:completionHandler];

Should this be 'setAllMediaPlaybackSuspended:NO'? I don't see :FALSE being used frequently.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:983
> +    [self setAllMediaPlaybackSuspended:TRUE completionHandler:completionHandler];

Ditto -- should this be :YES?

[youenn fablet]

Comment on attachment 428814 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:978
>> +    [self setAllMediaPlaybackSuspended:FALSE completionHandler:completionHandler];
> 
> Should this be 'setAllMediaPlaybackSuspended:NO'? I don't see :FALSE being used frequently.

Seems better indeed.

[Alex Christensen]

Comment on attachment 428814 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.h:377
> +- (void)requestMediaPlaybackState:(void (^)(WKMediaPlaybackState))completionHandler WK_API_DEPRECATED_WITH_REPLACEMENT("requestMediaPlaybackStateWithCompletionHandler:", macos(11.3, WK_MAC_TBA), ios(14.5, WK_IOS_TBA));

We shouldn't use WK_MAC_TBA or WK_IOS_TBA here.  It should be this:
macos(11.3, 11.3), ios(14.5, 14.5)
I'm also not convinced we should do this at all

[Alex Christensen]

Comment on attachment 428814 [details]
Patch

Just kidding.  They weren't deprecated in the SDK.  What a mess.

[youenn fablet]

Created attachment 428851 [details]
Patch for landing

[EWS]

Committed r277645 (237851@main): <https://commits.webkit.org/237851@main>

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