[webkit-reviews] review denied: [Bug 58436] [Qt] Upstream Meego/Maemo platform plugin : [Attachment 95343] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 30 07:53:08 PDT 2011
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.
More information about the webkit-reviews
mailing list