Refactor the TrayAudio code so that it can be used by other platforms.

Refactor the TrayAudio code so that it can be used by other platforms.
The motivation here is to introduce the volume slider tray UI into
Windows Ash.

This change does not actually add tray audio UI to other platforms,
only moves some non-chromeos specific things out of the chromeos
platform folders, so that we can hook in the native Audio APIs
of other platforms to the tray UI.

At a high level, this change does the following:

1) Move tray_audio out of chromeos/ so that other platforms can
use the UI that it provides.
2) Decouples TrayAudio from CrasAudioHandler by introducing a
new delegate, in a similar vein to what is done with all the other
handlers (e.g. the BluetoothHandler)
3) Move the additional views (VolumeView, etc) to their own files
so they can be used by other platforms as well.

TEST=
 On CrOS physical device, verified that:
 1) Keyboard volume buttons function appropriately and display the
    volume view
 2) AudioDetailView still is accessible at the correct times
 3) Adjusting the volume (via the keyboard shortcuts or via the
    slider) correctly change the output volume.
 4) UI still behaves correctly (e.g. icons update as the volume
    changes, mute button works, etc).

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

[zturner, 2014-02-13 23:24:33]

Note that because many files were moved to different folders, it might be
difficult to review this diff in Rietveld.  It should be easier to handle in an
external diff viewer though.  Apologies.

[jennyz, 2014-02-14 00:30:09]

Adding rkc@ to review, since his extension code also use cras audio apis.

[James Cook, 2014-02-14 00:52:59]

I'll take a look after jennyz/rkc

[jennyz, 2014-02-14 21:36:42]

Mostly nits. This cl is pretty big and hard to diff due to the files moving
around. I think it will make the review easier if you break up it into several
smaller cls. For example, removing the UI dependency on CrasAudioHandler and
introducing TrayAudioDelegate can be done in the separate UI and the UI
refactoring also can be done in several smaller cls. Next time, please do it
progressively, do not make all the refactoring happening in a single cl.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/audio_obser...
File ash/system/audio/audio_observer.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/audio_obser...
ash/system/audio/audio_observer.h:10: class AudioObserver {
It looks like you have removed several events from
CrasAudioHandler::AudioObserver, such as OnInputGainChanged and
OnInputMuteChanged. They are not used by tray_audio, but they are observed by
chrome/browser/extensions/api/audio/audio_service_chromeos.cc. Please make sure
you update the audio server code for the observer change too.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio.h
File ash/system/audio/tray_audio.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio....
ash/system/audio/tray_audio.h:27: system::TrayAudioDelegate* audio_delegate);
Remove explicit since the constructor now takes 2 parameters.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio_...
File ash/system/audio/tray_audio_delegate.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio_...
ash/system/audio/tray_audio_delegate.h:27: virtual int GetDeviceIconId() = 0;
The name is not accurate, how about lGetActiveOutputDeviceIconId? And please up
the comment accordingly.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view.cc
File ash/system/audio/volume_view.cc (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.cc:44: system::TrayAudioDelegate& audio_delegate)
Remove explicit, no need if there are more than one arguments for constructor.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.cc:93: system::TrayAudioDelegate& audio_delegate)
Remove explicit. Why not just pass system::TrayAudioDelegate*?

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.cc:237: }
You removed the else if case in original HandleVolumeDown, please make sure the
volume control behaves correctly in that case.
else if (!audio_handler->IsOutputVolumeBelowDefaultMuteLvel() &&
               audio_handler->IsOutputMuted()) {
      audio_handler->SetOutputMute(false);
    }

