Bug 225696 - Resurrect WKWebView media controls API removed in https://bugs.webkit.org/show_bug.cgi?id=221929
Summary: Resurrect WKWebView media controls API removed in https://bugs.webkit.org/sho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-12 05:33 PDT by youenn fablet
Modified: 2021-05-18 01:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.69 KB, patch)
2021-05-12 05:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.53 KB, patch)
2021-05-12 07:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (6.70 KB, patch)
2021-05-17 00:51 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (6.54 KB, patch)
2021-05-17 11:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-05-12 05:33:42 PDT
<rdar://77863194>
Comment 1 youenn fablet 2021-05-12 05:43:33 PDT
Created attachment 428370 [details]
Patch
Comment 2 youenn fablet 2021-05-12 07:09:30 PDT
Created attachment 428372 [details]
Patch
Comment 3 Kate Cheney 2021-05-12 08:30:06 PDT
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.
Comment 4 Tim Horton 2021-05-12 11:01:29 PDT
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.
Comment 5 Kate Cheney 2021-05-12 11:07:40 PDT
(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.
Comment 6 Tim Horton 2021-05-12 11:13:12 PDT
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).
Comment 7 Tim Horton 2021-05-12 11:13:28 PDT
Comment on attachment 428372 [details]
Patch

r- for now since this will break the module build
Comment 8 youenn fablet 2021-05-17 00:41:49 PDT
(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.
Comment 9 youenn fablet 2021-05-17 00:51:14 PDT
Created attachment 428814 [details]
Patch
Comment 10 Tim Horton 2021-05-17 02:08:16 PDT
(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.
Comment 11 Kate Cheney 2021-05-17 08:44:03 PDT
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?
Comment 12 youenn fablet 2021-05-17 08:54:51 PDT
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.
Comment 13 Alex Christensen 2021-05-17 09:57:23 PDT
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
Comment 14 Alex Christensen 2021-05-17 10:03:54 PDT
Comment on attachment 428814 [details]
Patch

Just kidding.  They weren't deprecated in the SDK.  What a mess.
Comment 15 youenn fablet 2021-05-17 11:44:41 PDT
Created attachment 428851 [details]
Patch for landing
Comment 16 EWS 2021-05-18 01:10:44 PDT
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].