Add Audible support to the whispernet client.

Add Audible support to the whispernet client.

This CL adds the ability to do encoding/decoding of tokens in the audible range.
It also simplifies the parameters we send in to the whispernet pnacl wrapper.
All the parameters removed are now picked up as defaults in the wrapper itself.
There is literally no reason we'd ever touch those defaults hence it made no
sense to send them hardcoded from wrapper.js.

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

[Charlie, 2014-08-07 15:46:47]

Cool!

https://codereview.chromium.org/444373004/diff/1/chrome/browser/copresence/ch...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444373004/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:66: const
std::string kZeroes = audible ? "MDAwMDA=" : "MDAwMDAw";
Move these to two static const char[]:

const std::string encoded_token = audible ? kaudibleZeroes : kUltrasoundZeroes;

Also add a comment explaining why the encodings are different.

https://codereview.chromium.org/444373004/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/js/wrapper.js (left):

https://codereview.chromium.org/444373004/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:47: var symbolCoder =
{};
What happened to all of these parameters?

[rkc, 2014-08-07 16:30:42]

https://codereview.chromium.org/444373004/diff/1/chrome/browser/copresence/ch...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444373004/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:66: const
std::string kZeroes = audible ? "MDAwMDA=" : "MDAwMDAw";
On 2014/08/07 15:46:47, Charlie wrote:
> Move these to two static const char[]:
> 
> const std::string encoded_token = audible ? kaudibleZeroes :
kUltrasoundZeroes;
> 
> Also add a comment explaining why the encodings are different.

Adding 3 constants seems overkill for one value. The encodings are different
because the value they are encoding are different. Isn't that expected?

https://codereview.chromium.org/444373004/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/js/wrapper.js (left):

https://codereview.chromium.org/444373004/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:47: var symbolCoder =
{};
On 2014/08/07 15:46:47, Charlie wrote:
> What happened to all of these parameters?

The defaults are embedded in the nacl binary. There was no reason to send them
from the JS since these aren't values we'd ever change in Chrome.

[Charlie, 2014-08-07 16:43:54]

https://codereview.chromium.org/444373004/diff/1/chrome/browser/copresence/ch...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444373004/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:66: const
std::string kZeroes = audible ? "MDAwMDA=" : "MDAwMDAw";
The point of short constants is to avoid typos and copy/paste errors. If they
were used in just this one test, it would make more sense. But you already have
two, and one can imagine adding more.

Didn't notice the differing number of zeros. Can you make the tokens more
different from each other? Like "00000000" vs. "0000", or "ultrasound" vs.
"audible"? What are the restrictions on tokens?

On 2014/08/07 16:30:41, Rahul Chaturvedi wrote:
> On 2014/08/07 15:46:47, Charlie wrote:
> > Move these to two static const char[]:
> > 
> > const std::string encoded_token = audible ? kaudibleZeroes :
> kUltrasoundZeroes;
> > 
> > Also add a comment explaining why the encodings are different.
> 
> Adding 3 constants seems overkill for one value. The encodings are different
> because the value they are encoding are different. Isn't that expected?

[rkc, 2014-08-07 17:17:02]

Also added changes to the client itself to now pick up the constants from the
copresence component.

https://codereview.chromium.org/444373004/diff/1/chrome/browser/copresence/ch...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444373004/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:66: const
std::string kZeroes = audible ? "MDAwMDA=" : "MDAwMDAw";
Added constants, explained token length in comment.
Done.

On 2014/08/07 16:43:54, Charlie wrote:
> The point of short constants is to avoid typos and copy/paste errors. If they
> were used in just this one test, it would make more sense. But you already
have
> two, and one can imagine adding more.
> 
> Didn't notice the differing number of zeros. Can you make the tokens more
> different from each other? Like "00000000" vs. "0000", or "ultrasound" vs.
> "audible"? What are the restrictions on tokens?
> 
> On 2014/08/07 16:30:41, Rahul Chaturvedi wrote:
> > On 2014/08/07 15:46:47, Charlie wrote:
> > > Move these to two static const char[]:
> > > 
> > > const std::string encoded_token = audible ? kaudibleZeroes :
> > kUltrasoundZeroes;
> > > 
> > > Also add a comment explaining why the encodings are different.
> > 
> > Adding 3 constants seems overkill for one value. The encodings are different
> > because the value they are encoding are different. Isn't that expected?
>

