Fix the volume controlling behaviors.

Fix the volume controlling behaviors.

Based on the description of crbug.com/128644:
- volume up from muted should be normal up (4%), not 25%
- but if the volume up happens after the mute button, it should go back to the original volume

BUG=128644
TEST=browser_test passed, manually checked on lumpy

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

[Daniel Erat, 2012-05-18 23:45:13]

I haven't looked at the tests yet.

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio/audio_handler.cc (right):

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.cc:94: return (mixer_->IsMuted()) ?
0.0 : mixer_->GetVolumePercent();
I don't think that this UX decision should be reflected in AudioHandler.  Please
limit it to the UI code (i.e. just display 0 instead of the value from
GetVolumePercent() if IsMuted() is true).

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.cc:109: return
mixer_->GetVolumePercent() == 0 || mixer_->IsMuted();
ditto here

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio/audio_handler.h (right):

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.h:37: static void
InitializeForTesting(AudioMixer* mixer);
nit: mention that this takes ownership of |mixer|

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.h:68: explicit
AudioHandler(AudioMixer* mixer);
nit: mention that this takes ownership of |mixer|

[Jun Mukai, 2012-05-18 23:53:18]

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio/audio_handler.cc (right):

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.cc:94: return (mixer_->IsMuted()) ?
0.0 : mixer_->GetVolumePercent();
On 2012/05/18 23:45:13, Daniel Erat wrote:
> I don't think that this UX decision should be reflected in AudioHandler. 
Please
> limit it to the UI code (i.e. just display 0 instead of the value from
> GetVolumePercent() if IsMuted() is true).

Removed.  I will make the UI change in another CL.

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.cc:109: return
mixer_->GetVolumePercent() == 0 || mixer_->IsMuted();
On 2012/05/18 23:45:13, Daniel Erat wrote:
> ditto here

Removed too.

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio/audio_handler.h (right):

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.h:37: static void
InitializeForTesting(AudioMixer* mixer);
On 2012/05/18 23:45:13, Daniel Erat wrote:
> nit: mention that this takes ownership of |mixer|

Done.

http://codereview.chromium.org/10411028/diff/5001/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.h:68: explicit
AudioHandler(AudioMixer* mixer);
On 2012/05/18 23:45:13, Daniel Erat wrote:
> nit: mention that this takes ownership of |mixer|

Done.

[Daniel Erat, 2012-05-19 00:02:06]

LGTM with a few nits.  Thanks for adding tests!

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio/audio_handler.h (right):

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.h:36: // Same as Initialize but
using the specified audio mixer.  It takes the
nit: s/takes the ownership/takes ownership/

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.h:69: // Takes the ownership of
|mixer|.
same here

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc
(right):

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:66:
volume_controller_.HandleVolumeMute(dummy);
nit: can you shorten this to:

  ...HandleVolumeMute(ui::Accelerator())

?

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:71:
volume_controller_.HandleVolumeUp(dummy);
same here

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:76:
volume_controller_.HandleVolumeDown(dummy);
and here

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:87: double
initial_volume = audio_mixer()->GetVolumePercent();
nit: call SetVolumePercent(50.0) at the beginning of this test?

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:122:
EXPECT_FALSE(audio_mixer()->IsMuted());
nit: s/EXPECT_FALSE/ASSERT_FALSE/, since the rest of this test will be
irrelevant if the starting state is wrong

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:128: //
Further mute buttons doesn't make effects.
nit: s/make/have/

[Jun Mukai, 2012-05-19 00:18:30]

Thanks for quick review.  I will wait the submit for the confirmation of
volume-up to 25%, in crbug.com/128644

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/chromeos/aud...
File chrome/browser/chromeos/audio/audio_handler.h (right):

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.h:36: // Same as Initialize but
using the specified audio mixer.  It takes the
On 2012/05/19 00:02:06, Daniel Erat wrote:
> nit: s/takes the ownership/takes ownership/

Done.

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/chromeos/aud...
chrome/browser/chromeos/audio/audio_handler.h:69: // Takes the ownership of
|mixer|.
On 2012/05/19 00:02:06, Daniel Erat wrote:
> same here

Done.

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc
(right):

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:66:
volume_controller_.HandleVolumeMute(dummy);
On 2012/05/19 00:02:06, Daniel Erat wrote:
> nit: can you shorten this to:
> 
>   ...HandleVolumeMute(ui::Accelerator())
> 
> ?

Done.

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:71:
volume_controller_.HandleVolumeUp(dummy);
On 2012/05/19 00:02:06, Daniel Erat wrote:
> same here

Done.

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:76:
volume_controller_.HandleVolumeDown(dummy);
On 2012/05/19 00:02:06, Daniel Erat wrote:
> and here

Done.

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:87: double
initial_volume = audio_mixer()->GetVolumePercent();
On 2012/05/19 00:02:06, Daniel Erat wrote:
> nit: call SetVolumePercent(50.0) at the beginning of this test?

Done.

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:122:
EXPECT_FALSE(audio_mixer()->IsMuted());
On 2012/05/19 00:02:06, Daniel Erat wrote:
> nit: s/EXPECT_FALSE/ASSERT_FALSE/, since the rest of this test will be
> irrelevant if the starting state is wrong

Done.

http://codereview.chromium.org/10411028/diff/7002/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:128: //
Further mute buttons doesn't make effects.
On 2012/05/19 00:02:06, Daniel Erat wrote:
> nit: s/make/have/

Done.

[sky, 2012-05-21 14:45:23]

LGTM

http://codereview.chromium.org/10411028/diff/5008/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc
(right):

http://codereview.chromium.org/10411028/diff/5008/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:78:
private:
indentation is off.

http://codereview.chromium.org/10411028/diff/5008/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:81: };
DISALLOW_...

[Jun Mukai, 2012-05-21 17:02:09]

http://codereview.chromium.org/10411028/diff/5008/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc
(right):

http://codereview.chromium.org/10411028/diff/5008/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:78:
private:
On 2012/05/21 14:45:27, sky wrote:
> indentation is off.

Done.

http://codereview.chromium.org/10411028/diff/5008/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/volume_controller_chromeos_browsertest.cc:81: };
On 2012/05/21 14:45:27, sky wrote:
> DISALLOW_...

Done.

[commit-bot: I haz the power, 2012-05-21 17:02:40]

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

[commit-bot: I haz the power, 2012-05-21 18:14:58]

Try job failure for 10411028-9003 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-05-21 18:19:19]

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

[commit-bot: I haz the power, 2012-05-21 18:20:57]

Try job failure for 10411028-9003 on mac_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-21 19:18:33]

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

[commit-bot: I haz the power, 2012-05-21 19:48:00]

Try job failure for 10411028-9003 on win for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-21 20:29:51]

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

[commit-bot: I haz the power, 2012-05-22 04:27:36]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 138208, job name
was 10411028-9003 (retry) (retry) (retry) (retry).