[webkit-reviews] review granted: [Bug 230784] Promote WKPreferences._fullScreenEnabled to API : [Attachment 443901] Patch

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has granted Peng Liu <peng.liu6 at apple.com>'s
request for review:
Bug 230784: Promote WKPreferences._fullScreenEnabled to API
https://bugs.webkit.org/show_bug.cgi?id=230784

Attachment 443901: Patch

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




--- Comment #6 from Jer Noble <jer.noble at apple.com> ---
Comment on attachment 443901
  --> https://bugs.webkit.org/attachment.cgi?id=443901
Patch

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

R=me with a nit:

> Source/WebKit/UIProcess/Cocoa/FullscreenClient.h:-40
> -#if PLATFORM(MAC)
> - at class NSView;
> -using WKFullscreenClientView = NSView;
> -#else
>  @class WKWebView;
>  using WKFullscreenClientView = WKWebView;
> -#endif

Nit: it seems like we could just get rid of WKFullscreenClientView entirely and
just use WKWebView everywhere, no?

> Source/WebKit/UIProcess/Cocoa/FullscreenClient.mm:66
> -	   [m_delegate.get() _webViewWillEnterFullscreen:m_webView];
> +	   [m_delegate.get() _webViewWillEnterFullscreen:static_cast<NSView
*>(m_webView)];

Nit: not sure why this was is necessary; can't we just #include "WKWebView.h"
here and get the cast for free?

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:77
> +    enum class FullscreenState : uint8_t {
> +	   NotInFullscreen,
> +	   EnteringFullscreen,
> +	   InFullscreen,
> +	   ExitingFullscreen,
> +    };
> +    FullscreenState fullscreenState() const { return m_fullscreenState; }
> +

Oooh, I just added something very similar to this inside WebKitTestRunner, with
almost exactly the same values. Once this lands, I could get rid of that enum
and adopt this one.