[Charlie, 2014-08-07 17:28:54]

LGTM with a nit.

https://codereview.chromium.org/444373004/diff/20001/chrome/browser/copresenc...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444373004/diff/20001/chrome/browser/copresenc...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:91:
expected_token_ = kZeros;
nit: You could also replace kZeros with calls to this:

std::string getToken(bool audible) {
  return audible ? kFiveZeros : kSixZeros;
}

Seems like less duplication with no loss of clarity.

[rkc, 2014-08-07 17:36:58]

https://codereview.chromium.org/444373004/diff/20001/chrome/browser/copresenc...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444373004/diff/20001/chrome/browser/copresenc...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:91:
expected_token_ = kZeros;
On 2014/08/07 17:28:53, Charlie wrote:
> nit: You could also replace kZeros with calls to this:
> 
> std::string getToken(bool audible) {
>   return audible ? kFiveZeros : kSixZeros;
> }
> 
> Seems like less duplication with no loss of clarity.

Done.

[rkc, 2014-08-07 17:41:21]

The CQ bit was checked by rkc@chromium.org

[Charlie, 2014-08-07 17:43:52]

LGTM

https://codereview.chromium.org/444373004/diff/20001/chrome/browser/copresenc...
File chrome/browser/copresence/chrome_whispernet_client_browsertest.cc (right):

https://codereview.chromium.org/444373004/diff/20001/chrome/browser/copresenc...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:91:
expected_token_ = kZeros;
I was thinking you could use getToken() everywhere and just not have kZeroes.
But this is fine too.

On 2014/08/07 17:36:58, Rahul Chaturvedi wrote:
> On 2014/08/07 17:28:53, Charlie wrote:
> > nit: You could also replace kZeros with calls to this:
> > 
> > std::string getToken(bool audible) {
> >   return audible ? kFiveZeros : kSixZeros;
> > }
> > 
> > Seems like less duplication with no loss of clarity.
> 
> Done.

[commit-bot: I haz the power, 2014-08-07 17:45:02]

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

[commit-bot: I haz the power, 2014-08-07 17:45:04]

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

[commit-bot: I haz the power, 2014-08-07 17:45:12]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[xiyuan, 2014-08-07 17:56:57]

lgtm

[rkc, 2014-08-07 17:58:25]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-07 18:09:19]

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

[rkc, 2014-08-07 22:58:22]

The CQ bit was unchecked by rkc@chromium.org

[rkc, 2014-08-08 01:32:51]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-08 01:40:29]

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

[commit-bot: I haz the power, 2014-08-08 06:00:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[commit-bot: I haz the power, 2014-08-08 06:06:19]

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

[commit-bot: I haz the power, 2014-08-08 06:06:20]

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

[rkc, 2014-08-08 06:40:00]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-08 06:41:26]

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

[commit-bot: I haz the power, 2014-08-08 06:48:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38990)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/44222)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 06:51:17]

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

[commit-bot: I haz the power, 2014-08-08 06:51:19]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38992)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rkc, 2014-08-08 07:09:05]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-08 07:11:33]

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

[rkc, 2014-08-08 08:14:06]

Adding kalman@ for OWNERs review of c/c/e/a/copresence_private.idl

[commit-bot: I haz the power, 2014-08-08 10:20:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-08 10:23:17]

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

[commit-bot: I haz the power, 2014-08-08 10:23:18]

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

[not at google - send to devlin, 2014-08-08 13:50:00]

lgtm

[rkc, 2014-08-08 16:28:09]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-08 16:29:13]

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

[commit-bot: I haz the power, 2014-08-08 16:33:43]

Change committed as 288374