[Webkit-unassigned] [Bug 31586] [GTK] set playbin mute property depending on volume value

[bugzilla-daemon at webkit.org]

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





--- Comment #22 from Philippe Normand <pnormand at igalia.com>  2010-01-07 05:46:25 PST ---
(In reply to comment #21)
> (From update of attachment 45274 [details])
> WebCore/ChangeLog
> 
>  1 2009-12-20  Philippe Normand  <pnormand at igalia.com>
>  2 
>  3         Reviewed by NOBODY (OOPS!).
>  4 
>  5         [GTK] set playbin mute property depending on volume value
>  6         https://bugs.webkit.org/show_bug.cgi?id=31586
>  7 
>  8         Adapted the other MediaPlayer implementations.,
> 
> You should write only 1 ChangeLog entry for the full change.
> 

Ok sorry, I did 3 commits for that in my branch, that's why there were 3
entries, I will merge them then.

> @@ void HTMLMediaElement::mediaPlayerTimeChanged(MediaPlayer*)
> 14101415 void HTMLMediaElement::mediaPlayerVolumeChanged(MediaPlayer*)
> 14111416 {
> 14121417     beginProcessingMediaPlayerCallback();
>  1418     if (m_player)
>  1419         m_volume = m_player->volume();
> 14131420     updateVolume();
> 14141421     endProcessingMediaPlayerCallback();
> 
> This looks wrong. Why are you changing this behavior?
> 

MediaPlayer received a volume notification from its "private player" instance
so m_volume needs to be updated to reflect the change downstream. Maybe I can
move that to updateVolume(), I don't know... 

>  1424 void HTMLMediaElement::mediaPlayerMuteChanged(MediaPlayer*)
>  1425 {
>  1426     beginProcessingMediaPlayerCallback();
>  1427     if (m_player)
>  1428         m_muted = m_player->muted();
>  1429     if (renderer())
>  1430         renderer()->updateFromElement();
>  1431     endProcessingMediaPlayerCallback();
>  1432 }
> 
> I don't think you should duplicate this code here. Make a setMuted call, and
> make it behave like updateVolume does (checking if it is handling a player
> notification, and treating it specially).
> 

ok :)

>  79     virtual bool supportsMuting() { return false; }
>  80     virtual void setMuted(bool) = 0;
> 
> supportsMuting should be have the const qualifier, like hasClosedCaptions.
> 

ok 

>  457 void MediaPlayerPrivate::volumeChangedCallback()
>  458 {
>  459     double volume;
>  460     g_object_get(m_playBin, "volume", &volume, NULL);
>  461     m_player->volumeChanged(static_cast<float>(volume));
> 
> I think you are mixing unrelated changes in this patch. And this looks a bit
> weird. Shouldn't you just set the volume?
> 

I need to get the new volume value because we received a notify::volume signal.

> I believe it would be a good idea to grab Eric Carlson and ask him about the
> layering, and where you want to insert this.

Once we found an agreement on all the issues above I can send a new patch and
we can discuss with Eric. Ok? :)

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