dbus changes: add SetGlobalOutputChannelRemix support

dbus changes: add SetGlobalOutputChannelRemix support

cras_audio_client_unittest is still wanted

BUG=598800
Committed: https://crrev.com/a377c049175591928ea1507ef76242d913363011
Cr-Commit-Position: refs/heads/master@{#384097}

[Qiang(Joe) Xu, 2016-03-29 18:13:03]

Description was changed from

==========
dbus changes: add SetGlobalOutputChannelRemix support

BUG=404701
==========

to

==========
dbus changes: add SetGlobalOutputChannelRemix support

BUG=404701
==========

[Qiang(Joe) Xu, 2016-03-29 18:13:03]

warx@chromium.org changed reviewers:
+ hychao@chromium.org, stevenjb@chromium.org

[Qiang(Joe) Xu, 2016-03-29 18:13:31]

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

[commit-bot: I haz the power, 2016-03-29 18:13:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839943003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839943003/1

[commit-bot: I haz the power, 2016-03-29 18:25:58]

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

[commit-bot: I haz the power, 2016-03-29 18:25:59]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[stevenjb, 2016-03-29 18:38:04]

cras_audio_client.cc should really have unit tests. It doesn't need to block
this issue, but can you file and assign an issue for that (or prioritize one if
it exists)? Thanks.

https://codereview.chromium.org/1839943003/diff/1/dbus/message.cc
File dbus/message.cc (right):

https://codereview.chromium.org/1839943003/diff/1/dbus/message.cc#newcode857
dbus/message.cc:857: *length = static_cast<int>(int_length);
static_cast<size_t> ?

[Qiang(Joe) Xu, 2016-03-29 18:48:02]

Description was changed from

==========
dbus changes: add SetGlobalOutputChannelRemix support

BUG=404701
==========

to

==========
dbus changes: add SetGlobalOutputChannelRemix support

BUG=598791
==========

[Qiang(Joe) Xu, 2016-03-29 18:54:22]

On 2016/03/29 18:38:04, stevenjb wrote:
> cras_audio_client.cc should really have unit tests. It doesn't need to block
> this issue, but can you file and assign an issue for that (or prioritize one
if
> it exists)? Thanks.
> 
> https://codereview.chromium.org/1839943003/diff/1/dbus/message.cc
> File dbus/message.cc (right):
> 
> https://codereview.chromium.org/1839943003/diff/1/dbus/message.cc#newcode857
> dbus/message.cc:857: *length = static_cast<int>(int_length);
> static_cast<size_t> ?

Update the BUG number to 598791

[Qiang(Joe) Xu, 2016-03-29 19:04:02]

Description was changed from

==========
dbus changes: add SetGlobalOutputChannelRemix support

BUG=598791
==========

to

==========
dbus changes: add SetGlobalOutputChannelRemix support

BUG=598800
==========

[Qiang(Joe) Xu, 2016-03-29 19:19:09]

https://codereview.chromium.org/1839943003/diff/1/dbus/message.cc
File dbus/message.cc (right):

https://codereview.chromium.org/1839943003/diff/1/dbus/message.cc#newcode857
dbus/message.cc:857: *length = static_cast<int>(int_length);
On 2016/03/29 18:38:03, stevenjb wrote:
> static_cast<size_t> ?

Done.

[stevenjb, 2016-03-29 20:35:31]

https://codereview.chromium.org/1839943003/diff/20001/dbus/message.cc
File dbus/message.cc (right):

https://codereview.chromium.org/1839943003/diff/20001/dbus/message.cc#newcode857
dbus/message.cc:857: *length = static_cast<int>(int_length);
Fixed above but not here.

[Qiang(Joe) Xu, 2016-03-29 21:55:21]

On 2016/03/29 20:35:31, stevenjb wrote:
> https://codereview.chromium.org/1839943003/diff/20001/dbus/message.cc
> File dbus/message.cc (right):
> 
>
https://codereview.chromium.org/1839943003/diff/20001/dbus/message.cc#newcode857
> dbus/message.cc:857: *length = static_cast<int>(int_length);
> Fixed above but not here.

done. Sorry for missing this.

About cras_audio_client.cc unittest, I am a little puzzled about which kind of
tests we want. As cras_audio_client is just a wrapper to do the writer's work.
I talked with jennyz, we can test on caller level, like cras_audio_handler.

[Qiang(Joe) Xu, 2016-03-30 02:06:47]

On 2016/03/29 21:55:21, Qiang (Joe) Xu wrote:
> On 2016/03/29 20:35:31, stevenjb wrote:
> > https://codereview.chromium.org/1839943003/diff/20001/dbus/message.cc
> > File dbus/message.cc (right):
> > 
> >
>
https://codereview.chromium.org/1839943003/diff/20001/dbus/message.cc#newcode857
> > dbus/message.cc:857: *length = static_cast<int>(int_length);
> > Fixed above but not here.
> 
> done. Sorry for missing this.
> 
> About cras_audio_client.cc unittest, I am a little puzzled about which kind of
> tests we want. As cras_audio_client is just a wrapper to do the writer's work.
> I talked with jennyz, we can test on caller level, like cras_audio_handler.

Now, I made cras_audio_handler the caller of cras_audio_client's method, and add
new method unittest in cras_audio_handler_unittest.cc
Does this toper level caller's unittests cover cras_audio_client's unittests(and
there are not any).
I have cras_audio_handler_unittest.cc new changes ready. I should submit into
this CL, right? It is depended on this CL.

[stevenjb, 2016-03-30 16:45:34]

On 2016/03/29 21:55:21, Qiang (Joe) Xu wrote:
> On 2016/03/29 20:35:31, stevenjb wrote:
> > https://codereview.chromium.org/1839943003/diff/20001/dbus/message.cc
> > File dbus/message.cc (right):
> > 
> >
>
https://codereview.chromium.org/1839943003/diff/20001/dbus/message.cc#newcode857
> > dbus/message.cc:857: *length = static_cast<int>(int_length);
> > Fixed above but not here.
> 
> done. Sorry for missing this.
> 
> About cras_audio_client.cc unittest, I am a little puzzled about which kind of
> tests we want. As cras_audio_client is just a wrapper to do the writer's work.
> I talked with jennyz, we can test on caller level, like cras_audio_handler.

Setting up a dbus message is non trivial. It may be "just a wrapper" but it is
easy to, for example, accidentally cast something to the wrong type, or miss
invoking a callback. This is why many of the other handlers have unit tests
(e.g. see shill_service_client_unittest.cc). Most of the logic in this file is
pretty straightforward, but GetNodes and GetAudioNode in particular have some
logic to them that would be easy to get wrong and might not even be obviously
noticed. Adding a unit test based on the DBus spec as a sanity check can help
prevent errors and regressions.

Again, I wouldn't necessarily add that to this CL, but I do think we should file
an issue to add them.

[stevenjb, 2016-03-30 16:46:34]

lgtm

[Qiang(Joe) Xu, 2016-03-30 16:49:42]

warx@chromium.org changed reviewers:
- hychao@chromium.org

[Qiang(Joe) Xu, 2016-03-30 16:51:34]

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

[commit-bot: I haz the power, 2016-03-30 16:52:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839943003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839943003/40001

[commit-bot: I haz the power, 2016-03-30 17:06:30]

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

[commit-bot: I haz the power, 2016-03-30 17:06:30]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Qiang(Joe) Xu, 2016-03-30 20:53:04]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2016-03-30 20:53:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1839943003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1839943003/40001

[Qiang(Joe) Xu, 2016-03-30 20:55:27]

Description was changed from

==========
dbus changes: add SetGlobalOutputChannelRemix support

BUG=598800
==========

to

==========
dbus changes: add SetGlobalOutputChannelRemix support

cras_audio_client_unittest is still required

BUG=598800
==========

[Qiang(Joe) Xu, 2016-03-30 20:55:53]

Description was changed from

==========
dbus changes: add SetGlobalOutputChannelRemix support

cras_audio_client_unittest is still required

BUG=598800
==========

to

==========
dbus changes: add SetGlobalOutputChannelRemix support

cras_audio_client_unittest is still wanted

BUG=598800
==========

[commit-bot: I haz the power, 2016-03-30 21:27:50]

Description was changed from

==========
dbus changes: add SetGlobalOutputChannelRemix support

cras_audio_client_unittest is still wanted

BUG=598800
==========

to

==========
dbus changes: add SetGlobalOutputChannelRemix support

cras_audio_client_unittest is still wanted

BUG=598800
==========

[commit-bot: I haz the power, 2016-03-30 21:27:53]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-30 21:29:27]

Description was changed from

==========
dbus changes: add SetGlobalOutputChannelRemix support

cras_audio_client_unittest is still wanted

BUG=598800
==========

to

==========
dbus changes: add SetGlobalOutputChannelRemix support

cras_audio_client_unittest is still wanted

BUG=598800

Committed: https://crrev.com/a377c049175591928ea1507ef76242d913363011
Cr-Commit-Position: refs/heads/master@{#384097}
==========

[commit-bot: I haz the power, 2016-03-30 21:29:28]

Patchset 3 (id:??) landed as
https://crrev.com/a377c049175591928ea1507ef76242d913363011
Cr-Commit-Position: refs/heads/master@{#384097}