[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