[webkit-reviews] review denied: [Bug 34631] [Qt] Switching from Phonon to QtMultimedia Backend for Qt 4.7 : [Attachment 48548] Updated Patch - Addressed issues from review

[bugzilla-daemon at webkit.org]

Tor Arne Vestbø <vestbo at webkit.org> has denied Nick Young
<nicholas.young at nokia.com>'s request for review:
Bug 34631: [Qt] Switching from Phonon to QtMultimedia Backend for Qt 4.7
https://bugs.webkit.org/show_bug.cgi?id=34631

Attachment 48548: Updated Patch - Addressed issues from review
https://bugs.webkit.org/attachment.cgi?id=48548&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
A few initial comments:

> Index: WebCore/WebCore.pro
> ===================================================================
> --- WebCore/WebCore.pro	(revision 54641)
> +++ WebCore/WebCore.pro	(working copy)
> @@ -131,12 +131,20 @@ mameo5|symbian|embedded {
>  
>  include($$PWD/../JavaScriptCore/JavaScriptCore.pri)
>  
> -# Disable HTML5 media compilation if phonon is unavailable
> -!contains(DEFINES, ENABLE_VIDEO=1) {
> -    !contains(QT_CONFIG, phonon) {
> -	   DEFINES -= ENABLE_VIDEO=1
> -	   DEFINES += ENABLE_VIDEO=0
> -    }
> +
> +# HTML5 Media Support
> +# We require phonon for versions of Qt < 4.7
> +# We require QtMultimedia for versions of Qt >= 4.7
> +DEFINES -= ENABLE_VIDEO=1
> +DEFINES += ENABLE_VIDEO=0

Please keep the !contains(DEFINES, ENABLE_VIDEO=1) logic, as it allow you to
pass --[no-]video to build-webkit to override the detection

>  audio::-webkit-media-controls-seek-back-button,
video::-webkit-media-controls-seek-back-button {
> -    /* Since MediaControlElements are always created with a renderer we have
to hide
> -	  the controls we don't use, so they don't mess up activation and event
handling */
> -    left: 0px;
> -    top: 0px;
> -    width: 0px;
> -    height: 0px;
> -
>      display: none;
>  }
>  
>  audio::-webkit-media-controls-seek-forward-button,
video::-webkit-media-controls-seek-forward-button {
> -    /* Since MediaControlElements are always created with a renderer we have
to hide
> -	  the controls we don't use, so they don't mess up activation and event
handling */
> -    left: 0px;
> -    top: 0px;
> -    width: 0px;
> -    height: 0px;
> -
>      display: none;
>  }
>  
>  audio::-webkit-media-controls-fullscreen-button,
video::-webkit-media-controls-fullscreen-button {
> -    /* Since MediaControlElements are always created with a renderer we have
to hide
> -	  the controls we don't use, so they don't mess up activation and event
handling */
> -    left: 0px;
> -    top: 0px;
> -    width: 0px;
> -    height: 0px;
> -
>      display: none;
>  }

The sizes-hack was used due to display: none not working. Have you verified
that this should work now? I remember seeing clicks in the areas of the
seek-button being picked up and causing the video to pause, even though the
element was set to display: none.

> Index: WebCore/platform/graphics/MediaPlayer.cpp
> ===================================================================
> --- WebCore/platform/graphics/MediaPlayer.cpp (revision 54641)
> +++ WebCore/platform/graphics/MediaPlayer.cpp (working copy)
> -#include "MediaPlayerPrivatePhonon.h"
> +    #if QT_VERSION < 0x040700
> +    #include "MediaPlayerPrivatePhonon.h"
> +    #else
> +    #include "MediaPlayerPrivateQt.h"
> +    #endif
>  #elif PLATFORM(CHROMIUM)

No indentation please.

> Index: WebCore/platform/qt/RenderThemeQt.cpp
> ===================================================================
> --- WebCore/platform/qt/RenderThemeQt.cpp	(revision 54641)
> +++ WebCore/platform/qt/RenderThemeQt.cpp	(working copy)


> +double RenderThemeQt::mediaControlsBaselineOpacity() const
> +{
> +    return 0.4;
> +}
> +
>  void RenderThemeQt::paintMediaBackground(QPainter* painter, const IntRect&
r) const
>  {
>      painter->setPen(Qt::NoPen);
> -    static QColor transparentBlack(0, 0, 0, 100);
> +    static const QColor shadow =
QApplication::palette().brush(QPalette::Active, QPalette::Shadow).color();
> +    static QColor transparentBlack(shadow.red(), shadow.green(),
shadow.blue(), mediaControlsBaselineOpacity() * 255);

This leaves me with a transparent light gray overlay instead of black
transparent one, probably due to the shadow color on mac. Dunno if this was
intentional (you didn't like the black i chose? :)

Anyways, the variable transparentBlack should probably be renamed
transparentShadow to match the logical change.

Also, we don't normally use const for variables like this.

Will continue review when new updated patch is up.