Bug 51249

Created attachment 77238 [details] first draft

The patch does three things, 1) add a new interface, QFullscreenVideoHandler in qwebkitplatformplugin.h 2) add a simple example implementation for the new interface in example/platformplugin 3) add a default fullscreen video handler under webkit\qt\webcoresupport Both 2) & 3) are very basic implementation. There is no gui for the fullscreen video player, since the overlay (control) widget can't work well with the qvideowidget. (Qt::WA_TranslucentBackground doesn't behave as expected on linux for Qt 4.7.1, so the overlay widget fully covers the video widget) There is a Qt bug related this issue and I will go check whether it is gone be fixed in 4.7.2. To play or pause the fullscreen video, click space key. To exit fullscreen mode, double click your mouse. All the suggestions are very very welcomed, especially the design of the new interface & fullscreen class.

Comment on attachment 77238 [details] first draft View in context: https://bugs.webkit.org/attachment.cgi?id=77238&action=review r- for some minor issues. > WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:111 > - m_mediaPlayer->bind(m_videoItem); > + m_mediaPlayer->setVideoOutput(m_videoItem); Any explanation why it is changed? It's not so obvious looking at the patch. > WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:592 > + m_mediaPlayer->setVideoOutput(static_cast<QGraphicsVideoItem*>(0)); What's with static cast a null pointer? > WebKit/qt/Api/qwebkitplatformplugin.h:30 > +#if defined ENABLE_VIDEO && ENABLE_VIDEO This is "semi public", it can't contain compiler defines from WebKit. > WebKit/qt/Api/qwebkitplatformplugin.h:121 > +class QFullscreenVideoHandler : public QObject { This is a new API, so it may require coordinated discussion with the Qt folks. Also, I guess the convention is to use QWeb- prefix. > WebKit/qt/Api/qwebkitplatformplugin.h:129 > + void closeFullscreen(); Does not follow the usual past-tense convention of Qt signal name. > WebKit/qt/Api/qwebkitplatformplugin.h:148 > + FullscreenVideoPlayer, Enum can't be dependent of compiler define, otherwise it may yield in same value having different meaning. > WebKit/qt/WebCoreSupport/FullscreenVideoQt.cpp:2 > + * This file is part of the popup menu implementation for <select> elements in WebCore. Probably not needed. > WebKit/qt/WebCoreSupport/QtFallbackFullscreenVideoHandler.cpp:3 > + * Copyright (C) 2010 Girish Ramakrishnan <girish@forwardbias.in> > + * Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies) Is this a new file or inherited from old one? > WebKit/qt/WebCoreSupport/QtFallbackFullscreenVideoHandler.h:2 > + * Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies) Was this worked on really back in 2009? > WebKit/qt/examples/platformplugin/WebPlugin.h:108 > + void spaceBtnPressed(); Don't shorten Button to Btn. > WebKit/qt/examples/platformplugin/platformplugin.pro:12 > + DEFINES += ENABLE_VIDEO=1 Shouldn't this block placed later on? Using this logic, there is no way to explicitly disable video support if the system has Qt Mobility Multimedia. > WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h:95 > +class QWebHapticFeedbackPlayer: public QObject > { > + Q_OBJECT > public: > + QWebHapticFeedbackPlayer() {} > + virtual ~QWebHapticFeedbackPlayer() {} > + Why is this change needed here?

