Bug 58435 - [Qt] Upstream Symbian platform plugin.
Summary: [Qt] Upstream Symbian platform plugin.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P1 Major
Assignee: Yi Shen
URL:
Keywords: Qt, QtTriaged
Depends on: 59271
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-13 06:06 PDT by Alexis Menard (darktears)
Modified: 2011-05-23 07:37 PDT (History)
10 users (show)

See Also:


Attachments
platformpluginV1 (21.49 KB, patch)
2011-04-20 10:01 PDT, Yi Shen
menard: review-
Details | Formatted Diff | Diff
platformpluginV1.1 (20.97 KB, patch)
2011-04-20 11:11 PDT, Yi Shen
menard: review-
Details | Formatted Diff | Diff
platformpluginV1.2 (23.57 KB, patch)
2011-04-21 07:14 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
platformpluginV1.3 (23.54 KB, patch)
2011-04-22 07:14 PDT, Yi Shen
no flags Details | Formatted Diff | Diff
platformpluginV2 (60.85 KB, patch)
2011-04-27 12:35 PDT, Yi Shen
menard: review-
Details | Formatted Diff | Diff
platformpluginV21 (61.24 KB, patch)
2011-04-28 12:56 PDT, Yi Shen
menard: review-
Details | Formatted Diff | Diff
platformpluginV22 (61.29 KB, patch)
2011-05-11 08:42 PDT, Yi Shen
menard: review-
Details | Formatted Diff | Diff
platformpluginV23 (61.13 KB, patch)
2011-05-11 13:30 PDT, Yi Shen
menard: review-
Details | Formatted Diff | Diff
platformpluginV24 (60.93 KB, patch)
2011-05-12 11:17 PDT, 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 Alexis Menard (darktears) 2011-04-13 06:06:47 PDT
The plugin needs to be upstreamed for a better maintainability.
Comment 1 Yi Shen 2011-04-20 10:01:42 PDT
Created attachment 90355 [details]
platformpluginV1
Comment 2 WebKit Review Bot 2011-04-20 10:02:54 PDT
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 3 Yi Shen 2011-04-20 10:06:35 PDT
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 4 Alexis Menard (darktears) 2011-04-20 10:07:31 PDT
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.
Comment 5 Yi Shen 2011-04-20 10:20:19 PDT
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 6 Yael 2011-04-20 10:33:40 PDT
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 7 Alexis Menard (darktears) 2011-04-20 10:37:03 PDT
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.
Comment 8 Yi Shen 2011-04-20 11:11:14 PDT
Created attachment 90366 [details]
platformpluginV1.1
Comment 9 WebKit Review Bot 2011-04-20 11:14:38 PDT
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.
Comment 10 Yi Shen 2011-04-20 11:19:24 PDT
(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.
Comment 11 Alexis Menard (darktears) 2011-04-20 11:25:04 PDT
(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.
Comment 12 Yi Shen 2011-04-20 11:33:23 PDT
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.
Comment 13 Yi Shen 2011-04-20 14:20:02 PDT
I will add some check style exemptions for my patch.
Comment 14 Alexis Menard (darktears) 2011-04-20 14:31:36 PDT
(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.
Comment 15 Yi Shen 2011-04-20 18:23:46 PDT
(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!
Comment 16 Yi Shen 2011-04-21 06:36:46 PDT
I created a separated bug to fix the style issue for Api/qwebkitplatformplugin.h in https://bugs.webkit.org/show_bug.cgi?id=59097
Comment 17 Yi Shen 2011-04-21 07:14:42 PDT
Created attachment 90527 [details]
platformpluginV1.2

fix style issues
Comment 18 Kenneth Rohde Christiansen 2011-04-21 12:49:22 PDT
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
Comment 19 Yi Shen 2011-04-21 13:00:14 PDT
(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
Comment 20 Laszlo Gombos 2011-04-21 17:04:24 PDT
(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.
Comment 21 Yi Shen 2011-04-22 07:14:59 PDT
Created attachment 90705 [details]
platformpluginV1.3

updated with Kenneth's comments
Comment 22 WebKit Commit Bot 2011-04-22 07:43:23 PDT
Comment on attachment 90705 [details]
platformpluginV1.3

Clearing flags on attachment: 90705

Committed r84627: <http://trac.webkit.org/changeset/84627>
Comment 23 WebKit Commit Bot 2011-04-22 07:43:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Yi Shen 2011-04-22 08:12:47 PDT
Will provide the patchV2 with the video part.
Comment 25 WebKit Review Bot 2011-04-22 09:48:58 PDT
http://trac.webkit.org/changeset/84627 might have broken GTK Linux 32-bit Debug
Comment 26 Laszlo Gombos 2011-04-22 10:04:12 PDT
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).
Comment 27 Csaba Osztrogonác 2011-04-22 20:59:47 PDT
Rolled out by http://trac.webkit.org/changeset/84734

But unfortunately the public Symbian bot is still sick. It seems it needs a kick.
Comment 28 Yi Shen 2011-04-23 06:03:21 PDT
(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.
Comment 29 Yi Shen 2011-04-27 10:55:56 PDT
Committed r85062: <http://trac.webkit.org/changeset/85062>
Comment 30 Yi Shen 2011-04-27 10:57:35 PDT
(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.
Comment 31 Yi Shen 2011-04-27 12:29:01 PDT
(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 :)
Comment 32 Yi Shen 2011-04-27 12:35:41 PDT
Created attachment 91324 [details]
platformpluginV2

Platformplugin part 2 which includes a html5 video player.
Comment 33 Alexis Menard (darktears) 2011-04-27 18:59:31 PDT
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?
Comment 34 Alexis Menard (darktears) 2011-04-27 19:00:18 PDT
Though good job Yi to work on upstreaming that.
Comment 35 Yi Shen 2011-04-28 10:24:31 PDT
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?
Comment 36 Yi Shen 2011-04-28 12:56:48 PDT
Created attachment 91542 [details]
platformpluginV21
Comment 37 Yi Shen 2011-04-28 12:59:08 PDT
> > 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.
Comment 38 Alexis Menard (darktears) 2011-05-03 15:01:39 PDT
(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 39 Alexis Menard (darktears) 2011-05-03 15:17:54 PDT
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.
Comment 40 Hui Huang 2011-05-03 21:25:48 PDT
> 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?
Comment 41 Alexis Menard (darktears) 2011-05-04 04:50:51 PDT
(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...
Comment 42 Alexis Menard (darktears) 2011-05-04 06:50:14 PDT
(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.
Comment 43 Yi Shen 2011-05-04 12:13:02 PDT
(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 :)
Comment 44 Yi Shen 2011-05-04 12:24:26 PDT
(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.
Comment 45 Hui Huang 2011-05-04 13:49:13 PDT
> 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)
{
Comment 46 Hui Huang 2011-05-09 13:50:54 PDT
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)
> {
Comment 47 Yi Shen 2011-05-11 08:42:48 PDT
Created attachment 93124 [details]
platformpluginV22

Thanks for Alexis & Hui's help :)
Comment 48 Alexis Menard (darktears) 2011-05-11 09:05:58 PDT
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.
Comment 49 Hui Huang 2011-05-11 11:09:42 PDT
> 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.
Comment 50 Yi Shen 2011-05-11 13:30:57 PDT
Created attachment 93170 [details]
platformpluginV23

Updated with Alexis's comments :)
Comment 51 Alexis Menard (darktears) 2011-05-11 13:44:00 PDT
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?
Comment 52 Yi Shen 2011-05-11 14:14:00 PDT
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?
Comment 53 Hui Huang 2011-05-11 14:43:49 PDT
> > > 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.
Comment 54 Alexis Menard (darktears) 2011-05-11 15:22:31 PDT
(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?
Comment 55 Yi Shen 2011-05-12 11:17:20 PDT
Created attachment 93310 [details]
platformpluginV24
Comment 56 Alexis Menard (darktears) 2011-05-12 11:22:11 PDT
Comment on attachment 93310 [details]
platformpluginV24

It seems good. Now you have to find a reviewer :D.
Comment 57 Yi Shen 2011-05-12 12:42:54 PDT
Thanks for your review, Alexis :D
Comment 58 Laszlo Gombos 2011-05-22 14:56:39 PDT
Comment on attachment 93310 [details]
platformpluginV24

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

r+.
Comment 59 WebKit Commit Bot 2011-05-22 15:18:39 PDT
Comment on attachment 93310 [details]
platformpluginV24

Clearing flags on attachment: 93310

Committed r87044: <http://trac.webkit.org/changeset/87044>
Comment 60 WebKit Commit Bot 2011-05-22 15:18:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 61 Ademar Reis 2011-05-23 07:37:39 PDT
Revision r87044 cherry-picked into qtwebkit-2.2 with commit c39655e <http://gitorious.org/webkit/qtwebkit/commit/c39655e>