Bug 59778

Summary: Implement FULLSCREEN_API on Windows, Part 1: Stubs
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: New BugsAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 59785, 59845    
Attachments:
Description Flags
Patch
none
Patch
none
Patch jhoneycutt: review+

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>