Bug 59778 – Implement FULLSCREEN_API on Windows, Part 1: Stubs

• Component: New Bugs
• Status: RESOLVED FIXED

[Jer Noble]

Implement FULLSCREEN_API on Windows, Part 1: Stubs

[Jer Noble]

Created attachment 91640 [details]
Patch

[Build Bot]

Attachment 91640 [details] did not build on win:
Build output: http://queues.webkit.org/results/8519645

[Jer Noble]

Created attachment 91649 [details]
Patch

E_NOIMPL -> E_NOTIMPL

[Build Bot]

Attachment 91649 [details] did not build on win:
Build output: http://queues.webkit.org/results/8524069

[Jer Noble]

Created attachment 91651 [details]
Patch

[Jer Noble]

Phew! finally.  All ews bots are green.  Form voltron.

[Jon Honeycutt]

Comment on attachment 91651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91651&action=review

> Source/WebKit/win/ChangeLog:12
> +        * Interfaces/IWebPreferencesPrivate.idl: Add funcitons for enabling

Typo: funcitons.

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:881
> +bool WebChromeClient::supportsFullScreenForElement(const Element* element, bool withKeyboard)

I'm not sure "withKeyboard" is clear - maybe "elementWantsKeyboardAccess"?

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:893
> +    return FALSE

Missing trailing ;

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:902
> +        if (uiDelegatePrivate4 && SUCCEEDED(uiDelegatePrivate4->enterFullScreenForElement(domElement.get())))

No need to test whether the call succeeded.

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:913
> +        if (uiDelegatePrivate4 && SUCCEEDED(uiDelegatePrivate4->exitFullScreenForElement(domElement.get())))

Ditto.

> Source/WebKit/win/WebPreferences.cpp:1535
> +#if ENABLE(FULLSCREEN_API)
> +    *enabled = boolValueForKey(CFSTR(WebKitFullScreenEnabledPreferenceKey));
> +    return S_OK;

I think we usually null check these out params and return E_POINTER if null.

> Source/WebKit/win/WebPreferences.cpp:1537
> +#else
> +    return E_NOTIMPL;

It might be good to set enabled to FALSE in this case, even though we return E_NOTIMPL.

> Source/WebKit2/UIProcess/win/WebFullScreenManagerProxyWin.cpp:3
> +/*
> + * Copyright (C) 2010 Apple Inc. All rights reserved.
> + *

It's 2011, man.

[Jer Noble]

Comment on attachment 91651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91651&action=review

>> Source/WebKit/win/ChangeLog:12
>> +        * Interfaces/IWebPreferencesPrivate.idl: Add funcitons for enabling
> 
> Typo: funcitons.

Fxied.

>> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:881
>> +bool WebChromeClient::supportsFullScreenForElement(const Element* element, bool withKeyboard)
> 
> I'm not sure "withKeyboard" is clear - maybe "elementWantsKeyboardAccess"?

How about "requestingKeyboardAccess"?

>> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:893
>> +    return FALSE
> 
> Missing trailing ;

Added.

>> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:902
>> +        if (uiDelegatePrivate4 && SUCCEEDED(uiDelegatePrivate4->enterFullScreenForElement(domElement.get())))
> 
> No need to test whether the call succeeded.

Not yet, no.  But in the WebKit patch that follows, we'll want to know whether this function succeeds or not;  if not, we want to provide the default behavior.

>> Source/WebKit/win/WebPreferences.cpp:1535
>> +    return S_OK;
> 
> I think we usually null check these out params and return E_POINTER if null.

Good idea.  Added.

>> Source/WebKit/win/WebPreferences.cpp:1537
>> +    return E_NOTIMPL;
> 
> It might be good to set enabled to FALSE in this case, even though we return E_NOTIMPL.

Hm; I'd say the opposite.  If a function isn't implemented, you wouldn't expect it to modify your input would you?  Anyway, the callGetter() in COMUtilities handles the error case correctly.

>> Source/WebKit2/UIProcess/win/WebFullScreenManagerProxyWin.cpp:3
>> + *
> 
> It's 2011, man.

I blame my iPhone.

[Adam Roben (:aroben)]

Comment on attachment 91651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91651&action=review

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:884
> +    if (SUCCEEDED(m_webView->uiDelegate(&uiDelegate))) {

Early return!

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:890
> +        if (uiDelegatePrivate4 && SUCCEEDED(uiDelegatePrivate4->supportsFullScreenForElement(domElement.get(), withKeyboard, &supports)))
> +            return supports;

You could just turn this into a return statement with another &&.E

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:899
> +    if (SUCCEEDED(m_webView->uiDelegate(&uiDelegate))) {

Early return!E

> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:910
> +    if (SUCCEEDED(m_webView->uiDelegate(&uiDelegate))) {

Early return!

> Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:25
> + */
> +#include "config.h"

Newline!

[Jer Noble]

Comment on attachment 91651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91651&action=review

>> Source/WebKit/win/WebCoreSupport/WebChromeClient.cpp:884
>> +    if (SUCCEEDED(m_webView->uiDelegate(&uiDelegate))) {
> 
> Early return!

Stay tuned for "Part 2", where the lack of an early return will become clear! (We want to fall through to the bottom, where default behavior will be added.)

This has tripped up a couple of people now, so I guess i shouldn't have broken up the function in two patches this way.

>> Source/WebKit2/WebProcess/FullScreen/win/WebFullScreenManagerWin.cpp:25
>> +#include "config.h"
> 
> Newline!

I guess I should have hit *return earlier*!  (See what I did there? :-D )

[Jer Noble]

Committed r85389: <http://trac.webkit.org/changeset/85389>