Bug 51249 - [Qt] Extend the Platform Plugin to support full screen video handler
Summary: [Qt] Extend the Platform Plugin to support full screen video handler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yi Shen
URL:
Keywords: Qt, QtTriaged
Depends on: 36663 37591 42087 42897 43702 43923 44013 44343 44370 44577 44743 44985 45263 45264 45306 45694 46777 47073 52521
Blocks: 51543
  Show dependency treegraph
 
Reported: 2010-12-17 06:32 PST by Yi Shen
Modified: 2011-01-24 08:37 PST (History)
15 users (show)

See Also:


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
akling: 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

Note You need to log in before you can comment on or make changes to this bug.
Description Yi Shen 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.
Comment 1 Yi Shen 2010-12-22 11:02:04 PST
Created attachment 77238 [details]
first draft
Comment 2 Yi Shen 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.
Comment 3 Ariya Hidayat 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?
Comment 4 Yi Shen 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.
Comment 5 Yi Shen 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.
Comment 6 Kenneth Rohde Christiansen 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?
Comment 7 David Levin 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
Comment 8 Yi Shen 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.
Comment 9 Yi Shen 2011-01-07 08:25:14 PST
Created attachment 78241 [details]
fix style
Comment 10 Andreas Kling 2011-01-07 08:44:53 PST
Comment on attachment 78241 [details]
fix style

Needs ChangeLog.
Comment 11 Yi Shen 2011-01-07 15:59:29 PST
Created attachment 78286 [details]
add change log
Comment 12 Yi Shen 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
Comment 13 Simon Hausmann 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?
Comment 14 Yi Shen 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 :)
Comment 15 Yi Shen 2011-01-10 13:12:56 PST
Created attachment 78440 [details]
updated with Simon's suggestion (still has default fullscreen widget)
Comment 16 Yi Shen 2011-01-10 13:13:34 PST
Created attachment 78441 [details]
updated with Simon's suggestion (removed default fullscreen widget)
Comment 17 Kenneth Rohde Christiansen 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?
Comment 18 Yi Shen 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 :-)
Comment 19 Yi Shen 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
Comment 20 Simon Hausmann 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 :)
Comment 21 Simon Hausmann 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.
Comment 22 Yi Shen 2011-01-12 07:40:55 PST
Created attachment 78691 [details]
fix style
Comment 23 Simon Hausmann 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?
Comment 24 Yi Shen 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 :)
Comment 25 Yi Shen 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.
Comment 26 Simon Hausmann 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.
Comment 27 Simon Hausmann 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.
Comment 28 Yi Shen 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.
Comment 29 Yi Shen 2011-01-12 15:24:00 PST
Created attachment 78745 [details]
updated with Simon's suggestion
Comment 30 WebKit Commit Bot 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.
Comment 31 WebKit Commit Bot 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>
Comment 32 WebKit Commit Bot 2011-01-13 03:47:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 33 Nancy Piedra 2011-01-13 07:14:12 PST
Before this can be cherry-picked to Webkit 2.2, some backporting needs to occur.
Comment 34 Nancy Piedra 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.
Comment 35 Benjamin Poulain 2011-01-15 14:37:55 PST
Helder just told me this breaks at least Mac. I reopen until this is tested / reverted.
Comment 36 Yi Shen 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
Comment 37 Nancy Piedra 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.
Comment 38 Benjamin Poulain 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.
Comment 39 Yi Shen 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.
Comment 40 Benjamin Poulain 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.
Comment 41 Yi Shen 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.
Comment 42 Nancy Piedra 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.
Comment 43 WebKit Commit Bot 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.
Comment 44 WebKit Commit Bot 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>
Comment 45 WebKit Commit Bot 2011-01-17 08:00:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 46 Csaba Osztrogonác 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.
Comment 47 Ademar Reis 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".
Comment 48 Ademar Reis 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.
Comment 49 Yi Shen 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.
Comment 50 Ademar Reis 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.
Comment 51 Nancy Piedra 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.
Comment 52 Yi Shen 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.
Comment 53 Ademar Reis 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. :-)
Comment 54 Ademar Reis 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>