[webkit-reviews] review denied: [Bug 59845] Implement FULLSCREEN_API on Windows, Part 3: WebKit2 : [Attachment 91984] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 59845: Implement FULLSCREEN_API on Windows, Part 3: WebKit2
https://bugs.webkit.org/show_bug.cgi?id=59845

Attachment 91984: Patch
https://bugs.webkit.org/attachment.cgi?id=91984&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
This is close! Just a few more things to clean up from the move into WebCore.

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

> Source/WebCore/platform/graphics/win/FullScreenController.h:47
> +class FullScreenControllerClient {
> +public:
> +    virtual HWND webViewWindow() const = 0;
> +    virtual HWND webViewHostWindow() const = 0;
> +    virtual void setWebViewHostWindow(HWND) = 0;
> +    virtual void webViewWillEnterFullScreen() = 0;
> +    virtual void webViewDidEnterFullScreen() = 0;
> +    virtual void webViewWillExitFullScreen() = 0;
> +    virtual void webViewDidExitFullScreen() = 0;
> +protected:
> +    virtual ~FullScreenControllerClient() { }
> +};

I usually prefer to put clients in their own header file so that headers of
derived classes don't end up pulling in the whole non-client class declaration.


"WebView" isn't something that WebCore should know about, so we'll need to
change these names somehow. You can probably just remove "webView" from the
will/did functions.

I'd suggest using "parent window" instead of "host window". The "host window"
terminology in WebKit1 is misleading (it was based on WebKit/mac's "host
window" concept, but isn't the same thing), and conflicts with
WebCore::HostWindow.

Chrome::platformPageClient can be used in place of webViewWindow (though it has
such an awful name!). Maybe we should move [set]WebViewHostWindow there, too
(if we give them appropriate names)?

>>>
Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:65
>>> +	 typedef bool StartingVisibilityFlag;
>> 
>> Is there a reason you typedef'd this to bool instead of just doing:
>> 
>> enum StartingVisibilityFlag {
>>     StartInvisible,
>>     StartVisible
>> };
>> 
>> ?
> 
> Yes, passing enums directly into functions have led to a number of compiler
errors in the past: whereas defining an enum twice will generate a compiler
error, it's trivial to redefine a typedef.  (This exact issue recently came up
when adding a function to WKSI.)

Can you point to a specific example where this was a problem? We use enum
parameters in lots of places in WebKit/WebCore. I think it's definitely
preferable to typedefing to bool. (But I'd also support making this behavior
non-configurable and leaving it up to the caller.)

> Source/WebKit/win/WebView.cpp:6808
> +    m_fullScreenElement = 0;

Please use nullptr.

> Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:72
> +    m_fullScreenRootLayer = layer;

I don't see any code that ever renders this layer. Am I missing something?