Bug 70477 – [chromium] Improve fullscreen API

• Component: WebKit API
• Status: RESOLVED FIXED

[Darin Fisher (:fishd, Google)]

[chromium] Improve fullscreen API

[Darin Fisher (:fishd, Google)]

Created attachment 111916 [details]
v1 patch

[WebKit Review Bot]

Attachment 111916 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/public/WebWidget.h:72:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebWidgetClient.h:87:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebWidgetClient.h:89:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Created attachment 111918 [details]
v1.1 patch

Revised ChangeLog.

[WebKit Review Bot]

Attachment 111918 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/public/WebWidget.h:72:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebWidgetClient.h:87:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebWidgetClient.h:89:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 111918 [details]
v1.1 patch

Attachment 111918 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10178774

New failing tests:
fullscreen/full-screen-placeholder.html
fullscreen/full-screen-remove-ancestor-after.html
fullscreen/full-screen-remove-sibling.html
fullscreen/full-screen-keyboard-disabled.html
fullscreen/full-screen-cancel.html
fullscreen/full-screen-render-inline.html
fast/dom/onload-open.html
fullscreen/full-screen-iframe-legacy.html
fullscreen/full-screen-keyboard-enabled.html
fullscreen/full-screen-iframe-allowed.html
fast/loader/document-with-fragment-url-1.html
fullscreen/full-screen-remove-children.html
fullscreen/full-screen-remove.html
fullscreen/full-screen-remove-ancestor.html
fullscreen/full-screen-iframe-zIndex.html
fullscreen/full-screen-css.html

[Jeremy Apthorp]

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1222
> +        return;

What if element != m_fullScreenElement?

i.e, if someone calls requestFullScreen on one element, then calls requestFullScreen on a second element without fullscreen being cancelled. I think with the current implementation, the second element will become document.webkitCurrentFullScreenElement, but no transition will occur.

[Darin Fisher (:fishd, Google)]

