Host-side implementation of ARC audio bridge.

Host-side implementation of ARC audio bridge.

BUG=b:26933097
Committed: https://crrev.com/beb9aec573dee671247e92715d706fb54f588889
Cr-Commit-Position: refs/heads/master@{#383926}

[chinyue, 2016-03-22 09:54:22]

Description was changed from

==========
Host-side implementation of ARC audio bridge.

BUG=b:26933097
==========

to

==========
Host-side implementation of ARC audio bridge.

BUG=b:26933097
==========

[chinyue, 2016-03-22 09:54:22]

chinyue@chromium.org changed reviewers:
+ dgreid@chromium.org, elijahtaylor@chromium.org, lhchavez@chromium.org

[chinyue, 2016-03-22 09:55:08]

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

[elijahtaylor1, 2016-03-23 01:24:55]

https://codereview.chromium.org/1817093003/diff/20001/components/arc/common/a...
File components/arc/common/audio.mojom (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/common/a...
components/arc/common/audio.mojom:18: Init(AudioHost host_ptr);
if you don't have a meaningful Host interface, you shouldn't need to declare one
and have an Init method.  See process.mojom for an example of an Instance only
interface

[Luis Héctor Chávez, 2016-03-23 16:22:13]

https://codereview.chromium.org/1817093003/diff/20001/components/arc/arc_brid...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:410: CloseAppChannel();
CloseAudioChannel();

https://codereview.chromium.org/1817093003/diff/20001/components/arc/common/a...
File components/arc/common/audio.mojom (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/common/a...
components/arc/common/audio.mojom:8: enum AudioSwitch {
Do you plan on adding entries to this enum or will it stay as-is forever? If it
is the former, add an [Extensible] tag, otherwise if you add something in the
future, everything will break horribly.

[jennyz, 2016-03-23 23:50:42]

https://codereview.chromium.org/1817093003/diff/20001/components/arc/audio/ar...
File components/arc/audio/arc_audio_bridge.cc (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:54: }
GetDeviceFromId from CrasAudioHandler is more convenient here. It is private,
but you can change it to public.

https://codereview.chromium.org/1817093003/diff/20001/components/arc/audio/ar...
File components/arc/audio/arc_audio_bridge.h (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.h:31: chromeos::CrasAudioHandler*
cras_audio_handler_;
InitializeL cras_audio_handler_ = nullptr;

[chinyue, 2016-03-25 09:50:31]

https://codereview.chromium.org/1817093003/diff/20001/components/arc/arc_brid...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:410: CloseAppChannel();
On 2016/03/23 16:22:13, Luis Héctor Chávez wrote:
> CloseAudioChannel();

Done.

https://codereview.chromium.org/1817093003/diff/20001/components/arc/audio/ar...
File components/arc/audio/arc_audio_bridge.cc (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:54: }
On 2016/03/23 23:50:42, jennyz wrote:
> GetDeviceFromId from CrasAudioHandler is more convenient here. It is private,
> but you can change it to public.

Done.

https://codereview.chromium.org/1817093003/diff/20001/components/arc/audio/ar...
File components/arc/audio/arc_audio_bridge.h (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.h:31: chromeos::CrasAudioHandler*
cras_audio_handler_;
On 2016/03/23 23:50:42, jennyz wrote:
> InitializeL cras_audio_handler_ = nullptr;

Done.

https://codereview.chromium.org/1817093003/diff/20001/components/arc/common/a...
File components/arc/common/audio.mojom (right):

https://codereview.chromium.org/1817093003/diff/20001/components/arc/common/a...
components/arc/common/audio.mojom:8: enum AudioSwitch {
On 2016/03/23 16:22:13, Luis Héctor Chávez wrote:
> Do you plan on adding entries to this enum or will it stay as-is forever? If
it
> is the former, add an [Extensible] tag, otherwise if you add something in the
> future, everything will break horribly.

Done.

https://codereview.chromium.org/1817093003/diff/20001/components/arc/common/a...
components/arc/common/audio.mojom:18: Init(AudioHost host_ptr);
On 2016/03/23 01:24:55, elijahtaylor1 wrote:
> if you don't have a meaningful Host interface, you shouldn't need to declare
one
> and have an Init method.  See process.mojom for an example of an Instance only
> interface

Done.

[Luis Héctor Chávez, 2016-03-25 16:08:23]

components/arc LGTM with nits.

https://codereview.chromium.org/1817093003/diff/40001/components/arc/audio/ar...
File components/arc/audio/arc_audio_bridge.cc (right):

https://codereview.chromium.org/1817093003/diff/40001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:28: void
ArcAudioBridge::OnAudioInstanceReady() {
This method does not actually do anything useful now. I'd recommend to drop it.

https://codereview.chromium.org/1817093003/diff/40001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:58: int switch_state = 0;
nit: Use uint32_t for consistency.

https://codereview.chromium.org/1817093003/diff/40001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:60: switch_state |= (1 <<
(int)AudioSwitch::SW_HEADPHONE_INSERT);
nit: static_cast<uint32_t>(...) or uint32_t{...}; same below.

[jennyz, 2016-03-25 21:03:12]

lgtm

[chinyue, 2016-03-28 08:23:17]

https://codereview.chromium.org/1817093003/diff/40001/components/arc/audio/ar...
File components/arc/audio/arc_audio_bridge.cc (right):

https://codereview.chromium.org/1817093003/diff/40001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:28: void
ArcAudioBridge::OnAudioInstanceReady() {
On 2016/03/25 16:08:23, Luis Héctor Chávez wrote:
> This method does not actually do anything useful now. I'd recommend to drop
it.

Done.

https://codereview.chromium.org/1817093003/diff/40001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:58: int switch_state = 0;
On 2016/03/25 16:08:23, Luis Héctor Chávez wrote:
> nit: Use uint32_t for consistency.

Done.

https://codereview.chromium.org/1817093003/diff/40001/components/arc/audio/ar...
components/arc/audio/arc_audio_bridge.cc:60: switch_state |= (1 <<
(int)AudioSwitch::SW_HEADPHONE_INSERT);
On 2016/03/25 16:08:23, Luis Héctor Chávez wrote:
> nit: static_cast<uint32_t>(...) or uint32_t{...}; same below.

Done.

[chinyue, 2016-03-28 08:27:25]

The CQ bit was checked by chinyue@chromium.org

[chinyue, 2016-03-28 08:27:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, jennyz@chromium.org
Link to the patchset: https://codereview.chromium.org/1817093003/#ps60001
(title: "Remove unused override and update data type.")

[commit-bot: I haz the power, 2016-03-28 08:27:35]

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

[commit-bot: I haz the power, 2016-03-28 08:35:52]

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

[commit-bot: I haz the power, 2016-03-28 08:35:53]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chinyue, 2016-03-28 14:56:24]

chinyue@chromium.org changed reviewers:
+ kerrnel@chromium.org

[chinyue, 2016-03-28 14:57:32]

On 2016/03/28 08:35:53, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    components/arc/common/arc_bridge.mojom
    components/arc/common/audio.mojom

Added kerrnel@ for components/arc/common changes. Thanks!

[Greg K, 2016-03-29 19:21:13]

On 2016/03/28 14:57:32, chinyue wrote:
> On 2016/03/28 08:35:53, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     components/arc/common/arc_bridge.mojom
>     components/arc/common/audio.mojom
> 
> Added kerrnel@ for components/arc/common changes. Thanks!

Providing OWNERS LGTM of the mojo interfaces files.

[Greg K, 2016-03-29 19:50:13]

On 2016/03/29 19:21:13, Greg Kerr wrote:
> On 2016/03/28 14:57:32, chinyue wrote:
> > On 2016/03/28 08:35:53, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for these files:
> >     components/arc/common/arc_bridge.mojom
> >     components/arc/common/audio.mojom
> > 
> > Added kerrnel@ for components/arc/common changes. Thanks!
> 
> Providing OWNERS LGTM of the mojo interfaces files.

Actually, not LGTM for now because I have some questions about the mojo enum.

[Greg K, 2016-03-29 20:02:44]

On 2016/03/29 19:50:13, Greg Kerr wrote:
> On 2016/03/29 19:21:13, Greg Kerr wrote:
> > On 2016/03/28 14:57:32, chinyue wrote:
> > > On 2016/03/28 08:35:53, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > > 
> > > ** Presubmit ERRORS **
> > > Missing LGTM from an OWNER for these files:
> > >     components/arc/common/arc_bridge.mojom
> > >     components/arc/common/audio.mojom
> > > 
> > > Added kerrnel@ for components/arc/common changes. Thanks!
> > 
> > Providing OWNERS LGTM of the mojo interfaces files.
> 
> Actually, not LGTM for now because I have some questions about the mojo enum.

Ok, sorry for the confusion. Since the enum is used as a bitmask, it's use in
the .mojom files seems fine. LGTM.

[chinyue, 2016-03-30 06:51:45]

The CQ bit was checked by chinyue@chromium.org

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

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

[commit-bot: I haz the power, 2016-03-30 07:49:28]

Description was changed from

==========
Host-side implementation of ARC audio bridge.

BUG=b:26933097
==========

to

==========
Host-side implementation of ARC audio bridge.

BUG=b:26933097
==========

[commit-bot: I haz the power, 2016-03-30 07:49:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-30 07:51:38]

Description was changed from

==========
Host-side implementation of ARC audio bridge.

BUG=b:26933097
==========

to

==========
Host-side implementation of ARC audio bridge.

BUG=b:26933097

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

[commit-bot: I haz the power, 2016-03-30 07:51:39]

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