Modify the volume visuals:

Modify the volume visuals:

According to crbug.com/128644
- the mute icon should be in the tray if volume is 0%
- volume slider point has to be 0.0 when muted

BUG=128644
TEST=manually

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138282

[sadrul, 2012-05-19 02:10:54]

http://codereview.chromium.org/10408036/diff/1/ash/system/audio/tray_volume.cc
File ash/system/audio/tray_volume.cc (right):

http://codereview.chromium.org/10408036/diff/1/ash/system/audio/tray_volume.c...
ash/system/audio/tray_volume.cc:157: delegate->SetAudioMuted(false);
I don't think this is the right place for this change. Volume can be changed by
pressing the corresponding accelerator keys etc., and you will need to duplicate
this same code there too. Instead, the audio should be muted when volume-level
is set to zero in AudioHandler::SetVolumePercent so that we have this logic in
only place.

[Daniel Erat, 2012-05-19 05:38:34]

http://codereview.chromium.org/10408036/diff/1/ash/system/audio/tray_volume.cc
File ash/system/audio/tray_volume.cc (right):

http://codereview.chromium.org/10408036/diff/1/ash/system/audio/tray_volume.c...
ash/system/audio/tray_volume.cc:157: delegate->SetAudioMuted(false);
On 2012/05/19 02:10:54, sadrul wrote:
> I don't think this is the right place for this change. Volume can be changed
by
> pressing the corresponding accelerator keys etc., and you will need to
duplicate
> this same code there too. Instead, the audio should be muted when volume-level
> is set to zero in AudioHandler::SetVolumePercent so that we have this logic in
> only place.

Sorry, I gave conflicting feedback in the review for
https://codereview.chromium.org/10411028/ (saying that a UX decision like this
one should be implemented in the UI rather than in the lower AudioHandler
layer).  My main concern was that enforcing the policy in AudioHandler removes
the flexibility to change the volume level that we'll restore to on unmute
without actually unmuting.

[Jun Mukai, 2012-05-21 17:05:37]

http://codereview.chromium.org/10408036/diff/1/ash/system/audio/tray_volume.cc
File ash/system/audio/tray_volume.cc (right):

http://codereview.chromium.org/10408036/diff/1/ash/system/audio/tray_volume.c...
ash/system/audio/tray_volume.cc:157: delegate->SetAudioMuted(false);
Still not sure which is better, but is there any real scenario where a user
wants to change the volume but not to unmute?  I couldn't imagine such case, so
sadrul's point sounds slightly more valid to me.

On 2012/05/19 05:38:34, Daniel Erat wrote:
> On 2012/05/19 02:10:54, sadrul wrote:
> > I don't think this is the right place for this change. Volume can be changed
> by
> > pressing the corresponding accelerator keys etc., and you will need to
> duplicate
> > this same code there too. Instead, the audio should be muted when
volume-level
> > is set to zero in AudioHandler::SetVolumePercent so that we have this logic
in
> > only place.
> 
> Sorry, I gave conflicting feedback in the review for
> https://codereview.chromium.org/10411028/ (saying that a UX decision like this
> one should be implemented in the UI rather than in the lower AudioHandler
> layer).  My main concern was that enforcing the policy in AudioHandler removes
> the flexibility to change the volume level that we'll restore to on unmute
> without actually unmuting.

[Jun Mukai, 2012-05-21 17:19:38]

Uploaded a new patchset which unmutes during audio_handler.
Again, either is fine but it seems not bad to me.

http://codereview.chromium.org/10408036/diff/1/ash/system/audio/tray_volume.cc
File ash/system/audio/tray_volume.cc (right):

http://codereview.chromium.org/10408036/diff/1/ash/system/audio/tray_volume.c...
ash/system/audio/tray_volume.cc:157: delegate->SetAudioMuted(false);
Note that same thing was introduced to volume_controller:
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/ash/v...

On 2012/05/21 17:05:38, Jun Mukai wrote:
> Still not sure which is better, but is there any real scenario where a user
> wants to change the volume but not to unmute?  I couldn't imagine such case,
so
> sadrul's point sounds slightly more valid to me.
> 
> On 2012/05/19 05:38:34, Daniel Erat wrote:
> > On 2012/05/19 02:10:54, sadrul wrote:
> > > I don't think this is the right place for this change. Volume can be
changed
> > by
> > > pressing the corresponding accelerator keys etc., and you will need to
> > duplicate
> > > this same code there too. Instead, the audio should be muted when
> volume-level
> > > is set to zero in AudioHandler::SetVolumePercent so that we have this
logic
> in
> > > only place.
> > 
> > Sorry, I gave conflicting feedback in the review for
> > https://codereview.chromium.org/10411028/ (saying that a UX decision like
this
> > one should be implemented in the UI rather than in the lower AudioHandler
> > layer).  My main concern was that enforcing the policy in AudioHandler
removes
> > the flexibility to change the volume level that we'll restore to on unmute
> > without actually unmuting.
>

[Daniel Erat, 2012-05-21 17:45:42]

lgtm

https://chromiumcodereview.appspot.com/10408036/diff/4/ash/system/audio/tray_...
File ash/system/audio/tray_volume.cc (right):

https://chromiumcodereview.appspot.com/10408036/diff/4/ash/system/audio/tray_...
ash/system/audio/tray_volume.cc:110: delegate->IsAudioMuted() ? 0.0 : 
delegate->GetVolumeLevel());
nit: remove extra space after ":" (you have two)

[Jun Mukai, 2012-05-21 17:56:07]

http://codereview.chromium.org/10408036/diff/4/ash/system/audio/tray_volume.cc
File ash/system/audio/tray_volume.cc (right):

http://codereview.chromium.org/10408036/diff/4/ash/system/audio/tray_volume.c...
ash/system/audio/tray_volume.cc:110: delegate->IsAudioMuted() ? 0.0 : 
delegate->GetVolumeLevel());
On 2012/05/21 17:45:43, Daniel Erat wrote:
> nit: remove extra space after ":" (you have two)

Done.

[commit-bot: I haz the power, 2012-05-21 17:56:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/10408036/8001

[commit-bot: I haz the power, 2012-05-22 02:57:35]

Try job failure for 10408036-8001 (retry) (retry) (retry) on win_rel for step
"sync_unit_tests".
It's a second try, previously, step "sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sadrul, 2012-05-22 17:13:24]

LGTM