Bug 220741 – WKWebView closeAllMediaPresentations API does not have a completion handler

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

This call involves async operations and should have a completion handler so clients know when it has completed.

[Kate Cheney]

<rdar://problem/73045904>

[Kate Cheney]

Created attachment 417901 [details]
Patch

[Kate Cheney]

Created attachment 417903 [details]
Patch

[Kate Cheney]

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?

[Peng Liu]

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.

[youenn fablet]

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

[Peng Liu]

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.

[youenn fablet]

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

[Kate Cheney]

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

[Kate Cheney]

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

[Kate Cheney]

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?

[Kate Cheney]

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.

[Kate Cheney]

Created attachment 418201 [details]
Patch

[Kate Cheney]

I think this will work. I append to a vector of completion handlers and call all of them when media presentations close.

[Radar WebKit Bug Importer]

<rdar://problem/73628499>

[youenn fablet]

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

[Kate Cheney]

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.

[Kate Cheney]

Created attachment 418569 [details]
Patch for landing

[Kate Cheney]

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.

[EWS]

Committed r271970: <https://trac.webkit.org/changeset/271970>

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