It has been a long time, I don't remember exactly which case this handles,
please make sure the volumecontrol browser tests all passed. I think the most
edge cases are covered there.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view.h
File ash/system/audio/volume_view.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.h:41: system::TrayAudioDelegate& audio_delegate,
Why not just pass system::TrayAudioDelegate*?

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.h:89: #endif
Please add // ASH_SYSTEM_AUDIO_VOLUME_VIEW_H_ after #endif.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
File ash/system/chromeos/audio/audio_detailed_view.cc (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
2012 -> 2014

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:19: namespace {
Add a blank line after namespace {

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:44: }
Add a blank line.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:45: }
add // namespace.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:110: void
AudioDetailedView::Update() {
Move this function after ~AudioDetailedView. The function sequence in .cc file
should be consistent with their sequence in .h.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/tr...
File ash/system/chromeos/audio/tray_audio_delegate_chromeos.cc (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/tr...
ash/system/chromeos/audio/tray_audio_delegate_chromeos.cc:49:
audio_handler->has_alternative_input());
Align with previous line after "(",  looks better.

[jennyz, 2014-02-14 21:38:34]

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/audio_obser...
File ash/system/audio/audio_observer.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/audio_obser...
ash/system/audio/audio_observer.h:10: class AudioObserver {
On 2014/02/14 21:36:43, jennyz wrote:
> It looks like you have removed several events from
> CrasAudioHandler::AudioObserver, such as OnInputGainChanged and
> OnInputMuteChanged. They are not used by tray_audio, but they are observed by
> chrome/browser/extensions/api/audio/audio_service_chromeos.cc. Please make
sure
> you update the audio server code for the observer change too.

Sorry, I didn't finish review the entire cl when I wrote this comment. Please
ignore it. I thought you moved out AudioObserver out of CrasAudioHandler, but
you didn't.

[jennyz, 2014-02-14 21:40:04]

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio_...
File ash/system/audio/tray_audio_delegate.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio_...
ash/system/audio/tray_audio_delegate.h:27: virtual int GetDeviceIconId() = 0;
On 2014/02/14 21:36:43, jennyz wrote:
> The name is not accurate, how about lGetActiveOutputDeviceIconId? And please
up
> the comment accordingly.

Sorry for the typo. No l before GetActiveOutputDeviceIconId.

[zturner, 2014-02-15 00:57:44]

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio.h
File ash/system/audio/tray_audio.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio....
ash/system/audio/tray_audio.h:27: system::TrayAudioDelegate* audio_delegate);
On 2014/02/14 21:36:43, jennyz wrote:
> Remove explicit since the constructor now takes 2 parameters.

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio_...
File ash/system/audio/tray_audio_delegate.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/tray_audio_...
ash/system/audio/tray_audio_delegate.h:27: virtual int GetDeviceIconId() = 0;
On 2014/02/14 21:40:05, jennyz wrote:
> On 2014/02/14 21:36:43, jennyz wrote:
> > The name is not accurate, how about lGetActiveOutputDeviceIconId? And please
> up
> > the comment accordingly.
> 
> Sorry for the typo. No l before GetActiveOutputDeviceIconId.

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view.cc
File ash/system/audio/volume_view.cc (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.cc:44: system::TrayAudioDelegate& audio_delegate)
On 2014/02/14 21:36:43, jennyz wrote:
> Remove explicit, no need if there are more than one arguments for constructor.

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.cc:93: system::TrayAudioDelegate& audio_delegate)
On 2014/02/14 21:36:43, jennyz wrote:
> Remove explicit. Why not just pass system::TrayAudioDelegate*?

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.cc:237: }
On 2014/02/14 21:36:43, jennyz wrote:
> You removed the else if case in original HandleVolumeDown, please make sure
the
> volume control behaves correctly in that case.
> else if (!audio_handler->IsOutputVolumeBelowDefaultMuteLvel() &&
>                audio_handler->IsOutputMuted()) {
>       audio_handler->SetOutputMute(false);
>     }
> 
> It has been a long time, I don't remember exactly which case this handles,
> please make sure the volumecontrol browser tests all passed. I think the most
> edge cases are covered there.

