The plugin needs to be upstreamed for a better maintainability.
Created attachment 90355 [details] platformpluginV1
Attachment 90355 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1 Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:21: #ifndef header guard has wrong style, please use: WTF_qwebkitplatformplugin_h [build/header_guard] [5] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:37: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:57: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:71: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:82: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:96: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:114: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:144: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:156: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:157: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 10 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
The file has style issue is copied from WebKit/qt/Api/qwebkitplatformplugin.h. Should we fix the sytle problem for WebKit/qt/Api/qwebkitplatformplugin.h? (In reply to comment #2) > Attachment 90355 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1 > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:21: #ifndef header guard has wrong style, please use: WTF_qwebkitplatformplugin_h [build/header_guard] [5] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:37: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:57: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:71: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:82: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:96: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:114: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:144: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:156: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:157: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] > Total errors found: 10 in 8 files > > > If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 90355 [details] platformpluginV1 View in context: https://bugs.webkit.org/attachment.cgi?id=90355&action=review Where is the implementation for the video part???? > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:26 > +#include <akndiscreetpopup.h> Sorting? > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:32 > +#if defined(WTF_USE_QT_MULTIMEDIA) && WTF_USE_QT_MULTIMEDIA I believe you can use USE(QT_MULTIMEDIA), moc will still be run on the classes below.
To speed up the review process, I plan to upstream the plugin in two steps. In this step 1, the patch doesn't include the video part (which needs to be cleared before upstream). Once the patch V1 gets landed, I will provide patch V2 with the video part. (In reply to comment #4) > (From update of attachment 90355 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90355&action=review > > Where is the implementation for the video part???? > > > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:26 > > +#include <akndiscreetpopup.h> > > Sorting? Sorry, I didn't get it :) > > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:32 > > +#if defined(WTF_USE_QT_MULTIMEDIA) && WTF_USE_QT_MULTIMEDIA > > I believe you can use USE(QT_MULTIMEDIA), moc will still be run on the classes below. I copied this from WebKit/qt/Api/qwebkitplatformplugin.h. Do I need to update WebKit/qt/Api/qwebkitplatformplugin.h also?
Comment on attachment 90355 [details] platformpluginV1 View in context: https://bugs.webkit.org/attachment.cgi?id=90355&action=review > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:34 > + QLibrary testLib("qttestability"); Do we need this test code? > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:38 > +#ifdef Q_OS_SYMBIAN This is a Symbian specific plugin. We don't need this check.
Comment on attachment 90355 [details] platformpluginV1 View in context: https://bugs.webkit.org/attachment.cgi?id=90355&action=review >>> Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:26 >>> +#include <akndiscreetpopup.h> >> >> Sorting? > > Sorry, I didn't get it :) The sorting of the headers? >> Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:34 >> + QLibrary testLib("qttestability"); > > Do we need this test code? Well in theory it would be nice :D. >>> Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:32 >>> +#if defined(WTF_USE_QT_MULTIMEDIA) && WTF_USE_QT_MULTIMEDIA >> >> I believe you can use USE(QT_MULTIMEDIA), moc will still be run on the classes below. > > I copied this from WebKit/qt/Api/qwebkitplatformplugin.h. Do I need to update WebKit/qt/Api/qwebkitplatformplugin.h also? Ok well then let it like there.
Created attachment 90366 [details] platformpluginV1.1
Attachment 90366 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1 Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:21: #ifndef header guard has wrong style, please use: WTF_qwebkitplatformplugin_h [build/header_guard] [5] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:37: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:57: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:71: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:82: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:96: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:114: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:144: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:156: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:157: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:21: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 11 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #7) > (From update of attachment 90355 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90355&action=review > > >>> Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:26 > >>> +#include <akndiscreetpopup.h> > >> > >> Sorting? > > > > Sorry, I didn't get it :) > > The sorting of the headers? I think it is correct, since no error from sytle-checker (the lower case is after upper case in alphabetically sorting) > > >> Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:34 > >> + QLibrary testLib("qttestability"); > > > > Do we need this test code? > > Well in theory it would be nice :D. Let's get rid of it for now and put it back if we need it in the future :) > > >>> Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:32 > >>> +#if defined(WTF_USE_QT_MULTIMEDIA) && WTF_USE_QT_MULTIMEDIA > >> > >> I believe you can use USE(QT_MULTIMEDIA), moc will still be run on the classes below. > > > > I copied this from WebKit/qt/Api/qwebkitplatformplugin.h. Do I need to update WebKit/qt/Api/qwebkitplatformplugin.h also? > > Ok well then let it like there.
(In reply to comment #9) > Attachment 90366 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1 > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:21: #ifndef header guard has wrong style, please use: WTF_qwebkitplatformplugin_h [build/header_guard] [5] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:37: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:57: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:71: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:82: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:96: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:114: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:144: This { should be at the end of the previous line [whitespace/braces] [4] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:156: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:157: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:21: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Total errors found: 11 in 8 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. You should probably run check-webkit-style locally before submitting.
Hi Alexis, I did run the check-webkit-style locally before the submit :) The errors show below doesn't make sense to me. First, Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h is the same copy of Source/WebKit/qt/Api/qwebkitplatformplugin.h and I don't think I should fix the style error in my qwebkitplatformplugin.h. Second, for Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:21: Found header this file implements before WebCore config.h, I didn't see any reason that I have to include "config.h" in WebPlugin.cpp. (The platform plugin should not include any unexposed webkit head file, right? ) Please let me know your thoughts, thanks (In reply to comment #11) > (In reply to comment #9) > > Attachment 90366 [details] [details] did not pass style-queue: > > > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit.pro', u'Source..." exit_code: 1 > > > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:21: #ifndef header guard has wrong style, please use: WTF_qwebkitplatformplugin_h [build/header_guard] [5] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:37: This { should be at the end of the previous line [whitespace/braces] [4] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:57: This { should be at the end of the previous line [whitespace/braces] [4] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:71: This { should be at the end of the previous line [whitespace/braces] [4] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:82: This { should be at the end of the previous line [whitespace/braces] [4] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:96: This { should be at the end of the previous line [whitespace/braces] [4] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:114: This { should be at the end of the previous line [whitespace/braces] [4] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:144: This { should be at the end of the previous line [whitespace/braces] [4] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:156: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:157: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5] > > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:21: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > > Total errors found: 11 in 8 files > > > > > > If any of these errors are false positives, please file a bug against check-webkit-style. > > You should probably run check-webkit-style locally before submitting.
I will add some check style exemptions for my patch.
(In reply to comment #13) > I will add some check style exemptions for my patch. Add it to the exemption because it's a Qt API so it does not follow the rules. Sorry for the noise.
(In reply to comment #14) > (In reply to comment #13) > > I will add some check style exemptions for my patch. > > Add it to the exemption because it's a Qt API so it does not follow the rules. Sorry for the noise. Thanks for the comment, Alexis. I am going to put something like below into Tools/Scripts/webkitpy/style/checker.py ([# Qt Symian platform plugin has no config.h or # exceptional header guards "Source/WebKit/qt/symbian/platformplugin"], ["-build/include", "-build/header_guard"]), However, it won't fix the style errors for the Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h. To fix the qwebkitplatformplugin, I need to add '-whitespace/braces' and '-readability/parameter_name' for Source/WebKit/qt/symbian/platformplugin, which I believe is not right. (I still want to check the parameter name and whitespace/braces for other files under Source/WebKit/qt/symbian/platformplugin, like WebPlugin.cpp). My suggestion is we can just ignore the style errors for qwebkitplatformplugin.h this time. Let me know your thoughts. Thanks!
I created a separated bug to fix the style issue for Api/qwebkitplatformplugin.h in https://bugs.webkit.org/show_bug.cgi?id=59097
Created attachment 90527 [details] platformpluginV1.2 fix style issues
Comment on attachment 90527 [details] platformpluginV1.2 View in context: https://bugs.webkit.org/attachment.cgi?id=90527&action=review > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:30 > + : QStyledItemDelegate(parent) seems to be indented a bit too much here > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:53 > +Popup::Popup(const QWebSelectData& data) : m_data(data) : part goes on next line > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:207 > + QAction* cancelAction = new QAction("Cancel", this); How are we dealing with translations? > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:212 > + QAction* okAction = new QAction("Ok", this); isnt it OK in English? > Source/WebKit/qt/symbian/platformplugin/WebPlugin.h:97 > + WebNotificationPresenter() {} > + ~WebNotificationPresenter() {} there needs to be a space within the {} > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:58 > + virtual ~QWebSelectMethod() {} also here > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:70 > + virtual ~QWebNotificationData() {} and here > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:92 > +class QWebHapticFeedbackPlayer: public QObject { > + Q_OBJECT Is this haptics support really in trunk? > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:95 > + QWebHapticFeedbackPlayer() {} > + virtual ~QWebHapticFeedbackPlayer() {} and here
(In reply to comment #18) > (From update of attachment 90527 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90527&action=review > > > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:30 > > + : QStyledItemDelegate(parent) > > seems to be indented a bit too much here Thanks, I will fix it. > > > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:53 > > +Popup::Popup(const QWebSelectData& data) : m_data(data) > > : part goes on next line Will fix it. > > > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:207 > > + QAction* cancelAction = new QAction("Cancel", this); > > How are we dealing with translations? Good question. Yael or Laszlo or Alexis, any thoughts? Thanks! > > > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:212 > > + QAction* okAction = new QAction("Ok", this); > > isnt it OK in English? > > > Source/WebKit/qt/symbian/platformplugin/WebPlugin.h:97 > > + WebNotificationPresenter() {} > > + ~WebNotificationPresenter() {} > > there needs to be a space within the {} > Will fix it. (hmmm, but the style check doesn't complain it) > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:58 > > + virtual ~QWebSelectMethod() {} > > also here > It is a only copy from Api/qwebkitplatformplugin.h. I am not sure I should fix this or not since the style check doesn't complain it. > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:70 > > + virtual ~QWebNotificationData() {} > > and here > > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:92 > > +class QWebHapticFeedbackPlayer: public QObject { > > + Q_OBJECT > > Is this haptics support really in trunk? > > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:95 > > + QWebHapticFeedbackPlayer() {} > > + virtual ~QWebHapticFeedbackPlayer() {} > > and here
(In reply to comment #18) > (From update of attachment 90527 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90527&action=review > > > Source/WebKit/qt/symbian/platformplugin/qwebkitplatformplugin.h:92 > > +class QWebHapticFeedbackPlayer: public QObject { > > + Q_OBJECT > > Is this haptics support really in trunk? Yes it is - http://trac.webkit.org/changeset/64557; It is not used at the moment however I do not see an urgency to remove it as we have plans to use it eventually.
Created attachment 90705 [details] platformpluginV1.3 updated with Kenneth's comments
Comment on attachment 90705 [details] platformpluginV1.3 Clearing flags on attachment: 90705 Committed r84627: <http://trac.webkit.org/changeset/84627>
All reviewed patches have been landed. Closing bug.
Will provide the patchV2 with the video part.
http://trac.webkit.org/changeset/84627 might have broken GTK Linux 32-bit Debug
Yi, the plugin does not build on the public SDK - see the buildlog on the Symbian buildbot. compile : Source\WebKit\qt\symbian\platformplugin\WebPlugin.cpp [armv5_urel.rvct4_0] "D:/webkit_qtsdk1.1/qt-symbian-release/build/Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp", line 25: Error: #5: cannot open source input file "akndiscreetpopup.h": No such file or directory #include <akndiscreetpopup.h> ^ D:/webkit_qtsdk1.1/qt-symbian-release/build/Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp: 0 warnings, 1 error We should either introduce a build flag to work around the problem or add the missing header to the SDK (perhaps you only need to update the SDK to a newer version on the bot).
Rolled out by http://trac.webkit.org/changeset/84734 But unfortunately the public Symbian bot is still sick. It seems it needs a kick.
(In reply to comment #27) > Rolled out by http://trac.webkit.org/changeset/84734 > > But unfortunately the public Symbian bot is still sick. It seems it needs a kick. Sorry, I only tested that patch on Qt SDK 1.1 RC, on which the build is fine. But for 1.1 Beta, seems it is missing epoc/include/platform/mw/akndiscreetpopup.h, which needed by our plugin. We may need to know more information about the buildbot.
Committed r85062: <http://trac.webkit.org/changeset/85062>
(In reply to comment #29) > Committed r85062: <http://trac.webkit.org/changeset/85062> We have updated the symbian buildbot with 1.1 RC SDK with Qt 4.7.3, so I just resubmit the patch and will check the buildbot again.
(In reply to comment #30) > (In reply to comment #29) > > Committed r85062: <http://trac.webkit.org/changeset/85062> > > We have updated the symbian buildbot with 1.1 RC SDK with Qt 4.7.3, so I just resubmit the patch and will check the buildbot again. The buildbot with 1.1 RC SDK is green for platformpluginV1.3 :)
Created attachment 91324 [details] platformpluginV2 Platformplugin part 2 which includes a html5 video player.
Comment on attachment 91324 [details] platformpluginV2 View in context: https://bugs.webkit.org/attachment.cgi?id=91324&action=review First round of comments :D. By the way there was no way a progress bar could be implemented rather than a refresh button? Like in trunk... > Source/WebKit/qt/symbian/platformplugin/Html5VideoPlugin.cpp:21 > +#include "Html5VideoPlugin.h" Proposal : Can we do like webkit HTMLXXX? > Source/WebKit/qt/symbian/platformplugin/Html5VideoPlugin.cpp:24 > +#include <QtGui> Just include the files you need. > Source/WebKit/qt/symbian/platformplugin/Html5VideoPlugin.cpp:43 > + if (player->state() == QMediaPlayer::PlayingState) Why pausing the playback? > Source/WebKit/qt/symbian/platformplugin/Html5VideoPlugin.cpp:106 > + videoWidget->onPlayerStopped(); Could we somehow connect directly the videowidget with the player? Why this intermediate slot? > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:52 > + overlayWidget->onPlayerStopped(); Same here, could you connect directly to the overlayWidget? > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:57 > + overlayWidget->onPlayerError(); Ditto. > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:63 > +} Ditto. > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:67 > + overlayWidget->onBufferingMedia(); Ditto. > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:72 > + overlayWidget->onBufferedMedia(); Ditto. > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:101 > + emit muted(isMuted); you can connect two signals directly so you can avoid those bunch of SLOTs connect(foo, SIGNAL(), bar, SIGNAL()); > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:106 > + emit volumeChanged(value); you can connect two signals directly so you can avoid those bunch of SLOTs connect(foo, SIGNAL(), bar, SIGNAL()); > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:117 > + overlayWidget->setVolume(volume); Connect directly same. > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:29 > + : QWidget(parent, Qt::Tool | Qt::FramelessWindowHint) Qt::Tool why? > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:85 > + timer = new QTimer(this); Could you give a name more explicit? > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:106 > + delete playerLabel; Some of the deletes are not necessary. The parenting system of Qt will take care. > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:162 > +QString OverlayWidget::timeToString(int seconds) Not sure it is feasible but can't we use the implementation in WebKit somehow? If not could you make sure the implementation is the same? > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:172 > + setStyleSheet("QSlider::groove { border: 2px solid black; border-radius: 5px; background: white; }" Perhaps in a file and a qrc? > Source/WebKit/qt/symbian/platformplugin/PlayerButton.cpp:27 > + : QToolButton(parent) A tool button? Why? > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:29 > +class PlayerButton : public QToolButton { Same why a QToolButton?
Though good job Yi to work on upstreaming that.
Thanks for reviewing, Alexis :) See my comments below, (In reply to comment #33) > (From update of attachment 91324 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91324&action=review > > First round of comments :D. > > By the way there was no way a progress bar could be implemented rather than a refresh button? Like in trunk... > > > Source/WebKit/qt/symbian/platformplugin/Html5VideoPlugin.cpp:21 > > +#include "Html5VideoPlugin.h" > > Proposal : Can we do like webkit HTMLXXX? Sure, I can change the names. > > > Source/WebKit/qt/symbian/platformplugin/Html5VideoPlugin.cpp:24 > > +#include <QtGui> > > Just include the files you need. > will remove it. > > Source/WebKit/qt/symbian/platformplugin/Html5VideoPlugin.cpp:43 > > + if (player->state() == QMediaPlayer::PlayingState) > > Why pausing the playback? > It was a requirement - disable the auto play. I will just remove it. > > Source/WebKit/qt/symbian/platformplugin/Html5VideoPlugin.cpp:106 > > + videoWidget->onPlayerStopped(); > > Could we somehow connect directly the videowidget with the player? Why this intermediate slot? > The intermediate slot checks the player state before calling videoWidget->onPlayerStopped(). Let's keep it for now :) > > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:52 > > + overlayWidget->onPlayerStopped(); > > Same here, could you connect directly to the overlayWidget? Can we keep this for now? As I remember, our developer designed the plugin in this way is to bypass some old symbian/qtmobility issues. These issues may be already get fixed, but I prefer to keep current design for the backward compatibility. > > > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:57 > > + overlayWidget->onPlayerError(); > > Ditto. > > > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:63 > > +} > > Ditto. > > > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:67 > > + overlayWidget->onBufferingMedia(); > > Ditto. > > > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:72 > > + overlayWidget->onBufferedMedia(); > > Ditto. > > > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:101 > > + emit muted(isMuted); > > you can connect two signals directly so you can avoid those bunch of SLOTs connect(foo, SIGNAL(), bar, SIGNAL()); > Will fix it. > > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:106 > > + emit volumeChanged(value); > > you can connect two signals directly so you can avoid those bunch of SLOTs connect(foo, SIGNAL(), bar, SIGNAL()); > Will Fix it. > > Source/WebKit/qt/symbian/platformplugin/Html5VideoWidget.cpp:117 > > + overlayWidget->setVolume(volume); > > Connect directly same. > > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:29 > > + : QWidget(parent, Qt::Tool | Qt::FramelessWindowHint) > > Qt::Tool why? > Not know why. Will remove and test it. > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:85 > > + timer = new QTimer(this); > > Could you give a name more explicit? > Will rename it > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:106 > > + delete playerLabel; > > Some of the deletes are not necessary. The parenting system of Qt will take care. > You are right. Will remove them > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:162 > > +QString OverlayWidget::timeToString(int seconds) > > Not sure it is feasible but can't we use the implementation in WebKit somehow? If not could you make sure the implementation is the same? > I couldn't find it in WebKit. The implementation looks fine for me :) > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:172 > > + setStyleSheet("QSlider::groove { border: 2px solid black; border-radius: 5px; background: white; }" > > Perhaps in a file and a qrc? > OK, I will remove them to a qss file. > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.cpp:27 > > + : QToolButton(parent) > > A tool button? Why? > Talked to Hui about this, and he said QToolButton's looks and feel fits the need in UI requirement, so we just used it here. > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:29 > > +class PlayerButton : public QToolButton { > > Same why a QToolButton?
Created attachment 91542 [details] platformpluginV21
> > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:29 > > + : QWidget(parent, Qt::Tool | Qt::FramelessWindowHint) > > Qt::Tool why? > > Not know why. Will remove and test it. Seems we need it to bypass a symbian/qt issue. Without Qt:Tool flag, this OverlayWidget(the control bar) doesn't show up. Let's keep it for now.
(In reply to comment #37) > > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:29 > > > + : QWidget(parent, Qt::Tool | Qt::FramelessWindowHint) > > > > Qt::Tool why? > > > > Not know why. Will remove and test it. > > Seems we need it to bypass a symbian/qt issue. Without Qt:Tool flag, this OverlayWidget(the control bar) doesn't show up. Let's keep it for now. Well it's more than a little workaround. I wonder why a simple QPushButton doesn't show up? Perhaps a little debugging in Qt would be nice.
Comment on attachment 91542 [details] platformpluginV21 View in context: https://bugs.webkit.org/attachment.cgi?id=91542&action=review > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:30 > + : QWidget(parent, Qt::Tool | Qt::FramelessWindowHint) I'm sure the button does not show up because of the parent (or something like that). > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:45 > + m_slider = new QSlider(Qt::Horizontal, this); better name? > Source/WebKit/qt/symbian/platformplugin/PlayerLabel.cpp:32 > + m_bufferingAnimationIterator = m_bufferingAnimation; Can you just take one icon and rotate it while you render? (with different angle). > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:23 > +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA Please be consistent with trunk. USE. > Source/WebKit/qt/symbian/platformplugin/WebPlugin.cpp:254 > +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA Ditto. > Source/WebKit/qt/symbian/platformplugin/platformplugin.pro:22 > + DEFINES += ENABLE_QT_MULTIMEDIA=1 Get rid of that one and update the code of the plugin.
> Can you just take one icon and rotate it while you render? (with different angle). Using multiple "refresh" icons (instead of a progress bar) for buffering animation was based on the UI Design. I think the UI Designer took these icons from Symbian platform. I'm not sure why Symbian doesn't take one icon and rotate as you suggested. Maybe for better performance?
(In reply to comment #40) > > Can you just take one icon and rotate it while you render? (with different angle). > > Using multiple "refresh" icons (instead of a progress bar) for buffering animation was based on the UI Design. I think the UI Designer took these icons from Symbian platform. I'm not sure why Symbian doesn't take one icon and rotate as you suggested. Maybe for better performance? Painting with a rotation should be fast, and it's better than having 3 or 4 images loaded in memory. Now perhaps there is a quality loss...
(In reply to comment #41) > (In reply to comment #40) > > > Can you just take one icon and rotate it while you render? (with different angle). > > > > Using multiple "refresh" icons (instead of a progress bar) for buffering animation was based on the UI Design. I think the UI Designer took these icons from Symbian platform. I'm not sure why Symbian doesn't take one icon and rotate as you suggested. Maybe for better performance? > > Painting with a rotation should be fast, and it's better than having 3 or 4 images loaded in memory. Now perhaps there is a quality loss... Well apparently visually it's better so keep it like this.
(In reply to comment #42) > (In reply to comment #41) > > (In reply to comment #40) > > > > Can you just take one icon and rotate it while you render? (with different angle). > > > > > > Using multiple "refresh" icons (instead of a progress bar) for buffering animation was based on the UI Design. I think the UI Designer took these icons from Symbian platform. I'm not sure why Symbian doesn't take one icon and rotate as you suggested. Maybe for better performance? > > > > Painting with a rotation should be fast, and it's better than having 3 or 4 images loaded in memory. Now perhaps there is a quality loss... > > Well apparently visually it's better so keep it like this. Thanks for the comments. I may update this patch in next week since I have some high priority work needed to be done in this week. But I will update it ASAP :)
(In reply to comment #39) > (From update of attachment 91542 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=91542&action=review > > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:30 > > + : QWidget(parent, Qt::Tool | Qt::FramelessWindowHint) > > I'm sure the button does not show up because of the parent (or something like that). The parent of the OverlayWidget is the HTML5VideoWidget, which has no parent. Would this be a problem? Hui, could you please take a look at this? I think we should just keep the workaround if this is a Qt/Symbian bug.
> The parent of the OverlayWidget is the HTML5VideoWidget, which has no parent. > Would this be a problem? Hui, could you please take a look at this? I think we should just keep the workaround if this is a Qt/Symbian bug. I prototyped this part of code long time ago. Forgot why a tool window was used. Guess it was used together with the Tool Buttons. Maybe we need to change overlay to a top level widget to see if the push buttons show up: OverlayWidget::OverlayWidget(int duration, QWidget *parent) - : QWidget(parent, Qt::Tool | Qt::FramelessWindowHint) + : QWidget(NULL, Qt::FramelessWindowHint) , isPaused(false) , isMuted(false) {
I tried the solution below on Symbian 3. It works. (In reply to comment #45) > I prototyped this part of code long time ago. Forgot why a tool window was used. Guess it was used together with the Tool Buttons. Maybe we need to change overlay to a top level widget to see if the push buttons show up: > OverlayWidget::OverlayWidget(int duration, QWidget *parent) > - : QWidget(parent, Qt::Tool | Qt::FramelessWindowHint) > + : QWidget(NULL, Qt::FramelessWindowHint) > , isPaused(false) > , isMuted(false) > {
Created attachment 93124 [details] platformpluginV22 Thanks for Alexis & Hui's help :)
Comment on attachment 93124 [details] platformpluginV22 View in context: https://bugs.webkit.org/attachment.cgi?id=93124&action=review Almost there :D, just very unhappy with the QToolButton. > Source/WebKit/qt/symbian/platformplugin/HTML5VideoPlugin.cpp:38 > + m_videoWidget = new HTML5VideoWidget(player->duration() / 1000); Please call setDuration and remove it from the constructor. > Source/WebKit/qt/symbian/platformplugin/HTML5VideoWidget.cpp:23 > +HTML5VideoWidget::HTML5VideoWidget(int duration, QWidget *parent) why the duration as parameter? You can call setDuration afterwards.
> Almost there :D, just very unhappy with the QToolButton. Replaced QToolButton with QPushButton on Symbian 3 as you suggested. It works fine. Maybe we should just use QPushButton since we are not using Qt::Tool flag or QToolBar. > why the duration as parameter? You can call setDuration afterwards. The method setDuration was added later as part of a bug fix. I agree that duration can be removed from the constructor after the introduction of this method.
Created attachment 93170 [details] platformpluginV23 Updated with Alexis's comments :)
Comment on attachment 93170 [details] platformpluginV23 View in context: https://bugs.webkit.org/attachment.cgi?id=93170&action=review > Source/WebKit/qt/ChangeLog:90 > + * symbian/platformplugin/qss/OverlayWidget.qss: Added. I trust you for regenerating the changelog at each round :D. > Source/WebKit/qt/symbian/platformplugin/HTML5VideoPlugin.cpp:30 > + m_savedOrientation = CAknAppUi::EAppUiOrientationUnspecified; could you move that with , m_fullScreen(false) and friends? > Source/WebKit/qt/symbian/platformplugin/HTML5VideoPlugin.cpp:40 > + return; Blank line after return. > Source/WebKit/qt/symbian/platformplugin/HTML5VideoWidget.cpp:26 > + m_overlayWidget = new OverlayWidget(this); Can you put that alongside with : QVideoWidget(parent) > Source/WebKit/qt/symbian/platformplugin/HTML5VideoWidget.cpp:37 > + delete m_overlayWidget; Don't need. m_overlayWidget = new OverlayWidget(this); it's parented :D. > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:96 > +{ Please remove, it's empty. > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.h:33 > + ~OverlayWidget(); remove. > Source/WebKit/qt/symbian/platformplugin/PlayerButton.cpp:28 > + , m_buttonImage(0) remove, seems useless. > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:24 > +#include <QIcon> remove. > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:39 > + QIcon* m_buttonImage; What it is used for?
Thanks for the carefully reviewing, Alexis :) (In reply to comment #51) > (From update of attachment 93170 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93170&action=review > > > Source/WebKit/qt/ChangeLog:90 > > + * symbian/platformplugin/qss/OverlayWidget.qss: Added. > > I trust you for regenerating the changelog at each round :D. What is the problem for this qss? Yes, I regenerate the changelog every time. > > > Source/WebKit/qt/symbian/platformplugin/HTML5VideoPlugin.cpp:30 > > + m_savedOrientation = CAknAppUi::EAppUiOrientationUnspecified; > > could you move that with , m_fullScreen(false) and friends? Sure, will change it > > > Source/WebKit/qt/symbian/platformplugin/HTML5VideoPlugin.cpp:40 > > + return; > > Blank line after return. > Will change it. > > Source/WebKit/qt/symbian/platformplugin/HTML5VideoWidget.cpp:26 > > + m_overlayWidget = new OverlayWidget(this); > > Can you put that alongside with : QVideoWidget(parent) > Will change it. > > Source/WebKit/qt/symbian/platformplugin/HTML5VideoWidget.cpp:37 > > + delete m_overlayWidget; > > Don't need. m_overlayWidget = new OverlayWidget(this); it's parented :D. > Will remove it. > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:96 > > +{ > > Please remove, it's empty. > OK. > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.h:33 > > + ~OverlayWidget(); > > remove. > OK. > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.cpp:28 > > + , m_buttonImage(0) > > remove, seems useless. > OK > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:24 > > +#include <QIcon> > > remove. > OK. > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:39 > > + QIcon* m_buttonImage; > > What it is used for? I think it is useless. Hui, any comments?
> > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:39 > > > + QIcon* m_buttonImage; > > > > What it is used for? > I think it is useless. Hui, any comments? It is useless. My oversight :-) Please remove.
(In reply to comment #52) > Thanks for the carefully reviewing, Alexis :) > > (In reply to comment #51) > > (From update of attachment 93170 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=93170&action=review > > > > > Source/WebKit/qt/ChangeLog:90 > > > + * symbian/platformplugin/qss/OverlayWidget.qss: Added. > > > > I trust you for regenerating the changelog at each round :D. > What is the problem for this qss? Yes, I regenerate the changelog every time. No it's no problem it was just to make sure because I didn't check every entries :D. > > > > > Source/WebKit/qt/symbian/platformplugin/HTML5VideoPlugin.cpp:30 > > > + m_savedOrientation = CAknAppUi::EAppUiOrientationUnspecified; > > > > could you move that with , m_fullScreen(false) and friends? > Sure, will change it > > > > > Source/WebKit/qt/symbian/platformplugin/HTML5VideoPlugin.cpp:40 > > > + return; > > > > Blank line after return. > > > Will change it. > > > Source/WebKit/qt/symbian/platformplugin/HTML5VideoWidget.cpp:26 > > > + m_overlayWidget = new OverlayWidget(this); > > > > Can you put that alongside with : QVideoWidget(parent) > > > Will change it. > > > Source/WebKit/qt/symbian/platformplugin/HTML5VideoWidget.cpp:37 > > > + delete m_overlayWidget; > > > > Don't need. m_overlayWidget = new OverlayWidget(this); it's parented :D. > > > Will remove it. > > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.cpp:96 > > > +{ > > > > Please remove, it's empty. > > > OK. > > > Source/WebKit/qt/symbian/platformplugin/OverlayWidget.h:33 > > > + ~OverlayWidget(); > > > > remove. > > > OK. > > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.cpp:28 > > > + , m_buttonImage(0) > > > > remove, seems useless. > > > OK > > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:24 > > > +#include <QIcon> > > > > remove. > > > OK. > > > Source/WebKit/qt/symbian/platformplugin/PlayerButton.h:39 > > > + QIcon* m_buttonImage; > > > > What it is used for? > I think it is useless. Hui, any comments?
Created attachment 93310 [details] platformpluginV24
Comment on attachment 93310 [details] platformpluginV24 It seems good. Now you have to find a reviewer :D.
Thanks for your review, Alexis :D
Comment on attachment 93310 [details] platformpluginV24 Looks good to me. Thanks for helping with the review Alexis. r+.
Comment on attachment 93310 [details] platformpluginV24 Clearing flags on attachment: 93310 Committed r87044: <http://trac.webkit.org/changeset/87044>
Revision r87044 cherry-picked into qtwebkit-2.2 with commit c39655e <http://gitorious.org/webkit/qtwebkit/commit/c39655e>