Simplify logic behind chrome.audio.setActiveDevices

Simplify logic behind chrome.audio.setActiveDevices

Main updates:
* Explicitly separates input and output device lists,
which makes setting only one set of devices more intuitive.
This is achieved by introducing DeviceIdLists object that
can be used as method argument.
* Provides a way to clear active devices - by setting
active device lists to an empty list. Old setActiveDevices
method would not change set of active input/output devices if no
input/output devices were added to the list of active device IDs.

Note that the old logic is preserved not to break current clients
but will eventually be removed. Old logic can be triggered by passing
list of device IDs as array of strings rather than a newly introduced
|DeviceIdLists| object.

The CL adds new public methods to CrasAudioHandler to support new way
of setting active devices: SetActiveInputNodes and
SetActiveOutputNodes. These methods are tested by replicating test
cases for ChangeActiveNodes - replacing calls To ChangeActiveNodes with
new methods.

BUG=680361
Review-Url: https://codereview.chromium.org/2605983002
Cr-Commit-Position: refs/heads/master@{#444178}
Committed: https://chromium.googlesource.com/chromium/src/+/cae06e2aab96be77610c27f4a1469f9570df1c3c

[tbarzic, 2016-12-29 01:50:02]

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

[commit-bot: I haz the power, 2016-12-29 01:50:08]

Dry run: 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, 2016-12-29 02:03:33]

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

[commit-bot: I haz the power, 2016-12-29 02:03:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbarzic, 2016-12-29 02:09:41]

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

[commit-bot: I haz the power, 2016-12-29 02:09:57]

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

[tbarzic, 2016-12-29 02:21:06]

Description was changed from

==========
Simplify logic behind chrome.audio.setActiveDevices


BUG=
==========

to

==========
Simplify logic behind chrome.audio.setActiveDevices

Introduce audio.setActiveDeviceLists to audio API - the new method is
meant to be replacement audio.setActiveDevices, but with simpler
semantics.

Main differences between methods:
* setDeviceLists explicitly separates input and output device lists,
which makes setting only one set of devices more intuitive.
* setDeviceLists provides a way to clear active devices - by setting
active device lists to an empty list. Deprecated setActiveDevices
method would not change set of active input/output devices if no
input/output devices were added to the list of active device IDs.

The CL adds new public methods to CrasAudioHandler to support new way
of setting active devices: SetActiveInputNodes and
SetActiveOutputNodes. These methods are tested by replicating test
cases for ChangeActiveNodes - replacing calls To ChangeActiveNodes with
new methods.

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

[tbarzic, 2016-12-29 02:22:36]

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

[commit-bot: I haz the power, 2016-12-29 03:13:32]

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

[commit-bot: I haz the power, 2016-12-29 03:13:33]

Dry run: This issue passed the CQ dry run.

[jennyz, 2017-01-07 00:33:00]

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:216: // DEPRECATED in favour of
|SetActiveInputNodes| and |SetActiveOutputNodes|.
nit: favour -> favor

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:2607: kUSBJabraSpeakerInput1,
kUSBJabraSpeakerInput2, kUSBCameraInput});
Add a comment here: This simulates a typical hotrod audio device configuration.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3124: // this happens.
The comment says only jabra input node in active node list, but the code has
both jabra and camera input ndoes in active node list. They are not consistent
to each other. The comment is from the old code, which is consistent with the
old code, but you have changed the code, please update the comment to make it
consistent.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3129: // jabra's input is selected
as active input.
why is jabra speaker's output relevant here? The comment is not consistent with
the code.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3282: // way of hotrod app.
The comment needs to be updated to be consistent with your new node. The old
code  from line 2999 - 3001 is different from your new code 3283-3284. You also
have active input set now.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3382: // jabra's input is selected
as active input.
Wrong comment.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3385:
cras_audio_handler_->GetPrimaryActiveInputNode());
You can also verify cras_audio_handler_->GetPrimaryActiveOutputNode() returns 0.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3410: // jabra's input is selected
as active input.
ditto.

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

https://codereview.chromium.org/2605983002/diff/100001/extensions/browser/api...
extensions/browser/api/audio/audio_service.h:61: const
std::unique_ptr<DeviceIdList>& output_devives) = 0;
Please add some comments about the new function.

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

