[webkit-reviews] review denied: [Bug 59845] Implement FULLSCREEN_API on Windows, Part 3: WebKit2 : [Attachment 91966] 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 91966: Patch
https://bugs.webkit.org/attachment.cgi?id=91966&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=91966&action=review

I don't see WebFullScreenControllerClient defined anywhere, so I only reviewed
part of the patch.

> Source/WebCore/ChangeLog:11
> +	   behalf by WebKit and WebKit2.  MediaPlayerPrivateFullscreenWindow
now only creates a 
> +	   CALayerHost once a root layer is set.

Why is this last part needed? (You should probably explain in the ChangeLog.)

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:27527
> +					       
RelativePath="..\platform\graphics\win\WebFullScreenController.cpp"

We don't normally use the "Web" prefix for WebCore classes. Can we rename this
to FullScreenController in another patch?

>
Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:107

> +	   m_layerTreeHost = 0;

Please use nullptr.

>
Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.cpp:112

> +    if (!m_layerTreeHost)
> +	   m_layerTreeHost = CACFLayerTreeHost::create();

Looks like we never call CACFLayerTreeHost::setWindow in the lazy-init case?
How can rendering work if we don't do that?

> Source/WebCore/platform/graphics/win/MediaPlayerPrivateFullscreenWindow.h:61
> +    void createWindow(HWND ownerWindow, bool startVisible = true);

Optional boolean arguments make me so sad! They're usually unreadable at the
call site ("what does true stand for?"), and they're easy for callers to misuse
(because so many types can be implicitly converted to bool). I'd suggest making
it a non-optional enum. Or maybe we should never start visible and have it be
the caller's responsibility to call ShowWindow as needed.

> Source/WebCore/platform/graphics/win/WebFullScreenController.cpp:57
> +    virtual ~Private() { }

The compiler should do this for you.