Summary: | WKWebView closeAllMediaPresentations API does not have a completion handler | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kate Cheney <katherine_cheney> | ||||||||||
Component: | WebKit Misc. | Assignee: | Kate Cheney <katherine_cheney> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, glenn, jer.noble, peng.liu6, philipj, sergio, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=221343 | ||||||||||||
Attachments: |
|
Description
Kate Cheney
2021-01-19 12:42:34 PST
Created attachment 417901 [details]
Patch
Created attachment 417903 [details]
Patch
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 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 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 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. > > 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?
(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 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 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 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. Created attachment 418201 [details]
Patch
I think this will work. I append to a vector of completion handlers and call all of them when media presentations close. 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 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. Created attachment 418569 [details]
Patch for landing
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. Committed r271970: <https://trac.webkit.org/changeset/271970> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418569 [details]. |