[webkit-reviews] review denied: [Bug 58436] [Qt] Upstream Meego/Maemo platform plugin : [Attachment 95343] patch

[bugzilla-daemon at webkit.org]

Alexis Menard <alexis.menard at openbossa.org> has denied Mahesh Kulkarni
<mahesh.kulkarni at nokia.com>'s request for review:
Bug 58436: [Qt] Upstream Meego/Maemo platform plugin
https://bugs.webkit.org/show_bug.cgi?id=58436

Attachment 95343: patch
https://bugs.webkit.org/attachment.cgi?id=95343&action=review

------- Additional Comments from Alexis Menard <alexis.menard at openbossa.org>
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.