Add audible support to the copresence API.

Add audible support to the copresence API.

This CL currently depends on 3 other CL's before it can land so it might be a
good idea to hold off on the review till at least 2 of them have landed and
the only files in the CL are truly the ones that this CL adds.

R=ckehoe@chromium.org
BUG=390393
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288571

[rkc, 2014-08-08 08:22:31]

Ignoring the code in //components/copresence/rpc/, the rest of the code should
be reviewable. The main CL this was depending on has landed.

[xiyuan, 2014-08-08 20:40:54]

lgtm

https://codereview.chromium.org/453793002/diff/20001/components/copresence/co...
File components/copresence/copresence_client.cc (right):

https://codereview.chromium.org/453793002/diff/20001/components/copresence/co...
components/copresence/copresence_client.cc:32: AsWeakPtr(),
If comment at line 30 is true, this change is not needed.

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler.cc (right):

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.cc:137:
inaudible_transmit->end_time - base::Time::Now());
nit: wrap with {} since it is more than 1 line.

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.cc:140:
audible_transmit->end_time - base::Time::Now());
ditto

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.cc:154:
audible_receive->end_time - base::Time::Now());
ditto

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_list.h (right):

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_list.h:111: bool
use_audible_encoding_;
nit: const bool if |use_audible_encoding_| does not change.

[rkc, 2014-08-08 21:24:37]

https://codereview.chromium.org/453793002/diff/20001/components/copresence/co...
File components/copresence/copresence_client.cc (right):

https://codereview.chromium.org/453793002/diff/20001/components/copresence/co...
components/copresence/copresence_client.cc:32: AsWeakPtr(),
On 2014/08/08 20:40:53, xiyuan wrote:
> If comment at line 30 is true, this change is not needed.

This code will go away with the RPC handler rebase.

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler.cc (right):

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.cc:137:
inaudible_transmit->end_time - base::Time::Now());
On 2014/08/08 20:40:54, xiyuan wrote:
> nit: wrap with {} since it is more than 1 line.

Done.

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.cc:140:
audible_transmit->end_time - base::Time::Now());
On 2014/08/08 20:40:54, xiyuan wrote:
> ditto

Done.

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.cc:154:
audible_receive->end_time - base::Time::Now());
On 2014/08/08 20:40:54, xiyuan wrote:
> ditto

Done.

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_list.h (right):

https://codereview.chromium.org/453793002/diff/20001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_list.h:111: bool
use_audible_encoding_;
On 2014/08/08 20:40:54, xiyuan wrote:
> nit: const bool if |use_audible_encoding_| does not change.

Done.

[Charlie, 2014-08-08 22:24:15]

https://codereview.chromium.org/453793002/diff/40001/components/copresence/co...
File components/copresence/copresence_client.cc (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/co...
components/copresence/copresence_client.cc:32: AsWeakPtr(),
Huh?

https://codereview.chromium.org/453793002/diff/40001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler.cc (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
Instead of all this wordy, duplicate code, why not just have audible and
inaudible instances of the AudioDirectiveHandler? We can have two
AudioInputStreams, right?

https://codereview.chromium.org/453793002/diff/40001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler.h (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.h:59:
AudioDirectiveList directive_list_inaudible_;
Call them just inaudible_directives_ and audible_directives_

https://codereview.chromium.org/453793002/diff/40001/components/copresence/pr...
File components/copresence/proto/data.proto (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/pr...
components/copresence/proto/data.proto:90: // TODO(rkc): Horrible hack. Get rid
of this once the server tells us whether
Add the server bug number: 16849245

https://codereview.chromium.org/453793002/diff/40001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:166:
token_technology->set_medium(AUDIO_ULTRASOUND_PASSBAND);
token_technology->set_medium(ExtractIsAudibleStrategy(request) ?
    AUDIO_AUDIBLE_DTMF : AUDIO_ULTRASOUND_PASSBAND);

[rkc, 2014-08-09 06:58:54]

https://codereview.chromium.org/453793002/diff/40001/components/copresence/co...
File components/copresence/copresence_client.cc (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/co...
components/copresence/copresence_client.cc:32: AsWeakPtr(),
On 2014/08/08 22:24:14, Charlie wrote:
> Huh?

Will go away with the rebase once you land the RPC handler.

https://codereview.chromium.org/453793002/diff/40001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler.cc (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/08/08 22:24:14, Charlie wrote:
> Instead of all this wordy, duplicate code, why not just have audible and
> inaudible instances of the AudioDirectiveHandler? We can have two
> AudioInputStreams, right?

Two audio streams would record the exact same samples and send it to whispernet.
It would be suboptimal code by enough of a margin that we would care.

https://codereview.chromium.org/453793002/diff/40001/components/copresence/ha...
File components/copresence/handlers/audio/audio_directive_handler.h (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/ha...
components/copresence/handlers/audio/audio_directive_handler.h:59:
AudioDirectiveList directive_list_inaudible_;
On 2014/08/08 22:24:14, Charlie wrote:
> Call them just inaudible_directives_ and audible_directives_

That is an incorrect description. These aren't inaudible directives but rather
directives that use inaudible audio.

https://codereview.chromium.org/453793002/diff/40001/components/copresence/pr...
File components/copresence/proto/data.proto (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/pr...
components/copresence/proto/data.proto:90: // TODO(rkc): Horrible hack. Get rid
of this once the server tells us whether
On 2014/08/08 22:24:15, Charlie wrote:
> Add the server bug number: 16849245

Done.

https://codereview.chromium.org/453793002/diff/40001/components/copresence/rp...
File components/copresence/rpc/rpc_handler.cc (right):

https://codereview.chromium.org/453793002/diff/40001/components/copresence/rp...
components/copresence/rpc/rpc_handler.cc:166:
token_technology->set_medium(AUDIO_ULTRASOUND_PASSBAND);
On 2014/08/08 22:24:15, Charlie wrote:
> token_technology->set_medium(ExtractIsAudibleStrategy(request) ?
>     AUDIO_AUDIBLE_DTMF : AUDIO_ULTRASOUND_PASSBAND);

We set audible in addition to ultrasound, not instead of it.

[rkc, 2014-08-09 08:58:02]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-09 08:59:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rkc@chromium.org/453793002/100001

[commit-bot: I haz the power, 2014-08-09 11:00:45]

Change committed as 288571