Bug 55273 – WebKit2: Plumb through the FULLSCREEN_API Chrome client calls

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jer Noble]

In WebKit2, the supportsFullscreenForNode, enterFullScreenForNode, and exitFullScreenForNode chrome client methods must be implemented.

[Jer Noble]

Created attachment 83901 [details]
Patch

[Early Warning System Bot]

Attachment 83901 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8043559

[WebKit Review Bot]

Attachment 83901 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8031612

[Jer Noble]

Created attachment 84077 [details]
Patch

[Jer Noble]

Created attachment 84083 [details]
Patch

Same patch as above, but rebased against current HEAD.

[Build Bot]

Attachment 84083 [details] did not build on win:
Build output: http://queues.webkit.org/results/8075346

[Jer Noble]

Created attachment 84087 [details]
Patch

Updated patch with feedback from Sam Weinig.

[Jer Noble]

Looks like I need to add the new files to the Windows project (and probably the qt one as well).

[Jer Noble]

Created attachment 84101 [details]
Patch

Added build commands for new files to the win/ and qt/ ports.

[Sam Weinig]

Comment on attachment 84101 [details]
Patch

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::WillEnterFullScreen(), 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.

[Jer Noble]

(In reply to comment #10)
> (From update of attachment 84101 [details])
> 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::WillEnterFullScreen(), 0);
> 
> This won't reach the correct (or any?) WebFullScreenManager.  It needs to be routed through the WebPage.

Indeed!

> > 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().

With the two changes below (removing the isClosed and isValid checks), the fullScreenManager() and fullScreenManagerProxy() functions will always return a non-NULL value, thus making these if checks extraneous.

What if I leave the lazy instantiation in, but remove all these unnecessary ifs?

> > 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.

Removed.

> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1390
> > +    if (m_isClosed)
> > +        return 0;
> 
> This should not be necessary.

Removed.

> > 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.

Same as above.

[Early Warning System Bot]

Attachment 84101 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8073457

[Build Bot]

Attachment 84101 [details] did not build on win:
Build output: http://queues.webkit.org/results/8073465

[Build Bot]

Attachment 84101 [details] did not build on win:
Build output: http://queues.webkit.org/results/8074425

[Jer Noble]

Created attachment 84242 [details]
Patch

This patch should fix the build issues on platforms which do not enable FULLSCREEN_API, as well as address (some of) Sam's review.

[Jer Noble]

Committed r80619: <http://trac.webkit.org/changeset/80619>