Bug 220741 - WKWebView closeAllMediaPresentations API does not have a completion handler
Summary: WKWebView closeAllMediaPresentations API does not have a completion handler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-19 12:42 PST by Kate Cheney
Modified: 2021-02-03 10:46 PST (History)
9 users (show)

See Also:


Attachments
Patch (36.91 KB, patch)
2021-01-19 13:14 PST, Kate Cheney
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (37.40 KB, patch)
2021-01-19 13:21 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (30.20 KB, patch)
2021-01-22 17:16 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (29.60 KB, patch)
2021-01-27 11:31 PST, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-01-19 12:42:34 PST
This call involves async operations and should have a completion handler so clients know when it has completed.
Comment 1 Kate Cheney 2021-01-19 12:43:25 PST
<rdar://problem/73045904>
Comment 2 Kate Cheney 2021-01-19 13:14:31 PST
Created attachment 417901 [details]
Patch
Comment 3 Kate Cheney 2021-01-19 13:21:24 PST
Created attachment 417903 [details]
Patch
Comment 4 Kate Cheney 2021-01-19 13:56:39 PST
Comment on attachment 417903 [details]
Patch

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

> Source/WebKit/ChangeLog:21
> +        has requested that media presentations close.

Am I correct that a webView cannot have both video fullscreen content and element fullscreen content simultaneously?
Comment 5 Peng Liu 2021-01-19 18:40:17 PST
Comment on attachment 417903 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Because many classes inherit from VideoFullscreenModel, we have to add

Looks like only three classes inherit from VideoFullscreenModel: VideoFullscreenModelVideoElement, VideoFullscreenModelContext, and VideoFullscreenControllerContext. :-)

>> Source/WebKit/ChangeLog:21
>> +        has requested that media presentations close.
> 
> Am I correct that a webView cannot have both video fullscreen content and element fullscreen content simultaneously?

This is unlikely to happen in practice, but it is possible in theory.
Comment 6 youenn fablet 2021-01-22 03:22:34 PST
Comment on attachment 417903 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:194
> +    void requestFullscreenMode(HTMLMediaElementEnums::VideoFullscreenMode, bool finishedWithMedia, CompletionHandler<void(void)>&&) override;

final

> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:589
> +    completionHandler();

Does it make sense to call completionHandler as part of  m_fullscreenModel->requestFullscreenMode completion handler (after a hop to UIThread?)

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:740
> +        return;

This is a bit problematic as most probably completionHandler will not be called if we return early, leading to a debug assert.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:757
>      m_page->send(Messages::VideoFullscreenManager::RequestFullscreenMode(contextId, mode, finishedWithMedia));

Why not simply making RequestFullscreenMode Async, using sendWithAsyncReply and passing the completionHandler to it.

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:160
> +    setCloseAllMediaPresentationsCallback(WTFMove(completionHandler));

It is not clear to me why we are setting close completionHandler here, it seems it could clash with completionHandler from VideoFullscreenManagerProxy::requestFullscreenMode.

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:97
> +    CompletionHandler<void(void)> m_closeAllMediaCallback;

void()
Comment 7 Peng Liu 2021-01-22 10:07:01 PST
Comment on attachment 417903 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:757
>>      m_page->send(Messages::VideoFullscreenManager::RequestFullscreenMode(contextId, mode, finishedWithMedia));
> 
> Why not simply making RequestFullscreenMode Async, using sendWithAsyncReply and passing the completionHandler to it.

The RequestFullscreenMode message is used by the UI process to request a presentation mode change. It will lead to multiple IPC messages between a Web process and the UI process. So we need to refactor the current implementation before we can use sendAsync() here.
Comment 8 youenn fablet 2021-01-22 10:12:43 PST
> > Why not simply making RequestFullscreenMode Async, using sendWithAsyncReply and passing the completionHandler to it.
> 
> The RequestFullscreenMode message is used by the UI process to request a
> presentation mode change. It will lead to multiple IPC messages between a
> Web process and the UI process. So we need to refactor the current
> implementation before we can use sendAsync() here.

The WebProcess could keep track of all these messages and call the IPC completion handler when it is done with mode change.

