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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 29 11:29:11 PST 2009


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


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #45274|review?                     |review-, commit-queue-
               Flag|                            |




--- Comment #21 from Gustavo Noronha (kov) <gns at gnome.org>  2009-12-29 11:29:10 PST ---
(From update of attachment 45274)
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.

@@ 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?

 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).

 79     virtual bool supportsMuting() { return false; }
 80     virtual void setMuted(bool) = 0;

supportsMuting should be have the const qualifier, like hasClosedCaptions.

 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 believe it would be a good idea to grab Eric Carlson and ask him about the
layering, and where you want to insert this.

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


More information about the webkit-unassigned mailing list