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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Feb 19 03:46:21 PST 2010


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.

More information about the webkit-unassigned mailing list