Bug 58436 – [Qt] Upstream Meego/Maemo platform plugin

• Component: WebKit Qt
• Status: RESOLVED WONTFIX

[Alexis Menard (darktears)]

The plugin needs to be upstreamed for a better maintainability.

[Mahesh Kulkarni]

Created attachment 95343 [details]
patch

patch to upstream meego platform plugin. Proposing to move symbian plugin as well under Source/WebKit/qt/platformplugin/symbian.

[Alexis Menard (darktears)]

Comment on attachment 95343 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95343&action=review

> Source/WebKit/qt/platformplugin/meego/README:7
> +   git clone git://gitorious.org/maemo-6-ui-framework/libdui.git

Can it just be installing libdui-dev or something like that?

> Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:20
> +#include "html5videoplugin.h"

Why the name of the file doesn't match the name of the class?

> Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:38
> +    if (!m_videoWidget)

I think it's useless.

> Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:64
> +    m_mediaPlayer->disconnect(m_videoWidget);

Would it be better to instanciate one time the video widget. It will require a bit more memory (but I on't think it's that big deal) but will make the switch from fullscreen back and forth smoother after the first time.

> Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:90
> +            m_screenSaver = new QtMobility::QSystemScreenSaver(this);   

Why namespacing QtMobility?

> Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:101
> +        delete m_screenSaver;

Why you don't store the screensaver as a member of the class but alocated once. At each time you pause/play it will allocate the object. Also when you start the playback you want full ressources of the system, so I'm wondering if it's good to make the OS busy by allocating something.

> Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:118
> +QObject* HTML5VideoPlugin::createExtension(Extension extension) const

Why is that used for?

> Source/WebKit/qt/platformplugin/meego/html5videoplugin.h:48
> +    QtMobility::QSystemScreenSaver* m_screenSaver;

Same why the prefix?

> Source/WebKit/qt/platformplugin/meego/meegotouchplatformplugin.cpp:55
> +        return new HTML5FullScreenVideoHandler();

why you didn't follow the pattern of the Notifications/haptics?

> Source/WebKit/qt/platformplugin/meego/overlaywidget.cpp:88
> +    delete m_controlButton;

You don't need those deletes because they are parented to their parent.

> Source/WebKit/qt/platformplugin/meego/overlaywidget.cpp:95
> +    delete m_toolBar;

That one is not for some reason.

[Mahesh Kulkarni]

Thanks Alexis for quick review. 

