Add the whispernet proxy.

Add the whispernet proxy.

To communicate with whispernet (which needs to be checked in as a pNacl binary),
the only way possible is for us to go through an extension. This code
implements the ChromeWhispernetClient, which uses the whispernet proxy compoent
extension. This extension is implemented on top of a private API that allows it
to communicate back and forth with the ChromeWhispernetClient.

The tests for this code exist only for the ChromeWhispernetClient since they
excersize the full flow back and forth from sending a request to encode a token
to receiving the samples, and then decoding samples and receiving the correct
token.

This is our temporary measure for M38 till we can find a better way to
communicate with the whispernet library.


Owners reviews requested:
xiyuan@ - For the overall review + the whispernet proxy code in
          c/b/r/whispernet_proxy
jochen@ - For adding components/copresence to c/b/DEPS
kalman@ - For the copresence_private API
ben@    - For adding the c/b/copresence directory and the OWNERS file + the
          addition to c/b/browser_resources.grd

R=ben@chromium.org, jochen@chromium.org, kalman@chromium.org, xiyuan@chromium.org
BUG=365493
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287976

[jochen (gone - plz use gerrit), 2014-07-31 14:12:49]

DEPS change lgtm

[not at google - send to devlin, 2014-07-31 15:24:18]

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.cc
(right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:19:
namespace util {
I don't think the namespace is necessary.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:26:
g_whispernet_client = new ChromeWhispernetClient(context);
this is a leak, use base::LazyInstance.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:36:
if (!util::GetWhispernetClient(browser_context()) ||
how can !util::GetWhispernetClient(browser_context()) be NULL? it looks like you
unconditionally create it.

likewise all callers.

and given you use this condition for every function, consider creating a
CopresencePrivateFunction class which contains a method like IsAvailable() or
whatever.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:40:
return true;
seems like this should be an error?

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:70:
params->samples.size());
this doesn't look safe to me, where is the guarantee that params->samples.size()
is <= the size of samples->channel(0)?

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:73:
params->token, samples);
should this take ownership of |samples|, and |samples| be a scoped_ptr with
.Pass()? refcounting should be a last resort.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:98:
LOG(ERROR) << "RKC: Got Init! context is: " << browser_context();
remove debugging?

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:107:
LOG(ERROR) << "RKC: Got Init and got profile!!";
remove debugging?

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.h
(right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.h:16:
class CopresencePrivateSendFoundFunction : public ChromeSyncExtensionFunction {
it's better to extend ChromeUIThreadExtension function and implement
ExtensionFunction::Run:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

see comments.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.h:18:
DECLARE_EXTENSION_FUNCTION("copresencePrivate.sendFound", UNKNOWN);
please add real UMA for this function.

https://codereview.chromium.org/438513002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/438513002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:238: "location": "component"
I'd prefer if you added a permission for this to force the whispernet proxy to
request something, rather than granting every component extension this API.

[rkc, 2014-07-31 15:47:32]

Just so everyone understands where this code is going, this code will eventually
look like,
https://codereview.chromium.org/428113002/

[xiyuan, 2014-07-31 19:22:23]

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

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:9: #include
"base/stl_util.h"
Is this used?

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:19: namespace copresence {
Why we use "copresence" namespace instead of an anonymous one?

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:31: const char
ChromeWhispernetClient::kWhispernetProxyExtensionId[] =
nit: // static

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:46: LOG(ERROR) << "RKC:
Initializing whispernet proxy client.";
nit: remove debugging log

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:63: LOG(ERROR) << "RKC:
Loading wproxy.";
nit: remove debugging log

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
File chrome/browser/copresence/chrome_whispernet_client.h (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.h:13: #include
"base/memory/scoped_ptr.h"
nit: not used.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.h:39: // WhispernetClient
overrides:
nit: move this down to just before "Initialize"

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.h:41: virtual
~ChromeWhispernetClient() OVERRIDE;
I don't think we need OVERRIDE for virtual dtor.

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

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:26: float*
buffer_view = reinterpret_cast<float*>(string_as_array(&buffer));
nit: #include "base/stl_util.h" for string_as_array

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:144: 
nit: private:

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:152: };
nit: DISALLOW_COPY_ASSIGN

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/com...
File chrome/browser/resources/component_extension_resources.grd (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/com...
chrome/browser/resources/component_extension_resources.grd:180: <include
name="IDR_WHISPERNET_PROXY_WHISPERNET_PROXY_PROXY_PEXE"
file="whispernet_proxy/whispernet_proxy_pnacl.pexe.js" type="BINDATA" />
Where is this "whispernet_proxy_pnacl.pexe.js" file?

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/background.html (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/background.html:2: <html
i18n-values="dir:textdirection;">
nit: i18n-values is not needed for extension background page.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/js/init.js (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/init.js:4: 
nit: 'use strict';

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/js/nacl.js (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/nacl.js:4: 
nit: 'use strict';

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

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:4: 
nit: 'use strict';

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:7: * @param {string}
bytes String containing the bytes we want to convert.
nit: Think |bytes| should be an Uint8Array (or we create a Uint8Array out of
it).

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:90:
WhisperEncoder.prototype.onAudioData = function(callback) {
nit: onAudioData -> setAudioDataCallback or some name more intuitive.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:179:
WhisperDecoder.prototype.onReceive = function(callback) {
similar here. onReceive -> setReceiveCallback.

https://codereview.chromium.org/438513002/diff/1/components/copresence/public...
File components/copresence/public/whispernet_client.h (right):

https://codereview.chromium.org/438513002/diff/1/components/copresence/public...
components/copresence/public/whispernet_client.h:13: #include
"base/memory/scoped_ptr.h"
not used?

https://codereview.chromium.org/438513002/diff/1/components/copresence/public...
components/copresence/public/whispernet_client.h:33: void(const std::string&,
const scoped_refptr<media::AudioBusRefCounted>&)>
nit: #include "base/memory/ref_counted.h"

[rkc, 2014-07-31 23:12:54]

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

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:9: #include
"base/stl_util.h"
On 2014/07/31 19:22:22, xiyuan wrote:
> Is this used?

Not anymore, neither is sstream, removed both.
Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:19: namespace copresence {
On 2014/07/31 19:22:22, xiyuan wrote:
> Why we use "copresence" namespace instead of an anonymous one?

To be able to simply delete this code and not need to change anything else when
we #include "components/public/copresence_constants.h"

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:31: const char
ChromeWhispernetClient::kWhispernetProxyExtensionId[] =
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: // static

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:46: LOG(ERROR) << "RKC:
Initializing whispernet proxy client.";
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: remove debugging log

Whoops.
Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.cc:63: LOG(ERROR) << "RKC:
Loading wproxy.";
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: remove debugging log

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
File chrome/browser/copresence/chrome_whispernet_client.h (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.h:13: #include
"base/memory/scoped_ptr.h"
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: not used.

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.h:39: // WhispernetClient
overrides:
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: move this down to just before "Initialize"

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client.h:41: virtual
~ChromeWhispernetClient() OVERRIDE;
On 2014/07/31 19:22:22, xiyuan wrote:
> I don't think we need OVERRIDE for virtual dtor.

Done.

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

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:26: float*
buffer_view = reinterpret_cast<float*>(string_as_array(&buffer));
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: #include "base/stl_util.h" for string_as_array

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:144: 
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: private:

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/copresence/ch...
chrome/browser/copresence/chrome_whispernet_client_browsertest.cc:152: };
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: DISALLOW_COPY_ASSIGN

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.cc
(right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:19:
namespace util {
On 2014/07/31 15:24:17, kalman wrote:
> I don't think the namespace is necessary.

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:26:
g_whispernet_client = new ChromeWhispernetClient(context);
On 2014/07/31 15:24:17, kalman wrote:
> this is a leak, use base::LazyInstance.

This code will never get exercised, removed. This function is _only in place
till we land the rest of the Copresence API code.
This code will be replaced by
https://codereview.chromium.org/428113002/diff/100001/chrome/browser/extensio...

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:36:
if (!util::GetWhispernetClient(browser_context()) ||
On 2014/07/31 15:24:17, kalman wrote:
> how can !util::GetWhispernetClient(browser_context()) be NULL? it looks like
you
> unconditionally create it.
> 
> likewise all callers.
> 
> and given you use this condition for every function, consider creating a
> CopresencePrivateFunction class which contains a method like IsAvailable() or
> whatever.

Answered in the comment above :) The current GetWhispernetClient() method is
only a placeholder.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:40:
return true;
On 2014/07/31 15:24:17, kalman wrote:
> seems like this should be an error?

If we don't have a callback registered right now, that just means that currently
we don't want to handle these tokens and we can safely drop them. We will have a
registered callback when we want to start receiving tokens. This is intended
behavior since we may simply unregister a callback (register an empty callback)
to stop receiving samples/tokens from Whispernet, but we don't want the
extension to get an exception when we do.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:70:
params->samples.size());
On 2014/07/31 15:24:17, kalman wrote:
> this doesn't look safe to me, where is the guarantee that
params->samples.size()
> is <= the size of samples->channel(0)?

The AudioBusRefCounted constructor ensures that we have enough memory in the
array we are memcpy'ing into. It allocate an aligned float array of the size
params->samples.size() / sizeof(float); i.e., the exact amount of memory that
params->samples.size() is.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:73:
params->token, samples);
On 2014/07/31 15:24:17, kalman wrote:
> should this take ownership of |samples|, and |samples| be a scoped_ptr with
> .Pass()? refcounting should be a last resort.

We store the samples in STL data structures, and in multiple of those. This
underwent discussion with //media owners before we settled on using RefCounted
(and added a RefCounted audio bus to the audio code, only for this purpose).

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:98:
LOG(ERROR) << "RKC: Got Init! context is: " << browser_context();
On 2014/07/31 15:24:17, kalman wrote:
> remove debugging?

Whoops.
Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:107:
LOG(ERROR) << "RKC: Got Init and got profile!!";
On 2014/07/31 15:24:17, kalman wrote:
> remove debugging?

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.h
(right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.h:16:
class CopresencePrivateSendFoundFunction : public ChromeSyncExtensionFunction {
On 2014/07/31 15:24:17, kalman wrote:
> it's better to extend ChromeUIThreadExtension function and implement
> ExtensionFunction::Run:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> 
> see comments.

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.h:18:
DECLARE_EXTENSION_FUNCTION("copresencePrivate.sendFound", UNKNOWN);
On 2014/07/31 15:24:17, kalman wrote:
> please add real UMA for this function.

Any particular reason to? We have absolutely no interest in tracking how often
this API is actually used. In fact, we want to get rid of this API pretty much
as soon as we can.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/com...
File chrome/browser/resources/component_extension_resources.grd (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/com...
chrome/browser/resources/component_extension_resources.grd:180: <include
name="IDR_WHISPERNET_PROXY_WHISPERNET_PROXY_PROXY_PEXE"
file="whispernet_proxy/whispernet_proxy_pnacl.pexe.js" type="BINDATA" />
On 2014/07/31 19:22:22, xiyuan wrote:
> Where is this "whispernet_proxy_pnacl.pexe.js" file?

I am having trouble with the presubmit on that file, I'll figure out how to add
it to the exclude list and upload it. The test will fail without it so this CL
can't land without it there anyway :)

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/background.html (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/background.html:2: <html
i18n-values="dir:textdirection;">
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: i18n-values is not needed for extension background page.

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/js/init.js (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/init.js:4: 
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: 'use strict';

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
File chrome/browser/resources/whispernet_proxy/js/nacl.js (right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/nacl.js:4: 
On 2014/07/31 19:22:22, xiyuan wrote:
> nit: 'use strict';

Done.

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

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:4: 
On 2014/07/31 19:22:23, xiyuan wrote:
> nit: 'use strict';

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:7: * @param {string}
bytes String containing the bytes we want to convert.
On 2014/07/31 19:22:23, xiyuan wrote:
> nit: Think |bytes| should be an Uint8Array (or we create a Uint8Array out of
> it).

This is the form this comes to us from Nacl. It is less than ideal, but I'll
look into making this better.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:90:
WhisperEncoder.prototype.onAudioData = function(callback) {
On 2014/07/31 19:22:23, xiyuan wrote:
> nit: onAudioData -> setAudioDataCallback or some name more intuitive.

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:179:
WhisperDecoder.prototype.onReceive = function(callback) {
On 2014/07/31 19:22:23, xiyuan wrote:
> similar here. onReceive -> setReceiveCallback.

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/438513002/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:238: "location": "component"
On 2014/07/31 15:24:17, kalman wrote:
> I'd prefer if you added a permission for this to force the whispernet proxy to
> request something, rather than granting every component extension this API.

Done.

https://codereview.chromium.org/438513002/diff/1/components/copresence/public...
File components/copresence/public/whispernet_client.h (right):

https://codereview.chromium.org/438513002/diff/1/components/copresence/public...
components/copresence/public/whispernet_client.h:13: #include
"base/memory/scoped_ptr.h"
On 2014/07/31 19:22:23, xiyuan wrote:
> not used?

Done.

https://codereview.chromium.org/438513002/diff/1/components/copresence/public...
components/copresence/public/whispernet_client.h:33: void(const std::string&,
const scoped_refptr<media::AudioBusRefCounted>&)>
On 2014/07/31 19:22:23, xiyuan wrote:
> nit: #include "base/memory/ref_counted.h"

Done.

[not at google - send to devlin, 2014-07-31 23:31:18]

thanks - almost done.

the ref counting thing isn't really in code that I own and it's not my place to
insist you change it. up to you I suppose, but refcounting adds complexity, and
it's unnecessary here.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.cc
(right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:36:
if (!util::GetWhispernetClient(browser_context()) ||
On 2014/07/31 23:12:52, Rahul Chaturvedi wrote:
> On 2014/07/31 15:24:17, kalman wrote:
> > how can !util::GetWhispernetClient(browser_context()) be NULL? it looks like
> you
> > unconditionally create it.
> > 
> > likewise all callers.
> > 
> > and given you use this condition for every function, consider creating a
> > CopresencePrivateFunction class which contains a method like IsAvailable()
or
> > whatever.
> 
> Answered in the comment above :) The current GetWhispernetClient() method is
> only a placeholder.

you haven't Done the second part of this comment.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:73:
params->token, samples);
On 2014/07/31 23:12:52, Rahul Chaturvedi wrote:
> On 2014/07/31 15:24:17, kalman wrote:
> > should this take ownership of |samples|, and |samples| be a scoped_ptr with
> > .Pass()? refcounting should be a last resort.
> 
> We store the samples in STL data structures, and in multiple of those. This
> underwent discussion with //media owners before we settled on using RefCounted
> (and added a RefCounted audio bus to the audio code, only for this purpose).

ref counting doesn't make good interfaces. It seems like the intention is most
definitely to pass ownership to this samples callback, so it should be a
scoped_ptr. what the whispernet client does with that pointer after it takes it
its own business.

the comment says "only use this when it's impossible to use scoped_ptr":

https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...

doesn't seem impossible to me.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.h
(right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.h:18:
DECLARE_EXTENSION_FUNCTION("copresencePrivate.sendFound", UNKNOWN);
On 2014/07/31 23:12:53, Rahul Chaturvedi wrote:
> On 2014/07/31 15:24:17, kalman wrote:
> > please add real UMA for this function.
> 
> Any particular reason to? We have absolutely no interest in tracking how often
> this API is actually used. In fact, we want to get rid of this API pretty much
> as soon as we can.

the argument that you want to get rid of this API is somewhat compelling, but
there are *no* other uses of the UNKNOWN value. every other private API adds an
UMA. it can't hurt.

https://codereview.chromium.org/438513002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.cc
(right):

https://codereview.chromium.org/438513002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:18:
// Once we add that code, this entire namespace will go away.
"this entire namespace will go away" - what namespace?

https://codereview.chromium.org/438513002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:35:
return RespondNow(ArgumentList(results_.Pass()));
given there are actually no results (and no results expected, as per the IDL)
you should probably just respond with NoArguments(). same everywhere else.

https://codereview.chromium.org/438513002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/438513002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:319: "location":
"component"
ideally this would be a whitelist, could you make sure you do that when the
extension(s) is/are in place?

[rkc, 2014-08-01 19:20:00]

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.cc
(right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:36:
if (!util::GetWhispernetClient(browser_context()) ||
On 2014/07/31 23:31:17, kalman wrote:
> On 2014/07/31 23:12:52, Rahul Chaturvedi wrote:
> > On 2014/07/31 15:24:17, kalman wrote:
> > > how can !util::GetWhispernetClient(browser_context()) be NULL? it looks
like
> > you
> > > unconditionally create it.
> > > 
> > > likewise all callers.
> > > 
> > > and given you use this condition for every function, consider creating a
> > > CopresencePrivateFunction class which contains a method like IsAvailable()
> or
> > > whatever.
> > 
> > Answered in the comment above :) The current GetWhispernetClient() method is
> > only a placeholder.
> 
> you haven't Done the second part of this comment.

Done.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:73:
params->token, samples);
On 2014/07/31 23:31:17, kalman wrote:
> On 2014/07/31 23:12:52, Rahul Chaturvedi wrote:
> > On 2014/07/31 15:24:17, kalman wrote:
> > > should this take ownership of |samples|, and |samples| be a scoped_ptr
with
> > > .Pass()? refcounting should be a last resort.
> > 
> > We store the samples in STL data structures, and in multiple of those. This
> > underwent discussion with //media owners before we settled on using
RefCounted
> > (and added a RefCounted audio bus to the audio code, only for this purpose).
> 
> ref counting doesn't make good interfaces. It seems like the intention is most
> definitely to pass ownership to this samples callback, so it should be a
> scoped_ptr. what the whispernet client does with that pointer after it takes
it
> its own business.
> 
> the comment says "only use this when it's impossible to use scoped_ptr":
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...
> 
> doesn't seem impossible to me.

Discussed offline.

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.h
(right):

https://codereview.chromium.org/438513002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/copresence_private/copresence_private_api.h:18:
DECLARE_EXTENSION_FUNCTION("copresencePrivate.sendFound", UNKNOWN);
On 2014/07/31 23:31:17, kalman wrote:
> On 2014/07/31 23:12:53, Rahul Chaturvedi wrote:
> > On 2014/07/31 15:24:17, kalman wrote:
> > > please add real UMA for this function.
> > 
> > Any particular reason to? We have absolutely no interest in tracking how
often
> > this API is actually used. In fact, we want to get rid of this API pretty
much
> > as soon as we can.
> 
> the argument that you want to get rid of this API is somewhat compelling, but
> there are *no* other uses of the UNKNOWN value. every other private API adds
an
> UMA. it can't hurt.

Done.

https://codereview.chromium.org/438513002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.cc
(right):

https://codereview.chromium.org/438513002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:18:
// Once we add that code, this entire namespace will go away.
On 2014/07/31 23:31:18, kalman wrote:
> "this entire namespace will go away" - what namespace?

This was referring to the util namespace, that I removed. Fixed the comment.

https://codereview.chromium.org/438513002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:35:
return RespondNow(ArgumentList(results_.Pass()));
On 2014/07/31 23:31:18, kalman wrote:
> given there are actually no results (and no results expected, as per the IDL)
> you should probably just respond with NoArguments(). same everywhere else.

Done.

https://codereview.chromium.org/438513002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/438513002/diff/20001/chrome/common/extensions...
chrome/common/extensions/api/_permission_features.json:319: "location":
"component"
On 2014/07/31 23:31:18, kalman wrote:
> ideally this would be a whitelist, could you make sure you do that when the
> extension(s) is/are in place?

Done.

[Alexei Svitkine (slow), 2014-08-01 19:22:04]

histograms lgtm % comment that should be addressed

https://codereview.chromium.org/438513002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/438513002/diff/40001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:915:
GCDPRIVATE_GETPREFETCHEDWIFINAMELIST,
Looks like the value is missing from histograms.xml - can you add it in?

[xiyuan, 2014-08-01 19:24:27]

lgtm

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

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:7: * @param {string}
bytes String containing the bytes we want to convert.
On 2014/07/31 23:12:53, Rahul Chaturvedi wrote:
> On 2014/07/31 19:22:23, xiyuan wrote:
> > nit: Think |bytes| should be an Uint8Array (or we create a Uint8Array out of
> > it).
> 
> This is the form this comes to us from Nacl. It is less than ideal, but I'll
> look into making this better.

Let's put a TODO for this. Ideally, nacl should send us an ArrayBuffer and we
could map that to a Uint8Array view here.

[not at google - send to devlin, 2014-08-01 21:11:59]

lgtm

https://codereview.chromium.org/438513002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.cc
(right):

https://codereview.chromium.org/438513002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:24:
copresence::WhispernetClient* GetWhispernetClient(
just inline the content of this function in the other GetWhispernetClient

https://codereview.chromium.org/438513002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.h
(right):

https://codereview.chromium.org/438513002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence_private/copresence_private_api.h:18:
copresence::WhispernetClient* GetWhispernetClient();
protected?

[rkc, 2014-08-01 21:21:13]

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

https://codereview.chromium.org/438513002/diff/1/chrome/browser/resources/whi...
chrome/browser/resources/whispernet_proxy/js/wrapper.js:7: * @param {string}
bytes String containing the bytes we want to convert.
On 2014/08/01 19:24:27, xiyuan wrote:
> On 2014/07/31 23:12:53, Rahul Chaturvedi wrote:
> > On 2014/07/31 19:22:23, xiyuan wrote:
> > > nit: Think |bytes| should be an Uint8Array (or we create a Uint8Array out
of
> > > it).
> > 
> > This is the form this comes to us from Nacl. It is less than ideal, but I'll
> > look into making this better.
> 
> Let's put a TODO for this. Ideally, nacl should send us an ArrayBuffer and we
> could map that to a Uint8Array view here.

Done.

https://codereview.chromium.org/438513002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.cc
(right):

https://codereview.chromium.org/438513002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence_private/copresence_private_api.cc:24:
copresence::WhispernetClient* GetWhispernetClient(
On 2014/08/01 21:11:58, kalman wrote:
> just inline the content of this function in the other GetWhispernetClient

Done.

https://codereview.chromium.org/438513002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/copresence_private/copresence_private_api.h
(right):

https://codereview.chromium.org/438513002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/copresence_private/copresence_private_api.h:18:
copresence::WhispernetClient* GetWhispernetClient();
On 2014/08/01 21:11:58, kalman wrote:
> protected?

Done.

https://codereview.chromium.org/438513002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_function_histogram_value.h (right):

https://codereview.chromium.org/438513002/diff/40001/extensions/browser/exten...
extensions/browser/extension_function_histogram_value.h:915:
GCDPRIVATE_GETPREFETCHEDWIFINAMELIST,
On 2014/08/01 19:22:04, Alexei Svitkine wrote:
> Looks like the value is missing from histograms.xml - can you add it in?

Done.

[rkc, 2014-08-05 19:48:25]

I've moved the binary assets for this CL out to
https://codereview.chromium.org/444773002/

Landing that first will allow us to run trybots on this one.

[Ben Goodger (Google), 2014-08-06 19:55:05]

lgtm

[rkc, 2014-08-06 20:24:24]

The CQ bit was checked by rkc@chromium.org

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

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

[commit-bot: I haz the power, 2014-08-06 21:23:27]

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/38158)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43365)
  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 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-06 21:25:32]

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

[commit-bot: I haz the power, 2014-08-06 21:25:33]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/38204)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[rkc, 2014-08-07 00:20:05]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-07 00:23:32]

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

[rkc, 2014-08-07 01:28:15]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-08-07 01:30:43]

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

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

Change committed as 287976

[architv07_gmail.com, 2015-05-24 09:07:10]

Whisper net proxy seems to eat up a lot of memory on chrome. Why is it 
required?

On Thursday, August 7, 2014 at 12:36:29 PM UTC+5:30, commi...@chromium.org 
wrote:
>
> Change committed as 287976 
>
> https://codereview.chromium.org/438513002/ 
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie, 2015-05-26 18:26:20]

On 2015/05/24 09:07:10, architv07_gmail.com wrote:
> Whisper net proxy seems to eat up a lot of memory on chrome. Why is it 
> required?
> 
> On Thursday, August 7, 2014 at 12:36:29 PM UTC+5:30,
mailto:commi...@chromium.org 
> wrote:
> >
> > Change committed as 287976 
> >
> > https://codereview.chromium.org/438513002/ 
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Whispernet is not loaded unless an extension explicitly calls it. There is a
known memory leak, but the whole Whispernet system is slated to be removed from
Chrome. A javascript-only replacement is in the works, which will not ship with
Chrome by default.