[Qt] Enable fullscreen API for WebKit2.
Created attachment 140011 [details] Patch
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.
Created attachment 140090 [details] Patch
(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 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?
(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).
Created attachment 140190 [details] Patch
Created attachment 140191 [details] Patch
Comment on attachment 140191 [details] Patch Are there any layout tests that we can also unskip?
(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 :).
(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