Final cleanup pass over audio device API

Final cleanup pass over audio device API

* Makes sure event names are not capitalized (old names are
  not kept, as events in question do not seem to be used)
* Renames onDevicesChanged to onDeviceListChanged to make it
  clearer that it's triggered only when list of devices change,
  not when properties of a present device changes
* Add getDevices method
  - replacement for getInfo method
  - returns device info using AudioDeviceInfo (unified class for both
    input and output devices), which is in line with properties
    returned by recomended onDeviceListChanged event
  - add support for filering returned device by stream type (input
    vs. outptu devices) and device active state.
* Unify |volume| and |gain| property as |level| property in
  setProperties method argument (consistent with usage of
  AudioDeviceInfo to describe audio devices).

BUG=673392
Review-Url: https://codereview.chromium.org/2635983006
Cr-Commit-Position: refs/heads/master@{#450797}
Committed: https://chromium.googlesource.com/chromium/src/+/de70284d458bdca36d42eabeba7971c7087e2492

[tbarzic, 2017-01-19 01:21:11]

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

[tbarzic, 2017-01-19 01:21:12]

This should be final set of changes to the API itself - though, I'
ll probably add means to restrict deprecated functionality only to current audio
API whitelist.

[tbarzic, 2017-01-25 19:27:51]

tbarzic@chromium.org changed reviewers:
+ mnilsson@chromium.org

[tbarzic, 2017-01-25 19:27:52]

+mnilsson: fyi

[jennyz, 2017-02-02 01:10:44]

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
extensions/browser/api/audio/audio_api.cc:183: level_value >= 0 ? level_value :
gain_value))
Please add a comment about the migration.

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
File extensions/browser/api/audio/audio_service.h (right):

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
extensions/browser/api/audio/audio_service.h:60: // Retrieves list of audio
devices that satisfy |filter|. Poulates
typo: Poulates -> Populates

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
File extensions/browser/api/audio/audio_service_chromeos.cc (right):

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
extensions/browser/api/audio/audio_service_chromeos.cc:189: DeviceInfoList*
device_info_list_out) {
Please keep the argument name consistent: device_info_list_out vs devices_out

[tbarzic, 2017-02-02 01:24:41]

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
extensions/browser/api/audio/audio_api.cc:183: level_value >= 0 ? level_value :
gain_value))
On 2017/02/02 01:10:44, jennyz wrote:
> Please add a comment about the migration.

Done.

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
File extensions/browser/api/audio/audio_service.h (right):

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
extensions/browser/api/audio/audio_service.h:60: // Retrieves list of audio
devices that satisfy |filter|. Poulates
On 2017/02/02 01:10:44, jennyz wrote:
> typo: Poulates -> Populates

Done.

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
File extensions/browser/api/audio/audio_service_chromeos.cc (right):

https://codereview.chromium.org/2635983006/diff/60001/extensions/browser/api/...
extensions/browser/api/audio/audio_service_chromeos.cc:189: DeviceInfoList*
device_info_list_out) {
On 2017/02/02 01:10:44, jennyz wrote:
> Please keep the argument name consistent: device_info_list_out vs devices_out

Done.

[tbarzic, 2017-02-07 20:22:24]

any updates on this?

[jennyz, 2017-02-10 18:13:53]

lgtm. Please ask mnilsson to take a look.

[tbarzic, 2017-02-10 18:16:01]

On 2017/02/10 18:13:53, jennyz wrote:
> lgtm. Please ask mnilsson to take a look.

I've already pinged him about these changes when I sent it out for review, and
he said he was fine with it (over IM) :)

[tbarzic, 2017-02-10 19:34:04]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 19:34:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-02-10 19:45:13]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 19:46:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-02-10 19:47:30]

tbarzic@chromium.org changed reviewers:
+ mpearson@chromium.org, rdevlin.cronin@chromium.org

[tbarzic, 2017-02-10 19:47:31]

+owners for histograms, and audio.idl

[Devlin, 2017-02-10 20:24:29]