Is the WebProcess not doing so/able to do it without too much trouble?
Or is it necessary that UIProcess calls this completion handler as soon as it knows it  is finished?
Comment 9 Kate Cheney 2021-01-22 10:16:56 PST
(In reply to youenn fablet from comment #8)
> > > Why not simply making RequestFullscreenMode Async, using sendWithAsyncReply and passing the completionHandler to it.
> > 
> > The RequestFullscreenMode message is used by the UI process to request a
> > presentation mode change. It will lead to multiple IPC messages between a
> > Web process and the UI process. So we need to refactor the current
> > implementation before we can use sendAsync() here.
> 
> The WebProcess could keep track of all these messages and call the IPC
> completion handler when it is done with mode change.
> 
> Is the WebProcess not doing so/able to do it without too much trouble?
> Or is it necessary that UIProcess calls this completion handler as soon as
> it knows it  is finished?

I think maybe one issue is it seems like PiP is closed (for Mac) in VideoFullscreenInterfaceMac pipDidClose: which we don't initiate from WebKit, so we can't pass our own completion handler there.
Comment 10 Kate Cheney 2021-01-22 10:18:21 PST
Comment on attachment 417903 [details]
Patch

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

>>> Source/WebKit/ChangeLog:21
>>> +        has requested that media presentations close.
>> 
>> Am I correct that a webView cannot have both video fullscreen content and element fullscreen content simultaneously?
> 
> This is unlikely to happen in practice, but it is possible in theory.

Ok. I need to use some sort of callback aggregator then, because now I return early after checking for video fullscreen.

>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:589
>> +    completionHandler();
> 
> Does it make sense to call completionHandler as part of  m_fullscreenModel->requestFullscreenMode completion handler (after a hop to UIThread?)

I didn't originally think it mattered because this path is not hit when closing fullscreen but I had to include this because it inherits from VideoFullscreenModel. I will double check, I might be wrong.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:740
>> +        return;
> 
> This is a bit problematic as most probably completionHandler will not be called if we return early, leading to a debug assert.

In the destructor I call it if it is not null, won't that mean no assert?

>> Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:160
>> +    setCloseAllMediaPresentationsCallback(WTFMove(completionHandler));
> 
> It is not clear to me why we are setting close completionHandler here, it seems it could clash with completionHandler from VideoFullscreenManagerProxy::requestFullscreenMode.

This one is for element fullscreen, the other is for video fullscreen. I originally thought they were mutually exclusive, but as Peng pointed out above it is possible so I think I need some sort of callback aggregator in [WKWebView closeAllMediaPresentations].
Comment 11 Kate Cheney 2021-01-22 14:06:07 PST
Comment on attachment 417903 [details]
Patch

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

>> Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:194
>> +    void requestFullscreenMode(HTMLMediaElementEnums::VideoFullscreenMode, bool finishedWithMedia, CompletionHandler<void(void)>&&) override;
> 
> final

Ok, I assume requestFullscreenMode in VideoFullscreenModelVideoElement and VideoFullscreenModelContext should also not be overridden?
Comment 12 Kate Cheney 2021-01-22 16:29:25 PST
Comment on attachment 417903 [details]
Patch

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

>>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:740
>>> +        return;
>> 
>> This is a bit problematic as most probably completionHandler will not be called if we return early, leading to a debug assert.
> 
> In the destructor I call it if it is not null, won't that mean no assert?

Oh -- I see what you are saying, this is maybe a flawed design because more than one callback can be associated with a single instance of this class.
Comment 13 Kate Cheney 2021-01-22 17:16:35 PST
Created attachment 418201 [details]
Patch
Comment 14 Kate Cheney 2021-01-22 17:17:16 PST
I think this will work. I append to a vector of completion handlers and call all of them when media presentations close.
Comment 15 Radar WebKit Bug Importer 2021-01-26 12:43:13 PST
<rdar://problem/73628499>
Comment 16 youenn fablet 2021-01-27 00:50:43 PST
Comment on attachment 418201 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:916
> +            model.requestFullscreenModeWithCallback(WebCore::HTMLMediaElementEnums::VideoFullscreenModeNone, false, [callbackAggregator] { });

callbackAggregator = WTFMove(callbackAggregator);

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:921
> +        fullScreenManager->closeWithCallback([callbackAggregator] { });

callbackAggregator = WTFMove(callbackAggregator);

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h:217
> +    Vector<CompletionHandler<void()>> m_closeAllMediaHandlers;

I would tend to name it m_closeCompletionHandlers.
And rename callCloseAllMediaHandlers accordingly.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:341
> +    ASSERT(m_closeAllMediaHandlers.isEmpty());

Why is this ASSERT guaranteed?

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:743
> +void VideoFullscreenManagerProxy::setCloseAllMediaPresentationsCallback(CompletionHandler<void()>&& completionHandler)

The name could be improved since we are appending a callback, not setting it.
I would tend to remove it and just append the callback in requestFullscreenModeWithCallback.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:760
> +    requestFullscreenMode(contextId, mode, finishedWithMedia);

What guarantees do we have didExitFullscreen will be called?
Let's say requestFullscreenModeWithCallback is called with mode none but the page is not in fullscreen.

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:54
> +    ASSERT(m_closeAllMediaHandlers.isEmpty());

Is it guaranteed?

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:95
> +    setCloseAllMediaPresentationsCallback(WTFMove(completionHandler));

Ditto for setCloseAllMediaPresentationsCallback.

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:83
> +    void setCloseAllMediaPresentationsCallback(CompletionHandler<void()>&&);

Can we make it private, or remove it?

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:91
> +    void callCloseAllMediaHandlers();

I would name it callCloseCompletionHandlers()

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:99
> +    Vector<CompletionHandler<void()>> m_closeAllMediaHandlers;

I would name it m_closeCompletionHandlers
Comment 17 Kate Cheney 2021-01-27 10:51:17 PST
Comment on attachment 418201 [details]
Patch

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

Thanks for the review, I will make these changes before landing.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:916
>> +            model.requestFullscreenModeWithCallback(WebCore::HTMLMediaElementEnums::VideoFullscreenModeNone, false, [callbackAggregator] { });
> 
> callbackAggregator = WTFMove(callbackAggregator);

Is it OK to WTFMove() the callbackAggregator if we might use it again below?

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:341
>> +    ASSERT(m_closeAllMediaHandlers.isEmpty());
> 
> Why is this ASSERT guaranteed?

It is not, I'll remove it.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:760
>> +    requestFullscreenMode(contextId, mode, finishedWithMedia);
> 
> What guarantees do we have didExitFullscreen will be called?
> Let's say requestFullscreenModeWithCallback is called with mode none but the page is not in fullscreen.

Ok, I will add a check for fullscreen or Picture in Picture so we know it will be called.

>> Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:54
>> +    ASSERT(m_closeAllMediaHandlers.isEmpty());
> 
> Is it guaranteed?

No, will remove.
Comment 18 Kate Cheney 2021-01-27 11:31:55 PST
Created attachment 418569 [details]
Patch for landing
Comment 19 Kate Cheney 2021-01-27 11:32:22 PST
Patch-for-landing has all changes except for WTFMove(callbackAggregator) because I did not think that was safe. I can post a followup patch if I am wrong.
Comment 20 EWS 2021-01-27 12:25:04 PST
Committed r271970: <https://trac.webkit.org/changeset/271970>

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