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

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted 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 92052: Patch
https://bugs.webkit.org/attachment.cgi?id=92052&action=review

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

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:118
> +    m_private->m_backgroundWindow = adoptPtr(new
MediaPlayerPrivateFullscreenWindow(m_private.get()));
> +    m_private->m_backgroundWindow->createWindow(0);
> +    ::AnimateWindow(m_private->m_backgroundWindow->hwnd(),
kFullScreenAnimationDuration, AW_BLEND | AW_ACTIVATE);

Is the job of the background window just to draw black? If so, would it be
better to have a simpler window class that just did that?

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:134
> +    ::InvalidateRect(m_private->m_client->fullScreenClientWindow(), 0,
true);
> +    ::UpdateWindow(m_private->m_client->fullScreenClientWindow());

You might be able to combine these into a single ::RedrawWindow call.

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:135
> +    m_private->m_originalHost = originalHost;

Why do we need the originalHost local? Can't we just use the member variable
from the start?

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:154
> +    ::DestroyWindow(m_private->m_fullScreenWindow->hwnd());
> +    m_private->m_fullScreenWindow = 0;

I think m_fullScreenWindow will call ::DestroyWindow for you when it is
destroyed.

Please use nullptr. Using 0 won't work anymore when we turn on strict
PassOwnPtr.

> Source/WebCore/platform/graphics/win/FullScreenController.cpp:160
> +    ASSERT(m_private->m_backgroundWindow);
> +    success = ::AnimateWindow(m_private->m_backgroundWindow->hwnd(),
kFullScreenAnimationDuration, AW_HIDE | AW_BLEND);
> +}

Why don't we need to clear out m_backgroundWindow?

> Source/WebCore/platform/graphics/win/FullScreenControllerClient.h:37
> +    virtual void setFullScreenClientParentWindow(HWND) = 0;

Maybe "fullScreenClientSetParentWindow" would be better? Then all the functions
will have a common "fullScreenClient" prefix.

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

> +	       ::FillRect(hdc, &ps.rcPaint,
(HBRUSH)::GetStockObject(BLACK_BRUSH));

Please use static_cast,

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

> +	       HDC context = (HDC)wParam;
> +	       ::GetClientRect(m_hwnd, &clientRect);
> +	       ::FillRect(context, &clientRect,
(HBRUSH)::GetStockObject(BLACK_BRUSH));
> +	   }

Please use C++ casts.

Why don't we need to implement WM_PRINTCLIENT in the case where we have a layer
host? Won't that make the ::AnimateWindow call for the full screen window not
work?

> Source/WebKit/win/WebView.cpp:6813
> +void WebView::fullScreenClientWillEnterFullScreen()
> +{
> +    ASSERT(m_fullScreenElement);
> +   
m_fullScreenElement->document()->webkitWillEnterFullScreenForElement(m_fullScre
enElement.get());
> +}
> +
> +void WebView::fullScreenClientDidEnterFullScreen()
> +{
> +    ASSERT(m_fullScreenElement);
> +   
m_fullScreenElement->document()->webkitDidEnterFullScreenForElement(m_fullScree
nElement.get());
> +}
> +
> +void WebView::fullScreenClientWillExitFullScreen()
> +{
> +    ASSERT(m_fullScreenElement);
> +   
m_fullScreenElement->document()->webkitWillExitFullScreenForElement(m_fullScree
nElement.get());
> +}
> +
> +void WebView::fullScreenClientDidExitFullScreen()
> +{
> +    ASSERT(m_fullScreenElement);
> +   
m_fullScreenElement->document()->webkitDidExitFullScreenForElement(m_fullScreen
Element.get());
> +    m_fullScreenElement = nullptr;
> +}

Seems like this code could move into WebCore at some point, too. Can't the
controller just keep track of this itself? I guess the WebKit2 implementation
is different.

>> Source/WebKit2/UIProcess/win/WebView.h:48
>> +	class FullScreenController;
> 
> Code inside a namespace should not be indented.  [whitespace/indent] [4]

Silly style bot!

> Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:69
> +    // FIXME: Disable setting RenderLayer::setAnimating() to make this
unnecessary.s

Typo: unnecessary.s