Add a new audio extension event OnLevelChanged.

Redesign audio extension apis. This adds the following P1 requests api features:
OnLevelChanged event: This is fired when the level of an active audio device changes(volume for output, gain for inout), node id and new value of level are passed as arguments of the event.

BUG=429312
Committed: https://crrev.com/481292c5ac84c13aee9a1e1d0ebd83fdd52d744d
Cr-Commit-Position: refs/heads/master@{#321679}

[jennyz, 2015-02-20 23:10:06]

jennyz@chromium.org changed reviewers:
+ mnilsson@chromium.org, rkc@chromium.org

[stevenjb, 2015-02-23 22:16:18]

stevenjb@chromium.org changed reviewers:
+ stevenjb@chromium.org

[stevenjb, 2015-02-23 22:16:19]

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

https://codereview.chromium.org/945103002/diff/1/ash/system/audio/tray_audio....
ash/system/audio/tray_audio.cc:100: void
TrayAudio::OnOutputNodeVolumeChanged(uint64 node_id, double volume) {
Comment out node_id and volume to indicate that they are unused.

https://codereview.chromium.org/945103002/diff/60001/ash/system/audio/audio_o...
File ash/system/audio/audio_observer.h (right):

https://codereview.chromium.org/945103002/diff/60001/ash/system/audio/audio_o...
ash/system/audio/audio_observer.h:16: // Called when output volume changed.
Document |node_id| and |volume| (i.e. range)

https://codereview.chromium.org/945103002/diff/60001/extensions/browser/api/a...
File extensions/browser/api/audio/audio_api.h (right):

https://codereview.chromium.org/945103002/diff/60001/extensions/browser/api/a...
extensions/browser/api/audio/audio_api.h:74: class AudioGetDeviceInfoFunction :
public AsyncExtensionFunction {
This is a large CL, maybe add the new extension API first, separately?

[rkc, 2015-02-23 22:31:15]

I'll give this a thorough review tomorrow. If I need to get to it before then,
let me know and I'll try to find some time today.

[jennyz, 2015-02-23 22:40:55]

Tomorrow will be fine. Thanks a lot!

Jenny

On Mon, Feb 23, 2015 at 2:31 PM, <rkc@chromium.org> wrote:

> I'll give this a thorough review tomorrow. If I need to get to it before
> then,
> let me know and I'll try to find some time today.
>
>
> https://codereview.chromium.org/945103002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jennyz, 2015-02-24 00:05:39]

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

https://codereview.chromium.org/945103002/diff/1/ash/system/audio/tray_audio....
ash/system/audio/tray_audio.cc:100: void
TrayAudio::OnOutputNodeVolumeChanged(uint64 node_id, double volume) {
On 2015/02/23 22:16:19, stevenjb wrote:
> Comment out node_id and volume to indicate that they are unused.

Done.

https://codereview.chromium.org/945103002/diff/60001/ash/system/audio/audio_o...
File ash/system/audio/audio_observer.h (right):

https://codereview.chromium.org/945103002/diff/60001/ash/system/audio/audio_o...
ash/system/audio/audio_observer.h:16: // Called when output volume changed.
On 2015/02/23 22:16:19, stevenjb wrote:
> Document |node_id| and |volume| (i.e. range)

Done.

https://codereview.chromium.org/945103002/diff/60001/extensions/browser/api/a...
File extensions/browser/api/audio/audio_api.h (right):

https://codereview.chromium.org/945103002/diff/60001/extensions/browser/api/a...
extensions/browser/api/audio/audio_api.h:74: class AudioGetDeviceInfoFunction :
public AsyncExtensionFunction {
On 2015/02/23 22:16:19, stevenjb wrote:
> This is a large CL, maybe add the new extension API first, separately?

Yes, I probably should have done this at the first place. Although the cl looks
big, a lot of changes are caused by the signature changes of the notification
OnOutputVolumeChanged -> OnOutputNodeVolumeChanged, they are quite straight
forward. So I might keep it as is for now, if people are really bothered by it,
I will split it.

[stevenjb, 2015-02-24 18:25:14]

as/system lgtm

https://codereview.chromium.org/945103002/diff/80001/ash/system/audio/tray_au...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/945103002/diff/80001/ash/system/audio/tray_au...
ash/system/audio/tray_audio.cc:103: // queries the current primary output
device's volume.
This comment is helpful, but we also like to use the following pattern for
unused variables:

OnOutputNodeVolumeChanged(uint64 /* node_id */, double /* volume */) {

[rkc, 2015-02-24 21:30:34]

https://codereview.chromium.org/945103002/diff/80001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/945103002/diff/80001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:58: void
CrasAudioHandler::AudioObserver::OnOutputNodeVolumeChanged(uint64 node_id,
Remove names for unused parameters.

https://codereview.chromium.org/945103002/diff/80001/extensions/browser/api/a...
File extensions/browser/api/audio/audio_service_chromeos.cc (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/browser/api/a...
extensions/browser/api/audio/audio_service_chromeos.cc:175: void
AudioServiceImpl::StartGetDeviceInfo(
Can some of this code be reused between this and StartGetInfo?

https://codereview.chromium.org/945103002/diff/80001/extensions/browser/api/a...
File extensions/browser/api/audio/audio_service_linux.cc (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/browser/api/a...
extensions/browser/api/audio/audio_service_linux.cc:22: class AudioServiceImpl :
public AudioService {
This entire class is an empty implementation. We should remove this class/file
completely since it only adds bloat and does nothing useful. If we ever decide
to implement this for Linux, it'll be easy enough to add back in.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:72: static void getInfo(GetInfoCallback
callback);
I presume this is now deprecated? If so, mark it as such (you should check with
the apps team what is the correct process to deprecate an API).

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:82: static void setActiveDevices(DOMString[]
ids,
Can we split this into two APIs, setActiveInputDevices and
setActiveOutputDevices? Leaving this as the same API looks really muddled.
Anytime an API use needs 7 lines to explain behavior, it usually is better to
split the behavior :)

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:101: static void
OnOutputNodeVolumeChanged(DOMString id, double volume);
Can this be renamed to OnVolumeChanged? It is understood that volume will only
be changed for an output node.

https://codereview.chromium.org/945103002/diff/80001/extensions/shell/browser...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/shell/browser...
extensions/shell/browser/shell_audio_controller_chromeos.cc:38: void
ShellAudioController::OnOutputNodeVolumeChanged(uint64 node_id,
Remove names for unused parameters.

[jennyz, 2015-02-25 22:47:42]

https://codereview.chromium.org/945103002/diff/80001/ash/system/audio/tray_au...
File ash/system/audio/tray_audio.cc (right):

https://codereview.chromium.org/945103002/diff/80001/ash/system/audio/tray_au...
ash/system/audio/tray_audio.cc:103: // queries the current primary output
device's volume.
On 2015/02/24 18:25:13, stevenjb wrote:
> This comment is helpful, but we also like to use the following pattern for
> unused variables:
> 
> OnOutputNodeVolumeChanged(uint64 /* node_id */, double /* volume */) {
> 

Done.

https://codereview.chromium.org/945103002/diff/80001/chromeos/audio/cras_audi...
File chromeos/audio/cras_audio_handler.cc (right):

https://codereview.chromium.org/945103002/diff/80001/chromeos/audio/cras_audi...
chromeos/audio/cras_audio_handler.cc:58: void
CrasAudioHandler::AudioObserver::OnOutputNodeVolumeChanged(uint64 node_id,
On 2015/02/24 21:30:33, Rahul Chaturvedi wrote:
> Remove names for unused parameters.

Done.

https://codereview.chromium.org/945103002/diff/80001/extensions/browser/api/a...
File extensions/browser/api/audio/audio_service_chromeos.cc (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/browser/api/a...
extensions/browser/api/audio/audio_service_chromeos.cc:175: void
AudioServiceImpl::StartGetDeviceInfo(
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> Can some of this code be reused between this and StartGetInfo?

Although this logic flow is similar in these two functions, but the data
structures for holding device info are different, and the function is not very
long, my thinking is it is simpler to keep them separated, instead of forking
into different details. And later, we may just remove the old API after
migration to new ones.

https://codereview.chromium.org/945103002/diff/80001/extensions/browser/api/a...
File extensions/browser/api/audio/audio_service_linux.cc (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/browser/api/a...
extensions/browser/api/audio/audio_service_linux.cc:22: class AudioServiceImpl :
public AudioService {
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> This entire class is an empty implementation. We should remove this class/file
> completely since it only adds bloat and does nothing useful. If we ever decide
> to implement this for Linux, it'll be easy enough to add back in.

Agree, removed this one.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:72: static void getInfo(GetInfoCallback
callback);
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> I presume this is now deprecated? If so, mark it as such (you should check
with
> the apps team what is the correct process to deprecate an API).
I searched the code, don't see any consistent semantics for marking an api being
deprecated. So I added it in doc/comments. Will have mnilsson review and see he
knows a better way.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:82: static void setActiveDevices(DOMString[]
ids,
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> Can we split this into two APIs, setActiveInputDevices and
> setActiveOutputDevices? Leaving this as the same API looks really muddled.
> Anytime an API use needs 7 lines to explain behavior, it usually is better to
> split the behavior :

Yah, we will discuss with app team about this, but this is a separate issue not
related to this cl.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:82: static void setActiveDevices(DOMString[]
ids,
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> Can we split this into two APIs, setActiveInputDevices and
> setActiveOutputDevices? Leaving this as the same API looks really muddled.
> Anytime an API use needs 7 lines to explain behavior, it usually is better to
> split the behavior :)

Yes, we will discuss with app team, but let's keep this away from this cl.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:101: static void
OnOutputNodeVolumeChanged(DOMString id, double volume);
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> Can this be renamed to OnVolumeChanged? It is understood that volume will only
> be changed for an output node.

Since it is referred as either OutputVolume or OutputNodeVolume everywhere else,
rooted from cras API and signal names, I think it might be better to be
consistent here.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:101: static void
OnOutputNodeVolumeChanged(DOMString id, double volume);
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> Can this be renamed to OnVolumeChanged? It is understood that volume will only
> be changed for an output node.

Since it is referred everywhere else as OutputVolume in the code, probably
originated from cras api and signal names. I prefer to keep it consistent here
as well.

[mnilsson, 2015-02-25 23:37:25]

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:53: DOMString displayName;
Is this still simply a copy of deviceName when no other suitable name has been
set? Out of principle, I'd prefer no redundancy.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:55: DOMString deviceName;
Is this string still assembled from parts? If it makes sense, I'd rather see it
broken up into it's logical components, i.e., card name, interface name, card
index, interface index.

The background for this ask is that indices tend to be unstable across reboots
or after hotplug events, and this string would have to be cleansed of those to
make it useful for persisting purposes. The required parsing is ambiguous, and
it would be better to leave it for the client to select what parts of the device
name to use and how to format it.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:61: double volumeGain;
How about 'level'? Sounds simpler and more generic. Also the client code would
read strange in context if it talked about an output device's 'volumeGain'.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:82: static void setActiveDevices(DOMString[]
ids,
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> Can we split this into two APIs, setActiveInputDevices and
> setActiveOutputDevices? Leaving this as the same API looks really muddled.
> Anytime an API use needs 7 lines to explain behavior, it usually is better to
> split the behavior :)

Or actually, since the AudioDeviceInfo now encodes the direction but otherwise
treats them the same, this api would be fine provided it does what the previous
description says, replaces previously active nodes with the ones provided.

In general I think we should strive to keep the various entry points as
symmetrical as possible. So if we operate on the input and output nodes together
somewhere, we should do so everywhere.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:101: static void
OnOutputNodeVolumeChanged(DOMString id, double volume);
On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> Can this be renamed to OnVolumeChanged? It is understood that volume will only
> be changed for an output node.

See above for motivation - I think it should be OnLevelChanged, and should apply
equally to input and output nodes.

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

https://codereview.chromium.org/945103002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:218: void
OnOutputNodeVolumeChanged(uint64 node_id, double volume) override;
Why Node in this name? It doesn't seem symmetrical to OnInputGainChanged.

https://codereview.chromium.org/945103002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:220: void
OnInputGainChanged() override;
Considering symmetry, shouldn't you also provide the node_id and new gain here?

[jennyz, 2015-02-26 22:23:55]

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:53: DOMString displayName;
On 2015/02/25 23:37:25, mnilsson wrote:
> Is this still simply a copy of deviceName when no other suitable name has been
> set? Out of principle, I'd prefer no redundancy.

No, they are not the same, but they both come from cras. The displayName is a
short user friendly name for display, for example: HP ZR2440w. The deviceName is
longer, such as: HDA Intel MID: HDMI 0:2,3. 

In the case cras passes an empty displayName to chrome, chrome will use the
deviceName as displayName, the values of these two field will be identical then.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:55: DOMString deviceName;
On 2015/02/25 23:37:25, mnilsson wrote:
> Is this string still assembled from parts? If it makes sense, I'd rather see
it
> broken up into it's logical components, i.e., card name, interface name, card
> index, interface index.
> 
> The background for this ask is that indices tend to be unstable across reboots
> or after hotplug events, and this string would have to be cleansed of those to
> make it useful for persisting purposes. The required parsing is ambiguous, and
> it would be better to leave it for the client to select what parts of the
device
> name to use and how to format it.

Let's discuss with dgreid@ for how to parse the deviceName string into the parts
you want.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:61: double volumeGain;
On 2015/02/25 23:37:25, mnilsson wrote:
> How about 'level'? Sounds simpler and more generic. Also the client code would
> read strange in context if it talked about an output device's 'volumeGain'.

Done.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:82: static void setActiveDevices(DOMString[]
ids,
On 2015/02/25 23:37:25, mnilsson wrote:
> On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> > Can we split this into two APIs, setActiveInputDevices and
> > setActiveOutputDevices? Leaving this as the same API looks really muddled.
> > Anytime an API use needs 7 lines to explain behavior, it usually is better
to
> > split the behavior :)
> 
> Or actually, since the AudioDeviceInfo now encodes the direction but otherwise
> treats them the same, this api would be fine provided it does what the
previous
> description says, replaces previously active nodes with the ones provided.
> 
> In general I think we should strive to keep the various entry points as
> symmetrical as possible. So if we operate on the input and output nodes
together
> somewhere, we should do so everywhere.

Acknowledged.

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:101: static void
OnOutputNodeVolumeChanged(DOMString id, double volume);
On 2015/02/25 23:37:25, mnilsson wrote:
> On 2015/02/24 21:30:34, Rahul Chaturvedi wrote:
> > Can this be renamed to OnVolumeChanged? It is understood that volume will
only
> > be changed for an output node.
> 
> See above for motivation - I think it should be OnLevelChanged, and should
apply
> equally to input and output nodes.

Done.

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

https://codereview.chromium.org/945103002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:218: void
OnOutputNodeVolumeChanged(uint64 node_id, double volume) override;
On 2015/02/25 23:37:25, mnilsson wrote:
> Why Node in this name? It doesn't seem symmetrical to OnInputGainChanged.

Yes, you're right, we'd better make it consistent. Added node_id for input gain
change.

https://codereview.chromium.org/945103002/diff/120001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/system_tray_delegate_chromeos.h:220: void
OnInputGainChanged() override;
On 2015/02/25 23:37:25, mnilsson wrote:
> Considering symmetry, shouldn't you also provide the node_id and new gain
here?

Good catch. We are not paying much attention on input gain in application yet,
but I should get this consistent. Fixed.

[mnilsson, 2015-02-27 09:44:20]

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:53: DOMString displayName;
On 2015/02/26 22:23:55, jennyz wrote:
> On 2015/02/25 23:37:25, mnilsson wrote:
> > Is this still simply a copy of deviceName when no other suitable name has
been
> > set? Out of principle, I'd prefer no redundancy.
> 
> No, they are not the same, but they both come from cras. The displayName is a
> short user friendly name for display, for example: HP ZR2440w. The deviceName
is
> longer, such as: HDA Intel MID: HDMI 0:2,3. 
> 
> In the case cras passes an empty displayName to chrome, chrome will use the
> deviceName as displayName, the values of these two field will be identical
then.

I think less smarts here, and stricter definitions of what goes in what field.
Specifically, I think it is perfectly fine to have an empty displayName. The
client will be able to check for an empty string and choose its own fallback
mechanism.

[jennyz, 2015-02-27 20:47:41]

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/945103002/diff/80001/extensions/common/api/au...
extensions/common/api/audio.idl:53: DOMString displayName;
On 2015/02/27 09:44:20, mnilsson wrote:
> On 2015/02/26 22:23:55, jennyz wrote:
> > On 2015/02/25 23:37:25, mnilsson wrote:
> > > Is this still simply a copy of deviceName when no other suitable name has
> been
> > > set? Out of principle, I'd prefer no redundancy.
> > 
> > No, they are not the same, but they both come from cras. The displayName is
a
> > short user friendly name for display, for example: HP ZR2440w. The
deviceName
> is
> > longer, such as: HDA Intel MID: HDMI 0:2,3. 
> > 
> > In the case cras passes an empty displayName to chrome, chrome will use the
> > deviceName as displayName, the values of these two field will be identical
> then.
> 
> I think less smarts here, and stricter definitions of what goes in what field.
> Specifically, I think it is perfectly fine to have an empty displayName. The
> client will be able to check for an empty string and choose its own fallback
> mechanism.

The original thinking is displayName is designed as a user friendly name to
render in UI, so we wrapped the logic of this in AudioDevice code.
AudioDevice::display_name maps to AudioNode::name field. AudioNode is the raw
data from cras. There are cases cras may pass empty string or a string like
"(default)" in the name field. For both case, chrome OS UI does not want to
display empty string or "(default)" to UI, and we replace it with device_name.
If I pass the raw name string to display_name in AudioDevice, then both chromeOS
ash tray UI and hotrod UI client code has to know how to handle these cases. It
looks like simpler to me to handle it in one place, then distributed the logic
in different client codes, unless they want to make different decisions.

[jennyz, 2015-03-13 23:16:56]

I made some changes to the cl:
1. Remove getDeviceInfo API: Per my discussion with mnilsson@, it does not
really help hotrod app to add this one without passing the stable/persisted
device id string. I filed a separate bug for this work item: crbug.com/466768.

2. Change level/volume/gain data type to int as advised by dgreid@ in design doc
review.

rkc@ and mnilsson@, PTAL, thanks!

[mnilsson, 2015-03-16 20:04:36]

lgtm

The CL description should probably reflect that this is just about the part
merging the Volume/Gain and introducing onLevelChanged.

I see you left some references to the AUDIO_GETDEVICEINFO. They seem orphaned
here, but maybe there is some other reason you need/want to keep them.

https://codereview.chromium.org/945103002/diff/180001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/945103002/diff/180001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:1042:
AUDIO_GETDEVICEINFO,
Shouldn't this have been removed too?

https://codereview.chromium.org/945103002/diff/180001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/945103002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:48910: +  <int value="981"
label="AUDIO_GETDEVICEINFO"/>
Remove this?

[jennyz, 2015-03-16 21:39:55]

https://codereview.chromium.org/945103002/diff/180001/extensions/browser/exte...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/945103002/diff/180001/extensions/browser/exte...
extensions/browser/extension_function_histogram_value.h:1042:
AUDIO_GETDEVICEINFO,
On 2015/03/16 20:04:36, mnilsson wrote:
> Shouldn't this have been removed too?

Done.

https://codereview.chromium.org/945103002/diff/180001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/945103002/diff/180001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:48910: +  <int value="981"
label="AUDIO_GETDEVICEINFO"/>
On 2015/03/16 20:04:36, mnilsson wrote:
> Remove this?

Done.

[jennyz, 2015-03-16 21:48:33]

jennyz@chromium.org changed reviewers:
+ jamescook@chromium.org

[jennyz, 2015-03-16 21:56:38]

jamescook@, would you please review the following files:
chrome/browser/ui/ash/*
extensions/browser/BUILD.gn
extensions/shell/browser/*
These are mostly caused by semantics change of
CrasAudioHandler::Observer::OnOutputNodeVolumeChanged and
OnInputNodeGainChanged.
extensions/browser/BUILD.gn is caused by removing the unnecessary file:
extensions/browser/api/audio/audio_service_linux.cc.

[jennyz, 2015-03-16 21:57:18]

jennyz@chromium.org changed reviewers:
+ asargent@chromium.org

[jennyz, 2015-03-16 21:58:55]

asargent@, would you please review the files:
extensions/extensions.gyp
extensions/extensions.gypi
These changes are caused by removing the unnecessary file:
extensions/browser/api/audio/audio_service_linux.cc

[James Cook, 2015-03-16 22:14:01]

ash and extensions/shell LGTM, but please fix uint64 -> uint64_t

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

https://codereview.chromium.org/945103002/diff/200001/ash/system/audio/audio_...
ash/system/audio/audio_observer.h:19: virtual void
OnOutputNodeVolumeChanged(uint64 node_id, double volume) = 0;
You're gonna hate me, but uint64 is deprecated. This should be uint64_t and
#include <stdint.h>. See note in basictypes.h.

https://codereview.chromium.org/945103002/diff/200001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/945103002/diff/200001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.h:40: virtual void
OnOutputNodeVolumeChanged(uint64 node_id, int volume);
uint64 -> uint64_t

https://codereview.chromium.org/945103002/diff/200001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.h:46: virtual void
OnInputNodeGainChanged(uint64 node_id, int gain);
ditto

[jennyz, 2015-03-16 23:02:21]

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

https://codereview.chromium.org/945103002/diff/200001/ash/system/audio/audio_...
ash/system/audio/audio_observer.h:19: virtual void
OnOutputNodeVolumeChanged(uint64 node_id, double volume) = 0;
On 2015/03/16 22:14:01, James Cook wrote:
> You're gonna hate me, but uint64 is deprecated. This should be uint64_t and
> #include <stdint.h>. See note in basictypes.h.

Done.

https://codereview.chromium.org/945103002/diff/200001/chromeos/audio/cras_aud...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/945103002/diff/200001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.h:40: virtual void
OnOutputNodeVolumeChanged(uint64 node_id, int volume);
On 2015/03/16 22:14:01, James Cook wrote:
> uint64 -> uint64_t

Done.

https://codereview.chromium.org/945103002/diff/200001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.h:40: virtual void
OnOutputNodeVolumeChanged(uint64 node_id, int volume);
On 2015/03/16 22:14:01, James Cook wrote:
> uint64 -> uint64_t

Done.

https://codereview.chromium.org/945103002/diff/200001/chromeos/audio/cras_aud...
chromeos/audio/cras_audio_handler.h:46: virtual void
OnInputNodeGainChanged(uint64 node_id, int gain);
On 2015/03/16 22:14:01, James Cook wrote:
> ditto

Done.

[jennyz, 2015-03-16 23:18:35]

Patchset #13 (id:240001) has been deleted

[asargent_no_longer_on_chrome, 2015-03-17 16:21:17]

https://codereview.chromium.org/945103002/diff/260001/extensions/common/api/a...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/945103002/diff/260001/extensions/common/api/a...
extensions/common/api/audio.idl:32: // The input gain ranging from 0.0 to 100.0.
As discussed in chat, if this is a supported API on stable channel then we
cannot change the meaning of parameters like this since it breaks backwards
compatibility. (unless the documentation was just wrong, and the implementation
has always actually used 100 instead of 1 as the maximum value)

https://codereview.chromium.org/945103002/diff/260001/extensions/common/api/a...
extensions/common/api/audio.idl:79: // |level|: new sound level of the
node(volume for output, gain for input).
it might be worth mentioning the scale of the |level| parameter here (eg 0 to
100)

Also, why is |leve|l a long here whereas we use a double for volume/gain in the
InputDeviceInfo/OutputDeviceInfo dictionaries above?

https://codereview.chromium.org/945103002/diff/260001/extensions/extensions.gypi
File extensions/extensions.gypi (right):

https://codereview.chromium.org/945103002/diff/260001/extensions/extensions.g...
extensions/extensions.gypi:795: 'browser/api/audio/audio_service.cc',
Just to make sure I'm understanding this change, are we doing it because the
only platform where this API works is ChromeOS? I.e. the audio_service_linux.cc
implementation code had no differences from the audio_service.cc file? (both are
empty stub implementations that don't actually do anything?)

https://codereview.chromium.org/945103002/diff/260001/extensions/shell/browse...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/945103002/diff/260001/extensions/shell/browse...
extensions/shell/browser/shell_audio_controller_chromeos.cc:39: int /* volume
*/) {
nit: why use comments instead of just using parameter names? (here and below)

[jennyz, 2015-03-17 18:17:28]

https://codereview.chromium.org/945103002/diff/260001/extensions/common/api/a...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/945103002/diff/260001/extensions/common/api/a...
extensions/common/api/audio.idl:32: // The input gain ranging from 0.0 to 100.0.
On 2015/03/17 16:21:15, Antony Sargent wrote:
> As discussed in chat, if this is a supported API on stable channel then we
> cannot change the meaning of parameters like this since it breaks backwards
> compatibility. (unless the documentation was just wrong, and the
implementation
> has always actually used 100 instead of 1 as the maximum value)

This was documented incorrectly before, now it is corrected.

https://codereview.chromium.org/945103002/diff/260001/extensions/common/api/a...
extensions/common/api/audio.idl:79: // |level|: new sound level of the
node(volume for output, gain for input).
On 2015/03/17 16:21:15, Antony Sargent wrote:
> it might be worth mentioning the scale of the |level| parameter here (eg 0 to
> 100)
> 
> Also, why is |leve|l a long here whereas we use a double for volume/gain in
the
> InputDeviceInfo/OutputDeviceInfo dictionaries above?

We are in the process to transit the old audio extension APIs to the new APIs.
The InputDeviceInfo/OutputDeviceInfo will eventually be deprecated and become
legacy. In the old definition, the volume and gain are defined as double, but
actually they should be defined as int, because the data sent from cras audio
api are actually int32. The legacy may come some even older data migration
story, which does not exist anymore. Therefore, we would like to make it right
in the new APIs.

https://codereview.chromium.org/945103002/diff/260001/extensions/extensions.gypi
File extensions/extensions.gypi (right):

https://codereview.chromium.org/945103002/diff/260001/extensions/extensions.g...
extensions/extensions.gypi:795: 'browser/api/audio/audio_service.cc',
On 2015/03/17 16:21:17, Antony Sargent wrote:
> Just to make sure I'm understanding this change, are we doing it because the
> only platform where this API works is ChromeOS? I.e. the
audio_service_linux.cc
> implementation code had no differences from the audio_service.cc file? (both
are
> empty stub implementations that don't actually do anything?)

Yes.

https://codereview.chromium.org/945103002/diff/260001/extensions/shell/browse...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/945103002/diff/260001/extensions/shell/browse...
extensions/shell/browser/shell_audio_controller_chromeos.cc:39: int /* volume
*/) {
On 2015/03/17 16:21:17, Antony Sargent wrote:
> nit: why use comments instead of just using parameter names? (here and below)
> 

I think this is a convention we used to indicate the parameters are not used in
the code.

[asargent_no_longer_on_chrome, 2015-03-18 00:39:25]

https://codereview.chromium.org/945103002/diff/260001/extensions/shell/browse...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/945103002/diff/260001/extensions/shell/browse...
extensions/shell/browser/shell_audio_controller_chromeos.cc:39: int /* volume
*/) {
On 2015/03/17 18:17:28, jennyz wrote:
> On 2015/03/17 16:21:17, Antony Sargent wrote:
> > nit: why use comments instead of just using parameter names? (here and
below)
> > 
> 
> I think this is a convention we used to indicate the parameters are not used
in
> the code.

Interesting - I don't think I've seen this pattern used before in chrome code.
Do you know if this is explicitly allowed or disallowed in the style guide? Can
you point to some other examples of this?

[jennyz, 2015-03-18 18:24:36]

https://codereview.chromium.org/945103002/diff/260001/extensions/shell/browse...
File extensions/shell/browser/shell_audio_controller_chromeos.cc (right):

https://codereview.chromium.org/945103002/diff/260001/extensions/shell/browse...
extensions/shell/browser/shell_audio_controller_chromeos.cc:39: int /* volume
*/) {
On 2015/03/18 00:39:25, Antony Sargent wrote:
> On 2015/03/17 18:17:28, jennyz wrote:
> > On 2015/03/17 16:21:17, Antony Sargent wrote:
> > > nit: why use comments instead of just using parameter names? (here and
> below)
> > > 
> > 
> > I think this is a convention we used to indicate the parameters are not used
> in
> > the code.
> 
> Interesting - I don't think I've seen this pattern used before in chrome code.
> Do you know if this is explicitly allowed or disallowed in the style guide?
Can
> you point to some other examples of this? 
> 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Decl...
At the end of the section, it mentions the following with an example.
If some parameters are unused, comment out the variable name in the function
definition.

[asargent_no_longer_on_chrome, 2015-03-20 21:44:56]

lgtm

[jennyz, 2015-03-20 22:12:33]

The CQ bit was checked by jennyz@chromium.org

[jennyz, 2015-03-20 22:12:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, mnilsson@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/945103002/#ps260001
(title: "Update audio.idl comment")

[commit-bot: I haz the power, 2015-03-20 22:13:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/945103002/260001

[commit-bot: I haz the power, 2015-03-21 01:49:51]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-03-21 01:50:38]

Patchset 13 (id:??) landed as
https://crrev.com/481292c5ac84c13aee9a1e1d0ebd83fdd52d744d
Cr-Commit-Position: refs/heads/master@{#321679}