[webkit-reviews] review denied: [Bug 36299] Volume control not correctly initialized : [Attachment 75117] proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Nov 30 09:30:58 PST 2010


Martin Robinson <mrobinson at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 36299: Volume control not correctly initialized
https://bugs.webkit.org/show_bug.cgi?id=36299

Attachment 75117: proposed patch
https://bugs.webkit.org/attachment.cgi?id=75117&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=75117&action=review

Sorry, that I missed this! Looks great in general, but I have a few
questions...

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:184
> +    MediaPlayerPrivateGStreamer* mp =
reinterpret_cast<MediaPlayerPrivateGStreamer*>(data);
> +    mp->notifyVolumeToPlayer();

Instead of doing the reinterpret_cast here, why not do it when you set the
callback. What I mean is this:
gboolean
mediaPlayerPrivateVolumeChangeTimeoutCallback(MediaPlayerPrivateGStreamer*
player)
...
m_volumeTimerHandler = g_timeout_add(0,
reinterpret_cast<GSourceFunc>(mediaPlayerPrivateVolumeChangeTimeoutCallback),
this);

If you do it here, it only needs to be a static_cast too.

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:201
> +gboolean mediaPlayerPrivateMuteChangeTimeoutCallback(gpointer data)
> +{
> +    // This is the callback of the timeout source created in ::muteChanged.
> +    MediaPlayerPrivateGStreamer* mp =
reinterpret_cast<MediaPlayerPrivateGStreamer*>(data);
> +    mp->notifyMuteToPlayer();
> +    return FALSE;
> +}

Same comment here.

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:357
> +    if (m_muteTimerHandler)
> +	   g_source_remove(m_muteTimerHandler);
> +    m_muteTimerHandler = 0;
> +
> +    if (m_volumeTimerHandler)
> +	   g_source_remove(m_volumeTimerHandler);
> +    m_volumeTimerHandler = 0;

Is there some chance that these id's may be re-used after the callback fires?
Maybe you should set them to 0 in notifyMuteToPlayer and notifyVolumeToPlayer.
If they are re-used you'll be removing some unknown GSource.

> WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:91
> -	       void
volumeChangedTimerFired(Timer<MediaPlayerPrivateGStreamer>*);
> +	       void notifyVolumeToPlayer();
>  
>	       bool supportsMuting() const;
>	       void setMuted(bool);
>	       void muteChanged();
> -	       void muteChangedTimerFired(Timer<MediaPlayerPrivateGStreamer>*);

> +	       void notifyMuteToPlayer();

The names of the methods is just slightly funky. Maybe "notifyPlayerOfMute" and
"notifyPlayerOfVolumeChange"?


More information about the webkit-reviews mailing list