(In reply to comment #3) > (From update of attachment 77238 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77238&action=review > > r- for some minor issues. > > > WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:111 > > - m_mediaPlayer->bind(m_videoItem); > > + m_mediaPlayer->setVideoOutput(m_videoItem); > > Any explanation why it is changed? It's not so obvious looking at the patch. > The setVideoOutput is a wrapper for the bind function. void QMediaPlayer::setVideoOutput(QVideoWidget *output) { if (d->videoOutput) unbind(d->videoOutput); d->videoOutput = output; if (d->videoOutput) bind(d->videoOutput); } > > WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:592 > > + m_mediaPlayer->setVideoOutput(static_cast<QGraphicsVideoItem*>(0)); > > What's with static cast a null pointer? It is because there are two setVideoOutput with different signature in the QMediaPlayer void QMediaPlayer::setVideoOutput(QGraphicsVideoItem *output) void QMediaPlayer::setVideoOutput(QVideoWidget *output) > > > WebKit/qt/Api/qwebkitplatformplugin.h:30 > > +#if defined ENABLE_VIDEO && ENABLE_VIDEO > > This is "semi public", it can't contain compiler defines from WebKit. > Sorry, could you let me know how to handle this? Thank you so much for reviewing it. I will fix all other issues you point out.

Created attachment 77888 [details] updated with Ariya's suggestion I got one error when running the check-webkit-style, WebKit/qt/WebCoreSupport/FullscreenVideoQt.h:50: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] 50 MediaPlayerPrivateQt* mediaPlayerForNode(Node* node = 0); Seems it is a bug in the check-webkit-style script.

Simon, how similar is this to your work on the WebKit2 side? Can you comment on the patch?

(In reply to comment #5) > Created an attachment (id=77888) [details] > updated with Ariya's suggestion > > I got one error when running the check-webkit-style, > WebKit/qt/WebCoreSupport/FullscreenVideoQt.h:50: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] > > 50 MediaPlayerPrivateQt* mediaPlayerForNode(Node* node = 0); > > Seems it is a bug in the check-webkit-style script. How so? That api should be "MediaPlayerPrivateQt* mediaPlayerForNode(Node* = 0);" because the parameter name "node" adds no information (unless there is some exception for this directory for some reason). The check is flagging code that fails this guideline "Leave meaningless variable names out of function declarations." -- http://webkit.org/coding/coding-style.html

(In reply to comment #7) > (In reply to comment #5) > > Created an attachment (id=77888) [details] [details] > > updated with Ariya's suggestion > > > > I got one error when running the check-webkit-style, > > WebKit/qt/WebCoreSupport/FullscreenVideoQt.h:50: The parameter name "node" adds no information, so it should be removed. [readability/parameter_name] [5] > > > > 50 MediaPlayerPrivateQt* mediaPlayerForNode(Node* node = 0); > > > > Seems it is a bug in the check-webkit-style script. > > How so? > > That api should be "MediaPlayerPrivateQt* mediaPlayerForNode(Node* = 0);" because the parameter name "node" adds no information (unless there is some exception for this directory for some reason). > > The check is flagging code that fails this guideline "Leave meaningless variable names out of function declarations." -- http://webkit.org/coding/coding-style.html Oh, thanks, I will correct it in next patch.

Created attachment 78241 [details] fix style

Comment on attachment 78241 [details] fix style Needs ChangeLog.

Created attachment 78286 [details] add change log

Also, I tried to run LayoutTests/media/media-fullscreen-inline.html for this patch, however, the test still failed for Qt (QtMediaPlayer), since the durationchange event gets fired twice when loading a video. (which is expected be fired once). So, this test is still skipped for Qt. Following is log, --- /tmp/layout-test-results/media/media-fullscreen-inline-expected.txt 2011-01-07 19:06:42.885423685 -0500 +++ /tmp/layout-test-results/media/media-fullscreen-inline-actual.txt 2011-01-07 19:06:42.885423685 -0500 @@ -17,6 +17,7 @@ *** Creating <video> element with "content/silence.mpg" in the document, should NOT support fullscreen because it is an audio-only <video> element EVENT(loadstart) EVENT(durationchange) +EVENT(durationchange) EVENT(canplaythrough) * event handler NOT triggered by a user gesture EXPECTED (mediaElement.webkitSupportsFullscreen == 'false') OK @@ -30,6 +31,7 @@ *** Creating <video> element with "content/test.mp4" in the document, should support fullscreen because it is a <video> element with video media EVENT(loadstart) EVENT(durationchange) +EVENT(durationchange) EVENT(canplaythrough) * event handler NOT triggered by a user gesture EXPECTED (mediaElement.webkitSupportsFullscreen == 'true') OK

Comment on attachment 78286 [details] add change log View in context: https://bugs.webkit.org/attachment.cgi?id=78286&action=review In general this looks pretty good to me actually, my nitpicking aside. Good work Yi :). I however think that at this point we should not have a default fullscreen playback widget in Qt, unless you have plans to also give it some controls, etc. In other words: Given the scope and the future WebKit2 related work, I suggest that we support fullscreen playback if the platform plugin provides it, otherwise we let bool ChromeClientQt::supportsFullscreenForNode(const Node* node) return false. What do you think? r- because of the nitpicks :), but I also think we should discuss having a default or not. Would also like to hear the opinion from the other CC'ed folks :) > WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:602 > + // FIXME a qtmobility bug, need to reset the size when restore the videoitem, otherwise the size is 0 > + nativeSizeChanged(QSize(m_oldNaturalSize)); Can you add a link to the bug report for qtmobility, so that we can remove this workaround once the bug is fixed upstream? > WebCore/platform/graphics/qt/MediaPlayerPrivateQt.h:111 > + QMediaPlayer* mediaPlayer() { return m_mediaPlayer; } This function should be const. (as weird as it is :) > WebKit/qt/Api/qwebkitplatformplugin.h:31 > +#if defined ENABLE_VIDEO && ENABLE_VIDEO Some pre-processors don't like this syntax and would prefer #if defined(ENABLE_VIDEO) && ENABLE_VIDEO > WebKit/qt/Api/qwebkitplatformplugin.h:121 > +#if defined ENABLE_VIDEO && ENABLE_VIDEO Same here :) > WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:708 > + if (platformMedia.type != PlatformMedia::QtMediaPlayerType) { > + videoElement->exitFullscreen(); > + return; > + } I doubt the call to exitFullscreen() is going to work. Have you tried it? Otherwise I suggest to have an ASSERT() there (for debug builds) as well as a simple "return". > WebKit/qt/WebCoreSupport/FullscreenVideoQt.cpp:90 > + Q_ASSERT(m_fullscreenVideoHandler); Doesn't this ASSERT belong in the constructor? I mean, shouldn't we catch this (and similar errors other ASSERTs guard) in the constructor once?

(In reply to comment #13) > (From update of attachment 78286 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78286&action=review > > In general this looks pretty good to me actually, my nitpicking aside. Good work Yi :). I however think that at this point we should not have a default fullscreen playback widget in Qt, unless you have plans to also give it some controls, etc. > > In other words: Given the scope and the future WebKit2 related work, I suggest that we support fullscreen playback if the platform plugin provides it, > otherwise we let bool ChromeClientQt::supportsFullscreenForNode(const Node* node) return false. > > What do you think? > > r- because of the nitpicks :), but I also think we should discuss having a default or not. Would also like to hear the opinion from the other CC'ed folks :) Thanks a lot Simon :) and I will fix all the issues you found for me. For the default fullscreen playback widget, I do have a plan to add some controls for it as long as I can. However, it may take a while for me to figure out why the overlay widget doesn't work well with the video widget on Linux. My original thought is that we can provide a simple fullscreen playback widget, whose requiresFullscreenForVideoPlayback() always returns false, then we can add a fullscreen control button for the inline video. With the default fullscreen widget, user/tester can trigger the fullscreen mode even platform plugin doesn't provide one, either by 1) clicking the fullscreen control button on the inline video, or 2) calling the javascript code "mediaElement.webkitEnterFullScreen()". If all above can work well, then we can focus on making the fullscreen widget works/looks better (e.g. add overlay controls) But I will be with you, Simon :) If you think it is not the right time/place to put this default fullscreen widget, I can remove it from this patch :)

Created attachment 78440 [details] updated with Simon's suggestion (still has default fullscreen widget)

Created attachment 78441 [details] updated with Simon's suggestion (removed default fullscreen widget)

Comment on attachment 78441 [details] updated with Simon's suggestion (removed default fullscreen widget) View in context: https://bugs.webkit.org/attachment.cgi?id=78441&action=review I will let Simon do the actual review, a few comments. > Source/WebCore/ChangeLog:8 > + Make the MediaPlayerPrivateQt to support fullscreen player. Bad English: remove the "to" :-) Maybe this is better : Make MediaPlayerPrivateQt support a fullscreen player. > Source/WebCore/ChangeLog:10 > + No new tests because LayoutTests/media/media-fullscreen-inline.html is already existing. exists.* > Source/WebCore/ChangeLog:11 > + However, this test failed for Qt (QtMediaPlayer) due to durationchange event gets double fired. getting fired twice* > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:601 > + // FIXME a qtmobility bug, need to reset the size when restore the videoitem, otherwise the size is 0 We usually add : after FIXME, ie. FIXME: > WebKit/qt/Api/qwebkitplatformplugin.h:130 > + void fullscreenClosed(); What about leftFullscreen. ie, you enter/leave. Maybe Simon has some better ideas. > WebKit/qt/Api/qwebkitplatformplugin.h:134 > + virtual void enterFullscreen(QMediaPlayer*) = 0; > + virtual void exitFullscreen() = 0; leaveFullscreen > WebKit/qt/examples/platformplugin/WebPlugin.h:126 > + void onPauseOrPlay(); onStateChange?

(In reply to comment #17) > (From update of attachment 78441 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78441&action=review > > I will let Simon do the actual review, a few comments. > > > Source/WebCore/ChangeLog:8 > > + Make the MediaPlayerPrivateQt to support fullscreen player. > > Bad English: remove the "to" :-) > > Maybe this is better : Make MediaPlayerPrivateQt support a fullscreen player. > > > Source/WebCore/ChangeLog:10 > > + No new tests because LayoutTests/media/media-fullscreen-inline.html is already existing. > > exists.* > > > Source/WebCore/ChangeLog:11 > > + However, this test failed for Qt (QtMediaPlayer) due to durationchange event gets double fired. > > getting fired twice* > > > Source/WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:601 > > + // FIXME a qtmobility bug, need to reset the size when restore the videoitem, otherwise the size is 0 > > We usually add : after FIXME, ie. FIXME: > > > WebKit/qt/Api/qwebkitplatformplugin.h:130 > > + void fullscreenClosed(); > > What about leftFullscreen. ie, you enter/leave. Maybe Simon has some better ideas. > > > WebKit/qt/Api/qwebkitplatformplugin.h:134 > > + virtual void enterFullscreen(QMediaPlayer*) = 0; > > + virtual void exitFullscreen() = 0; > > leaveFullscreen > > > WebKit/qt/examples/platformplugin/WebPlugin.h:126 > > + void onPauseOrPlay(); > > onStateChange? Thanks Kenneth, I will fix them :-)

Created attachment 78445 [details] updated with Kenneth's suggestion (removed default fullscreen widget) I didn't change following function names, one reason is that in HTMLMeidaElement, it uses enterfullscreen/exitfullscreen. Simon, could you give some comments for these function name? Still, thank you, Kenneth :-) > WebKit/qt/Api/qwebkitplatformplugin.h:130 > + void fullscreenClosed(); What about leftFullscreen. ie, you enter/leave. Maybe Simon has some better ideas. > WebKit/qt/Api/qwebkitplatformplugin.h:134 > + virtual void enterFullscreen(QMediaPlayer*) = 0; > + virtual void exitFullscreen() = 0; leaveFullscreen

Comment on attachment 78445 [details] updated with Kenneth's suggestion (removed default fullscreen widget) View in context: https://bugs.webkit.org/attachment.cgi?id=78445&action=review > WebKit/qt/examples/platformplugin/WebPlugin.cpp:238 > +FullscreenVideoHandler::FullscreenVideoHandler(): m_mediaPlayer(0), m_mediaWidget(0) Coding style nitpick - this line should be split :)

Comment on attachment 78445 [details] updated with Kenneth's suggestion (removed default fullscreen widget) View in context: https://bugs.webkit.org/attachment.cgi?id=78445&action=review > WebKit/qt/Api/qwebkitplatformplugin.h:134 > + void fullscreenClosed(); > + > +public Q_SLOTS: > + virtual void enterFullscreen(QMediaPlayer*) = 0; > + virtual void exitFullscreen() = 0; I think the names are fine :) > WebKit/qt/examples/platformplugin/WebPlugin.cpp:234 > + if (ev->key() == Qt::Key_Space) { > + emit spaceButtonPressed(); > + return; I'm not sure if this abstraction is really worth it. I'd just pass the QMediaPlayer to FullscreenVideoWidget, so it can act on the player directly, instead of the signal/slot indirection.

Created attachment 78691 [details] fix style

In general this looks fine to me now. The only other change I would suggest is to fold the code of the FullscreenVideoQt class simply into ChromeClientQt. There's barely any logic in there, and the class name isn't that descriptive IMHO :) If you want to be _really_ consistent with Qt, then the platform plugin API should also use Full_S_creen, instead of Full_s_creen. I'm not too particular about the latter, but I'd really like to hear your (Yi) opinion about merging FullscreenVideoQt into ChromeClientQt. Also I think QWebFullscreenVideoHandler* ChromeClientQt::createFullscreenVideoHandler() const can be folded into wherever it is called from, given that it's only one line of code and called only from one place. Yi, what do you think?

(In reply to comment #23) > In general this looks fine to me now. The only other change I would suggest is to fold the code of the FullscreenVideoQt class simply into ChromeClientQt. There's barely any logic in there, and the class name isn't that descriptive IMHO :) > > If you want to be _really_ consistent with Qt, then the platform plugin API should also use Full_S_creen, instead of Full_s_creen. > > I'm not too particular about the latter, but I'd really like to hear your (Yi) opinion about merging FullscreenVideoQt into ChromeClientQt. > > Also I think QWebFullscreenVideoHandler* ChromeClientQt::createFullscreenVideoHandler() const can be folded into wherever it is called from, given that it's only one line of code and called only from one place. > > Yi, what do you think? Thanks for reviewing, Simon :) I can remove the fullscreenvideoQt class and also ChromeClientQt::createFullscreenVideoHandler(), since now there is no default fullscreen video widget ( I introduced this class and function to make the code be clear for different fullscreen video widget). To be consistent with Qt, also I will rename QWebFullscreenVideoHandler to QWebFullScreenVideoHandler, as well as other Full_S_creen functions. Will provide a new patch ASAP :)

(In reply to comment #24) > (In reply to comment #23) > > In general this looks fine to me now. The only other change I would suggest is to fold the code of the FullscreenVideoQt class simply into ChromeClientQt. There's barely any logic in there, and the class name isn't that descriptive IMHO :) > > > > If you want to be _really_ consistent with Qt, then the platform plugin API should also use Full_S_creen, instead of Full_s_creen. > > > > I'm not too particular about the latter, but I'd really like to hear your (Yi) opinion about merging FullscreenVideoQt into ChromeClientQt. > > > > Also I think QWebFullscreenVideoHandler* ChromeClientQt::createFullscreenVideoHandler() const can be folded into wherever it is called from, given that it's only one line of code and called only from one place. > > > > Yi, what do you think? > > Thanks for reviewing, Simon :) I can remove the fullscreenvideoQt class and also ChromeClientQt::createFullscreenVideoHandler(), since now there is no default fullscreen video widget ( I introduced this class and function to make the code be clear for different fullscreen video widget). > > To be consistent with Qt, also I will rename QWebFullscreenVideoHandler to QWebFullScreenVideoHandler, as well as other Full_S_creen functions. > > Will provide a new patch ASAP :) (In reply to comment #24) > (In reply to comment #23) > > In general this looks fine to me now. The only other change I would suggest is to fold the code of the FullscreenVideoQt class simply into ChromeClientQt. There's barely any logic in there, and the class name isn't that descriptive IMHO :) > > > > If you want to be _really_ consistent with Qt, then the platform plugin API should also use Full_S_creen, instead of Full_s_creen. > > > > I'm not too particular about the latter, but I'd really like to hear your (Yi) opinion about merging FullscreenVideoQt into ChromeClientQt. > > > > Also I think QWebFullscreenVideoHandler* ChromeClientQt::createFullscreenVideoHandler() const can be folded into wherever it is called from, given that it's only one line of code and called only from one place. > > > > Yi, what do you think? > > Thanks for reviewing, Simon :) I can remove the fullscreenvideoQt class and also ChromeClientQt::createFullscreenVideoHandler(), since now there is no default fullscreen video widget ( I introduced this class and function to make the code be clear for different fullscreen video widget). > > To be consistent with Qt, also I will rename QWebFullscreenVideoHandler to QWebFullScreenVideoHandler, as well as other Full_S_creen functions. > > Will provide a new patch ASAP :) Simon, still there? I just remember there is another reason why I added this FullscreenVideoQt class, is because I need a QObject derived class to connect the signals from QWebFullscreenVideoHandler, while ChromeClientQt is NOT a QObject derived class. So either we keep the FullscreenVideoQt, or make ChromeClientQt to be QObject based. Please let me know your thoughts.

(In reply to comment #25) > Simon, still there? I just remember there is another reason why I added this FullscreenVideoQt class, is because I need a QObject derived class to connect the signals from QWebFullscreenVideoHandler, while ChromeClientQt is NOT a QObject derived class. So either we keep the FullscreenVideoQt, or make ChromeClientQt to be QObject based. Please let me know your thoughts. Oops, you're right.

Comment on attachment 78691 [details] fix style View in context: https://bugs.webkit.org/attachment.cgi?id=78691&action=review r=me otherwise. Would be nice to fix the method folding before landing, but isn't super critical to me. > WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:670 > +QWebFullscreenVideoHandler* ChromeClientQt::createFullscreenVideoHandler() const > +{ > + return m_platformPlugin.createFullscreenVideoHandler(); > +} I'd say get rid of that method and do the one-liner in FullScreenVideoQt.

(In reply to comment #27) > (From update of attachment 78691 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78691&action=review > > r=me otherwise. Would be nice to fix the method folding before landing, but isn't super critical to me. > > > WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:670 > > +QWebFullscreenVideoHandler* ChromeClientQt::createFullscreenVideoHandler() const > > +{ > > + return m_platformPlugin.createFullscreenVideoHandler(); > > +} > > I'd say get rid of that method and do the one-liner in FullScreenVideoQt. Yes, I can fix it. I will submit a new patch which has following changes according your comments 1) pass QMediaPlayer to FullScreenVideoWidget, so it can act on the player directly, instead of the signal/slot indirection. > WebKit/qt/examples/platformplugin/WebPlugin.cpp:234 > + if (ev->key() == Qt::Key_Space) { > + emit spaceButtonPressed(); > + return; 2) use Full_S_creen, instead of Full_s_creen, e.g. FullScreenVideoQt, QWebFullScreenVideoHandler 3) get rid of ChromeClientQt::createFullscreenVideoHandler() Still building my patch and will submit once it's done.

Created attachment 78745 [details] updated with Simon's suggestion

The commit-queue encountered the following flaky tests while processing attachment 78745 [details]: http/tests/xmlhttprequest/basic-auth.html bug 51613 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 78745 [details] updated with Simon's suggestion Clearing flags on attachment: 78745 Committed r75708: <http://trac.webkit.org/changeset/75708>

All reviewed patches have been landed. Closing bug.

Before this can be cherry-picked to Webkit 2.2, some backporting needs to occur.

Created attachment 79076 [details] This is a backport to Qt Webkit 2.2. This attachment is a backport of the change to 51249 plus many changes of the relative Media components. The following changesets were backported to get the media components more up-to-date: 64085 64884 64997 65170 65324 65494 65514 65758 65779 66023 66110 66311 66541 66895 66956 66961 68181 68526 68532 68682 68772 68774 68777 69291 In addition 52315 & 52252 are also included in this patch because those had not yet be commited to qtwebkit 2.2 but are needed for the testing.

Helder just told me this breaks at least Mac. I reopen until this is tested / reverted.

(In reply to comment #35) > Helder just told me this breaks at least Mac. I reopen until this is tested / reverted. Hi, Benjamin Poulain & Helder, what does it break, build or test? Could you give me more information about it? Thanks

Created attachment 79079 [details] Backport to webkit 2.2 There was a small mistake in the previous patch. Submitted a 2nd patch for backporting.

(In reply to comment #36) > Hi, Benjamin Poulain & Helder, what does it break, build or test? Could you give me more information about it? Thanks The patch actually breaks the build on all platform if mobility is not installed.

(In reply to comment #38) > (In reply to comment #36) > > Hi, Benjamin Poulain & Helder, what does it break, build or test? Could you give me more information about it? Thanks > > The patch actually breaks the build on all platform if mobility is not installed. Thanks Benjamin. Nancy and I will test the build on Windows & Linux without qtmobility installed. We will fix this issue today.(In reply to comment #38) > (In reply to comment #36) > > Hi, Benjamin Poulain & Helder, what does it break, build or test? Could you give me more information about it? Thanks > > The patch actually breaks the build on all platform if mobility is not installed. Thanks Benjamin. Nancy and I will test the build on Windows & Linux without qtmobility installed. We will fix this issue today.

I reverted since this was annoying contributors and the fix is not trivial. You can just submit the updated version here. You guys should come on IRC, channel #qtwebkit.

Created attachment 79090 [details] fix build issue The build issue occurs if 1) QtMobility is not installed,and 2) Qt has phonon support. To fix it, I introduce a new flag named 'ENABLE_QT_MULTIMEDIA' which is defined in Source/WebCore/features.pri. The code depends on QMediaPlayer only be compiled when ENABLE_QT_MULTIMEDIA is set to 1.

Created attachment 79100 [details] Patch to backport to Qt Webkit 2.2 Attached Qt Webkit 2.2 backporting patch that includes Yi's latest changes to fix build issues.

The commit-queue encountered the following flaky tests while processing attachment 79090 [details]: inspector/debugger-pause-on-breakpoint.html bug 51320 (author: podivilov@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 79090 [details] fix build issue Clearing flags on attachment: 79090 Committed r75944: <http://trac.webkit.org/changeset/75944>

All reviewed patches have been landed. Closing bug.

(In reply to comment #44) > (From update of attachment 79090 [details]) > Clearing flags on attachment: 79090 > > Committed r75944: <http://trac.webkit.org/changeset/75944> And a build fix landed in http://trac.webkit.org/changeset/75945 because of moving WebKit to Source.

(In reply to comment #34) > Created an attachment (id=79076) [details] > This is a backport to Qt Webkit 2.2. > > This attachment is a backport of the change to 51249 plus many changes of the relative Media components. > > The following changesets were backported to get the media components more up-to-date: > 64085 > 64884 > 64997 > 65170 > 65324 > 65494 > 65514 > 65758 > 65779 > 66023 > 66110 > 66311 > 66541 > 66895 > 66956 > 66961 > 68181 > 68526 > 68532 > 68682 > 68772 > 68774 > 68777 > 69291 That's a *huge* patch. Please make an extra effort in providing a patch series in cases like that. Instead of a large patch, there should be a git branch and a set of bugzilla dependencies. I could even help with the backporting effort. With splitted patches we can: - review it better; - understand clearly what has been backported/cherry-picked into each branch (there are several tools that parse the git logs and generate reports); - git bisect any regression; - easily revert a particular regression; - have proper release-notes. Please have this in mind in the future. Also, this patch is missing two files: WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp WebKit/qt/WebCoreSupport/FullScreenVideoQt.h For now, as I know you need this change ASAP, I've released a test branch with this patch integrated with all other changes from the previous week + the files copied from trunk. During the next days I'll split the patch and add the proper changelogs before pushing it to the official branch. The branch is called "qtwebkit-2.2-fullscreen-video".

(In reply to comment #34) > Created an attachment (id=79076) [details] > This is a backport to Qt Webkit 2.2. > > This attachment is a backport of the change to 51249 plus many changes of the relative Media components. > <snip> This patch introduced a regression in one of the layout tests: fast/dom/URL-attribute-reflection-pretty-diff.html is now failing: -PASS testURLReflection('src', 'source') is 'non-empty URL' +FAIL testURLReflection('src', 'source') should be non-empty URL. Was URL. To test it, you can either use the tessting VM (http://webkit.sed.hu/blog/20101028/qtwebkit-builder-and-tester-virtual-machine) or submit a branch to the bot: http://webkit.sed.hu/buildbot/builders/QtWebKit2.2-branch%20x86-32%20Linux%20Release It would be nice to have this investigated/fixed before merging it into the official 2.2 branch.

(In reply to comment #48) > (In reply to comment #34) > > Created an attachment (id=79076) [details] [details] > > This is a backport to Qt Webkit 2.2. > > > > This attachment is a backport of the change to 51249 plus many changes of the relative Media components. > > > > <snip> > > This patch introduced a regression in one of the layout tests: > > fast/dom/URL-attribute-reflection-pretty-diff.html is now failing: > > -PASS testURLReflection('src', 'source') is 'non-empty URL' > +FAIL testURLReflection('src', 'source') should be non-empty URL. Was URL. > > To test it, you can either use the tessting VM (http://webkit.sed.hu/blog/20101028/qtwebkit-builder-and-tester-virtual-machine) or submit a branch to the bot: http://webkit.sed.hu/buildbot/builders/QtWebKit2.2-branch%20x86-32%20Linux%20Release > > It would be nice to have this investigated/fixed before merging it into the official 2.2 branch. Hi Ademar, I will take a look at it.

(In reply to comment #47) <snip> > That's a *huge* patch. Please make an extra effort in providing a patch series in cases like that. Instead of a large patch, there should be a git branch and a set of bugzilla dependencies. I've managed to cherry-pick the 25 individual patches, solving the conflicts along the way (it was easier than I though, actually). I've pushed them to a test branch: qtwebkit-2.2-fullscreen-video-cherry-picks. It includes all Changelogs and testcases. I also noticed that r65494 was missing in the original list, so I cherry-picked it as well. Now we need to backport the original patch attached to this bug on top of that branch, as it doesn't apply cleanly. I'll work more on it tomorrow, but meanwhile I decided to push the branch and leave this note here as it may be useful to someone else.

Thanks Ademar for all the help with the backporting! I am in the process of testing this new branch. I have already backported the original patch from Yi. I have found at least one merge problem in HTMLMediaElement::updatePlayState. I am still testing so I will get back to you with the fix for that and any others if needed.

Created attachment 79756 [details] backport patch for the test branch Ademar, you can use this to patch qtwebkit-2.2-fullscreen-video-cherry-picks branch. I have tested it on Linux.

(In reply to comment #52) > Created an attachment (id=79756) [details] > backport patch for the test branch > > Ademar, you can use this to patch qtwebkit-2.2-fullscreen-video-cherry-picks branch. I have tested it on Linux. Please try to submit it in git format. It should be the original patch with conflicts resolved, not a new patch striped of changelogs, tests and examples. The best way to do that, IMO, is to cherry-pick the original patch that went into trunk, solve the conflicts, commit (in your local branch) and export it using git format-patch. Anyway, I did that for you (your patch was missing the Changelogs and examples) and pushed it into the qtwebkit-2.2-fullscreen-video-cherry-picks branch. It should be reviewed and also tested for the regression (see Comment #48). Hopefully we'll be able to push it into the official branch on Monday. :-)

Revision r75944 cherry-picked into qtwebkit-2.2 with commit 6ac72a2 <http://gitorious.org/webkit/qtwebkit/commit/6ac72a2>