[webkit-reviews] review granted: [Bug 220741] WKWebView closeAllMediaPresentations API does not have a completion handler : [Attachment 418201] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted katherine_cheney at apple.com's
request for review:
Bug 220741: WKWebView closeAllMediaPresentations API does not have a completion
handler
https://bugs.webkit.org/show_bug.cgi?id=220741

Attachment 418201: Patch

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




--- Comment #16 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 418201
  --> https://bugs.webkit.org/attachment.cgi?id=418201
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::VideoFu
llscreenModeNone, 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(CompletionHa
ndler<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