[Webkit-unassigned] [Bug 34631] [Qt] Switching from Phonon to QtMultimedia Backend for Qt 4.7

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34631


Tor Arne Vestbø <vestbo at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #48548|review?                     |review-
               Flag|                            |




--- Comment #24 from Tor Arne Vestbø <vestbo at webkit.org>  2010-02-17 00:54:55 PST ---
(From update of attachment 48548)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.