Summary: | [Qt] Implement fullscreen playback for the GStreamer backend. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexis Menard (darktears) <menard> | ||||||||||||||||
Component: | Media | Assignee: | Alexis Menard (darktears) <menard> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, hausmann, kenneth, kling, vestbo, webkit.review.bot | ||||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
Attachments: |
|
Description
Alexis Menard (darktears)
2011-03-22 06:34:53 PDT
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. |