[Webkit-unassigned] [Bug 31586] [GTK] set playbin mute property depending on volume value
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 7 05:46:31 PST 2010
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.
More information about the webkit-unassigned
mailing list