Bug 85498 – [Qt] Enable fullscreen API for WebKit2.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Alexis Menard (darktears)]

[Qt] Enable fullscreen API for WebKit2.

[Alexis Menard (darktears)]

Created attachment 140011 [details]
Patch

[Simon Hausmann]

Comment on attachment 140011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140011&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:190
> +    webPageProxy->fullScreenManager()->setWebView(q_ptr);

Any change the manager can become a QObject so that you can simply connect things via signal -> signal connections instead?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1268
> +    WebFullScreenManagerProxy* fullScreenManagerProxy = d->webPageProxy->fullScreenManager();
> +    fullScreenManagerProxy->willEnterFullScreen();
> +    emit enterFullScreenRequested();
> +    fullScreenManagerProxy->didEnterFullScreen();
> +#endif

If this method is called by the WebFullScreenManagerProxy and the first thing this method does is go back and get a reference to it and call back, then wouldn't it be cleaner if this was all done in the WebFullScreenManagerProxy? Then all that's left is a friend declaration to allow the proxy to emit the signal.

[Alexis Menard (darktears)]

Created attachment 140090 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #2)
> (From update of attachment 140011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140011&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:190
> > +    webPageProxy->fullScreenManager()->setWebView(q_ptr);
> 
> Any change the manager can become a QObject so that you can simply connect things via signal -> signal connections instead?

Not really possible.

> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:1268
> > +    WebFullScreenManagerProxy* fullScreenManagerProxy = d->webPageProxy->fullScreenManager();
> > +    fullScreenManagerProxy->willEnterFullScreen();
> > +    emit enterFullScreenRequested();
> > +    fullScreenManagerProxy->didEnterFullScreen();
> > +#endif
> 
> If this method is called by the WebFullScreenManagerProxy and the first thing this method does is go back and get a reference to it and call back, then wouldn't it be cleaner if this was all done in the WebFullScreenManagerProxy? Then all that's left is a friend declaration to allow the proxy to emit the signal.

Right, it was before I introduced the signal.

[Simon Hausmann]

Comment on attachment 140090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140090&action=review

> Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:263
> +#if ENABLE(FULLSCREEN_API)
> +    d->setAttribute(QWebPreferencesPrivate::FullScreenEnabled, enable);
> +    emit fullScreenEnabledChanged();
> +#endif

#else
    UNUSED_PARAM(enable)
#endif

> Source/WebKit2/UIProcess/qt/WebFullScreenManagerProxyQt.cpp:56
> -    notImplemented();
> +    willEnterFullScreen();
> +    emit m_webView->experimental()->enterFullScreenRequested();
> +    didEnterFullScreen();

This looks much better - thanks :)

> Tools/MiniBrowser/qt/qml/BrowserWindow.qml:311
> +        experimental.preferences.fullScreenEnabled: true

No connection to the signal in the minibrowser along with a simple implementation?

> Tools/qmake/mkspecs/features/features.prf:257
> +!contains(DEFINES, ENABLE_FULLSCREEN_API=.) {
> +    no_webkit2 {
> +        DEFINES += ENABLE_FULLSCREEN_API=0
> +    } else {
> +        DEFINES += ENABLE_FULLSCREEN_API=1
> +    }
> +}

Hm, so if webkit2 is enabled, the API is also enabled. But how is that handled in WK1 at run-time then?

[Alexis Menard (darktears)]

(In reply to comment #5)
> (From update of attachment 140090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140090&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qwebpreferences.cpp:263
> > +#if ENABLE(FULLSCREEN_API)
> > +    d->setAttribute(QWebPreferencesPrivate::FullScreenEnabled, enable);
> > +    emit fullScreenEnabledChanged();
> > +#endif
> 
> #else
>     UNUSED_PARAM(enable)
> #endif

Will fix.

> 
> > Source/WebKit2/UIProcess/qt/WebFullScreenManagerProxyQt.cpp:56
> > -    notImplemented();
> > +    willEnterFullScreen();
> > +    emit m_webView->experimental()->enterFullScreenRequested();
> > +    didEnterFullScreen();
> 
> This looks much better - thanks :)
> 
> > Tools/MiniBrowser/qt/qml/BrowserWindow.qml:311
> > +        experimental.preferences.fullScreenEnabled: true
> 
> No connection to the signal in the minibrowser along with a simple implementation?

Well I wanted to do it in a separate patch.

> 
> > Tools/qmake/mkspecs/features/features.prf:257
> > +!contains(DEFINES, ENABLE_FULLSCREEN_API=.) {
> > +    no_webkit2 {
> > +        DEFINES += ENABLE_FULLSCREEN_API=0
> > +    } else {
> > +        DEFINES += ENABLE_FULLSCREEN_API=1
> > +    }
> > +}
> 
> Hm, so if webkit2 is enabled, the API is also enabled. But how is that handled in WK1 at run-time then?

Well unless you set the flag to true like in MiniBrowser it shouldn't conflict. Since WK1 doesn't use QWebPreferences class I don't see how WK1 could set the flag (QWebSettings doesn't expose it). No flag set == old path is triggered (old path which is BTW proper broken due to QWidget dependency removal).

[Alexis Menard (darktears)]

Created attachment 140190 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 140191 [details]
Patch

[Simon Hausmann]

Comment on attachment 140191 [details]
Patch

Are there any layout tests that we can also unskip?

[Alexis Menard (darktears)]

(In reply to comment #9)
> (From update of attachment 140191 [details])
> Are there any layout tests that we can also unskip?

Yep but I will land that in a separate patch :) as if a rollout is needed we don't rollout the entire patch :).

[Alexis Menard (darktears)]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 140191 [details] [details])
> > Are there any layout tests that we can also unskip?
> 
> Yep but I will land that in a separate patch :) as if a rollout is needed we don't rollout the entire patch :).

Commited r116089 : http://trac.webkit.org/changeset/116089