Today when building the Qt port with the GStreamer backend (DEFINES+=USE_GSTREAMER=1) there is no fullscreen playback support.
Created attachment 86457 [details] Proposed patch. This patch implement the support in FullScreenVideoQt alongside with the Qt Multimedia support. I though about reusing the base class QWebFullScreenVideoHandler of the platform plugin but it required to change the interface of the plugin and the show(QMediaPlayer*) was also an issue (and I found very ugly to have a plugin interface with some ifdef inside). The #if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER is for moc to compile the class definition (moc does not expand the USE macro). Any feedback?
Attachment 86457 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/graphics/gstreamer..." exit_code: 1 Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:21: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 86458 [details] V2 with changelog
Attachment 86458 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:21: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 86481 [details] V3 with style fixes
Comment on attachment 86481 [details] V3 with style fixes View in context: https://bugs.webkit.org/attachment.cgi?id=86481&action=review r- because of a massive style problem ;) And maybe a little because this is not tested and you don't say why :) > Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowPrivate.h:50 > + HTMLVideoElement *m_mediaElement; Andreas: Style issue.
Created attachment 87719 [details] V4 with comments taken into accounts.
Created attachment 87721 [details] V5 with Changelog fixed.
How is this related to Simon's work?
(In reply to comment #9) > How is this related to Simon's work? It's not. Nothing to do with it. WebKit2 stuff is completely different. It won't conflict.
Comment on attachment 87721 [details] V5 with Changelog fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review > Source/WebCore/ChangeLog:14 > + No new tests because layout tests cover it. They are not yet activated > + but will be any time soon. Which tests? :)
(In reply to comment #11) > (From update of attachment 87721 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review > > > Source/WebCore/ChangeLog:14 > > + No new tests because layout tests cover it. They are not yet activated > > + but will be any time soon. > > Which tests? :) media/media-fullscreen-inline.html media/media-fullscreen-not-in-document.html But I need to investigate why they don't pass yet despite the code is here. (They also fail for the QtMM implementation).
Comment on attachment 87721 [details] V5 with Changelog fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review Looks mostly good, but let's do one more iteration. :) > Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:25 > #include "PlatformVideoWindow.h" > > +#include "HTMLVideoElement.h" > #include "PlatformVideoWindowPrivate.h" Includes should be in alphabetic order. > Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:53 > +void FullScreenVideoWindow::closeEvent(QCloseEvent* event) No need for the 'event' name on the parameter. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:47 > + Unnecessary empty line. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:69 > + m_fullScreenWidget = reinterpret_cast<FullScreenVideoWindow* >(gstreamerGWorld->platformVideoWindow()->window()); Extra space after *. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:75 > + m_fullScreenWidget->setVideoElement(m_videoElement); > + > + connect(m_fullScreenWidget, SIGNAL(closed()), this, SLOT(windowClosed())); > + > + m_fullScreenWidget->showFullScreen(); No need for the blank lines. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:90 > + m_fullScreenWidget->setVideoElement(0); > + > + m_fullScreenWidget->close(); Ditto. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:103 > + Ditto. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:175 > + Ditto. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:202 > + Ditto. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:23 > +#include "Platform.h" #include <wtf/Platform.h> > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42 > +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER #if USE(GSTREAMER) > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:49 > + ~GStreamerFullScreenVideoHandler() {}; Coding style, need a space between { and }. And remove the semicolon at end of line. > Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64 > +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA #if ENABLE(QT_MULTIMEDIA)
Comment on attachment 87721 [details] V5 with Changelog fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42 >> +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER > > #if USE(GSTREAMER) moc will barf on the USE() macro, that's why we need to do it like this :( >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64 >> +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA > > #if ENABLE(QT_MULTIMEDIA) ditto
(In reply to comment #14) > (From update of attachment 87721 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review > > >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42 > >> +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER > > > > #if USE(GSTREAMER) > > moc will barf on the USE() macro, that's why we need to do it like this :( > > >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64 > >> +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA > > > > #if ENABLE(QT_MULTIMEDIA) > > ditto Right, so the ENABLE(QT_MULTIMEDIA) earlier in that file should be removed instead. :)
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 87721 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review > > > > >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42 > > >> +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER > > > > > > #if USE(GSTREAMER) > > > > moc will barf on the USE() macro, that's why we need to do it like this :( > > > > >> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64 > > >> +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA > > > > > > #if ENABLE(QT_MULTIMEDIA) > > > > ditto > > Right, so the ENABLE(QT_MULTIMEDIA) earlier in that file should be removed instead. :) Well I did it on purpose. Where I used ENABLE(QT_MULTIMEDIA) it's because it's not related to moc (includes for example). So I picked readability vs consistency. But I can change :D.
Comment on attachment 87721 [details] V5 with Changelog fixed. View in context: https://bugs.webkit.org/attachment.cgi?id=87721&action=review >> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:25 > > Includes should be in alphabetic order. Well PlatformVideoWindow.h is the primary header it should be on the first position. Though I will remove the extra space.
Created attachment 88448 [details] V6
Created attachment 88454 [details] V7 with the changelog. Sorry for the noise.
Comment on attachment 88454 [details] V7 with the changelog. LGTM.
Comment on attachment 88454 [details] V7 with the changelog. Clearing flags on attachment: 88454 Committed r83078: <http://trac.webkit.org/changeset/83078>
All reviewed patches have been landed. Closing bug.