Bug 66031 – Chromium plumbing for webkitRequestFullScreen

• Component: New Bugs
• Status: RESOLVED FIXED

[James Kozianski]

Chromium plumbing for webkitRequestFullScreen

[James Kozianski]

Created attachment 103577 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103577 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 103577 [details]
Patch

Attachment 103577 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9352079

[Darin Fisher (:fishd, Google)]

Comment on attachment 103577 [details]
Patch

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

> Source/WebKit/chromium/public/WebView.h:366
>  

add another new line here

> Source/WebKit/chromium/public/WebView.h:367
> +    // Fullscreen -----------------------------------------------------------

nit: new line after the section break

> Source/WebKit/chromium/public/WebView.h:369
> +

though folks have not been entirely consistent, we have a convention of putting two new
lines at the end of a section.  so, add another new line here.

> Source/WebKit/chromium/public/WebViewClient.h:218
> +    virtual void enterFullscreenForElement() { }

shouldn't we pass a WebElement to these functions?  if not, then the functions
should probably be renamed to not have the ForElement suffix.

what's the difference between these functions and the ForNode variants?  why
do we need both?

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:914
> +    // be restructured to wait for an ACK from the browser that we did enter

we normally try to avoid leaving comments in this code that reference the
multi-process architecture details of Chromium.  they can easily bit-rot
since people on the Chromium side might be unlikely to fix up comments
here.  maybe you should just talk in terms of "the embedder" needing to
notify you asynchronously, etc.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2680
> +    Element* fullscreenElement = page()->mainFrame()->document()->webkitCurrentFullScreenElement();

it might be nice to cache a pointer to the m_page->mainFrame()->document() in a local variable
to help readability since you reuse that expression a lot.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2681
> +    if (fullscreenElement)

shouldn't this be a "if (!fullscreenElement)"?  i would think that if we have a full screen element that we would want to exit full screen mode.

> Source/WebKit/chromium/src/WebViewImpl.h:375
> +    virtual void exitFullscreen();

there is actually a section in WebViewImpl.h that is dedicated for listing
the methods of WebView in the same order as they appear in the header file.
unfortunately, the folks who authored setVisibilityState and graphicsContext3D
did not follow that rule.  please don't add to that mess :-)

[James Kozianski]

Comment on attachment 103577 [details]
Patch

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

>> Source/WebKit/chromium/public/WebView.h:366

> 
> add another new line here

Done.

>> Source/WebKit/chromium/public/WebView.h:369
>> +
> 
> though folks have not been entirely consistent, we have a convention of putting two new
> lines at the end of a section.  so, add another new line here.

Done.

>> Source/WebKit/chromium/public/WebViewClient.h:218
>> +    virtual void enterFullscreenForElement() { }
> 
> shouldn't we pass a WebElement to these functions?  if not, then the functions
> should probably be renamed to not have the ForElement suffix.
> 
> what's the difference between these functions and the ForNode variants?  why
> do we need both?

The ForNode variants seem to be a WebKit specific way to do fullscreen specifically for video elements. In HTMLMediaElement.cpp there is code added by [e-mail] that short-circuits the calls to enterFullscreenForNode(Node*) with calls to document->requestFullscreenForElement(Element*) if FULLSCREEN_API is enabled, so it seems that enterFullscreenForNode() is deprecated (as it seems that Safari builds with FULLSCREEN_API on?)

The ForNode variants are NOTIMPLEMNTED() in Chromium, but they are plumbed through to RenderView. Should I "hijack" them and remove the ForElement() ones? My feeling is that it would be best to leave the ForElement() functions as they are and try and remove the apparently deprecated ForNode() variants. In fact, hijacking them would be messy because they accept Node*s, not Element*s.

I've called them *ForElement because it is the fullscreen mode that focuses the renderer around an element, which is different to the embedder simply entering fullscreen mode. That is, toggling the browser into fullscreen with the button is different to a site going fullscreen.

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:914
>> +    // be restructured to wait for an ACK from the browser that we did enter
> 
> we normally try to avoid leaving comments in this code that reference the
> multi-process architecture details of Chromium.  they can easily bit-rot
> since people on the Chromium side might be unlikely to fix up comments
> here.  maybe you should just talk in terms of "the embedder" needing to
> notify you asynchronously, etc.

Okay. I've updated the comment.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2680
>> +    Element* fullscreenElement = page()->mainFrame()->document()->webkitCurrentFullScreenElement();
> 
> it might be nice to cache a pointer to the m_page->mainFrame()->document() in a local variable
> to help readability since you reuse that expression a lot.

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2681
>> +    if (fullscreenElement)
> 
> shouldn't this be a "if (!fullscreenElement)"?  i would think that if we have a full screen element that we would want to exit full screen mode.

Yep, that's right. Sorry, this was amongst some local changes I'd made but forgot to upload.

>> Source/WebKit/chromium/src/WebViewImpl.h:375
>> +    virtual void exitFullscreen();
> 
> there is actually a section in WebViewImpl.h that is dedicated for listing
> the methods of WebView in the same order as they appear in the header file.
> unfortunately, the folks who authored setVisibilityState and graphicsContext3D
> did not follow that rule.  please don't add to that mess :-)

