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

[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 49060: Updated Patch..
https://bugs.webkit.org/attachment.cgi?id=49060&action=review

------- Additional Comments from Tor Arne Vestbø <vestbo at webkit.org>
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 :)