Bug 230784 – Promote WKPreferences._fullScreenEnabled to API

• Component: New Bugs
• Status: RESOLVED FIXED

[Peng Liu]

Promote WKPreferences._fullScreenEnabled to API

[Peng Liu]

<rdar://83255308>

[Peng Liu]

Created attachment 439239 [details]
WIP patch

[Peng Liu]

Created attachment 439363 [details]
WIP patch

[Peng Liu]

Created attachment 439820 [details]
WIP patch

[Peng Liu]

Created attachment 443901 [details]
[fast-cq] Patch

[Jer Noble]

Comment on attachment 443901 [details]
[fast-cq] 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)
> -@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.

[Darin Adler]

Comment on attachment 443901 [details]
[fast-cq] Patch

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

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:75
> +    enum class FullscreenState : uint8_t {
> +        NotInFullscreen,
> +        EnteringFullscreen,
> +        InFullscreen,
> +        ExitingFullscreen,
> +    };

Is there some way to keep this clear, but avoid saying Fullscreen twice every time we use any of these values?

[Darin Adler]

Comment on attachment 443901 [details]
[fast-cq] Patch

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

>> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:75
>> +    };
> 
> Is there some way to keep this clear, but avoid saying Fullscreen twice every time we use any of these values?

Like maybe we can use "NotIn", "Entering", "In", and "Exiting".

[Peng Liu]

Comment on attachment 443901 [details]
[fast-cq] Patch

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

>>> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:75
>>> +    };
>> 
>> Is there some way to keep this clear, but avoid saying Fullscreen twice every time we use any of these values?
> 
> Like maybe we can use "NotIn", "Entering", "In", and "Exiting".

Maybe "NotActive", "Entering", "Active", and "Exiting"?

[Peng Liu]

(In reply to Darin Adler from comment #8)
> Comment on attachment 443901 [details]
> [fast-cq] Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443901&action=review
> 
> >> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:75
> >> +    };
> > 
> > Is there some way to keep this clear, but avoid saying Fullscreen twice every time we use any of these values?
> 
> Like maybe we can use "NotIn", "Entering", "In", and "Exiting".

I will make this change in a follow-up patch.

[EWS]

Committed r285741 (244200@main): <https://commits.webkit.org/244200@main>

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