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

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> 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 45274: new API in MediaPlayer for mute control and mute control in
MediaPlayerPrivateGStreamer
https://bugs.webkit.org/attachment.cgi?id=45274&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
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.