(In reply to comment #2)
> (From update of attachment 95343 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95343&action=review
> 
> > Source/WebKit/qt/platformplugin/meego/README:7
> > +   git clone git://gitorious.org/maemo-6-ui-framework/libdui.git
> 
> Can it just be installing libdui-dev or something like that?
> 

Not sure because libdui is not a single component which can be installed. We have to either do something like libdui-* or leave the README content as is, as meego-touch instructions seems to have same details.


> Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:90
> +            m_screenSaver = new QtMobility::QSystemScreenSaver(this);   

> Why namespacing QtMobility?

Because QtSystemInfo is still in QtMobility namespace where as QtMultiMedia is moved to Qt namespace?

> > Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:90
> > +            m_screenSaver = new QtMobility::QSystemScreenSaver(this);   
> 
> Why namespacing QtMobility?
> 
> > Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:101
> > +        delete m_screenSaver;
> 
> Why you don't store the screensaver as a member of the class but alocated once. At each time you pause/play it will allocate the object. Also when you start the playback you want full ressources of the system, so I'm wondering if it's good to make the OS busy by allocating something.
>

Because deleting the object is the only way to re-enable screen saver mode. When video is paused/stopped we have to enable to screen saver again (As per qtmobility doc)
 
> You don't need those deletes because they are parented to their parent.
> 
> > Source/WebKit/qt/platformplugin/meego/overlaywidget.cpp:95
> > +    delete m_toolBar;
> 
> That one is not for some reason.

Yes, because video widget sets background as transparent so all controls/images appear without a background. This toolbar is not connected to the same parent to make it appear with solid background color. This was the work around suggested by Mobility guys.

[Mahesh Kulkarni]

Created attachment 95359 [details]
patch

second attempt.

[Alexis Menard (darktears)]

(In reply to comment #3)
> Thanks Alexis for quick review. 
> 
> (In reply to comment #2)
> > (From update of attachment 95343 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95343&action=review
> > 
> > > Source/WebKit/qt/platformplugin/meego/README:7
> > > +   git clone git://gitorious.org/maemo-6-ui-framework/libdui.git
> > 
> > Can it just be installing libdui-dev or something like that?
> > 
> 
> Not sure because libdui is not a single component which can be installed. We have to either do something like libdui-* or leave the README content as is, as meego-touch instructions seems to have same details.

ok seems weird that there is no dev packages for that.

> 
> 
> > Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:90
> > +            m_screenSaver = new QtMobility::QSystemScreenSaver(this);   
> 
> > Why namespacing QtMobility?
> 
> Because QtSystemInfo is still in QtMobility namespace where as QtMultiMedia is moved to Qt namespace?

And? I believe you don't need it or you can use "using namespace QtMobility".

> 
> > > Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:90
> > > +            m_screenSaver = new QtMobility::QSystemScreenSaver(this);   
> > 
> > Why namespacing QtMobility?
> > 
> > > Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:101
> > > +        delete m_screenSaver;
> > 
> > Why you don't store the screensaver as a member of the class but alocated once. At each time you pause/play it will allocate the object. Also when you start the playback you want full ressources of the system, so I'm wondering if it's good to make the OS busy by allocating something.
> >
> 
> Because deleting the object is the only way to re-enable screen saver mode. When video is paused/stopped we have to enable to screen saver again (As per qtmobility doc)

You are right. What an ugly and useless API. I'm wondering what is the reasoning behind. There could be an activate()/deactivate() slot or something.

http://doc.qt.nokia.com/qtmobility/qsystemscreensaver.html#details

> 
> > You don't need those deletes because they are parented to their parent.
> > 
> > > Source/WebKit/qt/platformplugin/meego/overlaywidget.cpp:95
> > > +    delete m_toolBar;
> > 
> > That one is not for some reason.
> 
> Yes, because video widget sets background as transparent so all controls/images appear without a background. This toolbar is not connected to the same parent to make it appear with solid background color. This was the work around suggested by Mobility guys.

Ok then it's fine to me.

[Alexis Menard (darktears)]

(In reply to comment #5)
> (In reply to comment #3)
> > Thanks Alexis for quick review. 
> > 
> > (In reply to comment #2)
> > > (From update of attachment 95343 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=95343&action=review
> > > 
> > > > Source/WebKit/qt/platformplugin/meego/README:7
> > > > +   git clone git://gitorious.org/maemo-6-ui-framework/libdui.git
> > > 
> > > Can it just be installing libdui-dev or something like that?
> > > 
> > 
> > Not sure because libdui is not a single component which can be installed. We have to either do something like libdui-* or leave the README content as is, as meego-touch instructions seems to have same details.
> 
> ok seems weird that there is no dev packages for that.
> 
> > 
> > 
> > > Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:90
> > > +            m_screenSaver = new QtMobility::QSystemScreenSaver(this);   
> > 
> > > Why namespacing QtMobility?
> > 
> > Because QtSystemInfo is still in QtMobility namespace where as QtMultiMedia is moved to Qt namespace?
> 
> And? I believe you don't need it or you can use "using namespace QtMobility".
> 
> > 
> > > > Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:90
> > > > +            m_screenSaver = new QtMobility::QSystemScreenSaver(this);   
> > > 
> > > Why namespacing QtMobility?
> > > 
> > > > Source/WebKit/qt/platformplugin/meego/html5videoplugin.cpp:101
> > > > +        delete m_screenSaver;
> > > 
> > > Why you don't store the screensaver as a member of the class but alocated once. At each time you pause/play it will allocate the object. Also when you start the playback you want full ressources of the system, so I'm wondering if it's good to make the OS busy by allocating something.
> > >
> > 
> > Because deleting the object is the only way to re-enable screen saver mode. When video is paused/stopped we have to enable to screen saver again (As per qtmobility doc)
> 
> You are right. What an ugly and useless API. I'm wondering what is the reasoning behind. There could be an activate()/deactivate() slot or something.
> 
> http://doc.qt.nokia.com/qtmobility/qsystemscreensaver.html#details

Ok 1.2 have a better API. Though the doc doesn't say that you need to re-create the object to re-enable the screen saver. It sounds like a bug which should be reported and the plugin code should have a FIXME.

> 
> > 
> > > You don't need those deletes because they are parented to their parent.
> > > 
> > > > Source/WebKit/qt/platformplugin/meego/overlaywidget.cpp:95
> > > > +    delete m_toolBar;
> > > 
> > > That one is not for some reason.
> > 
> > Yes, because video widget sets background as transparent so all controls/images appear without a background. This toolbar is not connected to the same parent to make it appear with solid background color. This was the work around suggested by Mobility guys.
> 
> Ok then it's fine to me.

[Mahesh Kulkarni]

> > You are right. What an ugly and useless API. I'm wondering what is the reasoning behind. There could be an activate()/deactivate() slot or something.
> > 
> > http://doc.qt.nokia.com/qtmobility/qsystemscreensaver.html#details
> 
> Ok 1.2 have a better API. Though the doc doesn't say that you need to re-create the object to re-enable the screen saver. It sounds like a bug which should be reported and the plugin code should have a FIXME.


Sure will add that comment and also update "using namespace QtMobility" in my next patch. Will hunt for a reviewer now :)

[Alexis Menard (darktears)]

Comment on attachment 95359 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95359&action=review

> Source/WebKit/qt/platformplugin/meego/README:10
> +

according to the debian part of the patch just libmeegotouch-dev is good enough. I believe you don't need to clone.

> Source/WebKit/qt/platformplugin/meego/selectdialog.cpp:87
> +            item->setText("   " + m_data.itemText(i));

Why this "    "?

[Simon Hausmann]

Hm, why should we have this upstream if we know that it's not going to be used in the future?

Meegotouch is dead, MeeGo is moving away from it. Future versions of MeeGo aren't going to use this code, and Nokia isn't going to use it in the future neither, right?

[Mahesh Kulkarni]

Created attachment 95577 [details]
patch

Simon, I guess upstreaming for the only meego device coming in future and probably we can get rid of meego-touch dependency can be removed in future if needed? Laszlo any thoughts? 

Anyway uploading a new patch addressing all above comments. Please review.

[Ademar Reis]

(In reply to comment #9)
> Hm, why should we have this upstream if we know that it's not going to be used in the future?
> 
> Meegotouch is dead, MeeGo is moving away from it. Future versions of MeeGo aren't going to use this code, and Nokia isn't going to use it in the future neither, right?

I'm not sure, but we're trying to make 2.2 compatible with all devices running 2.1 so that it's a candidate for an update. So since this plugin is part of 2.1, it would be nice to have it in 2.2.

[Simon Hausmann]

(In reply to comment #11)
> (In reply to comment #9)
> > Hm, why should we have this upstream if we know that it's not going to be used in the future?
> > 
> > Meegotouch is dead, MeeGo is moving away from it. Future versions of MeeGo aren't going to use this code, and Nokia isn't going to use it in the future neither, right?
> 
> I'm not sure, but we're trying to make 2.2 compatible with all devices running 2.1 so that it's a candidate for an update. So since this plugin is part of 2.1, it would be nice to have it in 2.2.

How likely is it that the MeeGo platform we're talking about is going to include 2.2?

We could also consider including it only in the 2.2 branch. But beyond that, who is going to maintain the code and test it on MeeGo with newer releases?

Differently put: I have the feeling that we'd be submitting dead code into trunk.

I might be wrong, so take this with a grain of salt :)

[Ademar Reis]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > Hm, why should we have this upstream if we know that it's not going to be used in the future?
> > > 
> > > Meegotouch is dead, MeeGo is moving away from it. Future versions of MeeGo aren't going to use this code, and Nokia isn't going to use it in the future neither, right?
> > 
> > I'm not sure, but we're trying to make 2.2 compatible with all devices running 2.1 so that it's a candidate for an update. So since this plugin is part of 2.1, it would be nice to have it in 2.2.
> 
> How likely is it that the MeeGo platform we're talking about is going to include 2.2?

The idea is to obsolete 2.1 by releasing 2.2 as an update. I'm not sure how realistic that goal is though...

> 
> We could also consider including it only in the 2.2 branch. But beyond that, who is going to maintain the code and test it on MeeGo with newer releases?
> 
> Differently put: I have the feeling that we'd be submitting dead code into trunk.
> 
> I might be wrong, so take this with a grain of salt :)

If newer versions of meego won't use this code (who can confirm that?), then you're right. It should be included only in 2.2.

[Alexis Menard (darktears)]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #9)
> > > > Hm, why should we have this upstream if we know that it's not going to be used in the future?
> > > > 
> > > > Meegotouch is dead, MeeGo is moving away from it. Future versions of MeeGo aren't going to use this code, and Nokia isn't going to use it in the future neither, right?
> > > 
> > > I'm not sure, but we're trying to make 2.2 compatible with all devices running 2.1 so that it's a candidate for an update. So since this plugin is part of 2.1, it would be nice to have it in 2.2.
> > 
> > How likely is it that the MeeGo platform we're talking about is going to include 2.2?
> 
> The idea is to obsolete 2.1 by releasing 2.2 as an update. I'm not sure how realistic that goal is though...
> 
> > 
> > We could also consider including it only in the 2.2 branch. But beyond that, who is going to maintain the code and test it on MeeGo with newer releases?
> > 
> > Differently put: I have the feeling that we'd be submitting dead code into trunk.
> > 
> > I might be wrong, so take this with a grain of salt :)
> 
> If newer versions of meego won't use this code (who can confirm that?), then you're right. It should be included only in 2.2.

Unless someone tells me that MeegoTouch has a long live and will be part on Meego for a while I don't think we should upstream this code (and also a proper commitment to maintain it). It can stay as a separate package like today. The time was not wasted anyway because the code of the plugin has improved with the review process. Please include all those feedbacks back in the deb files where it comes from.

Thanks for the work.