https://codereview.chromium.org/2605983002/diff/100001/extensions/browser/api...
extensions/browser/api/audio/audio_service_chromeos.cc:186: success = success &&
cras_audio_handler_->SetActiveInputNodes(input_nodes);
success is set initialized to true, what's the point && in the above line?

[tbarzic, 2017-01-07 02:46:11]

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

[commit-bot: I haz the power, 2017-01-07 02:46:26]

Dry run: 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-01-07 02:47:57]

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

[commit-bot: I haz the power, 2017-01-07 02:47:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[tbarzic, 2017-01-07 05:16:19]

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

[commit-bot: I haz the power, 2017-01-07 05:16:27]

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

[tbarzic, 2017-01-07 05:16:32]

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler.h (right):

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler.h:216: // DEPRECATED in favour of
|SetActiveInputNodes| and |SetActiveOutputNodes|.
On 2017/01/07 00:33:00, jennyz wrote:
> nit: favour -> favor

Done.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
File chromeos/audio/cras_audio_handler_unittest.cc (right):

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:2607: kUSBJabraSpeakerInput1,
kUSBJabraSpeakerInput2, kUSBCameraInput});
On 2017/01/07 00:33:00, jennyz wrote:
> Add a comment here: This simulates a typical hotrod audio device
configuration.

Done.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3124: // this happens.
On 2017/01/07 00:33:00, jennyz wrote:
> The comment says only jabra input node in active node list, but the code has
> both jabra and camera input ndoes in active node list. They are not consistent
> to each other. The comment is from the old code, which is consistent with the
> old code, but you have changed the code, please update the comment to make it
> consistent.

Oh, sorry, I blindly copied comment from kine 2862 - fixed.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3129: // jabra's input is selected
as active input.
On 2017/01/07 00:33:00, jennyz wrote:
> why is jabra speaker's output relevant here? The comment is not consistent
with
> the code.

ditto

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3282: // way of hotrod app.
On 2017/01/07 00:33:00, jennyz wrote:
> The comment needs to be updated to be consistent with your new node. The old
> code  from line 2999 - 3001 is different from your new code 3283-3284. You
also
> have active input set now.

this is supposed to be the same test case as 2963 - 2966

Updated the comment, though.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3382: // jabra's input is selected
as active input.
On 2017/01/07 00:33:00, jennyz wrote:
> Wrong comment.

Done.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3385:
cras_audio_handler_->GetPrimaryActiveInputNode());
On 2017/01/07 00:33:00, jennyz wrote:
> You can also verify cras_audio_handler_->GetPrimaryActiveOutputNode() returns
0.

Done.

https://codereview.chromium.org/2605983002/diff/100001/chromeos/audio/cras_au...
chromeos/audio/cras_audio_handler_unittest.cc:3410: // jabra's input is selected
as active input.
On 2017/01/07 00:33:00, jennyz wrote:
> ditto.

Done.

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

https://codereview.chromium.org/2605983002/diff/100001/extensions/browser/api...
extensions/browser/api/audio/audio_service.h:61: const
std::unique_ptr<DeviceIdList>& output_devives) = 0;
On 2017/01/07 00:33:00, jennyz wrote:
> Please add some comments about the new function.

Done.

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

https://codereview.chromium.org/2605983002/diff/100001/extensions/browser/api...
extensions/browser/api/audio/audio_service_chromeos.cc:186: success = success &&
cras_audio_handler_->SetActiveInputNodes(input_nodes);
On 2017/01/07 00:33:00, jennyz wrote:
> success is set initialized to true, what's the point && in the above line?

There is none - mostly was like this to keep it consistent with the line bellow
- fixed (and order of input and output handling, so both lines fit to 80 chars).

[commit-bot: I haz the power, 2017-01-07 06:12:34]

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

[commit-bot: I haz the power, 2017-01-07 06:12:35]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2017-01-12 02:04:16]

Description was changed from

==========
Simplify logic behind chrome.audio.setActiveDevices

Introduce audio.setActiveDeviceLists to audio API - the new method is
meant to be replacement audio.setActiveDevices, but with simpler
semantics.

Main differences between methods:
* setDeviceLists explicitly separates input and output device lists,
which makes setting only one set of devices more intuitive.
* setDeviceLists provides a way to clear active devices - by setting
active device lists to an empty list. Deprecated setActiveDevices
method would not change set of active input/output devices if no
input/output devices were added to the list of active device IDs.

