Bug 59785 - Implement FULLSCREEN_API on Windows, Part 2: WebKit
Summary: Implement FULLSCREEN_API on Windows, Part 2: WebKit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on: 59778
Blocks: 59798
  Show dependency treegraph
 
Reported: 2011-04-29 01:51 PDT by Jer Noble
Modified: 2011-04-30 01:15 PDT (History)
0 users

See Also:


Attachments
Patch (16.45 KB, patch)
2011-04-29 02:10 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (16.70 KB, patch)
2011-04-29 17:51 PDT, Jer Noble
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-04-29 01:51:52 PDT
Implement FULLSCREEN_API on Windows, Part 2: WebKit
Comment 1 Jer Noble 2011-04-29 02:10:49 PDT
Created attachment 91653 [details]
Patch
Comment 2 Jer Noble 2011-04-29 09:06:51 PDT
This patch depends on the one in bug #59778, which seems to be why it's not applying cleanly.
Comment 3 Jer Noble 2011-04-29 17:51:22 PDT
Created attachment 91776 [details]
Patch

Build fix.
Comment 4 Adam Roben (:aroben) 2011-04-29 18:46:37 PDT
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.
Comment 5 Jer Noble 2011-04-29 19:31:46 PDT
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!
Comment 6 Jer Noble 2011-04-29 20:36:37 PDT
(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.
Comment 7 Jer Noble 2011-04-30 01:15:34 PDT
Committed r85393: <http://trac.webkit.org/changeset/85393>