WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
51249
[Qt] Extend the Platform Plugin to support full screen video handler
https://bugs.webkit.org/show_bug.cgi?id=51249
Summary
[Qt] Extend the Platform Plugin to support full screen video handler
Yi Shen
Reported
2010-12-17 06:32:04 PST
By extending the platform plugin, the browser vendor can make their own fullscreen video player plugin for playing html5 video.
Attachments
first draft
(28.81 KB, patch)
2010-12-22 11:02 PST
,
Yi Shen
ariya.hidayat
: review-
Details
Formatted Diff
Diff
updated with Ariya's suggestion
(27.32 KB, patch)
2011-01-04 08:12 PST
,
Yi Shen
no flags
Details
Formatted Diff
Diff
fix style
(27.28 KB, patch)
2011-01-07 08:25 PST
,
Yi Shen
kling
: review-
Details
Formatted Diff
Diff
add change log
(32.37 KB, patch)
2011-01-07 15:59 PST
,
Yi Shen
hausmann
: review-
Details
Formatted Diff
Diff
updated with Simon's suggestion (still has default fullscreen widget)
(32.64 KB, patch)
2011-01-10 13:12 PST
,
Yi Shen
no flags
Details
Formatted Diff
Diff
updated with Simon's suggestion (removed default fullscreen widget)
(26.63 KB, patch)
2011-01-10 13:13 PST
,
Yi Shen
no flags
Details
Formatted Diff
Diff
updated with Kenneth's suggestion (removed default fullscreen widget)
(26.62 KB, patch)
2011-01-10 13:45 PST
,
Yi Shen
no flags
Details
Formatted Diff
Diff
fix style
(26.60 KB, patch)
2011-01-12 07:40 PST
,
Yi Shen
hausmann
: review+
hausmann
: commit-queue-
Details
Formatted Diff
Diff
updated with Simon's suggestion
(26.19 KB, patch)
2011-01-12 15:24 PST
,
Yi Shen
no flags
Details
Formatted Diff
Diff
This is a backport to Qt Webkit 2.2.
(99.26 KB, application/octet-stream)
2011-01-15 13:50 PST
,
Nancy Piedra
no flags
Details
Backport to webkit 2.2
(99.33 KB, application/octet-stream)
2011-01-15 15:02 PST
,
Nancy Piedra
no flags
Details
fix build issue
(27.37 KB, patch)
2011-01-15 20:41 PST
,
Yi Shen
no flags
Details
Formatted Diff
Diff
Patch to backport to Qt Webkit 2.2
(100.75 KB, patch)
2011-01-16 09:41 PST
,
Nancy Piedra
no flags
Details
Formatted Diff
Diff
backport patch for the test branch
(12.09 KB, patch)
2011-01-21 10:03 PST
,
Yi Shen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Yi Shen
Comment 1
2010-12-22 11:02:04 PST
Created
attachment 77238
[details]
first draft
Yi Shen
Comment 2
2010-12-22 11:20:13 PST
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.
Ariya Hidayat
Comment 3
2010-12-31 08:05:49 PST
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?
Yi Shen
Comment 4
2011-01-03 07:14:45 PST
(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.
Yi Shen
Comment 5
2011-01-04 08:12:17 PST
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.
Kenneth Rohde Christiansen
Comment 6
2011-01-04 08:15:56 PST
Simon, how similar is this to your work on the WebKit2 side? Can you comment on the patch?
David Levin
Comment 7
2011-01-05 13:12:24 PST
(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
Yi Shen
Comment 8
2011-01-05 14:05:54 PST
(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.
Yi Shen
Comment 9
2011-01-07 08:25:14 PST
Created
attachment 78241
[details]
fix style
Andreas Kling
Comment 10
2011-01-07 08:44:53 PST
Comment on
attachment 78241
[details]
fix style Needs ChangeLog.
Yi Shen
Comment 11
2011-01-07 15:59:29 PST
Created
attachment 78286
[details]
add change log
Yi Shen
Comment 12
2011-01-07 16:09:52 PST
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
Simon Hausmann
Comment 13
2011-01-10 01:46:49 PST
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?
Yi Shen
Comment 14
2011-01-10 07:27:50 PST
(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 :)
Yi Shen
Comment 15
2011-01-10 13:12:56 PST
Created
attachment 78440
[details]
updated with Simon's suggestion (still has default fullscreen widget)
Yi Shen
Comment 16
2011-01-10 13:13:34 PST
Created
attachment 78441
[details]
updated with Simon's suggestion (removed default fullscreen widget)
Kenneth Rohde Christiansen
Comment 17
2011-01-10 13:20:26 PST
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?
Yi Shen
Comment 18
2011-01-10 13:23:37 PST
(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 :-)
Yi Shen
Comment 19
2011-01-10 13:45:37 PST
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
Simon Hausmann
Comment 20
2011-01-12 07:29:03 PST
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 :)
Simon Hausmann
Comment 21
2011-01-12 07:40:25 PST
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.
Yi Shen
Comment 22
2011-01-12 07:40:55 PST
Created
attachment 78691
[details]
fix style
Simon Hausmann
Comment 23
2011-01-12 07:43:24 PST
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?
Yi Shen
Comment 24
2011-01-12 07:59:47 PST
(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 :)
Yi Shen
Comment 25
2011-01-12 10:04:43 PST
(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.
Simon Hausmann
Comment 26
2011-01-12 13:58:41 PST
(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.
Simon Hausmann
Comment 27
2011-01-12 13:59:56 PST
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.
Yi Shen
Comment 28
2011-01-12 14:14:20 PST
(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.
Yi Shen
Comment 29
2011-01-12 15:24:00 PST
Created
attachment 78745
[details]
updated with Simon's suggestion
WebKit Commit Bot
Comment 30
2011-01-13 03:45:35 PST
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.
WebKit Commit Bot
Comment 31
2011-01-13 03:47:07 PST
Comment on
attachment 78745
[details]
updated with Simon's suggestion Clearing flags on attachment: 78745 Committed
r75708
: <
http://trac.webkit.org/changeset/75708
>
WebKit Commit Bot
Comment 32
2011-01-13 03:47:17 PST
All reviewed patches have been landed. Closing bug.
Nancy Piedra
Comment 33
2011-01-13 07:14:12 PST
Before this can be cherry-picked to Webkit 2.2, some backporting needs to occur.
Nancy Piedra
Comment 34
2011-01-15 13:50:34 PST
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.
Benjamin Poulain
Comment 35
2011-01-15 14:37:55 PST
Helder just told me this breaks at least Mac. I reopen until this is tested / reverted.
Yi Shen
Comment 36
2011-01-15 14:51:07 PST
(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
Nancy Piedra
Comment 37
2011-01-15 15:02:47 PST
Created
attachment 79079
[details]
Backport to webkit 2.2 There was a small mistake in the previous patch. Submitted a 2nd patch for backporting.
Benjamin Poulain
Comment 38
2011-01-15 15:05:21 PST
(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.
Yi Shen
Comment 39
2011-01-15 15:26:54 PST
(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.
Benjamin Poulain
Comment 40
2011-01-15 16:39:24 PST
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.
Yi Shen
Comment 41
2011-01-15 20:41:46 PST
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.
Nancy Piedra
Comment 42
2011-01-16 09:41:29 PST
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.
WebKit Commit Bot
Comment 43
2011-01-17 07:57:38 PST
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.
WebKit Commit Bot
Comment 44
2011-01-17 08:00:50 PST
Comment on
attachment 79090
[details]
fix build issue Clearing flags on attachment: 79090 Committed
r75944
: <
http://trac.webkit.org/changeset/75944
>
WebKit Commit Bot
Comment 45
2011-01-17 08:00:59 PST
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 46
2011-01-17 08:23:16 PST
(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.
Ademar Reis
Comment 47
2011-01-17 15:23:49 PST
(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".
Ademar Reis
Comment 48
2011-01-19 12:37:24 PST
(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.
Yi Shen
Comment 49
2011-01-19 13:26:00 PST
(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.
Ademar Reis
Comment 50
2011-01-20 12:29:39 PST
(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.
Nancy Piedra
Comment 51
2011-01-21 04:27:44 PST
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.
Yi Shen
Comment 52
2011-01-21 10:03:45 PST
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.
Ademar Reis
Comment 53
2011-01-21 13:56:13 PST
(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. :-)
Ademar Reis
Comment 54
2011-01-24 07:46:58 PST
Revision
r75944
cherry-picked into qtwebkit-2.2 with commit 6ac72a2 <
http://gitorious.org/webkit/qtwebkit/commit/6ac72a2
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug