Bug 230784 - Promote WKPreferences._fullScreenEnabled to API
Summary: Promote WKPreferences._fullScreenEnabled to API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-24 23:09 PDT by Peng Liu
Modified: 2021-11-12 15:46 PST (History)
3 users (show)

See Also:


Attachments
WIP patch (31.24 KB, patch)
2021-09-24 23:18 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch (30.53 KB, patch)
2021-09-27 09:40 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch (15.25 KB, patch)
2021-09-30 22:19 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
[fast-cq] Patch (16.15 KB, patch)
2021-11-10 20:35 PST, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2021-09-24 23:09:31 PDT
Promote WKPreferences._fullScreenEnabled to API
Comment 1 Peng Liu 2021-09-24 23:16:11 PDT
<rdar://83255308>
Comment 2 Peng Liu 2021-09-24 23:18:09 PDT Comment hidden (obsolete)
Comment 3 Peng Liu 2021-09-27 09:40:18 PDT Comment hidden (obsolete)
Comment 4 Peng Liu 2021-09-30 22:19:53 PDT
Created attachment 439820 [details]
WIP patch
Comment 5 Peng Liu 2021-11-10 20:35:12 PST
Created attachment 443901 [details]
[fast-cq] Patch
Comment 6 Jer Noble 2021-11-11 10:57:58 PST
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.
Comment 7 Darin Adler 2021-11-11 11:53:27 PST
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?
Comment 8 Darin Adler 2021-11-11 11:53:59 PST
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".
Comment 9 Peng Liu 2021-11-11 12:22:05 PST
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"?
Comment 10 Peng Liu 2021-11-12 14:13:14 PST
(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.
Comment 11 EWS 2021-11-12 14:14:35 PST
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].