[webkit-reviews] review denied: [Bug 51249] [Qt] Extend the Platform Plugin to support full screen video handler : [Attachment 77238] first draft

[bugzilla-daemon at webkit.org]

Ariya Hidayat <ariya.hidayat at gmail.com> has denied yi shen
<yi.4.shen at nokia.com>'s request for review:
Bug 51249: [Qt] Extend the Platform Plugin to support full screen video handler
https://bugs.webkit.org/show_bug.cgi?id=51249

Attachment 77238: first draft
https://bugs.webkit.org/attachment.cgi?id=77238&action=review

------- Additional Comments from Ariya Hidayat <ariya.hidayat at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77238&action=review

r- for some minor issues.

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:111
> -    m_mediaPlayer->bind(m_videoItem);
> +    m_mediaPlayer->setVideoOutput(m_videoItem);

Any explanation why it is changed? It's not so obvious looking at the patch.

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:592
> +    m_mediaPlayer->setVideoOutput(static_cast<QGraphicsVideoItem*>(0));

What's with static cast a null pointer?

> WebKit/qt/Api/qwebkitplatformplugin.h:30
> +#if defined ENABLE_VIDEO && ENABLE_VIDEO

This is "semi public", it can't contain compiler defines from WebKit.

> WebKit/qt/Api/qwebkitplatformplugin.h:121
> +class QFullscreenVideoHandler : public QObject {

This is a new API, so it may require coordinated discussion with the Qt folks.

Also, I guess the convention is to use QWeb- prefix.

> WebKit/qt/Api/qwebkitplatformplugin.h:129
> +    void closeFullscreen();

Does not follow the usual past-tense convention of Qt signal name.

> WebKit/qt/Api/qwebkitplatformplugin.h:148
> +	   FullscreenVideoPlayer,

Enum can't be dependent of compiler define, otherwise it may yield in same
value having different meaning.

> WebKit/qt/WebCoreSupport/FullscreenVideoQt.cpp:2
> + * This file is part of the popup menu implementation for <select> elements
in WebCore.

Probably not needed.

> WebKit/qt/WebCoreSupport/QtFallbackFullscreenVideoHandler.cpp:3
> + * Copyright (C) 2010 Girish Ramakrishnan <girish at forwardbias.in>
> + * Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)

Is this a new file or inherited from old one?

> WebKit/qt/WebCoreSupport/QtFallbackFullscreenVideoHandler.h:2
> + * Copyright (C) 2009 Nokia Corporation and/or its subsidiary(-ies)

Was this worked on really back in 2009?

> WebKit/qt/examples/platformplugin/WebPlugin.h:108
> +    void spaceBtnPressed();

Don't shorten Button to Btn.

> WebKit/qt/examples/platformplugin/platformplugin.pro:12
> +    DEFINES += ENABLE_VIDEO=1

Shouldn't this block placed later on? Using this logic, there is no way to
explicitly disable video support if the system has Qt Mobility Multimedia.

> WebKit/qt/examples/platformplugin/qwebkitplatformplugin.h:95
> +class QWebHapticFeedbackPlayer: public QObject
>  {
> +    Q_OBJECT
>  public:
> +    QWebHapticFeedbackPlayer() {}
> +    virtual ~QWebHapticFeedbackPlayer() {}
> +

Why is this change needed here?