Implement FULLSCREEN_API on Windows, Part 1: Stubs
Created attachment 91640 [details] Patch
Attachment 91640 [details] did not build on win: Build output: http://queues.webkit.org/results/8519645
Created attachment 91649 [details] Patch E_NOIMPL -> E_NOTIMPL
Attachment 91649 [details] did not build on win: Build output: http://queues.webkit.org/results/8524069
Created attachment 91651 [details] Patch
Phew! finally. All ews bots are green. Form voltron.
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 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 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 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 )
Committed r85389: <http://trac.webkit.org/changeset/85389>