Bug 58435 – [Qt] Upstream Symbian platform plugin.

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

The plugin needs to be upstreamed for a better maintainability.

[Yi Shen]

Created attachment 90355 [details]
platformpluginV1

[WebKit Review Bot]

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.

[Yi Shen]

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.

[Alexis Menard (darktears)]

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.

[Yi Shen]

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?

[Yael]

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.

[Alexis Menard (darktears)]

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.

[Yi Shen]

Created attachment 90366 [details]
platformpluginV1.1

[WebKit Review Bot]

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.

[Yi Shen]

(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.

[Alexis Menard (darktears)]

(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.

[Yi Shen]

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.

[Yi Shen]

I will add some check style exemptions for my patch.

[Alexis Menard (darktears)]

(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.

[Yi Shen]

(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!

[Yi Shen]

I created a separated bug to fix the style issue for Api/qwebkitplatformplugin.h in https://bugs.webkit.org/show_bug.cgi?id=59097

[Yi Shen]

Created attachment 90527 [details]
platformpluginV1.2

fix style issues

[Kenneth Rohde Christiansen]

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

[Yi Shen]

(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

[Laszlo Gombos]

(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.

[Yi Shen]

Created attachment 90705 [details]
platformpluginV1.3

updated with Kenneth's comments

[WebKit Commit Bot]

Comment on attachment 90705 [details]
platformpluginV1.3

Clearing flags on attachment: 90705

Committed r84627: <http://trac.webkit.org/changeset/84627>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yi Shen]

Will provide the patchV2 with the video part.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84627 might have broken GTK Linux 32-bit Debug

[Laszlo Gombos]

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).

[Csaba Osztrogonác]

Rolled out by http://trac.webkit.org/changeset/84734

But unfortunately the public Symbian bot is still sick. It seems it needs a kick.

[Yi Shen]

(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.

[Yi Shen]

Committed r85062: <http://trac.webkit.org/changeset/85062>

[Yi Shen]

(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.

[Yi Shen]

(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 :)

[Yi Shen]

Created attachment 91324 [details]
platformpluginV2

Platformplugin part 2 which includes a html5 video player.

[Alexis Menard (darktears)]

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?

[Alexis Menard (darktears)]

Though good job Yi to work on upstreaming that.

[Yi Shen]

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?

[Yi Shen]

Created attachment 91542 [details]
platformpluginV21

[Yi Shen]

> > 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.

[Alexis Menard (darktears)]

(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.

[Alexis Menard (darktears)]

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.

[Hui Huang]

> 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?

[Alexis Menard (darktears)]

(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...

[Alexis Menard (darktears)]

(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.

[Yi Shen]

(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 :)

[Yi Shen]

(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.

[Hui Huang]

> 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)
{

[Hui Huang]

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)
> {

[Yi Shen]

Created attachment 93124 [details]
platformpluginV22

Thanks for Alexis & Hui's help :)

[Alexis Menard (darktears)]

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.

[Hui Huang]

> 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.

[Yi Shen]

Created attachment 93170 [details]
platformpluginV23

Updated with Alexis's comments :)

[Alexis Menard (darktears)]

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?

[Yi Shen]

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?

[Hui Huang]

> > > 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.

[Alexis Menard (darktears)]

(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?

[Yi Shen]

Created attachment 93310 [details]
platformpluginV24

[Alexis Menard (darktears)]

Comment on attachment 93310 [details]
platformpluginV24

It seems good. Now you have to find a reviewer :D.

[Yi Shen]

Thanks for your review, Alexis :D

[Laszlo Gombos]

Comment on attachment 93310 [details]
platformpluginV24

Looks good to me. Thanks for helping with the review Alexis. 

r+.

[WebKit Commit Bot]

Comment on attachment 93310 [details]
platformpluginV24

Clearing flags on attachment: 93310

Committed r87044: <http://trac.webkit.org/changeset/87044>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r87044 cherry-picked into qtwebkit-2.2 with commit c39655e <http://gitorious.org/webkit/qtwebkit/commit/c39655e>