[webkit-reviews] review granted: [Bug 59778] Implement FULLSCREEN_API on Windows, Part 1: Stubs : [Attachment 91651] Patch

[bugzilla-daemon at webkit.org]

Jon Honeycutt <jhoneycutt at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 59778: Implement FULLSCREEN_API on Windows, Part 1: Stubs
https://bugs.webkit.org/show_bug.cgi?id=59778

Attachment 91651: Patch
https://bugs.webkit.org/attachment.cgi?id=91651&action=review

------- Additional Comments from Jon Honeycutt <jhoneycutt at apple.com>
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.