Bug 59778 - Implement FULLSCREEN_API on Windows, Part 1: Stubs
Summary: Implement FULLSCREEN_API on Windows, Part 1: Stubs
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:
Blocks: 59785 59845
  Show dependency treegraph
 
Reported: 2011-04-28 22:31 PDT by Jer Noble
Modified: 2011-04-29 21:33 PDT (History)
1 user (show)

See Also:


Attachments
Patch (20.13 KB, patch)
2011-04-28 22:54 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (20.13 KB, patch)
2011-04-29 00:55 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (20.13 KB, patch)
2011-04-29 01:27 PDT, Jer Noble
jhoneycutt: 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-28 22:31:08 PDT
Implement FULLSCREEN_API on Windows, Part 1: Stubs
Comment 1 Jer Noble 2011-04-28 22:54:24 PDT
Created attachment 91640 [details]
Patch
Comment 2 Build Bot 2011-04-28 23:15:31 PDT
Attachment 91640 [details] did not build on win:
Build output: http://queues.webkit.org/results/8519645
Comment 3 Jer Noble 2011-04-29 00:55:44 PDT
Created attachment 91649 [details]
Patch

E_NOIMPL -> E_NOTIMPL
Comment 4 Build Bot 2011-04-29 01:15:39 PDT
Attachment 91649 [details] did not build on win:
Build output: http://queues.webkit.org/results/8524069
Comment 5 Jer Noble 2011-04-29 01:27:28 PDT
Created attachment 91651 [details]
Patch
Comment 6 Jer Noble 2011-04-29 01:48:38 PDT
Phew! finally.  All ews bots are green.  Form voltron.
Comment 7 Jon Honeycutt 2011-04-29 18:08:30 PDT
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.
Comment 8 Jer Noble 2011-04-29 18:30:28 PDT
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.
Comment 9 Adam Roben (:aroben) 2011-04-29 18:35:07 PDT
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!
Comment 10 Jer Noble 2011-04-29 18:40:22 PDT
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 )
Comment 11 Jer Noble 2011-04-29 21:33:42 PDT
Committed r85389: <http://trac.webkit.org/changeset/85389>