Done.

[James Kozianski]

Created attachment 104002 [details]
Patch

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> The ForNode variants seem to be a WebKit specific way to do fullscreen specifically for video elements. In HTMLMediaElement.cpp there is code added by [e-mail] that short-circuits the calls to enterFullscreenForNode(Node*) with calls to document->requestFullscreenForElement(Element*) if FULLSCREEN_API is enabled, so it seems that enterFullscreenForNode() is deprecated (as it seems that Safari builds with FULLSCREEN_API on?)
> 
> The ForNode variants are NOTIMPLEMNTED() in Chromium, but they are plumbed through to RenderView. Should I "hijack" them and remove the ForElement() ones? My feeling is that it would be best to leave the ForElement() functions as they are and try and remove the apparently deprecated ForNode() variants. In fact, hijacking them would be messy because they accept Node*s, not Element*s.
> 
> I've called them *ForElement because it is the fullscreen mode that focuses the renderer around an element, which is different to the embedder simply entering fullscreen mode. That is, toggling the browser into fullscreen with the button is different to a site going fullscreen.

Ah!  Makes sense now.  The ForNode methods are probably the leftovers from the work that scherkus' intern did to try to implement fullscreen mode for video.  I agree that it should be deleted in place of these new methods.

However, I still don't see the point in calling these ForElement.  From the point of view of the browser, the only significant difference between F11 and webkitRequestFullscreen is the latter is content-initiated.  That means the browser may need to present different UI to the user.  The browser won't otherwise care at all about what is focused in the page.

I also think it is consistent to drop the ForElement suffix since that normally implies that there will be a corresponding parameter of type WebElement, but you don't pass such a parameter.

[Darin Fisher (:fishd, Google)]

Comment on attachment 104002 [details]
Patch

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

By the way, if you think that ForElement is the best suffix, then just pass a WebElement parameter like the ChromeClient methods do.

> Source/WebKit/chromium/public/WebView.h:367
> +    // Fullscreen -----------------------------------------------------------

sorry to be a nut about whitespace, but i had intended for there to be two blank
lines above each section heading.  apparently, a good number of people haven't
been following that convention, but if you scroll up far enough, you'll see that
convention in place.  sigh :(

[James Kozianski]

(In reply to comment #7)
> (In reply to comment #5)
> > The ForNode variants seem to be a WebKit specific way to do fullscreen specifically for video elements. In HTMLMediaElement.cpp there is code added by [e-mail] that short-circuits the calls to enterFullscreenForNode(Node*) with calls to document->requestFullscreenForElement(Element*) if FULLSCREEN_API is enabled, so it seems that enterFullscreenForNode() is deprecated (as it seems that Safari builds with FULLSCREEN_API on?)
> > 
> > The ForNode variants are NOTIMPLEMNTED() in Chromium, but they are plumbed through to RenderView. Should I "hijack" them and remove the ForElement() ones? My feeling is that it would be best to leave the ForElement() functions as they are and try and remove the apparently deprecated ForNode() variants. In fact, hijacking them would be messy because they accept Node*s, not Element*s.
> > 
> > I've called them *ForElement because it is the fullscreen mode that focuses the renderer around an element, which is different to the embedder simply entering fullscreen mode. That is, toggling the browser into fullscreen with the button is different to a site going fullscreen.
> 
> Ah!  Makes sense now.  The ForNode methods are probably the leftovers from the work that scherkus' intern did to try to implement fullscreen mode for video.  I agree that it should be deleted in place of these new methods.
> 
> However, I still don't see the point in calling these ForElement.  From the point of view of the browser, the only significant difference between F11 and webkitRequestFullscreen is the latter is content-initiated.  That means the browser may need to present different UI to the user.  The browser won't otherwise care at all about what is focused in the page.
> 
> I also think it is consistent to drop the ForElement suffix since that normally implies that there will be a corresponding parameter of type WebElement, but you don't pass such a parameter.

Yep, that makes sense. I've dropped the suffix.

>> Source/WebKit/chromium/public/WebView.h:367
> sorry to be a nut about whitespace, but i had intended for there to be two blank
> lines above each section heading.  apparently, a good number of people haven't
> been following that convention, but if you scroll up far enough, you'll see that
> convention in place.  sigh :(

Sorry, my bad! Done.

[James Kozianski]

Created attachment 104281 [details]
Patch

[James Kozianski]

Created attachment 104301 [details]
Patch

[James Kozianski]

Created attachment 104302 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 104302 [details]
Patch for landing

Rejecting attachment 104302 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
Kit2/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Dump-as-markup conversion: editing/pasteboard/paste-text-010.html and editing/pasteboard/paste-text-015.html

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 146.

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

[James Kozianski]

Created attachment 104429 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 104429 [details]
Patch for landing

Clearing flags on attachment: 104429

Committed r93381: <http://trac.webkit.org/changeset/93381>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.