Bug 56318 – WebKit2: WKTR should support WebKit2 full screen APIs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jer Noble]

Currently, DRT only supports WebKit full screen APIs. Now that Bug #56264 has landed, DRT should be able to run the WebKit2 full screen layout tests.

[Jer Noble]

And by DRT, I mean WebKitTestRunner.

[Jer Noble]

Created attachment 88338 [details]
Patch

[Jer Noble]

Whoops, missed the WebKit2/ChangeLog.  But that's ok, as it gives the a chance to rebase and re-upload anyway.

[Jer Noble]

Created attachment 88344 [details]
Patch

[Build Bot]

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

[Jer Noble]

Created attachment 88362 [details]
Patch

Forgot one #if ENABLE(FULLSCREEN_API) in WebPage.h.

[Early Warning System Bot]

Attachment 88362 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8341071

[Jer Noble]

Created attachment 88363 [details]
Patch

Corrected the order of #endifs and end-braces in InjectedBundlePageFullScreenClient.h.

[Build Bot]

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

[Early Warning System Bot]

Attachment 88363 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8341073

[Jer Noble]

Created attachment 88366 [details]
Patch

Made the #if in WKBundlePage.cpp more inclusive.

[Build Bot]

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

[Sam Weinig]

Comment on attachment 88366 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:143
> +void WKBundlePageWillEnterFullScreenForElement(WKBundlePageRef pageRef, WKBundleNodeHandleRef elementRef)
> +{
> +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> +    if (!coreNode || !coreNode->isElementNode())
> +        return;
> +
> +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> +    element->document()->webkitWillEnterFullScreenForElement(element);
> +}
> +
> +void WKBundlePageDidEnterFullScreenForElement(WKBundlePageRef pageRef, WKBundleNodeHandleRef elementRef)
> +{
> +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> +    if (!coreNode || !coreNode->isElementNode())
> +        return;
> +    
> +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> +    element->document()->webkitDidEnterFullScreenForElement(element);
> +}
> +
> +void WKBundlePageWillExitFullScreenForElement(WKBundlePageRef pageRef, WKBundleNodeHandleRef elementRef)
> +{
> +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> +    if (!coreNode || !coreNode->isElementNode())
> +        return;
> +    
> +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> +    element->document()->webkitWillExitFullScreenForElement(element);
> +}
> +
> +void WKBundlePageDidExitFullScreenForElement(WKBundlePageRef pageRef, WKBundleNodeHandleRef elementRef)
> +{
> +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> +    if (!coreNode || !coreNode->isElementNode())
> +        return;
> +    
> +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> +    element->document()->webkitDidExitFullScreenForElement(element);
> +}

All the logic should be in WebPage, not in the API layer.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:247
> +#if defined(ENABLE_FULLSCREEN_API) && ENABLE_FULLSCREEN_API
> +// Full Screen client

Please don't put #ifdefs like these in API headers.  Clients will not not know the correct value to set.  Instead, the functions should just be no-ops if the feature is not supported.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:257
> +    WKBundlePageEnterFullScreenForElement                               enterFullScreenForElement;
> +    WKBundlePageExitFullScreenForElement                                exitFullScreenForElement;

We usually prefix the set type of clients with Will or Did.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFullScreenClient.h:46
> +    bool supportsFullScreen(WebPage*, bool withKeyboard);

Instead of a bool, this should probably be an enum.

[Jer Noble]

(In reply to comment #13)
> (From update of attachment 88366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88366&action=review
> 
> > +void WKBundlePageDidExitFullScreenForElement(WKBundlePageRef pageRef, WKBundleNodeHandleRef elementRef)
> > +{
> > +    WebCore::Node* coreNode = toImpl(elementRef)->coreNode();
> > +    if (!coreNode || !coreNode->isElementNode())
> > +        return;
> > +    
> > +    WebCore::Element* element = static_cast<WebCore::Element*>(coreNode);
> > +    element->document()->webkitDidExitFullScreenForElement(element);
> > +}
> 
> All the logic should be in WebPage, not in the API layer.

Done.  (Well, in the page->fullScreenController())

> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:247
> > +#if defined(ENABLE_FULLSCREEN_API) && ENABLE_FULLSCREEN_API
> > +// Full Screen client
> 
> Please don't put #ifdefs like these in API headers.  Clients will not not know the correct value to set.  Instead, the functions should just be no-ops if the feature is not supported.

Ah, I saw ENABLE_INSPECTOR in there and thought it was kosher.  Unifdefd.

> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:257
> > +    WKBundlePageEnterFullScreenForElement                               enterFullScreenForElement;
> > +    WKBundlePageExitFullScreenForElement                                exitFullScreenForElement;
> 
> We usually prefix the set type of clients with Will or Did.

These are commands, not notifications.  The will and did notifications come the other direction, and are also defined in this header.  Or maybe I'm misunderstanding what "the set type of clients" means?

> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageFullScreenClient.h:46
> > +    bool supportsFullScreen(WebPage*, bool withKeyboard);
> 
> Instead of a bool, this should probably be an enum.

Sure thing boss.

[Jer Noble]

Created attachment 89339 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 89339 [details]
Patch

Rejecting attachment 89339 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
atching file Source/WebKit2/WebProcess/WebPage/WebPage.h
Hunk #4 succeeded at 610 (offset 3 lines).
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp
patching file Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.h
patching file Tools/WebKitTestRunner/TestController.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Sam Weinig', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8452238

[Jer Noble]

Well, I'll just land it manually then.

[Jer Noble]

Committed r84206: <http://trac.webkit.org/changeset/84206>