I added this back.  I think I removed it because the condition didn't make sense
to me and it looked like a bug, but on second thought since this is supposed to
be a pure refactor with no functional change, it doesn't make sense for me to
remove it.

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view.h
File ash/system/audio/volume_view.h (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/audio/volume_view...
ash/system/audio/volume_view.h:41: system::TrayAudioDelegate& audio_delegate,
On 2014/02/14 21:36:43, jennyz wrote:
> Why not just pass system::TrayAudioDelegate*?

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
File ash/system/chromeos/audio/audio_detailed_view.cc (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2014/02/14 21:36:43, jennyz wrote:
> 2012 -> 2014

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:19: namespace {
On 2014/02/14 21:36:43, jennyz wrote:
> Add a blank line after namespace {

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:44: }
On 2014/02/14 21:36:43, jennyz wrote:
> Add a blank line.

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:45: }
On 2014/02/14 21:36:43, jennyz wrote:
> add // namespace.

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/au...
ash/system/chromeos/audio/audio_detailed_view.cc:110: void
AudioDetailedView::Update() {
On 2014/02/14 21:36:43, jennyz wrote:
> Move this function after ~AudioDetailedView. The function sequence in .cc file
> should be consistent with their sequence in .h.

Done.

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/tr...
File ash/system/chromeos/audio/tray_audio_delegate_chromeos.cc (right):

https://codereview.chromium.org/163953007/diff/1/ash/system/chromeos/audio/tr...
ash/system/chromeos/audio/tray_audio_delegate_chromeos.cc:49:
audio_handler->has_alternative_input());
On 2014/02/14 21:36:43, jennyz wrote:
> Align with previous line after "(",  looks better.

Done.

[jennyz, 2014-02-18 17:57:48]

lgtm with a minor nit.

https://codereview.chromium.org/163953007/diff/300001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):

https://codereview.chromium.org/163953007/diff/300001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:92: #include
"chromeos/audio/cras_audio_handler.h"
nit: cras_audio_handler.h is already included in the
systerm_tray_delegate_chromeos.h, no need to include again here.

[zturner, 2014-02-18 18:11:04]

On 2014/02/18 17:57:48, jennyz wrote:
> lgtm with a minor nit.
> 
>
https://codereview.chromium.org/163953007/diff/300001/chrome/browser/ui/ash/s...
> File chrome/browser/ui/ash/system_tray_delegate_chromeos.cc (right):
> 
>
https://codereview.chromium.org/163953007/diff/300001/chrome/browser/ui/ash/s...
> chrome/browser/ui/ash/system_tray_delegate_chromeos.cc:92: #include
> "chromeos/audio/cras_audio_handler.h"
> nit: cras_audio_handler.h is already included in the
> systerm_tray_delegate_chromeos.h, no need to include again here.

jcook: Can you take a look now?  Remaining files missing LGTM are:

    chrome\browser\ui\ash\system_tray_delegate_chromeos.cc
    chrome\browser\ui\ash\system_tray_delegate_chromeos.h

jennyz: Thanks, I will upload a fix for your suggestion at the same
as addressing any comments from jcook.

[James Cook, 2014-02-18 19:34:48]

chrome/browser/ui/ash LGTM with nits

Also, please fix the copyright date across all added files before committing.
Thanks!

https://codereview.chromium.org/163953007/diff/300001/ash/system/audio/audio_...
File ash/system/audio/audio_observer.h (right):

https://codereview.chromium.org/163953007/diff/300001/ash/system/audio/audio_...
ash/system/audio/audio_observer.h:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
nit: "Copyright (c) 2012" -> "Copyright 2014"

https://codereview.chromium.org/163953007/diff/300001/ash/system/audio/tray_a...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/163953007/diff/300001/ash/system/audio/tray_a...
ash/system/audio/tray_audio.cc:1: // Copyright (c) 2012 The Chromium Authors.
All rights reserved.
ditto

https://codereview.chromium.org/163953007/diff/300001/ash/system/audio/tray_a...
File ash/system/audio/tray_audio_delegate.h (right):

https://codereview.chromium.org/163953007/diff/300001/ash/system/audio/tray_a...
ash/system/audio/tray_audio_delegate.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
looks like you need to fix them all

https://codereview.chromium.org/163953007/diff/300001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/system_tray_delegate_chromeos.h (right):

https://codereview.chromium.org/163953007/diff/300001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:188: // Overrideen from
CrasAudioHandler::AudioObserver.
Overrideen -> Overridden

[zturner, 2014-02-18 22:39:14]

The CQ bit was checked by zturner@chromium.org

[commit-bot: I haz the power, 2014-02-18 22:42:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zturner@chromium.org/163953007/640001

[commit-bot: I haz the power, 2014-02-19 00:29:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 00:29:09]

Retried try job too often on linux_chromeos_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[zturner, 2014-02-19 00:46:07]

The CQ bit was checked by zturner@chromium.org

[commit-bot: I haz the power, 2014-02-19 00:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zturner@chromium.org/163953007/710002

[commit-bot: I haz the power, 2014-02-19 06:09:48]

Change committed as 251959

[earthdok, 2014-02-19 08:30:21]

On 2014/02/19 06:09:48, I haz the power (commit-bot) wrote:
> Change committed as 251959

This broke the ASan bots:
http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20...