[Webkit-unassigned] [Bug 36299] Volume control not correctly initialized

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36299


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #75117|review?                     |review-
               Flag|                            |




--- Comment #9 from Martin Robinson <mrobinson at webkit.org>  2010-11-30 09:30:58 PST ---
(From update of attachment 75117)
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"?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.