The CL adds new public methods to CrasAudioHandler to support new way
of setting active devices: SetActiveInputNodes and
SetActiveOutputNodes. These methods are tested by replicating test
cases for ChangeActiveNodes - replacing calls To ChangeActiveNodes with
new methods.

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

to

==========
Simplify logic behind chrome.audio.setActiveDevices

Introduce audio.setActiveDeviceLists to audio API - the new method is
meant to be replacement audio.setActiveDevices, but with simpler
semantics.

Main differences between methods:
* setDeviceLists explicitly separates input and output device lists,
which makes setting only one set of devices more intuitive.
* setDeviceLists provides a way to clear active devices - by setting
active device lists to an empty list. Deprecated setActiveDevices
method would not change set of active input/output devices if no
input/output devices were added to the list of active device IDs.

The CL adds new public methods to CrasAudioHandler to support new way
of setting active devices: SetActiveInputNodes and
SetActiveOutputNodes. These methods are tested by replicating test
cases for ChangeActiveNodes - replacing calls To ChangeActiveNodes with
new methods.

BUG=680361
==========

[jennyz, 2017-01-12 21:49:47]

lgtm

[tbarzic, 2017-01-13 01:05:28]

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

[tbarzic, 2017-01-13 01:05:29]

adding owners for histograms and audio.idl

[Ilya Sherman, 2017-01-13 01:44:19]

histograms.xml lgtm

[Devlin, 2017-01-13 16:11:01]

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

https://codereview.chromium.org/2605983002/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:105: static void
setActiveDeviceLists(DeviceIdLists ids, EmptyCallback callback);
Could we instead update the signature of the current method (setActiveDevices)
and add a hook in the bindings and update the function impl to allow both
versions to work?  It'd be nice to avoid having two methods here, and
"setActiveDevices" is a friendlier name. :)

[tbarzic, 2017-01-13 18:50:43]

Description was changed from

==========
Simplify logic behind chrome.audio.setActiveDevices

Introduce audio.setActiveDeviceLists to audio API - the new method is
meant to be replacement audio.setActiveDevices, but with simpler
semantics.

Main differences between methods:
* setDeviceLists explicitly separates input and output device lists,
which makes setting only one set of devices more intuitive.
* setDeviceLists provides a way to clear active devices - by setting
active device lists to an empty list. Deprecated setActiveDevices
method would not change set of active input/output devices if no
input/output devices were added to the list of active device IDs.

The CL adds new public methods to CrasAudioHandler to support new way
of setting active devices: SetActiveInputNodes and
SetActiveOutputNodes. These methods are tested by replicating test
cases for ChangeActiveNodes - replacing calls To ChangeActiveNodes with
new methods.

BUG=680361
==========

to

==========
Simplify logic behind chrome.audio.setActiveDevices

Main updates:
* Explicitly separates input and output device lists,
which makes setting only one set of devices more intuitive.
This is achieved by introducing DeviceIdLists object that
can be used as method argument.
* Provides a way to clear active devices - by setting
active device lists to an empty list. Old setActiveDevices
method would not change set of active input/output devices if no
input/output devices were added to the list of active device IDs.

Note that the old logic is preserved not to break current clients
but will eventually be removed. Old logic can be triggered by passing
list of device IDs as array of strings rather than a newly introduced
|DeviceIdLists| object.

The CL adds new public methods to CrasAudioHandler to support new way
of setting active devices: SetActiveInputNodes and
SetActiveOutputNodes. These methods are tested by replicating test
cases for ChangeActiveNodes - replacing calls To ChangeActiveNodes with
new methods.

BUG=680361
==========

[tbarzic, 2017-01-13 18:52:49]

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

https://codereview.chromium.org/2605983002/diff/160001/extensions/common/api/...
extensions/common/api/audio.idl:105: static void
setActiveDeviceLists(DeviceIdLists ids, EmptyCallback callback);
On 2017/01/13 16:11:00, Devlin wrote:
> Could we instead update the signature of the current method (setActiveDevices)
> and add a hook in the bindings and update the function impl to allow both
> versions to work?  It'd be nice to avoid having two methods here, and
> "setActiveDevices" is a friendlier name. :)

