[webkit-reviews] review granted: [Bug 59785] Implement FULLSCREEN_API on Windows, Part 2: WebKit : [Attachment 91776] 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 59785: Implement FULLSCREEN_API on Windows, Part 2: WebKit
https://bugs.webkit.org/show_bug.cgi?id=59785

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

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

> Source/WebKit/win/WebFullScreenController.cpp:34
> +#include <WTF/RefPtr.h>

Lowercase wtf please.E

> Source/WebKit/win/WebFullScreenController.cpp:42
> +class WebFullScreenController::Private  : public
MediaPlayerPrivateFullscreenClient {

Extra space after Private.

Why not use struct, since all the members are public?

> Source/WebKit/win/WebFullScreenController.cpp:47
> +	   , fullScreenWindow(0)

No need for this.

> Source/WebKit/win/WebFullScreenController.cpp:50
> +	   , isFullScreen(0)

Should be false, not 0.

> Source/WebKit/win/WebFullScreenController.cpp:51
> +    { }

Please put these on their own lines.

> Source/WebKit/win/WebFullScreenController.cpp:88
> +	       lResult = ::DefWindowProc(hwnd, msg, wParam, lParam);

Why do we only need to call ::DefWindowProc in this one case? What about all
the messages we don't handle? Maybe fullscreenClientWndProc isn't responsible
for taking care of messages it doesn't handle, but in that case why are we
calling ::DefWindowProc here?

> Source/WebKit/win/WebFullScreenController.cpp:98
> +    : m_private(new WebFullScreenController::Private(this, webView))

Please use adoptPtr or add a ::create function to the Private class.

> Source/WebKit/win/WebFullScreenController.cpp:168
> +    ::ShowWindow(m_private->hostWindow, SW_HIDE);
> +    ::DestroyWindow(m_private->hostWindow);

Why bother hiding before destroying?

> Source/WebKit/win/WebFullScreenController.h:33
> +#include <WTF/OwnPtr.h>
> +#include <WTF/RefPtr.h>

Lowercase wtf, please.

> Source/WebKit/win/WebFullScreenController.h:38
> +    class GraphicsLayer;
> +    class IntSize;

I don't think these are needed.

> Source/WebKit/win/WebFullScreenController.h:60
> +protected:
> +    class Private;
> +    friend class Private;
> +    OwnPtr<WebFullScreenController::Private> m_private;

Why use the pimpl pattern here, given that there are so many places in WebKit
where we could but don't use it?

> Source/WebKit/win/WebView.cpp:6755
> +    if (!m_preferences ||
FAILED(m_preferences->isFullScreenEnabled(&enabled)))
> +	   return false;
> +
> +    return enabled;

You could turn this into a single return statement.

> Source/WebKit/win/WebView.h:947
> +    WebFullScreenController* fullScreenController();

Maybe this should be a const member function and m_newFullScreenController
should be mutable?

> Source/WebKit/win/WebView.h:1106
> +    OwnPtr<WebFullScreenController> m_newFullScreenController;

"new" isn't so great. It's better to give old things the weird name and use
normal names for "new" things.