Bug 56318

Summary: WebKit2: WKTR should support WebKit2 full screen APIs
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: WebKit2Assignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch sam: review+, commit-queue: commit-queue-

Description Jer Noble 2011-03-14 10:58:25 PDT
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.
Comment 1 Jer Noble 2011-04-05 08:56:41 PDT
And by DRT, I mean WebKitTestRunner.
Comment 2 Jer Noble 2011-04-05 17:07:55 PDT
Created attachment 88338 [details]
Patch
Comment 3 Jer Noble 2011-04-05 17:32:45 PDT
Whoops, missed the WebKit2/ChangeLog.  But that's ok, as it gives the a chance to rebase and re-upload anyway.
Comment 4 Jer Noble 2011-04-05 17:37:28 PDT
Created attachment 88344 [details]
Patch
Comment 5 Build Bot 2011-04-05 20:16:09 PDT
Attachment 88344 [details] did not build on win:
Build output: http://queues.webkit.org/results/8341043
Comment 6 Jer Noble 2011-04-05 21:30:03 PDT
Created attachment 88362 [details]
Patch

Forgot one #if ENABLE(FULLSCREEN_API) in WebPage.h.
Comment 7 Early Warning System Bot 2011-04-05 21:40:15 PDT
Attachment 88362 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8341071
Comment 8 Jer Noble 2011-04-05 21:47:28 PDT
Created attachment 88363 [details]
Patch

Corrected the order of #endifs and end-braces in InjectedBundlePageFullScreenClient.h.
Comment 9 Build Bot 2011-04-05 21:53:57 PDT
Attachment 88362 [details] did not build on win:
Build output: http://queues.webkit.org/results/8345052
Comment 10 Early Warning System Bot 2011-04-05 22:00:44 PDT
Attachment 88363 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8341073
Comment 11 Jer Noble 2011-04-05 22:11:20 PDT
Created attachment 88366 [details]
Patch

Made the #if in WKBundlePage.cpp more inclusive.
Comment 12 Build Bot 2011-04-05 22:53:29 PDT
Attachment 88363 [details] did not build on win:
Build output: http://queues.webkit.org/results/8342047
Comment 13 Sam Weinig 2011-04-12 17:02:40 PDT
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.
Comment 14 Jer Noble 2011-04-12 21:16:54 PDT
(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.
Comment 15 Jer Noble 2011-04-12 22:23:29 PDT
Created attachment 89339 [details]
Patch
Comment 16 WebKit Commit Bot 2011-04-15 15:01:57 PDT
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
Comment 17 Jer Noble 2011-04-15 15:06:40 PDT
Well, I'll just land it manually then.
Comment 18 Jer Noble 2011-04-18 16:29:12 PDT
Committed r84206: <http://trac.webkit.org/changeset/84206>