Yes. Or even better, we could define the first argument as union.

[tbarzic, 2017-01-13 18:53:20]

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

[commit-bot: I haz the power, 2017-01-13 18:53:40]

Dry run: 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-01-13 20:03:41]

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

[commit-bot: I haz the power, 2017-01-13 20:03:41]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-01-17 21:05:39]

idl lgtm

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

https://codereview.chromium.org/2605983002/diff/200001/extensions/common/api/...
extensions/common/api/audio.idl:91: // |ids|: <p>Specifies IDs of devices that
should be active. If the list
Rephrase slightly: If either the input or output list is empty, devices in that
category are unaffected.

https://codereview.chromium.org/2605983002/diff/200001/extensions/common/api/...
extensions/common/api/audio.idl:98: //     is deprecated and should not be
relied upon to work. Please, use
nit: No ',' after please

[tbarzic, 2017-01-17 21:40:25]

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

https://codereview.chromium.org/2605983002/diff/200001/extensions/common/api/...
extensions/common/api/audio.idl:91: // |ids|: <p>Specifies IDs of devices that
should be active. If the list
On 2017/01/17 21:05:39, Devlin (possibly slow) wrote:
> Rephrase slightly: If either the input or output list is empty, devices in
that
> category are unaffected.

Done (though, with 'not set' instead of 'empty').

https://codereview.chromium.org/2605983002/diff/200001/extensions/common/api/...
extensions/common/api/audio.idl:98: //     is deprecated and should not be
relied upon to work. Please, use
On 2017/01/17 21:05:39, Devlin (possibly slow) wrote:
> nit: No ',' after please

Done.

[tbarzic, 2017-01-17 21:56:37]

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

[commit-bot: I haz the power, 2017-01-17 21:57:00]

Dry run: 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-01-17 23:05:39]

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

[commit-bot: I haz the power, 2017-01-17 23:05:40]

Dry run: This issue passed the CQ dry run.

[tbarzic, 2017-01-17 23:06:23]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-01-17 23:06:23]

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

[commit-bot: I haz the power, 2017-01-17 23:07:24]

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-01-17 23:14:50]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484694383217460,
"parent_rev": "49207db970b2bff94d1ca2d0c2b3d5c7979a59c8", "commit_rev":
"cae06e2aab96be77610c27f4a1469f9570df1c3c"}

[commit-bot: I haz the power, 2017-01-17 23:15:26]

Description was changed from

==========
Simplify logic behind chrome.audio.setActiveDevices

Main updates:
* Explicitly separates input and output device lists,
which makes setting only one set of devices more intuitive.
This is achieved by introducing DeviceIdLists object that
can be used as method argument.
* Provides a way to clear active devices - by setting
active device lists to an empty list. Old setActiveDevices
method would not change set of active input/output devices if no
input/output devices were added to the list of active device IDs.

Note that the old logic is preserved not to break current clients
but will eventually be removed. Old logic can be triggered by passing
list of device IDs as array of strings rather than a newly introduced
|DeviceIdLists| object.

The CL adds new public methods to CrasAudioHandler to support new way
of setting active devices: SetActiveInputNodes and
SetActiveOutputNodes. These methods are tested by replicating test
cases for ChangeActiveNodes - replacing calls To ChangeActiveNodes with
new methods.

BUG=680361
==========

to

==========
Simplify logic behind chrome.audio.setActiveDevices

Main updates:
* Explicitly separates input and output device lists,
which makes setting only one set of devices more intuitive.
This is achieved by introducing DeviceIdLists object that
can be used as method argument.
* Provides a way to clear active devices - by setting
active device lists to an empty list. Old setActiveDevices
method would not change set of active input/output devices if no
input/output devices were added to the list of active device IDs.

Note that the old logic is preserved not to break current clients
but will eventually be removed. Old logic can be triggered by passing
list of device IDs as array of strings rather than a newly introduced
|DeviceIdLists| object.

The CL adds new public methods to CrasAudioHandler to support new way
of setting active devices: SetActiveInputNodes and
SetActiveOutputNodes. These methods are tested by replicating test
cases for ChangeActiveNodes - replacing calls To ChangeActiveNodes with
new methods.

BUG=680361

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

[commit-bot: I haz the power, 2017-01-17 23:15:27]

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