Bug 85498 - [Qt] Enable fullscreen API for WebKit2.
Summary: [Qt] Enable fullscreen API for WebKit2.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2012-05-03 07:41 PDT by Alexis Menard (darktears)
Modified: 2012-05-04 06:34 PDT (History)
6 users (show)

See Also:


Attachments
Patch (13.24 KB, patch)
2012-05-03 07:45 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (12.12 KB, patch)
2012-05-03 14:08 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (14.12 KB, patch)
2012-05-04 04:44 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch (13.20 KB, patch)
2012-05-04 04:45 PDT, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-05-03 07:41:23 PDT
[Qt] Enable fullscreen API for WebKit2.
Comment 1 Alexis Menard (darktears) 2012-05-03 07:45:31 PDT
Created attachment 140011 [details]
Patch
Comment 2 Simon Hausmann 2012-05-03 13:49:38 PDT
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.
Comment 3 Alexis Menard (darktears) 2012-05-03 14:08:03 PDT
Created attachment 140090 [details]
Patch
Comment 4 Alexis Menard (darktears) 2012-05-03 14:09:11 PDT
(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.
Comment 5 Simon Hausmann 2012-05-03 22:47:52 PDT
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?
Comment 6 Alexis Menard (darktears) 2012-05-04 04:32:30 PDT
(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).
Comment 7 Alexis Menard (darktears) 2012-05-04 04:44:03 PDT
Created attachment 140190 [details]
Patch
Comment 8 Alexis Menard (darktears) 2012-05-04 04:45:27 PDT
Created attachment 140191 [details]
Patch
Comment 9 Simon Hausmann 2012-05-04 05:15:29 PDT
Comment on attachment 140191 [details]
Patch

Are there any layout tests that we can also unskip?
Comment 10 Alexis Menard (darktears) 2012-05-04 05:53:31 PDT
(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 :).
Comment 11 Alexis Menard (darktears) 2012-05-04 06:34:03 PDT
(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