[webkit-reviews] review denied: [Bug 56826] [Qt] Implement fullscreen playback for the GStreamer backend. : [Attachment 87721] V5 with Changelog fixed.

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Alexis Menard
<alexis.menard at openbossa.org>'s request for review:
Bug 56826: [Qt] Implement fullscreen playback for the GStreamer backend.
https://bugs.webkit.org/show_bug.cgi?id=56826

Attachment 87721: V5 with Changelog fixed.
https://bugs.webkit.org/attachment.cgi?id=87721&action=review

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

Looks mostly good, but let's do one more iteration. :)

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:25
>  #include "PlatformVideoWindow.h"
>  
> +#include "HTMLVideoElement.h"
>  #include "PlatformVideoWindowPrivate.h"

Includes should be in alphabetic order.

> Source/WebCore/platform/graphics/gstreamer/PlatformVideoWindowQt.cpp:53
> +void FullScreenVideoWindow::closeEvent(QCloseEvent* event)

No need for the 'event' name on the parameter.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:47
> +

Unnecessary empty line.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:69
> +    m_fullScreenWidget = reinterpret_cast<FullScreenVideoWindow*
>(gstreamerGWorld->platformVideoWindow()->window());

Extra space after *.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:75
> +    m_fullScreenWidget->setVideoElement(m_videoElement);
> +
> +    connect(m_fullScreenWidget, SIGNAL(closed()), this,
SLOT(windowClosed()));
> +
> +    m_fullScreenWidget->showFullScreen();

No need for the blank lines.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:90
> +    m_fullScreenWidget->setVideoElement(0);
> +
> +    m_fullScreenWidget->close();

Ditto.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:103
> +

Ditto.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:175
> +

Ditto.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.cpp:202
> +

Ditto.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:23
> +#include "Platform.h"

#include <wtf/Platform.h>

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:42
> +#if defined(WTF_USE_GSTREAMER) && WTF_USE_GSTREAMER

#if USE(GSTREAMER)

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:49
> +    ~GStreamerFullScreenVideoHandler() {};

Coding style, need a space between { and }. And remove the semicolon at end of
line.

> Source/WebKit/qt/WebCoreSupport/FullScreenVideoQt.h:64
> +#if defined(ENABLE_QT_MULTIMEDIA) && ENABLE_QT_MULTIMEDIA

#if ENABLE(QT_MULTIMEDIA)