(In reply to comment #7)
> View in context: https://bugs.webkit.org/attachment.cgi?id=111918&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1222
> > +        return;
> 
> What if element != m_fullScreenElement?
> 
> i.e, if someone calls requestFullScreen on one element, then calls requestFullScreen on a second element without fullscreen being cancelled. I think with the current implementation, the second element will become document.webkitCurrentFullScreenElement, but no transition will occur.

This case is being discussed / argued on the whatwg list.  Given that our current implementation exits fullscreen when you do this, I'm leaning toward not allowing the fullscreen element to be switched.  Do you know if we have any use cases that require being able to switch the fullscreen element without first exiting fullscreen mode and re-entering it?

[Darin Fisher (:fishd, Google)]

(In reply to comment #8)
> This case is being discussed / argued on the whatwg list.  Given that our current implementation exits fullscreen when you do this, I'm leaning toward not allowing the fullscreen element to be switched.  Do you know if we have any use cases that require being able to switch the fullscreen element without first exiting fullscreen mode and re-entering it?

It looks like Safari 5.1 ignores requestFullScreen calls made when the page is already fullscreen.

[Darin Fisher (:fishd, Google)]

Created attachment 112197 [details]
v1.2 patch (fix style-bot nits)

[Darin Fisher (:fishd, Google)]

Comment on attachment 112197 [details]
v1.2 patch (fix style-bot nits)

Actually, I cannot commit this until http://codereview.chromium.org/8366032/ lands

[WebKit Review Bot]

Attachment 112197 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/public/WebWidgetClient.h:87:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/chromium/public/WebWidgetClient.h:89:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Created attachment 112209 [details]
v1.3 patch (more style fixes)

[WebKit Review Bot]

Comment on attachment 112209 [details]
v1.3 patch (more style fixes)

Attachment 112209 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10198995

New failing tests:
fullscreen/full-screen-placeholder.html
fullscreen/full-screen-remove-ancestor-after.html
fullscreen/full-screen-remove-sibling.html
fullscreen/full-screen-keyboard-disabled.html
fullscreen/full-screen-cancel.html
fast/dom/onload-open.html
fullscreen/full-screen-iframe-legacy.html
media/media-blocked-by-willsendrequest.html
fullscreen/full-screen-keyboard-enabled.html
fullscreen/full-screen-iframe-allowed.html
fast/loader/document-with-fragment-url-1.html
fullscreen/full-screen-remove-children.html
fullscreen/full-screen-remove.html
fullscreen/full-screen-remove-ancestor.html
fullscreen/full-screen-iframe-zIndex.html
fullscreen/full-screen-css.html

[Darin Fisher (:fishd, Google)]

Created attachment 112770 [details]
v1.4 patch (now with DRT fixes)

[Darin Fisher (:fishd, Google)]

Comment on attachment 112770 [details]
v1.4 patch (now with DRT fixes)

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

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:895
>  {

supportsFullscreenForNode, enterFullscreenForNode, and exitFullscreenForNode
are the old way of doing fullscreen just for video elements.  since we implement
the generic fullscreen mode for all elements, these old methods are never reached.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:912
> +    return true;

WebCore already checks the same setting for us.

> Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:299
> +    m_webFrame->viewImpl()->willDetachDocument(m_webFrame);

willDetachDocument gives us a way to ensure that we exit fullscreen mode
when the document containing the fullscreen element is detached (i.e.,
navigated or removed).

(in a separate bug, i want to expose this function to chromium to replace
uses of WebFrameClient::frameDetached and WebFrameClient::willClose.)

[Adam Barth]

Comment on attachment 112770 [details]
v1.4 patch (now with DRT fixes)

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

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:896
>      return false;

Do we want to add ASSERT_NOT_REACHED here?

[WebKit Review Bot]

Comment on attachment 112770 [details]
v1.4 patch (now with DRT fixes)

Attachment 112770 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10229683

New failing tests:
fullscreen/full-screen-placeholder.html
fullscreen/full-screen-keyboard-disabled.html
fast/dom/onload-open.html
fullscreen/full-screen-twice.html
fast/loader/document-with-fragment-url-1.html
fullscreen/parent-flow-inline-with-block-child.html
fullscreen/full-screen-remove.html
fullscreen/full-screen-css.html

[Darin Fisher (:fishd, Google)]

Comment on attachment 112770 [details]
v1.4 patch (now with DRT fixes)

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

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:896
>>      return false;
> 
> Do we want to add ASSERT_NOT_REACHED here?

Yes, good catch!

[Darin Fisher (:fishd, Google)]

Created attachment 114382 [details]
v1.5 patch

This version is a bit of a different take.  I've stepped back from trying to cancel fullscreen mode when the document is navigated.  I want to do that, but I think it needs to be done in WebCore, and so I will do that in a separate patch.  Likewise, because a layout test requires it, this patch supports changing the fullscreen element while in fullscreen mode.  Modifying that behavior should also be done in WebCore if it is to be done.

Instead, this patch focuses on getting the fullscreenchange event plumbed properly, and it fixes some layout tests to work properly when fullscreen changes happen asynchronously (as they do in the browser).

[Darin Fisher (:fishd, Google)]

Comment on attachment 114382 [details]
v1.5 patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1060
> +    if (m_provisionalFullScreenElement) {

This code switches from retaining the fullscreen element to only retaining
the fullscreen frame to make the subsequence code in didEnterFullScreen,
willExitFullScreen and didExitFullScreen more obvious.  In those functions,
given only a frame, it is necessary to get the document, and check if it
has a fullscreen element before asking the document to operate on its 
fullscreen element.

I think there is some work to be done here to move more of this logic into
WebCore, perhaps in the form of a FullScreenController hung off of Page.
With the exception of webkitWillEnterFullScreenForElement, the other methods
completely ignore their parameter, so I just pass null below.

[Adam Barth]

Comment on attachment 114382 [details]
v1.5 patch

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

> Source/WebKit/chromium/src/WebViewImpl.cpp:1068
> +    }

Prefer early return.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1271
> +void WebViewImpl::enterFullScreenForElement(WebCore::Element* element)
> +{
> +    if (m_provisionalFullScreenElement) {
> +        m_provisionalFullScreenElement = element;
> +        return;
> +    }
> +
> +    if (m_fullScreenFrame) {
> +        m_provisionalFullScreenElement = element;
> +        willEnterFullScreen();
> +        didEnterFullScreen();
> +        return;
> +    }
> +
> +    if (m_client && m_client->enterFullScreen())
> +        m_provisionalFullScreenElement = element;
> +}

It might be helpful to add some comments to this function to explain what's different about these three cases.  Presumably, the first case is when this function is called multiple times while we're in the process of becoming full screen, the second case is when we're already in fullscreen, but we're changing the element (do we need to do something different if the new element's frame is different from m_fullScreenFrame?  do we have a test for that case?), and the third is the normal case of not yet behing fullscreen.

If the above isn't right, then more explanation would definitely be helpful.  :)

> Source/WebKit/chromium/src/WebViewImpl.h:585
> +    // If set, the WebView is being displayed fullscreen for this element.
> +    RefPtr<WebCore::Element> m_provisionalFullScreenElement;

This comment makes it seem like m_provisionalFullScreenElement will be non-null while we're in fullscreen for this element, but then the word "provisional" in the name of the member makes me think that it's only non-null while we're in the process of entering fullscreen.

> Source/WebKit/chromium/src/WebViewImpl.h:586
> +    RefPtr<WebCore::Frame> m_fullScreenFrame;

Do we need a notification when the Frame is detached from the Page to exit fullscreen?  (Maybe that already happens in WebCore?  Do we have a test for that case?)

[Darin Fisher (:fishd, Google)]

(In reply to comment #22)
> > +void WebViewImpl::enterFullScreenForElement(WebCore::Element* element)
> > +{
> > +    if (m_provisionalFullScreenElement) {
> > +        m_provisionalFullScreenElement = element;
> > +        return;
> > +    }
> > +
> > +    if (m_fullScreenFrame) {
> > +        m_provisionalFullScreenElement = element;
> > +        willEnterFullScreen();
> > +        didEnterFullScreen();
> > +        return;
> > +    }
> > +
> > +    if (m_client && m_client->enterFullScreen())
> > +        m_provisionalFullScreenElement = element;
> > +}
> 
> It might be helpful to add some comments to this function to explain what's different about these three cases.  Presumably, the first case is when this function is called multiple times while we're in the process of becoming full screen, the second case is when we're already in fullscreen, but we're changing the element (do we need to do something different if the new element's frame is different from m_fullScreenFrame?  do we have a test for that case?), and the third is the normal case of not yet behing fullscreen.

Yes, good call.  This needs comments.  For the second case, willEnterFullScreen will update m_fullScreenFrame.

I don't know if we have a good testcase for switching elements.  There was debate about removing that capability on whatwg.  Earlier in this patch, I was trying to remove that functionality, but I've gone back from that to focus instead on the bigger problems (and on fewer problems).

There's a definite problem with insufficient test cases for fullscreen.  Moreover, all of the testcases assume that fullscreen transitions happen synchronously, which does not match reality (except for Chromium's DRT post this patch).  My plan is to go back and revise all DRTs to transition asynchronously, and then complement that with tests.


> > Source/WebKit/chromium/src/WebViewImpl.h:585
> > +    // If set, the WebView is being displayed fullscreen for this element.
> > +    RefPtr<WebCore::Element> m_provisionalFullScreenElement;
> 
> This comment makes it seem like m_provisionalFullScreenElement will be non-null while we're in fullscreen for this element, but then the word "provisional" in the name of the member makes me think that it's only non-null 
while we're in the process of entering fullscreen.

Good catch.  Your understanding of the code is correct.  (I went back and forth on the issue of whether I should be storing the element or the frame containing the element, once we are in fullscreen mode, and this comment is residual.)


> > Source/WebKit/chromium/src/WebViewImpl.h:586
> > +    RefPtr<WebCore::Frame> m_fullScreenFrame;
> 
> Do we need a notification when the Frame is detached from the Page to exit fullscreen?  (Maybe that already happens in WebCore?  Do we have a test for that case?)

Yes, this is a big problem.  I attempted to fix this in the earlier version of the patch by noticing when the document gets detached.  That caused layout test failures, and the WebKit2 equivalent of this code also suffers potentially from the same memory leak.  At any rate, I feel like this issue is best deferred to another bug.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/100821