[webkit-reviews] review granted: [Bug 70477] [chromium] Improve fullscreen API : [Attachment 114382] v1.5 patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Darin Fisher (:fishd, Google)
<fishd at chromium.org>'s request for review:
Bug 70477: [chromium] Improve fullscreen API
https://bugs.webkit.org/show_bug.cgi?id=70477

Attachment 114382: v1.5 patch
https://bugs.webkit.org/attachment.cgi?id=114382&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?)