Bug 59785 – Implement FULLSCREEN_API on Windows, Part 2: WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Implement FULLSCREEN_API on Windows, Part 2: WebKit

[Jer Noble]

Created attachment 91653 [details]
Patch

[Jer Noble]

This patch depends on the one in bug #59778, which seems to be why it's not applying cleanly.

[Jer Noble]

Created attachment 91776 [details]
Patch

Build fix.

[Adam Roben (:aroben)]

Comment on attachment 91776 [details]
Patch

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.

[Jer Noble]

Comment on attachment 91776 [details]
Patch

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

Lowered.

>> 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?

Space removed.

Because a struct with a constructor looks weird?

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

Too true.  Removed.

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

Replaced.

>> Source/WebKit/win/WebFullScreenController.cpp:51
>> +    { }
> 
> Please put these on their own lines.

Done.

>> 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?

Yes, this proc isn't responsible for other messages; they're handled by MediaPlayerPrivateFullscreenWindow.  But in the case of WM_KEYDOWN, I wanted to make sure not to break any of the default key code handling.  It looks like you're right, in that I should set lResult to 1 here, to indicate this message wasn't handled by the application.

Actually, the other place where this pattern is used (FullscreenVideoController), the client proc always passes messages to DefWindowProc, so perhaps I do need to add a default: to my case statement.

>> 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.

Adopted.

>> Source/WebKit/win/WebFullScreenController.cpp:168
>> +    ::DestroyWindow(m_private->hostWindow);
> 
> Why bother hiding before destroying?

Paranoia.

>> Source/WebKit/win/WebFullScreenController.h:33
>> +#include <WTF/RefPtr.h>
> 
> Lowercase wtf, please.

Lowered.

>> Source/WebKit/win/WebFullScreenController.h:38
>> +    class IntSize;
> 
> I don't think these are needed.

You're right, they're leftovers.  Removed.

>> Source/WebKit/win/WebFullScreenController.h:60
>> +    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?

I like the pimpl pattern.  It means I can have something approaching the equivalent of categories in ObjC.  It also reduces compile times for everyone who includes this file.

>> Source/WebKit/win/WebView.cpp:6755
>> +    return enabled;
> 
> You could turn this into a single return statement.

That would be a pretty long and complicated return statement, wouldn't it?  You mean something like?:

BOOL enabled = FALSE;
return withKeyboard && m_preferences && SUCCEEDED(m_preferences->isFullScreenEnabled(&enabled)) && enabled;

>> Source/WebKit/win/WebView.h:947
>> +    WebFullScreenController* fullScreenController();
> 
> Maybe this should be a const member function and m_newFullScreenController should be mutable?

That would work.

>> 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.

Sure thing; I'll rename the old ivar and make this one more normal.

Thanks!

[Jer Noble]

(In reply to comment #5)
> (From update of attachment 91776 [details])
> >> Source/WebKit/win/WebView.h:947
> >> +    WebFullScreenController* fullScreenController();
> > 
> > Maybe this should be a const member function and m_newFullScreenController should be mutable?
> 
> That would work.

Signs point to "no".  WebFullScreenController needs a non-const pointer to WebView, and if this function is const, the this pointer is as well.

[Jer Noble]

Committed r85393: <http://trac.webkit.org/changeset/85393>