https://codereview.chromium.org/2635983006/diff/160001/extensions/browser/api...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2635983006/diff/160001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:173: params->properties.level.get() ?
*params->properties.level : -1;
largely unrelated to this cl, but where do we validate that level, volume, and
gain are between 0 and 100?  I don't see a min/max set.

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
File extensions/common/api/audio.idl (left):

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:176: static void
OnDevicesChanged(AudioDeviceInfo[] devices);
Did we check that no one is currently using this event?  Is that why the name is
changed, instead of deprecated?

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:100: [deprecated="Use |level| to set output
volume."] double? volume;
Remind me - what's the plan for removing these?

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:112: long? level;
Would it make sense to rename gain/volume on the device infos to level?  WDYT?

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:156: static void getDevices(DeviceFilter filter,
GetDevicesCallback callback);
should filter be optional?

[tbarzic, 2017-02-10 21:05:18]

https://codereview.chromium.org/2635983006/diff/160001/extensions/browser/api...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2635983006/diff/160001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:173: params->properties.level.get() ?
*params->properties.level : -1;
On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> largely unrelated to this cl, but where do we validate that level, volume, and
> gain are between 0 and 100?  I don't see a min/max set.

volume is normalized to 0-100 here:
https://cs.chromium.org/chromium/src/chromeos/audio/cras_audio_handler.cc?rcl...

gain is intentionally not normalized (due
https://bugs.chromium.org/p/chromium/issues/detail?id=273872)


(rewrote this to better handle the case level is set to a negative value.)

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
File extensions/common/api/audio.idl (left):

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:176: static void
OnDevicesChanged(AudioDeviceInfo[] devices);
On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> Did we check that no one is currently using this event?  Is that why the name
is
> changed, instead of deprecated?

yes, I checked the event is not currently in use.

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:100: [deprecated="Use |level| to set output
volume."] double? volume;
On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> Remind me - what's the plan for removing these?

I'll send out a cl that makes these available only to currently whitelisted app
(https://codereview.chromium.org/2688773002/); I'll remove them when the current
usages are migrated (I'm not completely certain on timing of that, though).

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:112: long? level;
On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> Would it make sense to rename gain/volume on the device infos to level?  WDYT?

Those are actually also deprecated - they're only used with audio.getInfo.
(AudioDeviceInfo already uses level).

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:156: static void getDevices(DeviceFilter filter,
GetDevicesCallback callback);
On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> should filter be optional?

I slightly prefer it not being optional, but I can make it optional if you
prefer it like that.

[Devlin, 2017-02-13 23:18:10]

lgtm

https://codereview.chromium.org/2635983006/diff/160001/extensions/browser/api...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2635983006/diff/160001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:173: params->properties.level.get() ?
*params->properties.level : -1;
On 2017/02/10 21:05:18, tbarzic wrote:
> On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> > largely unrelated to this cl, but where do we validate that level, volume,
and
> > gain are between 0 and 100?  I don't see a min/max set.
> 
> volume is normalized to 0-100 here:
>
https://cs.chromium.org/chromium/src/chromeos/audio/cras_audio_handler.cc?rcl...
> 
> gain is intentionally not normalized (due
> https://bugs.chromium.org/p/chromium/issues/detail?id=273872)
> 
> 
> (rewrote this to better handle the case level is set to a negative value.)

Ideally, we'd have it set as a minimum/maximum in the bindings, so that the
developers are aware that calling it with anything but 0 <= x <= 100 is invalid.
 Any chance we can do that before moving the API to stable?  (Though doesn't
need to be in this cl)

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:156: static void getDevices(DeviceFilter filter,
GetDevicesCallback callback);
On 2017/02/10 21:05:18, tbarzic wrote:
> On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> > should filter be optional?
> 
> I slightly prefer it not being optional, but I can make it optional if you
> prefer it like that.

If there's no reason to require it, it seems odd to force developers to specify
{} (since we don't do that most places).  I'd say it should be optional.

[tbarzic, 2017-02-14 00:27:13]

https://codereview.chromium.org/2635983006/diff/160001/extensions/browser/api...
File extensions/browser/api/audio/audio_api.cc (right):

https://codereview.chromium.org/2635983006/diff/160001/extensions/browser/api...
extensions/browser/api/audio/audio_api.cc:173: params->properties.level.get() ?
*params->properties.level : -1;
On 2017/02/13 23:18:09, Devlin wrote:
> On 2017/02/10 21:05:18, tbarzic wrote:
> > On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> > > largely unrelated to this cl, but where do we validate that level, volume,
> and
> > > gain are between 0 and 100?  I don't see a min/max set.
> > 
> > volume is normalized to 0-100 here:
> >
>
https://cs.chromium.org/chromium/src/chromeos/audio/cras_audio_handler.cc?rcl...
> > 
> > gain is intentionally not normalized (due
> > https://bugs.chromium.org/p/chromium/issues/detail?id=273872)
> > 
> > 
> > (rewrote this to better handle the case level is set to a negative value.)
> 
> Ideally, we'd have it set as a minimum/maximum in the bindings, so that the
> developers are aware that calling it with anything but 0 <= x <= 100 is
invalid.
>  Any chance we can do that before moving the API to stable?  (Though doesn't
> need to be in this cl)

I agree, but from what I can see [0, 100] is not the complete set of valid
values, at least not for input devices.

Let me see with jennyz if this could be enforced/changed.

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
File extensions/common/api/audio.idl (right):

https://codereview.chromium.org/2635983006/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:156: static void getDevices(DeviceFilter filter,
GetDevicesCallback callback);
On 2017/02/13 23:18:09, Devlin wrote:
> On 2017/02/10 21:05:18, tbarzic wrote:
> > On 2017/02/10 20:24:29, Devlin (OOO feb 8 and feb 9) wrote:
> > > should filter be optional?
> > 
> > I slightly prefer it not being optional, but I can make it optional if you
> > prefer it like that.
> 
> If there's no reason to require it, it seems odd to force developers to
specify
> {} (since we don't do that most places).  I'd say it should be optional.

OK, done.

[Mark P, 2017-02-15 06:27:10]

histograms.xml lgtm

[tbarzic, 2017-02-15 18:39:25]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-02-15 18:39:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jennyz@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2635983006/#ps220001
(title: ".")

[commit-bot: I haz the power, 2017-02-15 18:40:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 21:15:23]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1487183965781100,
"parent_rev": "1f2d942975d4a03beef3a5183759770cfbdce219", "commit_rev":
"de70284d458bdca36d42eabeba7971c7087e2492"}

[commit-bot: I haz the power, 2017-02-15 21:16:04]

Description was changed from

==========
Final cleanup pass over audio device API

* Makes sure event names are not capitalized (old names are
  not kept, as events in question do not seem to be used)
* Renames onDevicesChanged to onDeviceListChanged to make it
  clearer that it's triggered only when list of devices change,
  not when properties of a present device changes
* Add getDevices method
  - replacement for getInfo method
  - returns device info using AudioDeviceInfo (unified class for both
    input and output devices), which is in line with properties
    returned by recomended onDeviceListChanged event
  - add support for filering returned device by stream type (input
    vs. outptu devices) and device active state.
* Unify |volume| and |gain| property as |level| property in
  setProperties method argument (consistent with usage of
  AudioDeviceInfo to describe audio devices).

BUG=673392
==========

to

==========
Final cleanup pass over audio device API

* Makes sure event names are not capitalized (old names are
  not kept, as events in question do not seem to be used)
* Renames onDevicesChanged to onDeviceListChanged to make it
  clearer that it's triggered only when list of devices change,
  not when properties of a present device changes
* Add getDevices method
  - replacement for getInfo method
  - returns device info using AudioDeviceInfo (unified class for both
    input and output devices), which is in line with properties
    returned by recomended onDeviceListChanged event
  - add support for filering returned device by stream type (input
    vs. outptu devices) and device active state.
* Unify |volume| and |gain| property as |level| property in
  setProperties method argument (consistent with usage of
  AudioDeviceInfo to describe audio devices).

BUG=673392

Review-Url: https://codereview.chromium.org/2635983006
Cr-Commit-Position: refs/heads/master@{#450797}
Committed:
https://chromium.googlesource.com/chromium/src/+/de70284d458bdca36d42eabeba79...
==========

[commit-bot: I haz the power, 2017-02-15 21:16:06]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/de70284d458bdca36d42eabeba79...