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

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied James Kozianski
<koz at chromium.org>'s request 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 Darin Fisher (:fishd, Google)
<fishd 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

> 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 :-)