[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 #49060|review?                     |review-
               Flag|                            |




--- Comment #30 from Tor Arne Vestbø <vestbo at webkit.org>  2010-02-19 03:46:20 PST ---
(From update of attachment 49060)
LGTM, a few nitpicks below. I also had to change QtMedia to QtMultimedia a few
places to make it compile against latest qt-multimedia-team's master, so r- for
now, but should be able to land this soon.


> +!contains(DEFINES, ENABLE_VIDEO=1):!contains(DEFINES, ENABLE_VIDEO=0) {

You can use the short-form !contains(DEFINES, ENABLE_VIDEO=.)


> +bool MediaPlayerPrivate::hasVideo() const
> +{
> +    // return m_mediaPlayer->isVideoAvailable();
> +    return true;
> +}

Is this not implemented yet in QtMultimedia?

> +    QLatin1String blKey("bytes-loaded");

Coding-style: bytesLoadedKey

> +    p.painter->setPen(mediaElement->muted() ? Qt::darkGray : Qt::lightGray);

Minor visual style nitpick: Is this to contrast the dark red? The gray outline
does not match the other controls, such as the play buttons etc. I'd say skip
this for now, and we can revisit the styling of the whole default control style
if neccecary.

> +bool RenderThemeQt::paintMediaVolumeSliderTrack(RenderObject *o, const RenderObject::PaintInfo &paintInfo, const IntRect &r)
> +    // Draw the outer rectangle
> +    p.painter->setPen(Qt::lightGray);

Same with this, I'd rather we keep the no-pen look for now :)

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