Prototype for copresenceSockets.

Prototype for copresenceSockets.
This is the prototype implementation of the chrome.copresenceSocket API. This
CL adds the API and the copresence_sockets component (which is a currently a
thin wrapper around the bluetooth APIs).

Only the receive parts of the code have been implemented with this CL.

Reviews requested,
rockot@ - Extension parts
armansito@ - BT parts
caitkp@ - Owners review for components

BUG=418615
Committed: https://crrev.com/a3e8914682cb7e046b5a76e93c053f070ea25a0b
Cr-Commit-Position: refs/heads/master@{#298577}

[rkc, 2014-09-30 00:53:37]

rkc@chromium.org changed reviewers:
+ armansito@chromium.org, caitkp@chromium.org, rockot@chromium.org

[rkc, 2014-09-30 01:03:25]

This is just the prototype implementation. As we decide the details of the
protocol between other platforms, I expect this to change quite a bit.

The purpose of this CL is to allow some of our users who are waiting for this
API, to start using it on the dev channel.

[Ken Rockot(use gerrit already), 2014-09-30 17:26:05]

lgtm with some optional nits.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets.gypi (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets.gypi:19: # Note: sources list duplicated in GN
build.
It's conventional to include a reference to the relevant GN build rule in a
comment here.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:31: return
device::BluetoothUUID(RandomHexBytes(8) + "-" + RandomHexBytes(4) +
nit: Use StringPrintf instead?

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:42: // device::PairingDelegate
overrides:
nit: device::BluetoothDevice::PairingDelegate

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:79:
base::Bind(&CopresencePeer::OnGetAdapter, AsWeakPtr()));
Here and elsewhere it looks like you only use a WeakPtr<T> inside of T's
implementation. In this case composing a WeakPtrFactory into T is preferred over
inheriting SupportsWeakPtr<T>.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h
(right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
nit: "media" or "transports" instead of "mediums"?

https://codereview.chromium.org/610633002/diff/40001/extensions/browser/api/c...
File extensions/browser/api/copresence_socket/copresence_socket_api.cc (right):

https://codereview.chromium.org/610633002/diff/40001/extensions/browser/api/c...
extensions/browser/api/copresence_socket/copresence_socket_api.cc:21: // Takes
ownership of peer.
Can you document this by using a scoped_ptr instead? Here and other places.

https://codereview.chromium.org/610633002/diff/40001/extensions/browser/api/c...
File extensions/browser/api/copresence_socket/copresence_socket_api.h (right):

https://codereview.chromium.org/610633002/diff/40001/extensions/browser/api/c...
extensions/browser/api/copresence_socket/copresence_socket_api.h:39: virtual
ExtensionFunction::ResponseAction Run() override;
Hurray for more code using the modern ExtensionFunction interface!

https://codereview.chromium.org/610633002/diff/40001/extensions/common/api/_a...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/610633002/diff/40001/extensions/common/api/_a...
extensions/common/api/_api_features.json:72: "copresenceSocket": {
I realize the restriction exists on the corpresence permission already, but can
you please add a "channel": "dev" restriction here as well just to make it more
explicit?

[Cait (Slow), 2014-09-30 17:56:02]

A few comments inline.

Also, consider adding a couple basic tests to make sure all callbacks are
getting called properly (and to help document how the pieces fit together).

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.cc
(right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.cc:48:
void CopresenceSocketBluetooth::OnSendError(const std::string& message) {
nit: blank line in between function definitions.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.cc:64:
device::BluetoothSocket::ErrorReason reason,
Looks like the |reason| param is not used?

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h
(right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h:26:
public base::SupportsWeakPtr<CopresenceSocketBluetooth> {
Use WeakPtrFactory here instead, too?

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets/public/copresence_peer.h (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:38: typedef
base::Callback<void(CopresenceSocket* socket)> AcceptCallback;
Pass a scoped_ptr instead?

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:69:
device::BluetoothDevice::PairingDelegate* delegate_;
scoped_ptr<device::BluetoothDevice::PairingDelegate> to avoid deleting manually
in the dtor?

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets/public/copresence_socket.h (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:28: typedef
base::Callback<void(const std::string&)> ReceiveCallback;
nit: typedef declarations should come before f'ns.

[armansito, 2014-09-30 20:37:58]

First round of comments.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:68:
accept_callback_(accept_callback),
It looks like |accept_callback_| and |create_callback_| get called as part of
the instantiation sequence, so wouldn't it be better to just pass them on as
callback parameters instead of caching them as members?

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:107: service_uuid_ =
GenerateRandomUuid();
I'm not sure about using a random UUID here. If the UUID is random, how will the
remote end know to connect to this UUID? Instead, you should probably establish
a special UUID for the Bluetooth service that you're building and consistently
use that. That UUID would basically represent the "Copresence Bluetooth Socket
Service".

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h
(right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h:26:
public base::SupportsWeakPtr<CopresenceSocketBluetooth> {
On 2014/09/30 17:56:01, Cait Phillips wrote:
> Use WeakPtrFactory here instead, too?

Agreed, unless weak pointers to this class are needed externally, use an
internal WeakPtrFactory.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets/public/copresence_peer.h (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:35: typedef
base::Callback<void(const std::string&)> CreatePeerCallback;
nit: Add newline after the typedefs.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:51: virtual
~CopresencePeer();
nit: Move destructor below the constructor.

[rkc, 2014-10-01 19:08:22]

rkc@chromium.org changed reviewers:
+ isherman@chromium.org, rsleevi@chromium.org

[rkc, 2014-10-01 19:08:24]

Adding rsleevi for adding //net deps and isherman for
extension_function_histogram.h

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets.gypi (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets.gypi:19: # Note: sources list duplicated in GN
build.
On 2014/09/30 17:26:04, Ken Rockot wrote:
> It's conventional to include a reference to the relevant GN build rule in a
> comment here.

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:31: return
device::BluetoothUUID(RandomHexBytes(8) + "-" + RandomHexBytes(4) +
On 2014/09/30 17:26:04, Ken Rockot wrote:
> nit: Use StringPrintf instead?

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:42: // device::PairingDelegate
overrides:
On 2014/09/30 17:26:04, Ken Rockot wrote:
> nit: device::BluetoothDevice::PairingDelegate

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:68:
accept_callback_(accept_callback),
On 2014/09/30 20:37:58, armansito wrote:
> It looks like |accept_callback_| and |create_callback_| get called as part of
> the instantiation sequence, so wouldn't it be better to just pass them on as
> callback parameters instead of caching them as members?

I had it that way earlier, but it seemed simpler to simply cache them in the
object, than to bind them to every single call made during instantiation (and
having to add 2 parameters to every "OnXXX" method.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:79:
base::Bind(&CopresencePeer::OnGetAdapter, AsWeakPtr()));
On 2014/09/30 17:26:04, Ken Rockot wrote:
> Here and elsewhere it looks like you only use a WeakPtr<T> inside of T's
> implementation. In this case composing a WeakPtrFactory into T is preferred
over
> inheriting SupportsWeakPtr<T>.

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:107: service_uuid_ =
GenerateRandomUuid();
On 2014/09/30 20:37:58, armansito wrote:
> I'm not sure about using a random UUID here. If the UUID is random, how will
the
> remote end know to connect to this UUID? Instead, you should probably
establish
> a special UUID for the Bluetooth service that you're building and consistently
> use that. That UUID would basically represent the "Copresence Bluetooth Socket
> Service".

This is how the Android side is currently implemented. I believe there are some
security reasons for this. I'll speak with the Android team and see if we can
use a fixed UUID instead. Once their side also does that, I'll update this code.

We need to pass the locator data to the other side anyway for communications to
get setup; the service UUID also goes part of that data.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.cc
(right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.cc:48:
void CopresenceSocketBluetooth::OnSendError(const std::string& message) {
On 2014/09/30 17:56:01, Cait Phillips wrote:
> nit: blank line in between function definitions.

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.cc:64:
device::BluetoothSocket::ErrorReason reason,
On 2014/09/30 17:56:01, Cait Phillips wrote:
> Looks like the |reason| param is not used?

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h
(right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/09/30 17:26:04, Ken Rockot wrote:
> nit: "media" or "transports" instead of "mediums"?

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h:26:
public base::SupportsWeakPtr<CopresenceSocketBluetooth> {
On 2014/09/30 17:56:01, Cait Phillips wrote:
> Use WeakPtrFactory here instead, too?

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/mediums/bluetooth/copresence_socket_bluetooth.h:26:
public base::SupportsWeakPtr<CopresenceSocketBluetooth> {
On 2014/09/30 20:37:58, armansito wrote:
> On 2014/09/30 17:56:01, Cait Phillips wrote:
> > Use WeakPtrFactory here instead, too?
> 
> Agreed, unless weak pointers to this class are needed externally, use an
> internal WeakPtrFactory.

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets/public/copresence_peer.h (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:35: typedef
base::Callback<void(const std::string&)> CreatePeerCallback;
On 2014/09/30 20:37:58, armansito wrote:
> nit: Add newline after the typedefs.

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:38: typedef
base::Callback<void(CopresenceSocket* socket)> AcceptCallback;
On 2014/09/30 17:56:01, Cait Phillips wrote:
> Pass a scoped_ptr instead?

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:51: virtual
~CopresencePeer();
On 2014/09/30 20:37:58, armansito wrote:
> nit: Move destructor below the constructor.

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:69:
device::BluetoothDevice::PairingDelegate* delegate_;
On 2014/09/30 17:56:01, Cait Phillips wrote:
> scoped_ptr<device::BluetoothDevice::PairingDelegate> to avoid deleting
manually
> in the dtor?

Done.

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
File components/copresence_sockets/public/copresence_socket.h (right):

https://codereview.chromium.org/610633002/diff/40001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:28: typedef
base::Callback<void(const std::string&)> ReceiveCallback;
On 2014/09/30 17:56:01, Cait Phillips wrote:
> nit: typedef declarations should come before f'ns.

Done.

https://codereview.chromium.org/610633002/diff/40001/extensions/browser/api/c...
File extensions/browser/api/copresence_socket/copresence_socket_api.cc (right):

https://codereview.chromium.org/610633002/diff/40001/extensions/browser/api/c...
extensions/browser/api/copresence_socket/copresence_socket_api.cc:21: // Takes
ownership of peer.
On 2014/09/30 17:26:04, Ken Rockot wrote:
> Can you document this by using a scoped_ptr instead? Here and other places.

Done.

https://codereview.chromium.org/610633002/diff/40001/extensions/browser/api/c...
File extensions/browser/api/copresence_socket/copresence_socket_api.h (right):

https://codereview.chromium.org/610633002/diff/40001/extensions/browser/api/c...
extensions/browser/api/copresence_socket/copresence_socket_api.h:39: virtual
ExtensionFunction::ResponseAction Run() override;
On 2014/09/30 17:26:05, Ken Rockot wrote:
> Hurray for more code using the modern ExtensionFunction interface!

Acknowledged.

https://codereview.chromium.org/610633002/diff/40001/extensions/common/api/_a...
File extensions/common/api/_api_features.json (right):

https://codereview.chromium.org/610633002/diff/40001/extensions/common/api/_a...
extensions/common/api/_api_features.json:72: "copresenceSocket": {
On 2014/09/30 17:26:05, Ken Rockot wrote:
> I realize the restriction exists on the corpresence permission already, but
can
> you please add a "channel": "dev" restriction here as well just to make it
more
> explicit?

Done.

[Ilya Sherman, 2014-10-01 19:27:38]

Please run the script to update histograms.xml.

[Ryan Sleevi, 2014-10-01 19:47:26]

As this is a new socket API (effectively), I have attempted to carefully review
this for design patterns and anti-patterns, and have found several that, based
on our past experiences with the Extensions Sockets API and the Cast API, I
believe should be corrected before landing.

I would also love to encourage you to reach out to net-dev@ before launching
this to request a formal review, much like security reviews, to see what other
concerns may exist. We're happy to share our experience and expertise in a way
that will hopefully allow you and your team to launch a great API on the first
launch.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:32:
base::RandGenerator(UINT16_MAX + UINT32_MAX)));
This sets my C++ rules a tingly in making sure that integer promotion rules
"work as expected", and you're not instead getting silent overflow behaviours
that would cap this at UINT16_MAX.

That is, the risk being that UINT16_MAX is scoped to a uint16_t (which is legal
according to 7.18.2, p2 of C99, although AFAIK, no one does this)

I haven't checked C11, but it's enough of a tingly sense to wonder if you
shouldn't just add a static const uint16_t = static_cast<...> + static_cast<...>

Or ask jyasskin

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:35: class
DefaultApprovalDelegate
Document

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:62: // Public methods.
unnecessary

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:138:
create_callback_.Run(std::string());
I find this pattern a little weird (with respect to the four exit points spread
across three methods - lines 100, 124, 129, 138)

It also creates confusion as to what can happen in create_callback_, and what it
can do to mutate this object (for example, line 130 makes it categorically
unsafe to mutate this object, but I don't know if that's easy to reason about)

I wonder if it's possible to rethink this in terms of an explicit state machine,
which makes it easier to reason about the transitions, and also lets you move
the invocation of create_callback_ to the terminal edge of the state. This is a
pattern we use exclusively in //net, and we've found when teams avoid this
pattern (Cast and Extensions), they tend to introduce new crashes and bugs.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File components/copresence_sockets/public/copresence_peer.h (right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:48: AcceptCallback
accept_callback);
STYLE: Callbacks should be passed as const-ref

The ONLY exceptions to the const-ref rules are PODs and scoped_ptr<>.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:57: void
OnGetAdapter(scoped_refptr<device::BluetoothAdapter> adapter);
STYLE: scoped_refptr<>'s should be passed as const-ref, as it avoids refcount
churn.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File components/copresence_sockets/public/copresence_socket.h (right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:16: // connection
request. This class is currently just a pure interface.
LAYERING: Document how to use this class, not how it is used. THis is a public
API, comments about who does what don't belong (unless it's a delegate and
you're documenting how/under what conditions the delegate is invoked)

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:20: typedef
base::Callback<void(const std::string&)> ReceiveCallback;
DESIGN: std::string vs base::StringPiece

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:23: virtual
~CopresenceSocket() {}
DESIGN: Is a public destructor correct for this class?

Only do so if ownership of a CopresenceSocket implementation is directly passed
into your interface. Otherwise, prefer a protected destructor, to ensure it's
the owning class that is responsible for managing the lifetime of this object.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:26: // the method
returns false.
STYLE: Pronouns in comments considered harmful -
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M

// Send |data| on this socket. If the socket was unable to send data, returns
false.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:28: virtual bool
Send(const std::string& data) = 0;
DESIGN: Don't use std::string for buffers of data.

Nominally, net::IOBuffer is the canonical form for this within //net and the
overall resource loading stack. Alternatively, base::StringPiece allows for
zero-copy when dealing with alternative forms (e.g. an std::vector<>)

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:28: virtual bool
Send(const std::string& data) = 0;
DESIGN: Synchronous methods for sockets are ALWAYS an anti-pattern and STRONGLY
discouraged as they have been a LARGE source of bugs and latency.

Your socket also assumes all data will always be sent, which seems like a poor
decision.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:29: virtual void
Receive(ReceiveCallback callback) = 0;
STYLE: Pass as const-ref.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:29: virtual void
Receive(ReceiveCallback callback) = 0;
DESIGN: Let the caller specify how much data they're interested in?

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.cc
(right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.cc:41:
4096,
DESIGN: Magic Constants are bad.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.h
(right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.h:26:
public base::SupportsWeakPtr<CopresenceSocketBluetooth> {
DESIGN: DO NOT expose SupportsWeakPtr on new objects.

It is a design flaw to expose the WeakPtr semantics of an object to external
consumers. This is a NOT LGTM until this can be resolved.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.h:28:
CopresenceSocketBluetooth(scoped_refptr<device::BluetoothSocket> socket);
STYLE: const-ref

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.h:39:
void OnReceive(int size, scoped_refptr<net::IOBuffer> io_buffer);
STYLE: const-ref

https://codereview.chromium.org/610633002/diff/60001/extensions/common/api/co...
File extensions/common/api/copresence_socket.idl (right):

https://codereview.chromium.org/610633002/diff/60001/extensions/common/api/co...
extensions/common/api/copresence_socket.idl:10: // Only use the internet as the
transport.
s/internet/Internet

[rkc, 2014-10-01 22:38:47]

rkc@chromium.org changed reviewers:
+ brettw@chromium.org

[rkc, 2014-10-01 22:38:49]

Edited histograms.xml by hand. Where is the script that does this automatically?

(Adding brettw@ for owners review to add a DEP on //base)

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:32:
base::RandGenerator(UINT16_MAX + UINT32_MAX)));
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> This sets my C++ rules a tingly in making sure that integer promotion rules
> "work as expected", and you're not instead getting silent overflow behaviours
> that would cap this at UINT16_MAX.
> 
> That is, the risk being that UINT16_MAX is scoped to a uint16_t (which is
legal
> according to 7.18.2, p2 of C99, although AFAIK, no one does this)
> 
> I haven't checked C11, but it's enough of a tingly sense to wonder if you
> shouldn't just add a static const uint16_t = static_cast<...> +
static_cast<...>
> 
> Or ask jyasskin

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:35: class
DefaultApprovalDelegate
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> Document

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:62: // Public methods.
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> unnecessary

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/copresence_peer.cc:138:
create_callback_.Run(std::string());
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> I find this pattern a little weird (with respect to the four exit points
spread
> across three methods - lines 100, 124, 129, 138)
> 
> It also creates confusion as to what can happen in create_callback_, and what
it
> can do to mutate this object (for example, line 130 makes it categorically
> unsafe to mutate this object, but I don't know if that's easy to reason about)
> 
> I wonder if it's possible to rethink this in terms of an explicit state
machine,
> which makes it easier to reason about the transitions, and also lets you move
> the invocation of create_callback_ to the terminal edge of the state. This is
a
> pattern we use exclusively in //net, and we've found when teams avoid this
> pattern (Cast and Extensions), they tend to introduce new crashes and bugs.

Discussed offline. I will look at code in //net to come up with a better design
to do this.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File components/copresence_sockets/public/copresence_peer.h (right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:48: AcceptCallback
accept_callback);
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> STYLE: Callbacks should be passed as const-ref
> 
> The ONLY exceptions to the const-ref rules are PODs and scoped_ptr<>.

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_peer.h:57: void
OnGetAdapter(scoped_refptr<device::BluetoothAdapter> adapter);
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> STYLE: scoped_refptr<>'s should be passed as const-ref, as it avoids refcount
> churn.

This isn't code I control. The BT API's require the callback to be in this
format. I've fixed this wherever used in my own code.

Fixing the entire BT stack is not in the scope of this CL.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File components/copresence_sockets/public/copresence_socket.h (right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:16: // connection
request. This class is currently just a pure interface.
On 2014/10/01 19:47:25, Ryan Sleevi wrote:
> LAYERING: Document how to use this class, not how it is used. THis is a public
> API, comments about who does what don't belong (unless it's a delegate and
> you're documenting how/under what conditions the delegate is invoked)

Changed.
Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:20: typedef
base::Callback<void(const std::string&)> ReceiveCallback;
On 2014/10/01 19:47:25, Ryan Sleevi wrote:
> DESIGN: std::string vs base::StringPiece

Changed to using net::IOBuffer instead.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:23: virtual
~CopresenceSocket() {}
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> DESIGN: Is a public destructor correct for this class?
> 
> Only do so if ownership of a CopresenceSocket implementation is directly
passed
> into your interface. Otherwise, prefer a protected destructor, to ensure it's
> the owning class that is responsible for managing the lifetime of this object.

So CopresenceSockets will be owned by classes other than the CopresencePeer also
(which will hold ownership only of the base pointer), so having a public
destructor is what we want.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:26: // the method
returns false.
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> STYLE: Pronouns in comments considered harmful -
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M
> 
> // Send |data| on this socket. If the socket was unable to send data, returns
> false.

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:28: virtual bool
Send(const std::string& data) = 0;
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> DESIGN: Synchronous methods for sockets are ALWAYS an anti-pattern and
STRONGLY
> discouraged as they have been a LARGE source of bugs and latency.
> 
> Your socket also assumes all data will always be sent, which seems like a poor
> decision.

We are designing this API so that Send only means that the send 'started', not
that it completed. This API can be implemented on BT, UDP, TCP, BLE, or a host
of other transports, not all of which will be able to give a send confirmation.
Added more documentation to this method to clarify.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:28: virtual bool
Send(const std::string& data) = 0;
On 2014/10/01 19:47:25, Ryan Sleevi wrote:
> DESIGN: Don't use std::string for buffers of data.
> 
> Nominally, net::IOBuffer is the canonical form for this within //net and the
> overall resource loading stack. Alternatively, base::StringPiece allows for
> zero-copy when dealing with alternative forms (e.g. an std::vector<>)

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:29: virtual void
Receive(ReceiveCallback callback) = 0;
On 2014/10/01 19:47:25, Ryan Sleevi wrote:
> DESIGN: Let the caller specify how much data they're interested in?

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/public/copresence_socket.h:29: virtual void
Receive(ReceiveCallback callback) = 0;
On 2014/10/01 19:47:24, Ryan Sleevi wrote:
> STYLE: Pass as const-ref.

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.cc
(right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.cc:41:
4096,
On 2014/10/01 19:47:25, Ryan Sleevi wrote:
> DESIGN: Magic Constants are bad.

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
File
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.h
(right):

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.h:26:
public base::SupportsWeakPtr<CopresenceSocketBluetooth> {
On 2014/10/01 19:47:25, Ryan Sleevi wrote:
> DESIGN: DO NOT expose SupportsWeakPtr on new objects.
> 
> It is a design flaw to expose the WeakPtr semantics of an object to external
> consumers. This is a NOT LGTM until this can be resolved.

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.h:28:
CopresenceSocketBluetooth(scoped_refptr<device::BluetoothSocket> socket);
On 2014/10/01 19:47:26, Ryan Sleevi wrote:
> STYLE: const-ref

Done.

https://codereview.chromium.org/610633002/diff/60001/components/copresence_so...
components/copresence_sockets/transports/bluetooth/copresence_socket_bluetooth.h:39:
void OnReceive(int size, scoped_refptr<net::IOBuffer> io_buffer);
On 2014/10/01 19:47:25, Ryan Sleevi wrote:
> STYLE: const-ref

As mentioned elsewhere, this is required to be this way since it is a callback
to BT code.

https://codereview.chromium.org/610633002/diff/60001/extensions/common/api/co...
File extensions/common/api/copresence_socket.idl (right):

https://codereview.chromium.org/610633002/diff/60001/extensions/common/api/co...
extensions/common/api/copresence_socket.idl:10: // Only use the internet as the
transport.
On 2014/10/01 19:47:26, Ryan Sleevi wrote:
> s/internet/Internet

Done.

[Ryan Sleevi, 2014-10-01 23:38:32]

I'm still a little shaky on the sync vs async on send/receive, but you've
resolved my blocking objections, and are on path to get design feedback, so
dependency on //net LGTM.

Please maintain flexibility with this API as much as possible. There's a lot
that feels "off" here, and discussing on net-dev@ is a great way to get some
others to chime in.

As much as possible, I think our goal should help authors (of Apps and
Extensions, and of Chrome code) find existing paradigms and patterns, even when
alternatives exist, so they can re-use their knowledge and expectations.

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
components/copresence_sockets/copresence_peer.cc:84: // other platforms.
Is there a bug for this? Is there some way for someone to know if this is ever
met, beyond checking with you?

If there is a bug (always good to file one for TODOs), great to link it here.

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
components/copresence_sockets/copresence_peer.cc:95: // Private methods.
Unnecessary

[Ilya Sherman, 2014-10-02 00:23:21]

On 2014/10/01 22:38:49, Rahul Chaturvedi wrote:
> Edited histograms.xml by hand. Where is the script that does this
automatically?

Please run //tools/metrics/histograms/update_extension_functions.py to verify
that your manual edits were correct.

[Cait (Slow), 2014-10-02 16:41:21]

components/ changes LGTM as far as style and respecting layering/DEPs rules.
I'll defer to those more versed in sockets and net/ for API design.

[armansito, 2014-10-03 15:27:54]

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
components/copresence_sockets/copresence_peer.cc:73: if
(!device::BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) {
This check may not be enough to determine if Bluetooth is available. This method
simply returns true if Bluetooth support is built on the current platform (Win,
OSX, CrOS), though this doesn't guarantee that a Bluetooth controller is
actually present.

For that, you can call adapter_->IsPresent() once you obtain the adapter. You
can observe when an adapter gets added or removed by implementing
BluetoothAdapter::Observer::AdapterPresentChanged.

Normally this doesn't really matter, though since copresence wants the ability
to determine the right transport best on availability, this may be important to
you. So experiment with the Bluetooth adapter not being present, manually
plug/unplug an adapter, and see if you're getting the behavior you want.

[rkc, 2014-10-03 21:57:40]

Ilya, update_extension_function.py (run from its own directory) doesn't do
anything for me. It just hangs in place.

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
components/copresence_sockets/copresence_peer.cc:73: if
(!device::BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) {
On 2014/10/03 15:27:54, armansito wrote:
> This check may not be enough to determine if Bluetooth is available. This
method
> simply returns true if Bluetooth support is built on the current platform
(Win,
> OSX, CrOS), though this doesn't guarantee that a Bluetooth controller is
> actually present.
> 
> For that, you can call adapter_->IsPresent() once you obtain the adapter. You
> can observe when an adapter gets added or removed by implementing
> BluetoothAdapter::Observer::AdapterPresentChanged.
> 
> Normally this doesn't really matter, though since copresence wants the ability
> to determine the right transport best on availability, this may be important
to
> you. So experiment with the Bluetooth adapter not being present, manually
> plug/unplug an adapter, and see if you're getting the behavior you want.

Added the check for IsAdapter() to OnGetAdapter.

If the Adapter is not available at that time, we are not going to attempt a
connection at all and will return an error. We expect the adapter to be present
and *enabled* for this code to work.

The adapter gets added later isn't a case we want to handle presently (we may
revisit this when we are building more fallbacks into the behavior).

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
components/copresence_sockets/copresence_peer.cc:84: // other platforms.
On 2014/10/01 23:38:32, Ryan Sleevi wrote:
> Is there a bug for this? Is there some way for someone to know if this is ever
> met, beyond checking with you?
> 
> If there is a bug (always good to file one for TODOs), great to link it here.

Yep, added the link here.

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
components/copresence_sockets/copresence_peer.cc:95: // Private methods.
On 2014/10/01 23:38:32, Ryan Sleevi wrote:
> Unnecessary

Done.

[armansito, 2014-10-03 22:43:21]

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
components/copresence_sockets/copresence_peer.cc:73: if
(!device::BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) {
On 2014/10/03 21:57:39, Rahul Chaturvedi wrote:
> On 2014/10/03 15:27:54, armansito wrote:
> > This check may not be enough to determine if Bluetooth is available. This
> method
> > simply returns true if Bluetooth support is built on the current platform
> (Win,
> > OSX, CrOS), though this doesn't guarantee that a Bluetooth controller is
> > actually present.
> > 
> > For that, you can call adapter_->IsPresent() once you obtain the adapter.
You
> > can observe when an adapter gets added or removed by implementing
> > BluetoothAdapter::Observer::AdapterPresentChanged.
> > 
> > Normally this doesn't really matter, though since copresence wants the
ability
> > to determine the right transport best on availability, this may be important
> to
> > you. So experiment with the Bluetooth adapter not being present, manually
> > plug/unplug an adapter, and see if you're getting the behavior you want.
> 
> Added the check for IsAdapter() to OnGetAdapter.
> 
> If the Adapter is not available at that time, we are not going to attempt a
> connection at all and will return an error. We expect the adapter to be
present
> and *enabled* for this code to work.
> 
> The adapter gets added later isn't a case we want to handle presently (we may
> revisit this when we are building more fallbacks into the behavior).

In that case usually also check if the adapter is enabled by calling
IsPowered(). So, check for IsPresent() && IsPowered().

[rkc, 2014-10-03 22:46:18]

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
File components/copresence_sockets/copresence_peer.cc (right):

https://codereview.chromium.org/610633002/diff/100001/components/copresence_s...
components/copresence_sockets/copresence_peer.cc:73: if
(!device::BluetoothAdapterFactory::IsBluetoothAdapterAvailable()) {
On 2014/10/03 22:43:21, armansito wrote:
> On 2014/10/03 21:57:39, Rahul Chaturvedi wrote:
> > On 2014/10/03 15:27:54, armansito wrote:
> > > This check may not be enough to determine if Bluetooth is available. This
> > method
> > > simply returns true if Bluetooth support is built on the current platform
> > (Win,
> > > OSX, CrOS), though this doesn't guarantee that a Bluetooth controller is
> > > actually present.
> > > 
> > > For that, you can call adapter_->IsPresent() once you obtain the adapter.
> You
> > > can observe when an adapter gets added or removed by implementing
> > > BluetoothAdapter::Observer::AdapterPresentChanged.
> > > 
> > > Normally this doesn't really matter, though since copresence wants the
> ability
> > > to determine the right transport best on availability, this may be
important
> > to
> > > you. So experiment with the Bluetooth adapter not being present, manually
> > > plug/unplug an adapter, and see if you're getting the behavior you want.
> > 
> > Added the check for IsAdapter() to OnGetAdapter.
> > 
> > If the Adapter is not available at that time, we are not going to attempt a
> > connection at all and will return an error. We expect the adapter to be
> present
> > and *enabled* for this code to work.
> > 
> > The adapter gets added later isn't a case we want to handle presently (we
may
> > revisit this when we are building more fallbacks into the behavior).
> 
> In that case usually also check if the adapter is enabled by calling
> IsPowered(). So, check for IsPresent() && IsPowered().

Done.

[armansito, 2014-10-04 00:33:56]

Bluetooth lgtm

[rkc, 2014-10-04 02:03:58]

rkc@chromium.org changed reviewers:
+ mark@chromium.org
- brettw@chromium.org

[rkc, 2014-10-04 02:03:59]

Adding mark for adding //base DEPS change.

Ilya, I got the script to run. It didn't make any changes to the .xml, so my
changes should be good.

[Mark Mentovai, 2014-10-04 04:10:57]

Rahul Chaturvedi wrote:
> Adding mark for adding //base DEPS change.

I don’t see any changes in base here.

[rkc, 2014-10-04 04:17:41]

I'm adding DEPS to base (in //components/copresence_sockets) which requires an
owners approvals from a //base owner.

[Mark Mentovai, 2014-10-05 15:27:28]

You’d need a base OWNERS approval to add a new dependency to base/DEPS, but for
other components add base to their own DEPS files, you shouldn’t need anyone
from base to approve.

I’m happy to stamp this if a failed presubmit check proves me wrong, but as a
matter of process, you should be fine as-is.

[rkc, 2014-10-05 17:00:14]

Adding a dependency on //base _really should not require an owners approval.
//base is needed by absolutely everything.

Unfortunately, the presubmit still requires it though, so I still need at least
a rubberstamp,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...

[Mark Mentovai, 2014-10-05 18:59:24]

That’s nuts. LGTM.

[Ilya Sherman, 2014-10-06 21:17:26]

Histograms LGTM, thanks.

[rkc, 2014-10-06 23:50:00]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-10-06 23:54:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610633002/160001

[commit-bot: I haz the power, 2014-10-07 01:38:52]

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

[commit-bot: I haz the power, 2014-10-07 01:38:54]

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

[rkc, 2014-10-07 16:14:29]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-10-07 16:15:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610633002/180001

[commit-bot: I haz the power, 2014-10-07 16:26:51]

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

[commit-bot: I haz the power, 2014-10-07 16:26:55]

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

[rkc, 2014-10-07 16:30:52]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-10-07 16:31:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610633002/180001

[rkc, 2014-10-07 16:35:30]

The CQ bit was unchecked by rkc@chromium.org

[rkc, 2014-10-07 16:57:47]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-10-07 16:59:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610633002/200001

[commit-bot: I haz the power, 2014-10-07 17:11:55]

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

[commit-bot: I haz the power, 2014-10-07 17:11:57]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[rkc, 2014-10-07 19:16:19]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2014-10-07 19:16:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/610633002/220001

[commit-bot: I haz the power, 2014-10-07 21:27:30]

Committed patchset #12 (id:220001) as e8fb23d21de9e8c026a39203a8d7e187c766e2be

[commit-bot: I haz the power, 2014-10-07 21:28:10]

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