[webkit-reviews] review requested: [Bug 66031] Chromium plumbing for webkitRequestFullScreen : [Attachment 103577] Patch

[bugzilla-daemon at webkit.org]

James Kozianski <koz at chromium.org> has asked  for review:
Bug 66031: Chromium plumbing for webkitRequestFullScreen
https://bugs.webkit.org/show_bug.cgi?id=66031

Attachment 103577: Patch
https://bugs.webkit.org/attachment.cgi?id=103577&action=review

------- Additional Comments from James Kozianski <koz at chromium.org>
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
jer.noble at apple.com 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.