[webkit-reviews] review denied: [Bug 58435] [Qt] Upstream Symbian platform plugin. : [Attachment 91324] platformpluginV2

[bugzilla-daemon at webkit.org]

Alexis Menard <alexis.menard at openbossa.org> has denied yi shen
<yi.4.shen at nokia.com>'s request for review:
Bug 58435: [Qt] Upstream Symbian platform plugin.
https://bugs.webkit.org/show_bug.cgi?id=58435

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

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