[webkit-reviews] review denied: [Bug 31586] [GTK] set playbin mute property depending on volume value : [Attachment 47688] new API in MediaPlayer for mute control

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> 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 47688: new API in MediaPlayer for mute control
https://bugs.webkit.org/attachment.cgi?id=47688&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> +++ b/WebCore/html/HTMLMediaElement.cpp
> @@ -1235,8 +1235,16 @@ void HTMLMediaElement::setMuted(bool muted)
>  {
>      if (m_muted != muted) {
>	   m_muted = muted;
> -	   updateVolume();
> -	   scheduleEvent(eventNames().volumechangeEvent);
> +	   // Avoid recursion when the player reports volume changes.
> +	   if (!processingMediaPlayerCallback()) {
> +	       if (m_player && m_player->supportsMuting()) {
> +		   m_player->setMuted(m_muted);
> +		   if (renderer())
> +		       renderer()->updateFromElement();
> +	       } else
> +		   updateVolume();
> +	       scheduleEvent(eventNames().volumechangeEvent);
> +	   }
>      }
>  }
>  
If a media engine's muted property changes without HTMLMediaElement knowing
about it, we won't post a 'volumechanged' notification because
processingMediaPlayerCallback() will be true. 


> @@ -275,6 +291,7 @@ MediaPlayerPrivate::MediaPlayerPrivate(MediaPlayer*
player)
>      , m_errorOccured(false)
>      , m_volumeIdleId(-1)
>      , m_mediaDuration(0.0)
> +    , m_muteIdleId(-1)
>  {
>      doGstInit();
>  }
> @@ -286,6 +303,11 @@ MediaPlayerPrivate::~MediaPlayerPrivate()
>	   m_volumeIdleId = -1;
>      }
>  
> +    if (m_muteIdleId) {
> +	   g_source_remove(m_muteIdleId);
> +	   m_muteIdleId = -1;
> +    }
> +
Why are m_muteIdleId and m_volumeIdleId set to -1 when the source is invalid,
but "!=0" is the test for validity? Shouldn't you set them to 0 when invalid?

Sorry I missed both of these the last time around!

r- for the missing notification.