[webkit-reviews] review denied: [Bug 51249] [Qt] Extend the Platform Plugin to support full screen video handler : [Attachment 78286] add change log

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> 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 78286: add change log
https://bugs.webkit.org/attachment.cgi?id=78286&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78286&action=review

In general this looks pretty good to me actually, my nitpicking aside. Good
work Yi :). I however think that at this point we should not have a default
fullscreen playback widget in Qt, unless you have plans to also give it some
controls, etc.

In other words: Given the scope and the future WebKit2 related work, I suggest
that we support fullscreen playback if the platform plugin provides it,
otherwise we let bool ChromeClientQt::supportsFullscreenForNode(const Node*
node) return false.

What do you think?

r- because of the nitpicks :), but I also think we should discuss having a
default or not. Would also like to hear the opinion from the other CC'ed folks
:)

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:602
> +    // FIXME a qtmobility bug, need to reset the size when restore the
videoitem, otherwise the size is 0
> +    nativeSizeChanged(QSize(m_oldNaturalSize));

Can you add a link to the bug report for qtmobility, so that we can remove this
workaround once the bug is fixed upstream?

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.h:111
> +    QMediaPlayer* mediaPlayer() { return m_mediaPlayer; }

This function should be const. (as weird as it is :)

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

Some pre-processors don't like this syntax and would prefer #if
defined(ENABLE_VIDEO) && ENABLE_VIDEO

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

Same here :)

> WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:708
> +    if (platformMedia.type != PlatformMedia::QtMediaPlayerType) {
> +	   videoElement->exitFullscreen();
> +	   return;
> +    }

I doubt the call to exitFullscreen() is going to work. Have you tried it?
Otherwise I suggest to have an ASSERT() there (for debug builds) as well as a
simple "return".

> WebKit/qt/WebCoreSupport/FullscreenVideoQt.cpp:90
> +    Q_ASSERT(m_fullscreenVideoHandler);

Doesn't this ASSERT belong in the constructor? I mean, shouldn't we catch this
(and similar errors other ASSERTs guard) in the constructor once?