[webkit-reviews] review denied: [Bug 55273] WebKit2: Plumb through the FULLSCREEN_API Chrome client calls : [Attachment 84101] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 55273: WebKit2: Plumb through the FULLSCREEN_API Chrome client calls
https://bugs.webkit.org/show_bug.cgi?id=55273

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84101&action=review

> Source/WebKit2/UIProcess/WebFullScreenManagerProxy.cpp:66
> +   
WebProcess::shared().connection()->send(Messages::WebFullScreenManager::WillEnt
erFullScreen(), 0);

This won't reach the correct (or any?) WebFullScreenManager.  It needs to be
routed through the WebPage.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1257
> +	   if (WebFullScreenManagerProxy* manager = this->fullScreenManager())
> +	       manager->didReceiveMessage(connection, messageID, arguments);
> +	   return;

This should probably just call a function like ensureFullScreenManager(), and
then you can use the fullScreenManager directly m_fullScreenManager.get().

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1279
> +    if (messageID.is<CoreIPC::MessageClassWebFullScreenManagerProxy>()) {
> +	   if (WebFullScreenManagerProxy* manager = this->fullScreenManager())
> +	       manager->didReceiveSyncMessage(connection, messageID, arguments,
reply);
> +	   return;
> +    }

Same here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1991
> +    if (isClosed() || !isValid())
> +	   return 0;

You can remove this.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1390
> +    if (m_isClosed)
> +	   return 0;

This should not be necessary.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1771
> +	   if (WebFullScreenManager* fullScreenManager =
this->fullScreenManager())
> +	       fullScreenManager->didReceiveMessage(connection, messageID,
arguments);

The fullScreenManager should always be non-null here, since only JS can start a
fullscreen.  I think you can probably just loose the lazy initialization stuff,
but that is up to you.