[webkit-reviews] review denied: [Bug 31586] [GTK] set playbin mute property depending on volume value : [Attachment 43852] 2009-11-25 Philippe Normand <pnormand at igalia.com>

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 31586: [GTK] set playbin mute property depending on volume value
https://bugs.webkit.org/show_bug.cgi?id=31586

Attachment 43852: 2009-11-25  Philippe Normand	<pnormand at igalia.com>
https://bugs.webkit.org/attachment.cgi?id=43852&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> +static gboolean notify_mute_idle_cb(gpointer data)
> +{
> +    MediaPlayerPrivate* mp = reinterpret_cast<MediaPlayerPrivate*>(data);
> +    mp->muteChanged();
> +    return FALSE;
> +}
> +
[...]
> +void MediaPlayerPrivate::muteChanged()
> +{
> +    // Some application (PulseAudio for instance) changed the mute
> +    // playbin property. Propagate the change to the player's client,
> +    // the HTMLMediaElement.
> +    gboolean mute;
> +    g_object_get(G_OBJECT(m_playBin), "mute", &mute, NULL);
> +    m_player->mediaPlayerClient()->setMuted(static_cast<bool>(mute));
> +}
> +

I don't think adding muteChanged to the MediaPlayerPrivate gains us anything.
You can do all the work that you are doing in MediaPlayerPrivate::muteChanged
in the callback function.

>      int width = 0, height = 0;
> -    GstCaps *caps = gst_buffer_get_caps(m_buffer);
> +    GstCaps* caps = gst_buffer_get_caps(m_buffer);
>      GstVideoFormat format;

Commit this separately, please! r=me on this change alone.

> -    g_object_set(G_OBJECT(m_playBin), "uri", url.utf8().data(),
> -	   "volume", static_cast<double>(m_player->volume()), NULL);
> +    g_signal_connect(G_OBJECT(m_playBin), "notify::mute",
G_CALLBACK(mediaPlayerPrivateMuteCallback), this);
> +
> +    g_object_set(G_OBJECT(m_playBin), "uri", url.utf8().data(), NULL);
> +
>  
>      m_videoSink = webkit_video_sink_new();

You're adding a blank line unnecessarily here.