[webkit-reviews] review denied: [Bug 54201] [Qt] In trunk with Qt Multimedia the full screen mode doesn't work. : [Attachment 81966] Patch to implement a default fullscreen handler...

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Alexis Menard
<alexis.menard at nokia.com>'s request for review:
Bug 54201: [Qt] In trunk with Qt Multimedia the full screen mode doesn't work.
https://bugs.webkit.org/show_bug.cgi?id=54201

Attachment 81966: Patch to implement a default fullscreen handler...
https://bugs.webkit.org/attachment.cgi?id=81966&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81966&action=review

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoWidget.cpp:36
> +static const int hideMouseCursorDelay = 3000;

gHideMouseCursorDelay

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoWidget.cpp:44
> +    QPalette palette;

Shouldn't we be amending the existing palette() here?

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoWidget.cpp:67
> +    connect(&m_cursorTimer, SIGNAL(timeout()), this, SLOT(hideCursor()));

Multiple calls to show() will cause signal/slot connections to accumulate.
You can pass Qt::UniqueConnection as the 5th argument to avoid this.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoWidget.cpp:108
> +    } else if (ev->key() == Qt::Key_Escape) {
> +	   close();
> +    }

Coding style, no { } for single-statement blocks.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoWidget.h:44
> +    ~FullScreenVideoWidget();

virtual ~FullScreenVideoWidget();

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoWidget.h:55
> +protected slots:

Might as well be private.