bluetooth: Add Device connection logic and accompanying user interface.

bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection and Device
  interface request.
Binds lifetime of Device to lifetime of BluetoothGattConnection and message
  pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/32391c11bdd0921e21331620fedf513b89a6a48d
Cr-Commit-Position: refs/heads/master@{#433332}

[mbrunson, 2016-10-24 23:15:12]

Description was changed from

==========
bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection.
Binds lifetime of BluetoothGattConnection to lifetime of Device message pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
==========

to

==========
bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection.
Binds lifetime of BluetoothGattConnection to lifetime of Device message pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mbrunson, 2016-10-24 23:16:34]

mbrunson@chromium.org changed reviewers:
+ ortuno@chromium.org, scheib@chromium.org

[ortuno, 2016-10-25 10:42:10]

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/adapter.cc
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/adapter.cc...
device/bluetooth/adapter.cc:40: if (device) {
nit: To avoid indenting lines:

if (!condition) {
  // run callback and return.
}

// success case

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/adapter.cc...
device/bluetooth/adapter.cc:107: error->code =
static_cast<mojom::ConnectError::Code>(error_code);
Hmmm this is dangerous. Someone could add a new error code and then this would
become undefined. Add a helper method with a switch statement to translate from
ConnectErrorCode to mojo::ConnectError.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:15: : adapter_(std::move(adapter)),
connection_(std::move(connection)) {}
You mention that the pipe is tied to the lifetime of the connection but we never
close the pipe if the connection to the device dies. We should close the pipe
when the connection dies.

I think that would also require some extra logic to route the DeviceChanged
event. scheib: I think it's getting harder to keep this as a thin layer. Do you
think we should add some tests?

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:56: device->GetGattServices()) {
To avoid returning services that no longer exist or not returning any services
we shouldn't call GetGattServices if IsGattDiscoveryComplete is false.

Ideally we would have something like Web Bluetooth's getPrimaryServices()[1]
which waits for IsGattDiscoveryComplete() but that might be too much logic i.e.
you would have to queue the calls and this would have to become an observer of
the adapter and filter out events for other devices.

Alternatives that come to mind:

1. Add the event to the AdapterClient and keep track of which devices have
completed GATT Discovery. Though, we might introduce some races since
AdapterClient and Device use two different pipes. We would still need to add
this information to DeviceInfo.

2. Add a DiscoverServices() method that runs its callback when the device
finishes the GATT discovery procedure. This will probably result in the same
amount of logic as the Web Bluetooth method but at least is closer to the
current interface.

3. Add a IsGattDiscoveryComplete() method that returns whether or not discovery
is complete. We would poll until this becomes true. This would keep the
implementation simple but polling is really ugly.

scheib, mbrunson: Can you think of any other alternatives?

My vote would be for following Web Bluetooth's pattern i.e. GetServices runs its
callback after IsGattDiscoveryComplete becomes true. It is more logic but I
don't think the other alternatives are any better :/

[1]
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:65: mojom::ServiceInfoPtr
Device::ConstructServiceInfoStruct(
nit: No need for this helper function. The code is only being used once and
there won't be a Service service.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/adapter.mojom:10: struct ConnectError {
http://crbug.com/657632 discourages this approach. It makes sense to use this
pattern for things like rssi or tx power where it's hard to come up with a value
that represents null but to keep the implementation code simply I would just use
the enum with a SUCCESS value pattern.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/adapter.mojom:11: enum Code {
Add DEVICE_NO_LONGER_IN_RANGE.

Also drop the ERROR_ prefix.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/adapter.mojom:47: // Creates a GATT
connection to the device with |address| and returns a
Please mention when ConnectError is null and when it isn't.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/device.mojom:18: bool connected;
nit: is_gatt_connected

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/device.mojom:24: bool primary;
nit: is_primary

[mbrunson, 2016-10-25 20:03:04]

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/adapter.cc
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/adapter.cc...
device/bluetooth/adapter.cc:40: if (device) {
On 2016/10/25 10:42:09, ortuno wrote:
> nit: To avoid indenting lines:
> 
> if (!condition) {
>   // run callback and return.
> }
> 
> // success case

Done.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/adapter.cc...
device/bluetooth/adapter.cc:107: error->code =
static_cast<mojom::ConnectError::Code>(error_code);
On 2016/10/25 10:42:09, ortuno wrote:
> Hmmm this is dangerous. Someone could add a new error code and then this would
> become undefined. Add a helper method with a switch statement to translate
from
> ConnectErrorCode to mojo::ConnectError.

Done.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:15: : adapter_(std::move(adapter)),
connection_(std::move(connection)) {}
On 2016/10/25 10:42:09, ortuno wrote:
> You mention that the pipe is tied to the lifetime of the connection but we
never
> close the pipe if the connection to the device dies. We should close the pipe
> when the connection dies.
> 
> I think that would also require some extra logic to route the DeviceChanged
> event. scheib: I think it's getting harder to keep this as a thin layer. Do
you
> think we should add some tests?

Hmm, ok. More logic would be needed in Adapter to handle this case.
Specifically, a collection (probably an unordered_set) of WeakPtrs to all of the
connected Device bindings. In DeviceChanged, we would forcibly close the
connection to a Device if the connection is dropped.

Alternatives?

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:56: device->GetGattServices()) {
On 2016/10/25 10:42:09, ortuno wrote:
> To avoid returning services that no longer exist or not returning any services
> we shouldn't call GetGattServices if IsGattDiscoveryComplete is false.
> 
> Ideally we would have something like Web Bluetooth's getPrimaryServices()[1]
> which waits for IsGattDiscoveryComplete() but that might be too much logic
i.e.
> you would have to queue the calls and this would have to become an observer of
> the adapter and filter out events for other devices.
> 
> Alternatives that come to mind:
> 
> 1. Add the event to the AdapterClient and keep track of which devices have
> completed GATT Discovery. Though, we might introduce some races since
> AdapterClient and Device use two different pipes. We would still need to add
> this information to DeviceInfo.
> 
> 2. Add a DiscoverServices() method that runs its callback when the device
> finishes the GATT discovery procedure. This will probably result in the same
> amount of logic as the Web Bluetooth method but at least is closer to the
> current interface.
> 
> 3. Add a IsGattDiscoveryComplete() method that returns whether or not
discovery
> is complete. We would poll until this becomes true. This would keep the
> implementation simple but polling is really ugly.
> 
> scheib, mbrunson: Can you think of any other alternatives?
> 
> My vote would be for following Web Bluetooth's pattern i.e. GetServices runs
its
> callback after IsGattDiscoveryComplete becomes true. It is more logic but I
> don't think the other alternatives are any better :/
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

The original pattern I was thinking of would be having the client wait for the
ServicesDiscovered callback before calling GetServices. Is this not recommended?
It does put more burden on the client.

Alternatively, we could wait for service discovery to complete when a device
connection is requested, but this would require some kind of queue to run the
ConnectToDevice callback when discovery is complete. I think ortuno brought this
idea up one time before.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:65: mojom::ServiceInfoPtr
Device::ConstructServiceInfoStruct(
On 2016/10/25 10:42:09, ortuno wrote:
> nit: No need for this helper function. The code is only being used once and
> there won't be a Service service.

The ServiceChanged callback in AdapterClient will require a ServiceInfo struct,
so eventually this will have to change to a public static function when that is
added.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/adapter.mojom:10: struct ConnectError {
On 2016/10/25 10:42:10, ortuno wrote:
> http://crbug.com/657632 discourages this approach. It makes sense to use this
> pattern for things like rssi or tx power where it's hard to come up with a
value
> that represents null but to keep the implementation code simply I would just
use
> the enum with a SUCCESS value pattern.

Done.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/adapter.mojom:11: enum Code {
On 2016/10/25 10:42:09, ortuno wrote:
> Add DEVICE_NO_LONGER_IN_RANGE.
> 
> Also drop the ERROR_ prefix.

Done.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/adapter.mojom:47: // Creates a GATT
connection to the device with |address| and returns a
On 2016/10/25 10:42:09, ortuno wrote:
> Please mention when ConnectError is null and when it isn't.

Done.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/device.mojom:18: bool connected;
On 2016/10/25 10:42:10, ortuno wrote:
> nit: is_gatt_connected

Done.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/public/int...
device/bluetooth/public/interfaces/device.mojom:24: bool primary;
On 2016/10/25 10:42:10, ortuno wrote:
> nit: is_primary

Done.

[ortuno, 2016-10-26 02:52:21]

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:15: : adapter_(std::move(adapter)),
connection_(std::move(connection)) {}
On 2016/10/25 at 20:03:04, mbrunson wrote:
> On 2016/10/25 10:42:09, ortuno wrote:
> > You mention that the pipe is tied to the lifetime of the connection but we
never
> > close the pipe if the connection to the device dies. We should close the
pipe
> > when the connection dies.
> > 
> > I think that would also require some extra logic to route the DeviceChanged
> > event. scheib: I think it's getting harder to keep this as a thin layer. Do
you
> > think we should add some tests?
> 
> Hmm, ok. More logic would be needed in Adapter to handle this case.
Specifically, a collection (probably an unordered_set) of WeakPtrs to all of the
connected Device bindings. In DeviceChanged, we would forcibly close the
connection to a Device if the connection is dropped.
> 
> Alternatives?

I don't think you need to add the logic in Adapter.
  1. Rather than creating a StrongBinding you could hold a regular Binding in
this class. If there is a connection error (e.g. the other side dropped the
pipe) or if the device disconnects for some reason then you can just close the
binding manually and delete Device.
  2. You would make Device a BluetoothAdapter::Observer that overrides
DeviceChanged. If BluetoothDevice in DeviceChanged has the same address as this
device and is no longer GattConnected then you can close the pipe and delete the
Device instance.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:56: device->GetGattServices()) {
On 2016/10/25 at 20:03:04, mbrunson wrote:
> On 2016/10/25 10:42:09, ortuno wrote:
> > To avoid returning services that no longer exist or not returning any
services
> > we shouldn't call GetGattServices if IsGattDiscoveryComplete is false.
> > 
> > Ideally we would have something like Web Bluetooth's getPrimaryServices()[1]
> > which waits for IsGattDiscoveryComplete() but that might be too much logic
i.e.
> > you would have to queue the calls and this would have to become an observer
of
> > the adapter and filter out events for other devices.
> > 
> > Alternatives that come to mind:
> > 
> > 1. Add the event to the AdapterClient and keep track of which devices have
> > completed GATT Discovery. Though, we might introduce some races since
> > AdapterClient and Device use two different pipes. We would still need to add
> > this information to DeviceInfo.
> > 
> > 2. Add a DiscoverServices() method that runs its callback when the device
> > finishes the GATT discovery procedure. This will probably result in the same
> > amount of logic as the Web Bluetooth method but at least is closer to the
> > current interface.
> > 
> > 3. Add a IsGattDiscoveryComplete() method that returns whether or not
discovery
> > is complete. We would poll until this becomes true. This would keep the
> > implementation simple but polling is really ugly.
> > 
> > scheib, mbrunson: Can you think of any other alternatives?
> > 
> > My vote would be for following Web Bluetooth's pattern i.e. GetServices runs
its
> > callback after IsGattDiscoveryComplete becomes true. It is more logic but I
> > don't think the other alternatives are any better :/
> > 
> > [1]
> >
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> 
> The original pattern I was thinking of would be having the client wait for the
ServicesDiscovered callback before calling GetServices. Is this not recommended?
It does put more burden on the client.
> 

Do you mean AdapterClient::ServicesDiscovered?

That event is only dispatched when the services are discovered so there is a
chance we might miss it e.g. User connects to a device and discovers its
services through Web Bluetooth. Then user opens the internals page and connects
to the device. Since the services have been discovered beforehand the
ServicesDiscovered event will never be dispatched.

Furthermore GATT discoveries can happen multiple times during a connection. So
we always need to check if the discovery is complete before returning the
services.

> Alternatively, we could wait for service discovery to complete when a device
connection is requested, but this would require some kind of queue to run the
ConnectToDevice callback when discovery is complete. I think ortuno brought this
idea up one time before.

Good suggestion but abstracting two procedures into one could lead to loss of
functionality for clients. In the past we've had users that needed to measure
the time between a GATT connection and GATT Discovery, if we were to abstract
them under the same function this use case would no longer be possible.

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/adapte...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/adapte...
device/bluetooth/adapter.cc:43: nullptr /* Device */);
nit: s/Device/device/

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/adapte...
device/bluetooth/adapter.cc:146: case
device::BluetoothDevice::ConnectErrorCode::NUM_CONNECT_ERROR_CODES:
This is not an actual error :) add a NOTREACHED and return UNKNOWN. Bonus: Add a
new UNTRANSLATED_CONNECT_ERROR_CODE and use that instead.

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/adapte...
device/bluetooth/adapter.cc:148: default:
When a new value is added to an enum the compiler will complain if the value is
not handled by a switch statement. This check is skipped if the switch statement
has a 'default' case. Remove the default case and just add a NOTREACHED followed
by a return, similar to what WebBluetooth does:

https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/device.cc
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/device...
device/bluetooth/device.cc:41:
adapter_->GetDevice(connection_->GetDeviceAddress());
nit: Add a GetAddress() helper to avoid writing this every time.

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/public...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/public...
device/bluetooth/public/interfaces/device.mojom:32: // Gets the discovered
services advertising on this device.
// Gets the GATT Services in this device's GATT Server.

Advertising is unrelated to GATT.
https://punchthrough.com/bean/guides/everything-else/how-gap-and-gatt-work/

[mbrunson, 2016-10-28 21:06:48]

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:15: : adapter_(std::move(adapter)),
connection_(std::move(connection)) {}
On 2016/10/26 02:52:21, ortuno wrote:
> On 2016/10/25 at 20:03:04, mbrunson wrote:
> > On 2016/10/25 10:42:09, ortuno wrote:
> > > You mention that the pipe is tied to the lifetime of the connection but we
> never
> > > close the pipe if the connection to the device dies. We should close the
> pipe
> > > when the connection dies.
> > > 
> > > I think that would also require some extra logic to route the
DeviceChanged
> > > event. scheib: I think it's getting harder to keep this as a thin layer.
Do
> you
> > > think we should add some tests?
> > 
> > Hmm, ok. More logic would be needed in Adapter to handle this case.
> Specifically, a collection (probably an unordered_set) of WeakPtrs to all of
the
> connected Device bindings. In DeviceChanged, we would forcibly close the
> connection to a Device if the connection is dropped.
> > 
> > Alternatives?
> 
> I don't think you need to add the logic in Adapter.
>   1. Rather than creating a StrongBinding you could hold a regular Binding in
> this class. If there is a connection error (e.g. the other side dropped the
> pipe) or if the device disconnects for some reason then you can just close the
> binding manually and delete Device.
>   2. You would make Device a BluetoothAdapter::Observer that overrides
> DeviceChanged. If BluetoothDevice in DeviceChanged has the same address as
this
> device and is no longer GattConnected then you can close the pipe and delete
the
> Device instance.

Oh ok. That makes sense. Done.

https://codereview.chromium.org/2448713002/diff/1/device/bluetooth/device.cc#...
device/bluetooth/device.cc:56: device->GetGattServices()) {
On 2016/10/26 02:52:21, ortuno wrote:
> On 2016/10/25 at 20:03:04, mbrunson wrote:
> > On 2016/10/25 10:42:09, ortuno wrote:
> > > To avoid returning services that no longer exist or not returning any
> services
> > > we shouldn't call GetGattServices if IsGattDiscoveryComplete is false.
> > > 
> > > Ideally we would have something like Web Bluetooth's
getPrimaryServices()[1]
> > > which waits for IsGattDiscoveryComplete() but that might be too much logic
> i.e.
> > > you would have to queue the calls and this would have to become an
observer
> of
> > > the adapter and filter out events for other devices.
> > > 
> > > Alternatives that come to mind:
> > > 
> > > 1. Add the event to the AdapterClient and keep track of which devices have
> > > completed GATT Discovery. Though, we might introduce some races since
> > > AdapterClient and Device use two different pipes. We would still need to
add
> > > this information to DeviceInfo.
> > > 
> > > 2. Add a DiscoverServices() method that runs its callback when the device
> > > finishes the GATT discovery procedure. This will probably result in the
same
> > > amount of logic as the Web Bluetooth method but at least is closer to the
> > > current interface.
> > > 
> > > 3. Add a IsGattDiscoveryComplete() method that returns whether or not
> discovery
> > > is complete. We would poll until this becomes true. This would keep the
> > > implementation simple but polling is really ugly.
> > > 
> > > scheib, mbrunson: Can you think of any other alternatives?
> > > 
> > > My vote would be for following Web Bluetooth's pattern i.e. GetServices
runs
> its
> > > callback after IsGattDiscoveryComplete becomes true. It is more logic but
I
> > > don't think the other alternatives are any better :/
> > > 
> > > [1]
> > >
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > 
> > The original pattern I was thinking of would be having the client wait for
the
> ServicesDiscovered callback before calling GetServices. Is this not
recommended?
> It does put more burden on the client.
> > 
> 
> Do you mean AdapterClient::ServicesDiscovered?
> 
> That event is only dispatched when the services are discovered so there is a
> chance we might miss it e.g. User connects to a device and discovers its
> services through Web Bluetooth. Then user opens the internals page and
connects
> to the device. Since the services have been discovered beforehand the
> ServicesDiscovered event will never be dispatched.
> 
> Furthermore GATT discoveries can happen multiple times during a connection. So
> we always need to check if the discovery is complete before returning the
> services.
> 
> > Alternatively, we could wait for service discovery to complete when a device
> connection is requested, but this would require some kind of queue to run the
> ConnectToDevice callback when discovery is complete. I think ortuno brought
this
> idea up one time before.
> 
> Good suggestion but abstracting two procedures into one could lead to loss of
> functionality for clients. In the past we've had users that needed to measure
> the time between a GATT connection and GATT Discovery, if we were to abstract
> them under the same function this use case would no longer be possible.

Queue implemented with tests. Done.

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/adapte...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/adapte...
device/bluetooth/adapter.cc:43: nullptr /* Device */);
On 2016/10/26 02:52:21, ortuno wrote:
> nit: s/Device/device/

Done.

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/adapte...
device/bluetooth/adapter.cc:146: case
device::BluetoothDevice::ConnectErrorCode::NUM_CONNECT_ERROR_CODES:
On 2016/10/26 02:52:21, ortuno wrote:
> This is not an actual error :) add a NOTREACHED and return UNKNOWN. Bonus: Add
a
> new UNTRANSLATED_CONNECT_ERROR_CODE and use that instead.

Done.

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/adapte...
device/bluetooth/adapter.cc:148: default:
On 2016/10/26 02:52:21, ortuno wrote:
> When a new value is added to an enum the compiler will complain if the value
is
> not handled by a switch statement. This check is skipped if the switch
statement
> has a 'default' case. Remove the default case and just add a NOTREACHED
followed
> by a return, similar to what WebBluetooth does:
> 
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

Done.

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/device.cc
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/device...
device/bluetooth/device.cc:41:
adapter_->GetDevice(connection_->GetDeviceAddress());
On 2016/10/26 02:52:21, ortuno wrote:
> nit: Add a GetAddress() helper to avoid writing this every time.

Done.

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/public...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2448713002/diff/20001/device/bluetooth/public...
device/bluetooth/public/interfaces/device.mojom:32: // Gets the discovered
services advertising on this device.
On 2016/10/26 02:52:21, ortuno wrote:
> // Gets the GATT Services in this device's GATT Server.
> 
> Advertising is unrelated to GATT.
> https://punchthrough.com/bean/guides/everything-else/how-gap-and-gatt-work/

Done.

[scheib, 2016-10-31 20:46:18]

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device.h
File device/bluetooth/device.h (right):

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device...
device/bluetooth/device.h:53: 
Keep all overrides in one block (no blank lines).

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device...
device/bluetooth/device_unittest.cc:133: // Run then flush the queue. Calls
GetGattServices twice.
Completes 2 queued GetServices tasks.

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device...
device/bluetooth/device_unittest.cc:136: // Queue should be empty so no more
calls to GetGattServices.
"no more GetServices calls will complete".
Let's find a way to verify that no more GetServices calls are completed. In
BluetoothTest this is done by having counters incremented on events, and then
checking the sum is the expected result.

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/public...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/adapter.mojom:47: ConnectToDevice(string
address) => (ConnectErrorCode error, Device? device);
"ConnectErrorCode error" -> "ConnectResult result"

[mbrunson, 2016-10-31 21:39:25]

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device.h
File device/bluetooth/device.h (right):

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device...
device/bluetooth/device.h:53: 
On 2016/10/31 20:46:17, scheib wrote:
> Keep all overrides in one block (no blank lines).

Done.

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device...
device/bluetooth/device_unittest.cc:133: // Run then flush the queue. Calls
GetGattServices twice.
On 2016/10/31 20:46:17, scheib wrote:
> Completes 2 queued GetServices tasks.

Done.

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/device...
device/bluetooth/device_unittest.cc:136: // Queue should be empty so no more
calls to GetGattServices.
On 2016/10/31 20:46:17, scheib wrote:
> "no more GetServices calls will complete".
> Let's find a way to verify that no more GetServices calls are completed. In
> BluetoothTest this is done by having counters incremented on events, and then
> checking the sum is the expected result.

Done.

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/public...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2448713002/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/adapter.mojom:47: ConnectToDevice(string
address) => (ConnectErrorCode error, Device? device);
On 2016/10/31 20:46:18, scheib wrote:
> "ConnectErrorCode error" -> "ConnectResult result"

Done.

[scheib, 2016-10-31 22:46:55]

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:151:
device_service_->GattServicesDiscovered(nullptr /* adapter */, device_.get());
high level comment: it's a bit hard to see which calls here are 'Client' calls,
vs which are coming from the lower levels. 

I'm not sure the best way to address this. 
- Could just have comments to that effect. 
  e.g. // Client:  Add more than one request...
  e.g. // Simulate: Discovery completes
- Could have the test fixture have wrappers for the lower level invocations, and
have names such as SimulateGattServicesDiscovered (similar to how BluetoothTest
based tests read). But, here that would be just adding complexity that isn't
otherwise needed.

[mbrunson, 2016-10-31 22:58:12]

Description was changed from

==========
bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection.
Binds lifetime of BluetoothGattConnection to lifetime of Device message pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection and Device
  interface request.
Binds lifetime of Device to lifetime of BluetoothGattConnection and message
  pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[ortuno, 2016-11-01 06:27:39]

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:112: mojom::ConnectResult
Adapter::BluetoothErrorCodeToMojomResult(
nit: Move this no an anonymous namespace i.e.:

namespace bluetooth {

namespace {

mojom::ConnectResult BluetoothErrorCodeToMojomResult(...) {
 // ...
}

}  // namespace

// ...

}  // namespace bluetooth

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
base::Unretained always raises some alarms. It's like using new. You have to be
sure that the object will not get deleted before the callback is called. In this
case the binding is owned by the class so it's safe but let's add a comment just
in case.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:31: connection_->Disconnect();
No need to call Disconnect. Destroying the object will close the connection.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:61: if (device->GetAddress() == GetAddress()) {
nit:

if (device->GetAddress() != GetAddress()) {
  return;
}
// ...

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:70: if (device->GetAddress() == GetAddress()) {
nit:

if (device->GetAddress() != GetAddress()) {
  return;
}

for (...) {
//...

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:72: request.Run();
Running closures in a for loop could lead to unexpected reentrancy issues. We've
had a couple of bugs like that in device/bluetooth. The recommended pattern is
to swap the callbacks first:

std::vector<base::Closure> requests;
requests.swap(pending_services_requests);

for (const base::Closure& request : requests) {
  request.Run();
}

// We should have no requests left.
DCHECK(requests.empty());

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:85: if (device) {
Now that lifetime of this class is owned by the connection I don't think you
need these if statements. A DCHECK should be enough.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:96: if (device) {
Same here. I don't think you need the if anymore.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:103: base::Bind(&Device::GetServicesImpl,
base::Unretained(this), callback));
Here it's a bit harder to see why base::Unretained is safe. Please add a
comment.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:123: mojom::ServiceInfoPtr
Device::ConstructServiceInfoStruct(
Since you know service will always be there just take a const &.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/device.h
File device/bluetooth/device.h (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.h:48: void set_connection_error_handler(const
base::Closure& closure);
Why do you need to expose this? Also since it's only used in tests, this should
be called SetConnectionErrorHandlerForTesting.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.h:67: std::string GetAddress();
hmm I might be missing something but it seems this could return a const &.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:134: TEST_F(DeviceTest,
GetServicesNotDiscovered) {
Can you add a test for when there are pending calls and the connection is
broken?

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:158: GetGetServicesCallback(Call::EXPECTED,
loop.QuitClosure()));
What do you think about adding the ability to specify the number of preceding
callbacks. 

https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:9: enum ConnectResult {
Comment please.

[mbrunson, 2016-11-02 01:25:46]

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:112: mojom::ConnectResult
Adapter::BluetoothErrorCodeToMojomResult(
On 2016/11/01 06:27:39, ortuno wrote:
> nit: Move this no an anonymous namespace i.e.:
> 
> namespace bluetooth {
> 
> namespace {
> 
> mojom::ConnectResult BluetoothErrorCodeToMojomResult(...) {
>  // ...
> }
> 
> }  // namespace
> 
> // ...
> 
> }  // namespace bluetooth

Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
On 2016/11/01 06:27:39, ortuno wrote:
> base::Unretained always raises some alarms. It's like using new. You have to
be
> sure that the object will not get deleted before the callback is called. In
this
> case the binding is owned by the class so it's safe but let's add a comment
just
> in case.

Hmm. Should I prefer WeakPtrFactory over base::Unretained then? If it could
cause some concern (whether justified or not), sticking to WeakPtrFactory seems
preferable.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:31: connection_->Disconnect();
On 2016/11/01 06:27:39, ortuno wrote:
> No need to call Disconnect. Destroying the object will close the connection.

Removed. Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:61: if (device->GetAddress() == GetAddress()) {
On 2016/11/01 06:27:39, ortuno wrote:
> nit:
> 
> if (device->GetAddress() != GetAddress()) {
>   return;
> }
> // ...

Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:70: if (device->GetAddress() == GetAddress()) {
On 2016/11/01 06:27:39, ortuno wrote:
> nit:
> 
> if (device->GetAddress() != GetAddress()) {
>   return;
> }
> 
> for (...) {
> //...

Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:72: request.Run();
On 2016/11/01 06:27:39, ortuno wrote:
> Running closures in a for loop could lead to unexpected reentrancy issues.
We've
> had a couple of bugs like that in device/bluetooth. The recommended pattern is
> to swap the callbacks first:
> 
> std::vector<base::Closure> requests;
> requests.swap(pending_services_requests);
> 
> for (const base::Closure& request : requests) {
>   request.Run();
> }
> 
> // We should have no requests left.
> DCHECK(requests.empty());

Are you saying running the requests removes them from the requests vector
automatically? That seems strange...

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:85: if (device) {
On 2016/11/01 06:27:39, ortuno wrote:
> Now that lifetime of this class is owned by the connection I don't think you
> need these if statements. A DCHECK should be enough.

Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:96: if (device) {
On 2016/11/01 06:27:39, ortuno wrote:
> Same here. I don't think you need the if anymore.

Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:123: mojom::ServiceInfoPtr
Device::ConstructServiceInfoStruct(
On 2016/11/01 06:27:39, ortuno wrote:
> Since you know service will always be there just take a const &.

Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/device.h
File device/bluetooth/device.h (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.h:48: void set_connection_error_handler(const
base::Closure& closure);
On 2016/11/01 06:27:39, ortuno wrote:
> Why do you need to expose this? Also since it's only used in tests, this
should
> be called SetConnectionErrorHandlerForTesting.

I was getting segfaults when calling Device functions before I added it. I've
changed the structure of the tests so I shouldn't need this function anymore.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.h:67: std::string GetAddress();
On 2016/11/01 06:27:39, ortuno wrote:
> hmm I might be missing something but it seems this could return a const &.

Ah yes. It should be. Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:134: TEST_F(DeviceTest,
GetServicesNotDiscovered) {
On 2016/11/01 06:27:39, ortuno wrote:
> Can you add a test for when there are pending calls and the connection is
> broken?

I added a few more tests for the various disconnection situations. Let me know
if I missed any of the little details.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:151:
device_service_->GattServicesDiscovered(nullptr /* adapter */, device_.get());
On 2016/10/31 22:46:54, scheib wrote:
> high level comment: it's a bit hard to see which calls here are 'Client'
calls,
> vs which are coming from the lower levels. 
> 
> I'm not sure the best way to address this. 
> - Could just have comments to that effect. 
>   e.g. // Client:  Add more than one request...
>   e.g. // Simulate: Discovery completes
> - Could have the test fixture have wrappers for the lower level invocations,
and
> have names such as SimulateGattServicesDiscovered (similar to how
BluetoothTest
> based tests read). But, here that would be just adding complexity that isn't
> otherwise needed. 

I think more descriptive comments will be enough here. If this gets more
complex, named functions would probably be better.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:158: GetGetServicesCallback(Call::EXPECTED,
loop.QuitClosure()));
On 2016/11/01 06:27:39, ortuno wrote:
> What do you think about adding the ability to specify the number of preceding
> callbacks. 
> 
>
https://cs.chromium.org/chromium/src/device/bluetooth/test/bluetooth_test.cc?...

Seems helpful. That way we know for sure the count is correct in the middle of
execution. Done.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:9: enum ConnectResult {
On 2016/11/01 06:27:39, ortuno wrote:
> Comment please.

Done.

[ortuno, 2016-11-02 07:56:04]

The device/bluetooth parts looks good. I haven't looked at the js part since it
depends on the previous patch.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
On 2016/11/02 at 01:25:45, mbrunson wrote:
> On 2016/11/01 06:27:39, ortuno wrote:
> > base::Unretained always raises some alarms. It's like using new. You have to
be
> > sure that the object will not get deleted before the callback is called. In
this
> > case the binding is owned by the class so it's safe but let's add a comment
just
> > in case.
> 
> Hmm. Should I prefer WeakPtrFactory over base::Unretained then? If it could
cause some concern (whether justified or not), sticking to WeakPtrFactory seems
preferable.

We shouldn't add complexity unless it's needed. A comment indicating that you've
thought about the lifetime and concluded that it's not a problem should be
enough.

https://codereview.chromium.org/2448713002/diff/100001/device/bluetooth/devic...
device/bluetooth/device.cc:72: request.Run();
On 2016/11/02 at 01:25:45, mbrunson wrote:
> On 2016/11/01 06:27:39, ortuno wrote:
> > Running closures in a for loop could lead to unexpected reentrancy issues.
We've
> > had a couple of bugs like that in device/bluetooth. The recommended pattern
is
> > to swap the callbacks first:
> > 
> > std::vector<base::Closure> requests;
> > requests.swap(pending_services_requests);
> > 
> > for (const base::Closure& request : requests) {
> >   request.Run();
> > }
> > 
> > // We should have no requests left.
> > DCHECK(requests.empty());
> 
> Are you saying running the requests removes them from the requests vector
automatically? That seems strange...

Or adds them. For example we in device/bluetooth we queue calls so
StartNotifySession. We had a bug in which if in the callback for
StartNotifySession someone called StartNotifySession we would queue the callback
and invalidate the iterator that was being used for the for loop.

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device.cc:30: if (has_pending_services_) {
Ah sorry I think I confused you. I don't think you need a DCHECK in this class.
The DCHECK is automatically generated by mojo.

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:165: MakeGetServicesRequests(0, 2,
Call::EXPECTED, loop.QuitWhenIdleClosure());
nit: I think scheib was proposing wrapping the GattServicesDiscovered() in
SimulateGattServicesDiscovered() rather than wrapping GetServices() calls.

Optional: I think wrapping GetServices() calls makes the code harder to follow,
consider unwrapping them. If not add comments to the arguments; see Function
Arguments Comments in
https://google.github.io/styleguide/cppguide.html#Implementation_Comments

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:166: EXPECT_EQ(2,
device_service_->GetPendingServiceRequestCountForTesting());
I'm curious as to why you expose this function. You have n calls coming in and
there should be n calls coming out. And if you want to be sure that the queue is
empty you can just simulate another event.

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:185: TEST_F(DeviceTest,
GetServicesLostConnectionWithPendingRequests) {
In theory the following destructor in Device should cause this test to crash.
Does it actually crash?

pending_services_requests_.clear();
binding_.close();

[mbrunson, 2016-11-02 21:57:08]

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device.cc:30: if (has_pending_services_) {
On 2016/11/02 07:56:04, ortuno wrote:
> Ah sorry I think I confused you. I don't think you need a DCHECK in this
class.
> The DCHECK is automatically generated by mojo.

Done.

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:165: MakeGetServicesRequests(0, 2,
Call::EXPECTED, loop.QuitWhenIdleClosure());
On 2016/11/02 07:56:04, ortuno wrote:
> nit: I think scheib was proposing wrapping the GattServicesDiscovered() in
> SimulateGattServicesDiscovered() rather than wrapping GetServices() calls.
> 
> Optional: I think wrapping GetServices() calls makes the code harder to
follow,
> consider unwrapping them. If not add comments to the arguments; see Function
> Arguments Comments in
> https://google.github.io/styleguide/cppguide.html#Implementation_Comments

Hmm ok. I'll just unwrap them then.

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:166: EXPECT_EQ(2,
device_service_->GetPendingServiceRequestCountForTesting());
On 2016/11/02 07:56:04, ortuno wrote:
> I'm curious as to why you expose this function. You have n calls coming in and
> there should be n calls coming out. And if you want to be sure that the queue
is
> empty you can just simulate another event.

Yeah. I probably don't need it here. It was really for the tests for
disconnection to make sure the requests were actually in the queue since they're
never actually called for me to verify they exist.

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:185: TEST_F(DeviceTest,
GetServicesLostConnectionWithPendingRequests) {
On 2016/11/02 07:56:04, ortuno wrote:
> In theory the following destructor in Device should cause this test to crash.
> Does it actually crash?
> 
> pending_services_requests_.clear();
> binding_.close();

No. It doesn't crash. Let me make sure DCHECK is turned on.

[ortuno, 2016-11-02 23:09:55]

https://codereview.chromium.org/2448713002/diff/140001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/140001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:188: EXPECT_EQ(2,
device_service_->GetPendingServiceRequestCountForTesting());
You could just check that callback_count_ is 0.

[mbrunson, 2016-11-03 18:50:19]

https://codereview.chromium.org/2448713002/diff/140001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/140001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:188: EXPECT_EQ(2,
device_service_->GetPendingServiceRequestCountForTesting());
On 2016/11/02 23:09:55, ortuno wrote:
> You could just check that callback_count_ is 0.

Done.

[mbrunson, 2016-11-03 22:02:18]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 22:03:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 23:47:22]

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

[commit-bot: I haz the power, 2016-11-03 23:47:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ortuno, 2016-11-04 01:50:42]

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:185: TEST_F(DeviceTest,
GetServicesLostConnectionWithPendingRequests) {
On 2016/11/02 at 21:57:08, mbrunson wrote:
> On 2016/11/02 07:56:04, ortuno wrote:
> > In theory the following destructor in Device should cause this test to
crash.
> > Does it actually crash?
> > 
> > pending_services_requests_.clear();
> > binding_.close();
> 
> No. It doesn't crash. Let me make sure DCHECK is turned on.

Where you able to get it to crash?

[mbrunson, 2016-11-04 02:47:06]

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:185: TEST_F(DeviceTest,
GetServicesLostConnectionWithPendingRequests) {
On 2016/11/04 01:50:42, ortuno wrote:
> On 2016/11/02 at 21:57:08, mbrunson wrote:
> > On 2016/11/02 07:56:04, ortuno wrote:
> > > In theory the following destructor in Device should cause this test to
> crash.
> > > Does it actually crash?
> > > 
> > > pending_services_requests_.clear();
> > > binding_.close();
> > 
> > No. It doesn't crash. Let me make sure DCHECK is turned on.
> 
> Where you able to get it to crash?

No. I've tried multiple configurations and orders and can't get a crash. Not
sure what's going on there.

[ortuno, 2016-11-04 03:01:27]

On 2016/11/04 at 02:47:06, mbrunson wrote:
>
https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
> File device/bluetooth/device_unittest.cc (right):
> 
>
https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
> device/bluetooth/device_unittest.cc:185: TEST_F(DeviceTest,
GetServicesLostConnectionWithPendingRequests) {
> On 2016/11/04 01:50:42, ortuno wrote:
> > On 2016/11/02 at 21:57:08, mbrunson wrote:
> > > On 2016/11/02 07:56:04, ortuno wrote:
> > > > In theory the following destructor in Device should cause this test to
> > crash.
> > > > Does it actually crash?
> > > > 
> > > > pending_services_requests_.clear();
> > > > binding_.close();
> > > 
> > > No. It doesn't crash. Let me make sure DCHECK is turned on.
> > 
> > Where you able to get it to crash?
> 
> No. I've tried multiple configurations and orders and can't get a crash. Not
sure what's going on there.

Hmm seems that the callbacks used in actual mojo calls are different:
https://cs.chromium.org/chromium/src/out/Debug/gen/device/bluetooth/public/in...

Could you create a helper class whose lifetime is attached to that of the
callback and when it's destroyed it checks if the connection is closed?

[mbrunson, 2016-11-04 21:13:54]

On 2016/11/04 03:01:27, ortuno wrote:
> On 2016/11/04 at 02:47:06, mbrunson wrote:
> >
>
https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
> > File device/bluetooth/device_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2448713002/diff/120001/device/bluetooth/devic...
> > device/bluetooth/device_unittest.cc:185: TEST_F(DeviceTest,
> GetServicesLostConnectionWithPendingRequests) {
> > On 2016/11/04 01:50:42, ortuno wrote:
> > > On 2016/11/02 at 21:57:08, mbrunson wrote:
> > > > On 2016/11/02 07:56:04, ortuno wrote:
> > > > > In theory the following destructor in Device should cause this test to
> > > crash.
> > > > > Does it actually crash?
> > > > > 
> > > > > pending_services_requests_.clear();
> > > > > binding_.close();
> > > > 
> > > > No. It doesn't crash. Let me make sure DCHECK is turned on.
> > > 
> > > Where you able to get it to crash?
> > 
> > No. I've tried multiple configurations and orders and can't get a crash. Not
> sure what's going on there.
> 
> Hmm seems that the callbacks used in actual mojo calls are different:
>
https://cs.chromium.org/chromium/src/out/Debug/gen/device/bluetooth/public/in...
> 
> Could you create a helper class whose lifetime is attached to that of the
> callback and when it's destroyed it checks if the connection is closed?

I've added a DeviceConnectionHelper class and used base::Owned to bind it to the
callback stored in the queue. Unit tests passed. Clearing the queue before
closing the pipe doesn't DCHECK no matter where I put it, but I know it's
calling the destructor because putting a NOTREACHED crashes all but one of the
tests (the first GetServices test doesn't queue anything). I must be missing
something... :\

[ortuno, 2016-11-06 22:47:53]

hmm could you upload the code where DeviceConnectionHelper is used. Also you
could just include the class in device_unittest.cc.

[mbrunson, 2016-11-07 22:06:26]

On 2016/11/06 22:47:53, ortuno wrote:
> hmm could you upload the code where DeviceConnectionHelper is used. Also you
> could just include the class in device_unittest.cc.

I've moved DeviceConnectionHelper use to device_unittest.cc.

[mbrunson, 2016-11-08 00:08:47]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 00:09:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mbrunson, 2016-11-08 00:25:57]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 00:26:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mbrunson, 2016-11-08 00:30:56]

On 2016/11/07 22:06:26, mbrunson wrote:
> On 2016/11/06 22:47:53, ortuno wrote:
> > hmm could you upload the code where DeviceConnectionHelper is used. Also you
> > could just include the class in device_unittest.cc.
> 
> I've moved DeviceConnectionHelper use to device_unittest.cc.

Made changes were discussed.

[mbrunson, 2016-11-08 00:30:59]

On 2016/11/07 22:06:26, mbrunson wrote:
> On 2016/11/06 22:47:53, ortuno wrote:
> > hmm could you upload the code where DeviceConnectionHelper is used. Also you
> > could just include the class in device_unittest.cc.
> 
> I've moved DeviceConnectionHelper use to device_unittest.cc.

Made changes were discussed.

[commit-bot: I haz the power, 2016-11-08 02:25:03]

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

[commit-bot: I haz the power, 2016-11-08 02:25:05]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-11-08 04:38:02]

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_collection.js (right):

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:92: if
(response.error ==
I would move this (if (response.error)... throw new Error(...);) to
AdapterBroker.

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:98: //
TODO(mbrunson): Replace with more descriptive error messages.
TODOs are usually used when there are other dependencies that need to be met in
order to fix an edge case or if the solution is too complex for this patch.
Either way, you should always point to an issue explaining the problem and, if
the former, block it on another issue. This way other contributors can
understand the context of the problem and submit a patch to fix it.

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:70:
handleConnectBtn_: function(row) {
So currently our code is split in two UI (Device Table) and Data (Device
Collection):
 - DeviceTable simply updates the UI whenever it's told to i.e. whenever
DeviceCollection fires events.
 - DeviceTable accesses Data (DeviceCollection) directly to retrieve items and
populate rows.
 
 - DeviceCollection is basically an array that fires events when it changes.
 - DeviceCollection is modified by AdapterBroker and DeviceCollection knows
nothing about DeviceTable.

This change makes it so that:
 - DeviceTable now calls functions on Device and needs to know about the
internals of device beyond just information.
 - Device in DeviceCollection now has functions to connect and disconnect.

These two changes are obscuring the definitions of our two modules (DeviceTable
and DeviceCollection) which will eventually make the code harder to follow. Also
this will make separating the DevicesView and DeviceDetailsView harder. I think
we can do better. More specifically:

1. DeviceTable shouldn't need to handle connection itself. Rather we should fire
an event to indicate that a connection has been requested.
2. We need to decide who's going to receive this event. Should this be
Navigation, BluetoothInternals or maybe just DeviceDetails?
3. Whoever receives this event should:
  1. Tell DeviceCollection that a device is connecting. DeviceCollection would
then fire an event to update the table.
  2. Start a connection.
  3. Once the connection is created we need to tell DeviceCollection that a
device is connected. Which would then update DeviceTable.
  4. Hold to the Device proxy.

Does that make sense? We can have a quick VC if you want to brainstorm some
more.

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:185: var
connectCell = row.cells[cellCount - 2];
Is there a better way to retrieve this? Would it make sense to have an enum of
the columns and then use that enum to retrieve these e.g.:

var COLUMNS = {
  NAME: 0,
  ADDRESS: 1,
  ...,
  CONNECTION_STATUS: 4,
  CONNECTION_ERROR: 5
};

var connectCell = row.cells[COLUMNS.CONNECTION_STATUS];

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:190: if
(device.info.is_gatt_connected) {
This is a little tricky because there are two different 'Connected' states. One
is the Wireless connection between the device and the computer and the other one
is the connection to the Device proxy.

This is really confusing and something I have to explain every time someone uses
the API, so I don't think we want to expose it in that way to users. What do you
think about just having a 'GATT Connection State' column that shows 'Connected'
if is_gatt_connected is true and 'Not Connected' otherwise. Then we could have a
button that just says 'Acquire Connection', or 'Explore' or 'Inspect', if we
don't have a proxy and 'Release Connection', or 'Forget', or 'Close' if we have
a proxy.

https://codereview.chromium.org/2448713002/diff/360001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2448713002/diff/360001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:49: ConnectToDevice(string
address) => (ConnectResult error, Device? device);
nit: s/error/result/

[mbrunson, 2016-11-08 22:45:13]

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_collection.js (right):

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:92: if
(response.error ==
On 2016/11/08 04:38:02, ortuno wrote:
> I would move this (if (response.error)... throw new Error(...);) to
> AdapterBroker.

Done.

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:98: //
TODO(mbrunson): Replace with more descriptive error messages.
On 2016/11/08 04:38:02, ortuno wrote:
> TODOs are usually used when there are other dependencies that need to be met
in
> order to fix an edge case or if the solution is too complex for this patch.
> Either way, you should always point to an issue explaining the problem and, if
> the former, block it on another issue. This way other contributors can
> understand the context of the problem and submit a patch to fix it.

Ok. I'll make an issue. Done.

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:70:
handleConnectBtn_: function(row) {
On 2016/11/08 04:38:02, ortuno wrote:
> So currently our code is split in two UI (Device Table) and Data (Device
> Collection):
>  - DeviceTable simply updates the UI whenever it's told to i.e. whenever
> DeviceCollection fires events.
>  - DeviceTable accesses Data (DeviceCollection) directly to retrieve items and
> populate rows.
>  
>  - DeviceCollection is basically an array that fires events when it changes.
>  - DeviceCollection is modified by AdapterBroker and DeviceCollection knows
> nothing about DeviceTable.
> 
> This change makes it so that:
>  - DeviceTable now calls functions on Device and needs to know about the
> internals of device beyond just information.
>  - Device in DeviceCollection now has functions to connect and disconnect.
> 
> These two changes are obscuring the definitions of our two modules
(DeviceTable
> and DeviceCollection) which will eventually make the code harder to follow.
Also
> this will make separating the DevicesView and DeviceDetailsView harder. I
think
> we can do better. More specifically:
> 
> 1. DeviceTable shouldn't need to handle connection itself. Rather we should
fire
> an event to indicate that a connection has been requested.
> 2. We need to decide who's going to receive this event. Should this be
> Navigation, BluetoothInternals or maybe just DeviceDetails?
> 3. Whoever receives this event should:
>   1. Tell DeviceCollection that a device is connecting. DeviceCollection would
> then fire an event to update the table.
>   2. Start a connection.
>   3. Once the connection is created we need to tell DeviceCollection that a
> device is connected. Which would then update DeviceTable.
>   4. Hold to the Device proxy.
> 
> Does that make sense? We can have a quick VC if you want to brainstorm some
> more.

Ahh. I see your point and agree with the event-based system.

1. Ok.
2. Probably bluetooth_internals or some messaging subsystem in it. Since it's
the container for all of the views, it is best suited to pass events between the
views. 
I added a publish/subscribe system for custom events in bluetooth_internals.
I've put the subscriber code in there for now until DevicesView is made.
3. Ok.

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:185: var
connectCell = row.cells[cellCount - 2];
On 2016/11/08 04:38:02, ortuno wrote:
> Is there a better way to retrieve this? Would it make sense to have an enum of
> the columns and then use that enum to retrieve these e.g.:
> 
> var COLUMNS = {
>   NAME: 0,
>   ADDRESS: 1,
>   ...,
>   CONNECTION_STATUS: 4,
>   CONNECTION_ERROR: 5
> };
> 
> var connectCell = row.cells[COLUMNS.CONNECTION_STATUS];

The connection-related columns are always at the end. The rest of the table is
markup driven, explicitly determined by the header layout in the HTML. An enum
seems like overkill.

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:190: if
(device.info.is_gatt_connected) {
On 2016/11/08 04:38:02, ortuno wrote:
> This is a little tricky because there are two different 'Connected' states.
One
> is the Wireless connection between the device and the computer and the other
one
> is the connection to the Device proxy.
> 
> This is really confusing and something I have to explain every time someone
uses
> the API, so I don't think we want to expose it in that way to users. What do
you
> think about just having a 'GATT Connection State' column that shows
'Connected'
> if is_gatt_connected is true and 'Not Connected' otherwise. Then we could have
a
> button that just says 'Acquire Connection', or 'Explore' or 'Inspect', if we
> don't have a proxy and 'Release Connection', or 'Forget', or 'Close' if we
have
> a proxy.

I'll use inspect/forget for now. Done.

https://codereview.chromium.org/2448713002/diff/360001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2448713002/diff/360001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:49: ConnectToDevice(string
address) => (ConnectResult error, Device? device);
On 2016/11/08 04:38:02, ortuno wrote:
> nit: s/error/result/

Done.

[ortuno, 2016-11-09 03:24:03]

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:185: var
connectCell = row.cells[cellCount - 2];
On 2016/11/08 at 22:45:13, mbrunson wrote:
> On 2016/11/08 04:38:02, ortuno wrote:
> > Is there a better way to retrieve this? Would it make sense to have an enum
of
> > the columns and then use that enum to retrieve these e.g.:
> > 
> > var COLUMNS = {
> >   NAME: 0,
> >   ADDRESS: 1,
> >   ...,
> >   CONNECTION_STATUS: 4,
> >   CONNECTION_ERROR: 5
> > };
> > 
> > var connectCell = row.cells[COLUMNS.CONNECTION_STATUS];
> 
> The connection-related columns are always at the end. The rest of the table is
markup driven, explicitly determined by the header layout in the HTML. An enum
seems like overkill.

Right, but getting a cell by counting from the end is really brittle.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:49: return
response;
Why not return just the device?

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:36:
bluetooth_internals.subscribe('connectrequest', function(event) {
I think we want to do this the other way around. DeviceTable shouldn't know
about bluetooth_internals. It should just know that is has an explore button and
that it will notify of the event to whoever is listening. So I would move
publish/subscribe to device_table and subscribe from bluetooth internals:

(Also it would be nice if we could use known concepts like addEventListener; has
there been any progress on using web components? If not then subscribe is fine
for now.)

// bluetooth_internals.js
deviceTable.subscribe('explorerequest', {
  // connect and update DeviceCollection.
});

// device_table.js
handleConnectBtn_: function(index) {
  var event = new CustomEvent('connectrequest', {
    detail: {
      index: index,
    }
  });
  dispatchEvent_(event);
},
dispatchEvent_: funtion(event) {
  // for each subscription
  subscription(event);
},

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:54: });
Should this be bound to bluetooth_internals?

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_collection.js (right):

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:75:
updateConnectionStatus: function(index, optional_error) {
I would use the address instead. If I understand correctly the location of the
device in the array could have changed during the connection.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:76: var
message = (optional_error && optional_error.message) || '';
I would save the information in the device object and just call
this.updateIndex(this.indexOf(device));

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:97:
Device.prototype = {
Before this patch this class used to only hold information about the device so
it made sense for it to be owned by DeviceCollection. But now that we are
exposing functions that are not going to be used by this class we should rethink
its ownership. So my question to you is: How will DeviceDetailsView going to
interact with this? 

My proposal would be to:
1. Turn Device into DeviceInfo and it would be basically be a struct i.e. no
functions and it will only be owned by DeviceCollection (DeviceTable will still
access it but only to retrieve information from it).
2. Have bluetooth_internals (and in the future DeviceDetailsView) manage the
connection process and hold on to the proxy.

// bluetooth_internals.js

let deviceAddressToProxy = new Map();

deviceTable.subscribe('explorepressed', (event) => {
  var address = event.detail.address;
  var proxy = deviceAddressToProxy.get(address);
  if (proxy !== undefined) {
    // Device is already connected, so we disconnect.
    proxy.disconnect();
    deviceAddressToProxy.delete(address);
    devices.updateConnectionStatus(address, 'disconnected');
  } else {
    devices.updateConnectionStatus(address, 'connecting');
    adapterBroker.connectToDevice(address)
      .then(proxy => {
        if (devices.getByAddress(address) === undefined) {
          // Device no longer in list so drop the connection.
          proxy.disconnect();
          return;
        }
        deviceAddressToProxy.set(address, proxy);
        devices.updateConnectionStatus(address, 'connected');
      }).catch(error => {
        // Show toast saying the connection has failed.
        device.updateConnectionStatus(address, 'disconnected');
      });
  }
});

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:73: index: index,
I would include the address rather than the index since the index can change
during the connection.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:120: var
connectErrorCell = row.insertCell();
Would it make sense to show a snackbar[1] instead? No need to do it in this
patch but if you decide to go in that direction you can just leave a TODO.

[1] https://material.google.com/components/snackbars-toasts.html#

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:164: if
(device.proxy) {
switch (device.connectionStatus) {
  case 'disconnected':
    connectButton.textContent = 'Forget';
    break;
  case 'connected':
    connectButton.textContent = 'Inspect';
    break;
  case 'connecting':
    // disable button.
    break;
  default: assert('case not handled');
}

[mbrunson, 2016-11-09 23:39:37]

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/360001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:185: var
connectCell = row.cells[cellCount - 2];
On 2016/11/09 03:24:03, ortuno wrote:
> On 2016/11/08 at 22:45:13, mbrunson wrote:
> > On 2016/11/08 04:38:02, ortuno wrote:
> > > Is there a better way to retrieve this? Would it make sense to have an
enum
> of
> > > the columns and then use that enum to retrieve these e.g.:
> > > 
> > > var COLUMNS = {
> > >   NAME: 0,
> > >   ADDRESS: 1,
> > >   ...,
> > >   CONNECTION_STATUS: 4,
> > >   CONNECTION_ERROR: 5
> > > };
> > > 
> > > var connectCell = row.cells[COLUMNS.CONNECTION_STATUS];
> > 
> > The connection-related columns are always at the end. The rest of the table
is
> markup driven, explicitly determined by the header layout in the HTML. An enum
> seems like overkill.
> 
> Right, but getting a cell by counting from the end is really brittle.

Done.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:49: return
response;
On 2016/11/09 03:24:03, ortuno wrote:
> Why not return just the device?

Done.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:36:
bluetooth_internals.subscribe('connectrequest', function(event) {
On 2016/11/09 03:24:03, ortuno wrote:
> I think we want to do this the other way around. DeviceTable shouldn't know
> about bluetooth_internals. It should just know that is has an explore button
and
> that it will notify of the event to whoever is listening. So I would move
> publish/subscribe to device_table and subscribe from bluetooth internals:
> 
> (Also it would be nice if we could use known concepts like addEventListener;
has
> there been any progress on using web components? If not then subscribe is fine
> for now.)
> 
> // bluetooth_internals.js
> deviceTable.subscribe('explorerequest', {
>   // connect and update DeviceCollection.
> });
> 
> // device_table.js
> handleConnectBtn_: function(index) {
>   var event = new CustomEvent('connectrequest', {
>     detail: {
>       index: index,
>     }
>   });
>   dispatchEvent_(event);
> },
> dispatchEvent_: funtion(event) {
>   // for each subscription
>   subscription(event);
> },

We can use addEventListener on DeviceTable since it's an HTMLElement which
inherits from EventTarget. Done.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:54: });
On 2016/11/09 03:24:03, ortuno wrote:
> Should this be bound to bluetooth_internals?

It doesn't really need to be.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_collection.js (right):

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:75:
updateConnectionStatus: function(index, optional_error) {
On 2016/11/09 03:24:03, ortuno wrote:
> I would use the address instead. If I understand correctly the location of the
> device in the array could have changed during the connection.

Done.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:76: var
message = (optional_error && optional_error.message) || '';
On 2016/11/09 03:24:03, ortuno wrote:
> I would save the information in the device object and just call
> this.updateIndex(this.indexOf(device));

Done.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:97:
Device.prototype = {
On 2016/11/09 03:24:03, ortuno wrote:
> Before this patch this class used to only hold information about the device so
> it made sense for it to be owned by DeviceCollection. But now that we are
> exposing functions that are not going to be used by this class we should
rethink
> its ownership. So my question to you is: How will DeviceDetailsView going to
> interact with this? 
> 
> My proposal would be to:
> 1. Turn Device into DeviceInfo and it would be basically be a struct i.e. no
> functions and it will only be owned by DeviceCollection (DeviceTable will
still
> access it but only to retrieve information from it).
> 2. Have bluetooth_internals (and in the future DeviceDetailsView) manage the
> connection process and hold on to the proxy.
> 
> // bluetooth_internals.js
> 
> let deviceAddressToProxy = new Map();
> 
> deviceTable.subscribe('explorepressed', (event) => {
>   var address = event.detail.address;
>   var proxy = deviceAddressToProxy.get(address);
>   if (proxy !== undefined) {
>     // Device is already connected, so we disconnect.
>     proxy.disconnect();
>     deviceAddressToProxy.delete(address);
>     devices.updateConnectionStatus(address, 'disconnected');
>   } else {
>     devices.updateConnectionStatus(address, 'connecting');
>     adapterBroker.connectToDevice(address)
>       .then(proxy => {
>         if (devices.getByAddress(address) === undefined) {
>           // Device no longer in list so drop the connection.
>           proxy.disconnect();
>           return;
>         }
>         deviceAddressToProxy.set(address, proxy);
>         devices.updateConnectionStatus(address, 'connected');
>       }).catch(error => {
>         // Show toast saying the connection has failed.
>         device.updateConnectionStatus(address, 'disconnected');
>       });
>   }
> });

Hmm ok. So in the final design, DeviceDetailsView would create the connection
and hold the proxy. So, each DeviceDetailsView has its own proxy that it
manages. That makes sense. Done.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:73: index: index,
On 2016/11/09 03:24:03, ortuno wrote:
> I would include the address rather than the index since the index can change
> during the connection.

Done.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:120: var
connectErrorCell = row.insertCell();
On 2016/11/09 03:24:03, ortuno wrote:
> Would it make sense to show a snackbar[1] instead? No need to do it in this
> patch but if you decide to go in that direction you can just leave a TODO.
> 
> [1] https://material.google.com/components/snackbars-toasts.html

Yeah. That would be ideal. TODO added.

https://codereview.chromium.org/2448713002/diff/380001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:164: if
(device.proxy) {
On 2016/11/09 03:24:03, ortuno wrote:
> switch (device.connectionStatus) {
>   case 'disconnected':
>     connectButton.textContent = 'Forget';
>     break;
>   case 'connected':
>     connectButton.textContent = 'Inspect';
>     break;
>   case 'connecting':
>     // disable button.
>     break;
>   default: assert('case not handled');
> }

Done.

[ortuno, 2016-11-10 02:16:39]

Some renames otherwise lgtm!

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:61:
deviceProxy.getServices().then(function(response) {
Should this information be saved in the DeviceDetailsView?

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:18:
CONNECTION_BUTTON: 5,
s/CONNECTION/INSPECT/

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:24: *
DeviceCollection. Fires events for connection requests from listed
s/connection/inspection/

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:77: * Fires a
connect pressed event for the row |index|.
nit: s/connect/inspect/

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:81:
handleConnectBtn_: function(index) {
s/Connect/Inspect/

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:126: var
connectButton = document.createElement('button');
s/connect/inspect/

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:163: switch
(device.connectionStatus) {
Ah I missed re-enabling the button.

[mbrunson, 2016-11-10 04:18:29]

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:61:
deviceProxy.getServices().then(function(response) {
On 2016/11/10 02:16:38, ortuno wrote:
> Should this information be saved in the DeviceDetailsView?

Yes. This will be cached in the DeviceDetailsView. I'll add a TODO for this
whole section of connection log to move when that gets added.

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:18:
CONNECTION_BUTTON: 5,
On 2016/11/10 02:16:38, ortuno wrote:
> s/CONNECTION/INSPECT/

Done.

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:24: *
DeviceCollection. Fires events for connection requests from listed
On 2016/11/10 02:16:38, ortuno wrote:
> s/connection/inspection/

Done.

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:77: * Fires a
connect pressed event for the row |index|.
On 2016/11/10 02:16:38, ortuno wrote:
> nit: s/connect/inspect/

Done.

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:81:
handleConnectBtn_: function(index) {
On 2016/11/10 02:16:39, ortuno wrote:
> s/Connect/Inspect/

Done.

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:126: var
connectButton = document.createElement('button');
On 2016/11/10 02:16:38, ortuno wrote:
> s/connect/inspect/

Done.

https://codereview.chromium.org/2448713002/diff/420001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:163: switch
(device.connectionStatus) {
On 2016/11/10 02:16:38, ortuno wrote:
> Ah I missed re-enabling the button.

Done.

[mbrunson, 2016-11-10 04:23:53]

mbrunson@chromium.org changed reviewers:
+ dcheng@chromium.org, dpapad@chromium.org

[mbrunson, 2016-11-10 04:23:54]

OWNERS review, please:

dcheng:
  device/bluetooth/public/interfaces/adapter.mojom
  device/bluetooth/public/interfaces/device.mojom

dpapad:
  chrome/browser/browser_resources.grd                                          
                                                                                
   
  chrome/browser/resources/bluetooth_internals/adapter_broker.js
  chrome/browser/resources/bluetooth_internals/bluetooth_internals.html
  chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
  chrome/browser/resources/bluetooth_internals/device_collection.js
  chrome/browser/resources/bluetooth_internals/device_table.js
  chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc

[dpapad, 2016-11-10 19:05:03]

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:32: * @return
{Promise<interfaces.BluetoothDevice.Device.proxyClass>}
!Promise<!...>

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:40: var
errorString = Object.keys(
Isn't this equivalent to 
interfaces.BluetoothAdapter.ConnectResult[response.result];

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:12: var
deviceAddressToProxy = new Map();
Can you annotate this with the appropriate K,V types?
@type {!Map<K,V>}

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:45: if
(proxy) {
Nit:
if (proxy) {
  ...
  return;
}

// No need for else, less indentation, more readable.
devices.updateConnectionStatus(address, 'connecting');
...

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:49:
devices.updateConnectionStatus(address, 'disconnected');
Please explain where those string literals come from (disconnected, connecting,
connected), what other data structure they should be matching, to save time on
future readers. Also how about putting all those in an enum and re-use them
across JS code? Currently they are repeated in device_table.js, which seems a
bit fragile.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_collection.js (right):

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:15: * @param
{!Array<device_collection.DeviceInfo>} array The starting collection
I am guessing that the array should not hold null values, so
!device_collection.DeviceInfo.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:51:
Object.assign(oldDeviceInfo, deviceInfo);
Are all properties of oldDeviceInfo overwritten by this? Is there any chance
that obsolete properties are left inside oldDeviceInfo (properties that exist on
oldDeviceInfo, but do not exist on deviceInfo)?

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:75: * @param
{Error} optional_error Optional Error object.
Please follow styleguide for naming optional parameters.
https://google.github.io/styleguide/javascriptguide.xml?showone=Naming#Naming

@param {!Error=} opt_error Optional Error object.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:80: var device
= this.getByAddress(address);
Nit (optional): You can also
var device = assert(this.getByAddress(address), '....');

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:156:
assert(this.body_.rows[index], 'Row ' + index + ' is not in the table.');
var row = this.body_.rows[index];
assert(row, 'Row ' + index + ' is not in the table.');

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:194: obj ? obj =
'Connected' : obj = 'Not Connected';
This usage of ternary operator is a bit odd
boolean ? foo=a : foo=b;

How about changing to
foo = boolean ? a : b;

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:198:
cell.textContent = (obj == null || obj == undefined) ? 'Unknown' : obj;
No need to check both for undefined and null, given that you are using the '=='
operator (instead of '===').

var a = null;
a == undefined; // true
a == null; // true;

var b = null;
b == undefined; // true
b == null; // true;

But besides just checking for one of null,undefined, you can simplify even more.

cell.textContent = obj || 'Unknown';

[mbrunson, 2016-11-10 22:02:22]

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:32: * @return
{Promise<interfaces.BluetoothDevice.Device.proxyClass>}
On 2016/11/10 19:05:02, dpapad wrote:
> !Promise<!...>

Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:40: var
errorString = Object.keys(
On 2016/11/10 19:05:02, dpapad wrote:
> Isn't this equivalent to 
> interfaces.BluetoothAdapter.ConnectResult[response.result];

response.result is a number. ConnectResult is an object that represents an enum
which maps a string to a number.

This actually should be replaced by a for loop. Object.keys order is not
guaranteed but has been correct since I put this here.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:12: var
deviceAddressToProxy = new Map();
On 2016/11/10 19:05:02, dpapad wrote:
> Can you annotate this with the appropriate K,V types?
> @type {!Map<K,V>}

Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:45: if
(proxy) {
On 2016/11/10 19:05:02, dpapad wrote:
> Nit:
> if (proxy) {
>   ...
>   return;
> }
> 
> // No need for else, less indentation, more readable.
> devices.updateConnectionStatus(address, 'connecting');
> ...

Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:49:
devices.updateConnectionStatus(address, 'disconnected');
On 2016/11/10 19:05:02, dpapad wrote:
> Please explain where those string literals come from (disconnected,
connecting,
> connected), what other data structure they should be matching, to save time on
> future readers. Also how about putting all those in an enum and re-use them
> across JS code? Currently they are repeated in device_table.js, which seems a
> bit fragile.

Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_collection.js (right):

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:15: * @param
{!Array<device_collection.DeviceInfo>} array The starting collection
On 2016/11/10 19:05:02, dpapad wrote:
> I am guessing that the array should not hold null values, so
> !device_collection.DeviceInfo.

Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:51:
Object.assign(oldDeviceInfo, deviceInfo);
On 2016/11/10 19:05:02, dpapad wrote:
> Are all properties of oldDeviceInfo overwritten by this? Is there any chance
> that obsolete properties are left inside oldDeviceInfo (properties that exist
on
> oldDeviceInfo, but do not exist on deviceInfo)?

Yes. deviceInfo.rssi may be null while oldDeviceInfo.rssi will definitely exist.
The lines that follow try to preserve the correct values.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:75: * @param
{Error} optional_error Optional Error object.
On 2016/11/10 19:05:02, dpapad wrote:
> Please follow styleguide for naming optional parameters.
> https://google.github.io/styleguide/javascriptguide.xml?showone=Naming#Naming
> 
> @param {!Error=} opt_error Optional Error object.

Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:80: var device
= this.getByAddress(address);
On 2016/11/10 19:05:02, dpapad wrote:
> Nit (optional): You can also
> var device = assert(this.getByAddress(address), '....');

Oh. That's cool! Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_table.js (right):

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:156:
assert(this.body_.rows[index], 'Row ' + index + ' is not in the table.');
On 2016/11/10 19:05:02, dpapad wrote:
> var row = this.body_.rows[index];
> assert(row, 'Row ' + index + ' is not in the table.');

Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:194: obj ? obj =
'Connected' : obj = 'Not Connected';
On 2016/11/10 19:05:03, dpapad wrote:
> This usage of ternary operator is a bit odd
> boolean ? foo=a : foo=b;
> 
> How about changing to
> foo = boolean ? a : b;

Done.

https://codereview.chromium.org/2448713002/diff/460001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_table.js:198:
cell.textContent = (obj == null || obj == undefined) ? 'Unknown' : obj;
On 2016/11/10 19:05:03, dpapad wrote:
> No need to check both for undefined and null, given that you are using the
'=='
> operator (instead of '===').
> 
> var a = null;
> a == undefined; // true
> a == null; // true;
> 
> var b = null;
> b == undefined; // true
> b == null; // true;
> 
> But besides just checking for one of null,undefined, you can simplify even
more.
> 
> cell.textContent = obj || 'Unknown';

Ah true. I forgot about the '=='/'===' thing with null. I'll do that then.

Empty string and zero are falsy, so I don't think I want it to show 'Unknown' if
it's a value that is actually supposed to be 0 or ''.

[dpapad, 2016-11-10 22:33:20]

LGTM with nits.

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:57:
adapterBroker.connectToDevice(address).then(function(deviceProxy) {
Can you chain instead of nest Promise callbacks? Chained callbacks are less
readable, but also your current catch() call at line 74 will only catch some
errors but not all.


  return adapterBroker.connectToDevice(address);
}).then(function(deviceProxy) {
...
...
  return deviceProxy.getServices();
}).then(response) {
...
}).catch(e) {
  // catches any error that might have been generated above.
})

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_collection.js (right):

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:15: * by
default.
@enum {number}

[dpapad, 2016-11-10 22:33:59]

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:57:
adapterBroker.connectToDevice(address).then(function(deviceProxy) {
On 2016/11/10 at 22:33:20, dpapad wrote:
> Can you chain instead of nest Promise callbacks? Chained callbacks are less
readable, but also your current catch() call at line 74 will only catch some
errors but not all.
> 
> 
>   return adapterBroker.connectToDevice(address);
> }).then(function(deviceProxy) {
> ...
> ...
>   return deviceProxy.getServices();
> }).then(response) {
> ...
> }).catch(e) {
>   // catches any error that might have been generated above.
> })

Correction:
*nested callbacks are less readable

[dcheng, 2016-11-11 01:25:33]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
Let's use EnumTraits for this:
https://www.chromium.org/developers/design-documents/mojo/type-mapping

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device.cc:20: // Binding is owned by this Device, so
base::Unretatined is safe.
Nit: typo

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
Let's just use StrongBinding. It does make this code a bit more complicated
(specifically, the delete this in DeviceChanged), but it requires less "delete
this"... so I think that's a net win overall.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:31: NiceMockBluetoothGattConnection;
Nit: prefer using A = B; to typedef B A; in new code.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:58:
std::unique_ptr<NiceMockBluetoothGattService> service1(
auto service1 = base::MakeUnique<NiceMockBluetoothGattService>(...);

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:63:
std::unique_ptr<NiceMockBluetoothGattService> service2(
Ditto.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:88: delete device_service_;
This is confusing. Previous comments seem to imply it's self-owned, but
apparently not really?

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:129: std::unique_ptr<base::MessageLoop>
message_loop_;
Nit: device_ and message_loop_ can just be members of the text fixture class
directly, I think.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/test/...
File device/bluetooth/test/device_connection_helper.h (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/test/...
device/bluetooth/test/device_connection_helper.h:15: // the owning callback is
destroyed before the message pipe is closed.
The test code is pretty complex, and part of that ends up bleeding over into
non-test code. Can you help me understand why it's necessary to verify these
sorts of things? What sort of problems does this test aim to catch?

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/test/...
device/bluetooth/test/device_connection_helper.h:18:
DeviceConnectionHelper(mojo::Binding<mojom::Device>* binding);
Nit: explicit

[mbrunson, 2016-11-11 21:26:15]

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:57:
adapterBroker.connectToDevice(address).then(function(deviceProxy) {
On 2016/11/10 22:33:59, dpapad wrote:
> On 2016/11/10 at 22:33:20, dpapad wrote:
> > Can you chain instead of nest Promise callbacks? Chained callbacks are less
> readable, but also your current catch() call at line 74 will only catch some
> errors but not all.
> > 
> > 
> >   return adapterBroker.connectToDevice(address);
> > }).then(function(deviceProxy) {
> > ...
> > ...
> >   return deviceProxy.getServices();
> > }).then(response) {
> > ...
> > }).catch(e) {
> >   // catches any error that might have been generated above.
> > })
> 
> Correction:
> *nested callbacks are less readable

Done.

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/device_collection.js (right):

https://codereview.chromium.org/2448713002/diff/480001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/device_collection.js:15: * by
default.
On 2016/11/10 22:33:20, dpapad wrote:
> @enum {number}

Done.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/11 01:25:32, dcheng wrote:
> Let's use EnumTraits for this:
> https://www.chromium.org/developers/design-documents/mojo/type-mapping

Is it possible to use EnumTraits when the C++ class only maps a subset of the
Mojom enum?

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device.cc:20: // Binding is owned by this Device, so
base::Unretatined is safe.
On 2016/11/11 01:25:33, dcheng wrote:
> Nit: typo

Done.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
On 2016/11/11 01:25:33, dcheng wrote:
> Let's just use StrongBinding. It does make this code a bit more complicated
> (specifically, the delete this in DeviceChanged), but it requires less "delete
> this"... so I think that's a net win overall.

So, would you recommend moving control of the binding to the Adapter since the
Device can't control its own lifetime?

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:31: NiceMockBluetoothGattConnection;
On 2016/11/11 01:25:33, dcheng wrote:
> Nit: prefer using A = B; to typedef B A; in new code.

Done.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:58:
std::unique_ptr<NiceMockBluetoothGattService> service1(
On 2016/11/11 01:25:33, dcheng wrote:
> auto service1 = base::MakeUnique<NiceMockBluetoothGattService>(...);

Done.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:63:
std::unique_ptr<NiceMockBluetoothGattService> service2(
On 2016/11/11 01:25:33, dcheng wrote:
> Ditto.

Done.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:88: delete device_service_;
On 2016/11/11 01:25:33, dcheng wrote:
> This is confusing. Previous comments seem to imply it's self-owned, but
> apparently not really?

There are a few tests that determine whether the Device actually deletes itself
if a disconnection occurs. If this isn't expected to occur, I delete it manually
here to prevent it from leaking.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:129: std::unique_ptr<base::MessageLoop>
message_loop_;
On 2016/11/11 01:25:33, dcheng wrote:
> Nit: device_ and message_loop_ can just be members of the text fixture class
> directly, I think.

Are you saying they should be put in the TEST_F sections or in the constructor?
They're already in the BluetoothInterfaceDeviceTest class.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/test/...
File device/bluetooth/test/device_connection_helper.h (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/test/...
device/bluetooth/test/device_connection_helper.h:15: // the owning callback is
destroyed before the message pipe is closed.
On 2016/11/11 01:25:33, dcheng wrote:
> The test code is pretty complex, and part of that ends up bleeding over into
> non-test code. Can you help me understand why it's necessary to verify these
> sorts of things? What sort of problems does this test aim to catch?

It's mainly to test whether the callbacks for a Device.GetServices request are
deleted before the Device's message pipe is closed.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/test/...
device/bluetooth/test/device_connection_helper.h:18:
DeviceConnectionHelper(mojo::Binding<mojom::Device>* binding);
On 2016/11/11 01:25:33, dcheng wrote:
> Nit: explicit

Done.

[dcheng, 2016-11-15 07:24:02]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/11 21:26:15, mbrunson wrote:
> On 2016/11/11 01:25:32, dcheng wrote:
> > Let's use EnumTraits for this:
> > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> 
> Is it possible to use EnumTraits when the C++ class only maps a subset of the
> Mojom enum?

Yes, that's not a problem.

(If the conversion is only one way, e.g. C++ -> mojom, it'd probably even be OK
to stub out the the deserialization and assert NOTREACHED())

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
On 2016/11/11 21:26:15, mbrunson wrote:
> On 2016/11/11 01:25:33, dcheng wrote:
> > Let's just use StrongBinding. It does make this code a bit more complicated
> > (specifically, the delete this in DeviceChanged), but it requires less
"delete
> > this"... so I think that's a net win overall.
> 
> So, would you recommend moving control of the binding to the Adapter since the
> Device can't control its own lifetime?

Yes, this implies Adapter will be calling MakeStrongBinding.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:129: std::unique_ptr<base::MessageLoop>
message_loop_;
On 2016/11/11 21:26:15, mbrunson wrote:
> On 2016/11/11 01:25:33, dcheng wrote:
> > Nit: device_ and message_loop_ can just be members of the text fixture class
> > directly, I think.
> 
> Are you saying they should be put in the TEST_F sections or in the
constructor?
> They're already in the BluetoothInterfaceDeviceTest class.

The unique_ptr indirection is unnecessary: they can just be
NiceMockBluetoothDevice device_ and base::MessageLoop message_loop_.

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/device.h
File device/bluetooth/device.h (right):

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/devic...
device/bluetooth/device.h:43: mojo::Binding<mojom::Device>*
GetBindingForTesting();
The test can just check the other end of the pipe if it needs to verify the pipe
is closed.

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:73:
std::unique_ptr<NiceMockBluetoothGattConnection> connection(
auto connection = base::MakeUnique<NiceMockBluetoothGattConnection>(...) will
reduce the amount of typing here.

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:85: delete device_service_;
With a strong binding, this can just close the message pipe.

[mbrunson, 2016-11-15 21:50:35]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
On 2016/11/15 07:24:02, dcheng wrote:
> On 2016/11/11 21:26:15, mbrunson wrote:
> > On 2016/11/11 01:25:33, dcheng wrote:
> > > Let's just use StrongBinding. It does make this code a bit more
complicated
> > > (specifically, the delete this in DeviceChanged), but it requires less
> "delete
> > > this"... so I think that's a net win overall.
> > 
> > So, would you recommend moving control of the binding to the Adapter since
the
> > Device can't control its own lifetime?
> 
> Yes, this implies Adapter will be calling MakeStrongBinding.

I hesitate to do this. 

As you said, this complicates the code quite a bit and will require many more
tests to verify the proper management of all of the active Device instances.
I've seen the current approach used in places with similar lifetime concerns [1]
where the interface's lifetime is managed by both the message pipe and some
other object.

[1]
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...

[ortuno, 2016-11-15 21:58:09]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
On 2016/11/15 at 21:50:35, mbrunson wrote:
> On 2016/11/15 07:24:02, dcheng wrote:
> > On 2016/11/11 21:26:15, mbrunson wrote:
> > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > Let's just use StrongBinding. It does make this code a bit more
complicated
> > > > (specifically, the delete this in DeviceChanged), but it requires less
> > "delete
> > > > this"... so I think that's a net win overall.
> > > 
> > > So, would you recommend moving control of the binding to the Adapter since
the
> > > Device can't control its own lifetime?
> > 
> > Yes, this implies Adapter will be calling MakeStrongBinding.
> 
> I hesitate to do this. 
> 
> As you said, this complicates the code quite a bit and will require many more
tests to verify the proper management of all of the active Device instances.
I've seen the current approach used in places with similar lifetime concerns [1]
where the interface's lifetime is managed by both the message pipe and some
other object.
> 
> [1]
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...

+1

[dcheng, 2016-11-15 22:07:36]

On 2016/11/15 21:58:09, ortuno wrote:
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> File device/bluetooth/device.cc (right):
> 
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
> base::Unretained(this)));
> On 2016/11/15 at 21:50:35, mbrunson wrote:
> > On 2016/11/15 07:24:02, dcheng wrote:
> > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > Let's just use StrongBinding. It does make this code a bit more
> complicated
> > > > > (specifically, the delete this in DeviceChanged), but it requires less
> > > "delete
> > > > > this"... so I think that's a net win overall.
> > > > 
> > > > So, would you recommend moving control of the binding to the Adapter
since
> the
> > > > Device can't control its own lifetime?
> > > 
> > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > 
> > I hesitate to do this. 
> > 
> > As you said, this complicates the code quite a bit and will require many
more
> tests to verify the proper management of all of the active Device instances.
> I've seen the current approach used in places with similar lifetime concerns
[1]
> where the interface's lifetime is managed by both the message pipe and some
> other object.
> > 
> > [1]
>
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> 
> +1

Just because there's prior precedent doesn't mean it's good =)

Having raw 'new' like this generally makes code harder to read: I have to read a
lot more to understand how it's deleted, and I need to read a lot more code to
understand when that might not happen. This isn't just a hypothetical concern:
I've found other mojo code that tries to use raw 'new' and ends up leaking the
impl object.

[dcheng, 2016-11-15 22:08:38]

dcheng@chromium.org changed reviewers:
+ rockot@chromium.org

[dcheng, 2016-11-15 22:08:40]

On 2016/11/15 22:07:36, dcheng wrote:
> On 2016/11/15 21:58:09, ortuno wrote:
> >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > File device/bluetooth/device.cc (right):
> > 
> >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
> > base::Unretained(this)));
> > On 2016/11/15 at 21:50:35, mbrunson wrote:
> > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > > Let's just use StrongBinding. It does make this code a bit more
> > complicated
> > > > > > (specifically, the delete this in DeviceChanged), but it requires
less
> > > > "delete
> > > > > > this"... so I think that's a net win overall.
> > > > > 
> > > > > So, would you recommend moving control of the binding to the Adapter
> since
> > the
> > > > > Device can't control its own lifetime?
> > > > 
> > > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > > 
> > > I hesitate to do this. 
> > > 
> > > As you said, this complicates the code quite a bit and will require many
> more
> > tests to verify the proper management of all of the active Device instances.
> > I've seen the current approach used in places with similar lifetime concerns
> [1]
> > where the interface's lifetime is managed by both the message pipe and some
> > other object.
> > > 
> > > [1]
> >
>
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> > 
> > +1
> 
> Just because there's prior precedent doesn't mean it's good =)
> 
> Having raw 'new' like this generally makes code harder to read: I have to read
a
> lot more to understand how it's deleted, and I need to read a lot more code to
> understand when that might not happen. This isn't just a hypothetical concern:
> I've found other mojo code that tries to use raw 'new' and ends up leaking the
> impl object.

+rockot as well, I have personally found StrongBinding a bit of an awkward fit
for these sort of instance as well, maybe there's something we can tweak to make
it easier to use.

[ortuno, 2016-11-15 22:31:44]

On 2016/11/15 at 22:08:40, dcheng wrote:
> On 2016/11/15 22:07:36, dcheng wrote:
> > On 2016/11/15 21:58:09, ortuno wrote:
> > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > File device/bluetooth/device.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
> > > base::Unretained(this)));
> > > On 2016/11/15 at 21:50:35, mbrunson wrote:
> > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > > > Let's just use StrongBinding. It does make this code a bit more
> > > complicated
> > > > > > > (specifically, the delete this in DeviceChanged), but it requires
less
> > > > > "delete
> > > > > > > this"... so I think that's a net win overall.
> > > > > > 
> > > > > > So, would you recommend moving control of the binding to the Adapter
> > since
> > > the
> > > > > > Device can't control its own lifetime?
> > > > > 
> > > > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > > > 
> > > > I hesitate to do this. 
> > > > 
> > > > As you said, this complicates the code quite a bit and will require many
> > more
> > > tests to verify the proper management of all of the active Device
instances.
> > > I've seen the current approach used in places with similar lifetime
concerns
> > [1]
> > > where the interface's lifetime is managed by both the message pipe and
some
> > > other object.
> > > > 
> > > > [1]
> > >
> >
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> > > 
> > > +1
> > 
> > Just because there's prior precedent doesn't mean it's good =)
> > 
> > Having raw 'new' like this generally makes code harder to read: I have to
read a
> > lot more to understand how it's deleted, and I need to read a lot more code
to
> > understand when that might not happen. This isn't just a hypothetical
concern:
> > I've found other mojo code that tries to use raw 'new' and ends up leaking
the
> > impl object.
> 
> +rockot as well, I have personally found StrongBinding a bit of an awkward fit
for these sort of instance as well, maybe there's something we can tweak to make
it easier to use.

I'm not sure how your proposal addresses the concern though. Currently, Device
manages its own lifetime and it's all encapsulated in the error handler,
DeviceChanged and Disconnect. Using strong binding would spread the lifetime
management into StrongBinding, Adapter and Device::Disconnect, which not only
increases the amount of code (now Adapter has to keep a map of all devices and
manage their lifetimes) but is also harder to follow. Furthermore, as mbrunson
mentioned, the amount of new testing code we would add for this is non trivial
as we would need a new suit of tests for Adapter.

[dcheng, 2016-11-15 22:44:46]

On 2016/11/15 22:31:44, ortuno wrote:
> On 2016/11/15 at 22:08:40, dcheng wrote:
> > On 2016/11/15 22:07:36, dcheng wrote:
> > > On 2016/11/15 21:58:09, ortuno wrote:
> > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > File device/bluetooth/device.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
> > > > base::Unretained(this)));
> > > > On 2016/11/15 at 21:50:35, mbrunson wrote:
> > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > > > > Let's just use StrongBinding. It does make this code a bit more
> > > > complicated
> > > > > > > > (specifically, the delete this in DeviceChanged), but it
requires
> less
> > > > > > "delete
> > > > > > > > this"... so I think that's a net win overall.
> > > > > > > 
> > > > > > > So, would you recommend moving control of the binding to the
Adapter
> > > since
> > > > the
> > > > > > > Device can't control its own lifetime?
> > > > > > 
> > > > > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > > > > 
> > > > > I hesitate to do this. 
> > > > > 
> > > > > As you said, this complicates the code quite a bit and will require
many
> > > more
> > > > tests to verify the proper management of all of the active Device
> instances.
> > > > I've seen the current approach used in places with similar lifetime
> concerns
> > > [1]
> > > > where the interface's lifetime is managed by both the message pipe and
> some
> > > > other object.
> > > > > 
> > > > > [1]
> > > >
> > >
>
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> > > > 
> > > > +1
> > > 
> > > Just because there's prior precedent doesn't mean it's good =)
> > > 
> > > Having raw 'new' like this generally makes code harder to read: I have to
> read a
> > > lot more to understand how it's deleted, and I need to read a lot more
code
> to
> > > understand when that might not happen. This isn't just a hypothetical
> concern:
> > > I've found other mojo code that tries to use raw 'new' and ends up leaking
> the
> > > impl object.
> > 
> > +rockot as well, I have personally found StrongBinding a bit of an awkward
fit
> for these sort of instance as well, maybe there's something we can tweak to
make
> it easier to use.
> 
> I'm not sure how your proposal addresses the concern though. Currently, Device
> manages its own lifetime and it's all encapsulated in the error handler,
> DeviceChanged and Disconnect.

It's easy to understand now since we just wrote it. However, someone coming
along later will just
see 'new X', and wonder how it's not leaked (and what prevents it from getting
leaked). Then they'll have to read through all the different bits of code and
try to understand how they fit together...

> Using strong binding would spread the lifetime
> management into StrongBinding, Adapter and Device::Disconnect, which not only
> increases the amount of code (now Adapter has to keep a map of all devices and
> manage their lifetimes) but is also harder to follow.

I'm not sure why Adapter comes into this; it can still more or less be
internally managed by Device: the awkwardness here is that the strong binding is
external to the thing it's managing, so it's harder for the managed object to
self-delete.

> Furthermore, as mbrunson
> mentioned, the amount of new testing code we would add for this is non trivial
> as we would need a new suit of tests for Adapter.

If there's no Adapter changes, I don't think we'd need new tests?

[ortuno, 2016-11-15 22:51:57]

On 2016/11/15 at 22:44:46, dcheng wrote:
> On 2016/11/15 22:31:44, ortuno wrote:
> > On 2016/11/15 at 22:08:40, dcheng wrote:
> > > On 2016/11/15 22:07:36, dcheng wrote:
> > > > On 2016/11/15 21:58:09, ortuno wrote:
> > > > >
> > > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > > File device/bluetooth/device.cc (right):
> > > > > 
> > > > >
> > > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > > device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
> > > > > base::Unretained(this)));
> > > > > On 2016/11/15 at 21:50:35, mbrunson wrote:
> > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > > > > > Let's just use StrongBinding. It does make this code a bit
more
> > > > > complicated
> > > > > > > > > (specifically, the delete this in DeviceChanged), but it
requires
> > less
> > > > > > > "delete
> > > > > > > > > this"... so I think that's a net win overall.
> > > > > > > > 
> > > > > > > > So, would you recommend moving control of the binding to the
Adapter
> > > > since
> > > > > the
> > > > > > > > Device can't control its own lifetime?
> > > > > > > 
> > > > > > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > > > > > 
> > > > > > I hesitate to do this. 
> > > > > > 
> > > > > > As you said, this complicates the code quite a bit and will require
many
> > > > more
> > > > > tests to verify the proper management of all of the active Device
> > instances.
> > > > > I've seen the current approach used in places with similar lifetime
> > concerns
> > > > [1]
> > > > > where the interface's lifetime is managed by both the message pipe and
> > some
> > > > > other object.
> > > > > > 
> > > > > > [1]
> > > > >
> > > >
> >
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> > > > > 
> > > > > +1
> > > > 
> > > > Just because there's prior precedent doesn't mean it's good =)
> > > > 
> > > > Having raw 'new' like this generally makes code harder to read: I have
to
> > read a
> > > > lot more to understand how it's deleted, and I need to read a lot more
code
> > to
> > > > understand when that might not happen. This isn't just a hypothetical
> > concern:
> > > > I've found other mojo code that tries to use raw 'new' and ends up
leaking
> > the
> > > > impl object.
> > > 
> > > +rockot as well, I have personally found StrongBinding a bit of an awkward
fit
> > for these sort of instance as well, maybe there's something we can tweak to
make
> > it easier to use.
> > 
> > I'm not sure how your proposal addresses the concern though. Currently,
Device
> > manages its own lifetime and it's all encapsulated in the error handler,
> > DeviceChanged and Disconnect.
> 
> It's easy to understand now since we just wrote it. However, someone coming
along later will just
> see 'new X', and wonder how it's not leaked (and what prevents it from getting
leaked). Then they'll have to read through all the different bits of code and
try to understand how they fit together...
> 
> > Using strong binding would spread the lifetime
> > management into StrongBinding, Adapter and Device::Disconnect, which not
only
> > increases the amount of code (now Adapter has to keep a map of all devices
and
> > manage their lifetimes) but is also harder to follow.
> 
> I'm not sure why Adapter comes into this; it can still more or less be
internally managed by Device: the awkwardness here is that the strong binding is
external to the thing it's managing, so it's harder for the managed object to
self-delete.
> 
> > Furthermore, as mbrunson
> > mentioned, the amount of new testing code we would add for this is non
trivial
> > as we would need a new suit of tests for Adapter.
> 
> If there's no Adapter changes, I don't think we'd need new tests?

Sorry I might have misunderstood your proposal:

>> mbrunson:
>> So, would you recommend moving control of the binding to the Adapter since
the
>> Device can't control its own lifetime?

> dcheng:
> Yes, this implies Adapter will be calling MakeStrongBinding.

I interpreted that as, Adapter will call MakeStrongBinding and manage the
lifetime of Device through the WeakPtr returned. So how would the Device manage
its lifetime if we use StrongBinding?

[dcheng, 2016-11-15 23:00:55]

On 2016/11/15 22:51:57, ortuno wrote:
> On 2016/11/15 at 22:44:46, dcheng wrote:
> > On 2016/11/15 22:31:44, ortuno wrote:
> > > On 2016/11/15 at 22:08:40, dcheng wrote:
> > > > On 2016/11/15 22:07:36, dcheng wrote:
> > > > > On 2016/11/15 21:58:09, ortuno wrote:
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > > > File device/bluetooth/device.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > > > device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
> > > > > > base::Unretained(this)));
> > > > > > On 2016/11/15 at 21:50:35, mbrunson wrote:
> > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > > > > > > Let's just use StrongBinding. It does make this code a bit
> more
> > > > > > complicated
> > > > > > > > > > (specifically, the delete this in DeviceChanged), but it
> requires
> > > less
> > > > > > > > "delete
> > > > > > > > > > this"... so I think that's a net win overall.
> > > > > > > > > 
> > > > > > > > > So, would you recommend moving control of the binding to the
> Adapter
> > > > > since
> > > > > > the
> > > > > > > > > Device can't control its own lifetime?
> > > > > > > > 
> > > > > > > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > > > > > > 
> > > > > > > I hesitate to do this. 
> > > > > > > 
> > > > > > > As you said, this complicates the code quite a bit and will
require
> many
> > > > > more
> > > > > > tests to verify the proper management of all of the active Device
> > > instances.
> > > > > > I've seen the current approach used in places with similar lifetime
> > > concerns
> > > > > [1]
> > > > > > where the interface's lifetime is managed by both the message pipe
and
> > > some
> > > > > > other object.
> > > > > > > 
> > > > > > > [1]
> > > > > >
> > > > >
> > >
>
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> > > > > > 
> > > > > > +1
> > > > > 
> > > > > Just because there's prior precedent doesn't mean it's good =)
> > > > > 
> > > > > Having raw 'new' like this generally makes code harder to read: I have
> to
> > > read a
> > > > > lot more to understand how it's deleted, and I need to read a lot more
> code
> > > to
> > > > > understand when that might not happen. This isn't just a hypothetical
> > > concern:
> > > > > I've found other mojo code that tries to use raw 'new' and ends up
> leaking
> > > the
> > > > > impl object.
> > > > 
> > > > +rockot as well, I have personally found StrongBinding a bit of an
awkward
> fit
> > > for these sort of instance as well, maybe there's something we can tweak
to
> make
> > > it easier to use.
> > > 
> > > I'm not sure how your proposal addresses the concern though. Currently,
> Device
> > > manages its own lifetime and it's all encapsulated in the error handler,
> > > DeviceChanged and Disconnect.
> > 
> > It's easy to understand now since we just wrote it. However, someone coming
> along later will just
> > see 'new X', and wonder how it's not leaked (and what prevents it from
getting
> leaked). Then they'll have to read through all the different bits of code and
> try to understand how they fit together...
> > 
> > > Using strong binding would spread the lifetime
> > > management into StrongBinding, Adapter and Device::Disconnect, which not
> only
> > > increases the amount of code (now Adapter has to keep a map of all devices
> and
> > > manage their lifetimes) but is also harder to follow.
> > 
> > I'm not sure why Adapter comes into this; it can still more or less be
> internally managed by Device: the awkwardness here is that the strong binding
is
> external to the thing it's managing, so it's harder for the managed object to
> self-delete.
> > 
> > > Furthermore, as mbrunson
> > > mentioned, the amount of new testing code we would add for this is non
> trivial
> > > as we would need a new suit of tests for Adapter.
> > 
> > If there's no Adapter changes, I don't think we'd need new tests?
> 
> Sorry I might have misunderstood your proposal:
> 
> >> mbrunson:
> >> So, would you recommend moving control of the binding to the Adapter since
> the
> >> Device can't control its own lifetime?
> 
> > dcheng:
> > Yes, this implies Adapter will be calling MakeStrongBinding.
> 
> I interpreted that as, Adapter will call MakeStrongBinding and manage the
> lifetime of Device through the WeakPtr returned. So how would the Device
manage
> its lifetime if we use StrongBinding?

The Device itself would keep a WeakPtr to its own StrongBinding--that's the
awkward part. Example of some code that does this:
https://codereview.chromium.org/2398833003

[ortuno, 2016-11-15 23:09:07]

On 2016/11/15 at 23:00:55, dcheng wrote:
> On 2016/11/15 22:51:57, ortuno wrote:
> > On 2016/11/15 at 22:44:46, dcheng wrote:
> > > On 2016/11/15 22:31:44, ortuno wrote:
> > > > On 2016/11/15 at 22:08:40, dcheng wrote:
> > > > > On 2016/11/15 22:07:36, dcheng wrote:
> > > > > > On 2016/11/15 21:58:09, ortuno wrote:
> > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > > > > File device/bluetooth/device.cc (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > > > > device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
> > > > > > > base::Unretained(this)));
> > > > > > > On 2016/11/15 at 21:50:35, mbrunson wrote:
> > > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > > > > > > > Let's just use StrongBinding. It does make this code a bit
> > more
> > > > > > > complicated
> > > > > > > > > > > (specifically, the delete this in DeviceChanged), but it
> > requires
> > > > less
> > > > > > > > > "delete
> > > > > > > > > > > this"... so I think that's a net win overall.
> > > > > > > > > > 
> > > > > > > > > > So, would you recommend moving control of the binding to the
> > Adapter
> > > > > > since
> > > > > > > the
> > > > > > > > > > Device can't control its own lifetime?
> > > > > > > > > 
> > > > > > > > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > > > > > > > 
> > > > > > > > I hesitate to do this. 
> > > > > > > > 
> > > > > > > > As you said, this complicates the code quite a bit and will
require
> > many
> > > > > > more
> > > > > > > tests to verify the proper management of all of the active Device
> > > > instances.
> > > > > > > I've seen the current approach used in places with similar
lifetime
> > > > concerns
> > > > > > [1]
> > > > > > > where the interface's lifetime is managed by both the message pipe
and
> > > > some
> > > > > > > other object.
> > > > > > > > 
> > > > > > > > [1]
> > > > > > >
> > > > > >
> > > >
> >
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> > > > > > > 
> > > > > > > +1
> > > > > > 
> > > > > > Just because there's prior precedent doesn't mean it's good =)
> > > > > > 
> > > > > > Having raw 'new' like this generally makes code harder to read: I
have
> > to
> > > > read a
> > > > > > lot more to understand how it's deleted, and I need to read a lot
more
> > code
> > > > to
> > > > > > understand when that might not happen. This isn't just a
hypothetical
> > > > concern:
> > > > > > I've found other mojo code that tries to use raw 'new' and ends up
> > leaking
> > > > the
> > > > > > impl object.
> > > > > 
> > > > > +rockot as well, I have personally found StrongBinding a bit of an
awkward
> > fit
> > > > for these sort of instance as well, maybe there's something we can tweak
to
> > make
> > > > it easier to use.
> > > > 
> > > > I'm not sure how your proposal addresses the concern though. Currently,
> > Device
> > > > manages its own lifetime and it's all encapsulated in the error handler,
> > > > DeviceChanged and Disconnect.
> > > 
> > > It's easy to understand now since we just wrote it. However, someone
coming
> > along later will just
> > > see 'new X', and wonder how it's not leaked (and what prevents it from
getting
> > leaked). Then they'll have to read through all the different bits of code
and
> > try to understand how they fit together...
> > > 
> > > > Using strong binding would spread the lifetime
> > > > management into StrongBinding, Adapter and Device::Disconnect, which not
> > only
> > > > increases the amount of code (now Adapter has to keep a map of all
devices
> > and
> > > > manage their lifetimes) but is also harder to follow.
> > > 
> > > I'm not sure why Adapter comes into this; it can still more or less be
> > internally managed by Device: the awkwardness here is that the strong
binding is
> > external to the thing it's managing, so it's harder for the managed object
to
> > self-delete.
> > > 
> > > > Furthermore, as mbrunson
> > > > mentioned, the amount of new testing code we would add for this is non
> > trivial
> > > > as we would need a new suit of tests for Adapter.
> > > 
> > > If there's no Adapter changes, I don't think we'd need new tests?
> > 
> > Sorry I might have misunderstood your proposal:
> > 
> > >> mbrunson:
> > >> So, would you recommend moving control of the binding to the Adapter
since
> > the
> > >> Device can't control its own lifetime?
> > 
> > > dcheng:
> > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > 
> > I interpreted that as, Adapter will call MakeStrongBinding and manage the
> > lifetime of Device through the WeakPtr returned. So how would the Device
manage
> > its lifetime if we use StrongBinding?
> 
> The Device itself would keep a WeakPtr to its own StrongBinding--that's the
awkward part. Example of some code that does this:
https://codereview.chromium.org/2398833003

Ah. That makes sense (definitely something that could be added to mojo).
mbrunson, any objections?

[mbrunson, 2016-11-15 23:11:39]

On 2016/11/15 23:09:07, ortuno wrote:
> On 2016/11/15 at 23:00:55, dcheng wrote:
> > On 2016/11/15 22:51:57, ortuno wrote:
> > > On 2016/11/15 at 22:44:46, dcheng wrote:
> > > > On 2016/11/15 22:31:44, ortuno wrote:
> > > > > On 2016/11/15 at 22:08:40, dcheng wrote:
> > > > > > On 2016/11/15 22:07:36, dcheng wrote:
> > > > > > > On 2016/11/15 21:58:09, ortuno wrote:
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > > > > > File device/bluetooth/device.cc (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
> > > > > > > > device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
> > > > > > > > base::Unretained(this)));
> > > > > > > > On 2016/11/15 at 21:50:35, mbrunson wrote:
> > > > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > > > > > > > > Let's just use StrongBinding. It does make this code a
bit
> > > more
> > > > > > > > complicated
> > > > > > > > > > > > (specifically, the delete this in DeviceChanged), but it
> > > requires
> > > > > less
> > > > > > > > > > "delete
> > > > > > > > > > > > this"... so I think that's a net win overall.
> > > > > > > > > > > 
> > > > > > > > > > > So, would you recommend moving control of the binding to
the
> > > Adapter
> > > > > > > since
> > > > > > > > the
> > > > > > > > > > > Device can't control its own lifetime?
> > > > > > > > > > 
> > > > > > > > > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > > > > > > > > 
> > > > > > > > > I hesitate to do this. 
> > > > > > > > > 
> > > > > > > > > As you said, this complicates the code quite a bit and will
> require
> > > many
> > > > > > > more
> > > > > > > > tests to verify the proper management of all of the active
Device
> > > > > instances.
> > > > > > > > I've seen the current approach used in places with similar
> lifetime
> > > > > concerns
> > > > > > > [1]
> > > > > > > > where the interface's lifetime is managed by both the message
pipe
> and
> > > > > some
> > > > > > > > other object.
> > > > > > > > > 
> > > > > > > > > [1]
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> > > > > > > > 
> > > > > > > > +1
> > > > > > > 
> > > > > > > Just because there's prior precedent doesn't mean it's good =)
> > > > > > > 
> > > > > > > Having raw 'new' like this generally makes code harder to read: I
> have
> > > to
> > > > > read a
> > > > > > > lot more to understand how it's deleted, and I need to read a lot
> more
> > > code
> > > > > to
> > > > > > > understand when that might not happen. This isn't just a
> hypothetical
> > > > > concern:
> > > > > > > I've found other mojo code that tries to use raw 'new' and ends up
> > > leaking
> > > > > the
> > > > > > > impl object.
> > > > > > 
> > > > > > +rockot as well, I have personally found StrongBinding a bit of an
> awkward
> > > fit
> > > > > for these sort of instance as well, maybe there's something we can
tweak
> to
> > > make
> > > > > it easier to use.
> > > > > 
> > > > > I'm not sure how your proposal addresses the concern though.
Currently,
> > > Device
> > > > > manages its own lifetime and it's all encapsulated in the error
handler,
> > > > > DeviceChanged and Disconnect.
> > > > 
> > > > It's easy to understand now since we just wrote it. However, someone
> coming
> > > along later will just
> > > > see 'new X', and wonder how it's not leaked (and what prevents it from
> getting
> > > leaked). Then they'll have to read through all the different bits of code
> and
> > > try to understand how they fit together...
> > > > 
> > > > > Using strong binding would spread the lifetime
> > > > > management into StrongBinding, Adapter and Device::Disconnect, which
not
> > > only
> > > > > increases the amount of code (now Adapter has to keep a map of all
> devices
> > > and
> > > > > manage their lifetimes) but is also harder to follow.
> > > > 
> > > > I'm not sure why Adapter comes into this; it can still more or less be
> > > internally managed by Device: the awkwardness here is that the strong
> binding is
> > > external to the thing it's managing, so it's harder for the managed object
> to
> > > self-delete.
> > > > 
> > > > > Furthermore, as mbrunson
> > > > > mentioned, the amount of new testing code we would add for this is non
> > > trivial
> > > > > as we would need a new suit of tests for Adapter.
> > > > 
> > > > If there's no Adapter changes, I don't think we'd need new tests?
> > > 
> > > Sorry I might have misunderstood your proposal:
> > > 
> > > >> mbrunson:
> > > >> So, would you recommend moving control of the binding to the Adapter
> since
> > > the
> > > >> Device can't control its own lifetime?
> > > 
> > > > dcheng:
> > > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > > 
> > > I interpreted that as, Adapter will call MakeStrongBinding and manage the
> > > lifetime of Device through the WeakPtr returned. So how would the Device
> manage
> > > its lifetime if we use StrongBinding?
> > 
> > The Device itself would keep a WeakPtr to its own StrongBinding--that's the
> awkward part. Example of some code that does this:
> https://codereview.chromium.org/2398833003
> 
> Ah. That makes sense (definitely something that could be added to mojo).
> mbrunson, any objections?

It's kind of awkward, but it works. I'll do that.

[mbrunson, 2016-11-16 03:32:04]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/15 07:24:02, dcheng wrote:
> On 2016/11/11 21:26:15, mbrunson wrote:
> > On 2016/11/11 01:25:32, dcheng wrote:
> > > Let's use EnumTraits for this:
> > > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > 
> > Is it possible to use EnumTraits when the C++ class only maps a subset of
the
> > Mojom enum?
> 
> Yes, that's not a problem.
> 
> (If the conversion is only one way, e.g. C++ -> mojom, it'd probably even be
OK
> to stub out the the deserialization and assert NOTREACHED())

I talked to yzshen about this. It's not possible. I would have to add values to
the C++ enum to have full compatibility between the types which doesn't make
sense for the original enum.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device.cc:22: base::Bind(&Device::Disconnect,
base::Unretained(this)));
On 2016/11/15 21:58:09, ortuno wrote:
> On 2016/11/15 at 21:50:35, mbrunson wrote:
> > On 2016/11/15 07:24:02, dcheng wrote:
> > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > On 2016/11/11 01:25:33, dcheng wrote:
> > > > > Let's just use StrongBinding. It does make this code a bit more
> complicated
> > > > > (specifically, the delete this in DeviceChanged), but it requires less
> > > "delete
> > > > > this"... so I think that's a net win overall.
> > > > 
> > > > So, would you recommend moving control of the binding to the Adapter
since
> the
> > > > Device can't control its own lifetime?
> > > 
> > > Yes, this implies Adapter will be calling MakeStrongBinding.
> > 
> > I hesitate to do this. 
> > 
> > As you said, this complicates the code quite a bit and will require many
more
> tests to verify the proper management of all of the active Device instances.
> I've seen the current approach used in places with similar lifetime concerns
[1]
> where the interface's lifetime is managed by both the message pipe and some
> other object.
> > 
> > [1]
>
https://cs.chromium.org/chromium/src/device/usb/mojo/device_manager_impl.cc?r...
> 
> +1

Done.

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:129: std::unique_ptr<base::MessageLoop>
message_loop_;
On 2016/11/15 07:24:02, dcheng wrote:
> On 2016/11/11 21:26:15, mbrunson wrote:
> > On 2016/11/11 01:25:33, dcheng wrote:
> > > Nit: device_ and message_loop_ can just be members of the text fixture
class
> > > directly, I think.
> > 
> > Are you saying they should be put in the TEST_F sections or in the
> constructor?
> > They're already in the BluetoothInterfaceDeviceTest class.
> 
> The unique_ptr indirection is unnecessary: they can just be
> NiceMockBluetoothDevice device_ and base::MessageLoop message_loop_.

Done.

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/device.h
File device/bluetooth/device.h (right):

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/devic...
device/bluetooth/device.h:43: mojo::Binding<mojom::Device>*
GetBindingForTesting();
On 2016/11/15 07:24:02, dcheng wrote:
> The test can just check the other end of the pipe if it needs to verify the
pipe
> is closed.

Done.

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:73:
std::unique_ptr<NiceMockBluetoothGattConnection> connection(
On 2016/11/15 07:24:02, dcheng wrote:
> auto connection = base::MakeUnique<NiceMockBluetoothGattConnection>(...) will
> reduce the amount of typing here.

Done.

https://codereview.chromium.org/2448713002/diff/500001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:85: delete device_service_;
On 2016/11/15 07:24:02, dcheng wrote:
> With a strong binding, this can just close the message pipe.

Done.

[ortuno, 2016-11-16 04:53:18]

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:147: device->SetStrongBindingPtr(binding);
Can this be done in Device? Similar to BatteryMonitorImpl
https://codereview.chromium.org/2398833003

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:89: continuation.Run();
Can you use base::RunLoop to simplify some of this?

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:197: proxy_.set_connection_error_handler(
I think you can do this when initializing proxy_ if you use base::RunLoop.

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:204: expect_device_service_deleted_ = true;
Why do you test this in TearDown rather than in the test itself?

[mbrunson, 2016-11-16 22:17:04]

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:147: device->SetStrongBindingPtr(binding);
On 2016/11/16 04:53:17, ortuno wrote:
> Can this be done in Device? Similar to BatteryMonitorImpl
> https://codereview.chromium.org/2398833003

Done.

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:89: continuation.Run();
On 2016/11/16 04:53:17, ortuno wrote:
> Can you use base::RunLoop to simplify some of this?

Done.

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:197: proxy_.set_connection_error_handler(
On 2016/11/16 04:53:17, ortuno wrote:
> I think you can do this when initializing proxy_ if you use base::RunLoop.

Done.

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:204: expect_device_service_deleted_ = true;
On 2016/11/16 04:53:17, ortuno wrote:
> Why do you test this in TearDown rather than in the test itself?

I wanted to verify the values after each test was finished. I figured putting it
in the TearDown was best so it applies to all tests.

[dcheng, 2016-11-17 01:31:48]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/16 03:32:03, mbrunson wrote:
> On 2016/11/15 07:24:02, dcheng wrote:
> > On 2016/11/11 21:26:15, mbrunson wrote:
> > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > Let's use EnumTraits for this:
> > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > 
> > > Is it possible to use EnumTraits when the C++ class only maps a subset of
> the
> > > Mojom enum?
> > 
> > Yes, that's not a problem.
> > 
> > (If the conversion is only one way, e.g. C++ -> mojom, it'd probably even be
> OK
> > to stub out the the deserialization and assert NOTREACHED())
> 
> I talked to yzshen about this. It's not possible. I would have to add values
to
> the C++ enum to have full compatibility between the types which doesn't make
> sense for the original enum.

Why would it need to do that? We only need to handle the values we expect to be
mapped, and DCHECK() / NOTREACHED() the other things.

https://codereview.chromium.org/2448713002/diff/620001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/620001/device/bluetooth/devic...
device/bluetooth/device.cc:27: auto* deviceImpl = new Device(adapter,
std::move(connection));
Nit: device_impl

https://codereview.chromium.org/2448713002/diff/620001/device/bluetooth/devic...
device/bluetooth/device.cc:85: callback.Run(std::move(device_info));
Optional: both line 85 and line 112 would be slightly shorter if we just
directly use the result of ConstructDeviceInfoStruct() rather than saving it
into a local first.

[mbrunson, 2016-11-17 03:34:43]

https://codereview.chromium.org/2448713002/diff/620001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/620001/device/bluetooth/devic...
device/bluetooth/device.cc:27: auto* deviceImpl = new Device(adapter,
std::move(connection));
On 2016/11/17 01:31:47, dcheng wrote:
> Nit: device_impl

Done.

https://codereview.chromium.org/2448713002/diff/620001/device/bluetooth/devic...
device/bluetooth/device.cc:85: callback.Run(std::move(device_info));
On 2016/11/17 01:31:47, dcheng wrote:
> Optional: both line 85 and line 112 would be slightly shorter if we just
> directly use the result of ConstructDeviceInfoStruct() rather than saving it
> into a local first.

Done.

[ortuno, 2016-11-17 03:54:45]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/17 at 01:31:47, dcheng wrote:
> On 2016/11/16 03:32:03, mbrunson wrote:
> > On 2016/11/15 07:24:02, dcheng wrote:
> > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > Let's use EnumTraits for this:
> > > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > 
> > > > Is it possible to use EnumTraits when the C++ class only maps a subset
of
> > the
> > > > Mojom enum?
> > > 
> > > Yes, that's not a problem.
> > > 
> > > (If the conversion is only one way, e.g. C++ -> mojom, it'd probably even
be
> > OK
> > > to stub out the the deserialization and assert NOTREACHED())
> > 
> > I talked to yzshen about this. It's not possible. I would have to add values
to
> > the C++ enum to have full compatibility between the types which doesn't make
> > sense for the original enum.
> 
> Why would it need to do that? We only need to handle the values we expect to
be mapped, and DCHECK() / NOTREACHED() the other things.

I think what mbrunson meant is that the mojo enum is a super set of the C++ enum
i.e. the C++ enum + some other errors.

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/560001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:204: expect_device_service_deleted_ = true;
On 2016/11/16 at 22:17:04, mbrunson wrote:
> On 2016/11/16 04:53:17, ortuno wrote:
> > Why do you test this in TearDown rather than in the test itself?
> 
> I wanted to verify the values after each test was finished. I figured putting
it in the TearDown was best so it applies to all tests.

We are loosing some clarity by doing it in TearDown since readers will have to
go back and look for where the variable is being used to understand its purpose.
Also, we actually end up with more code:

```
// constructor
bool expect_device_service_deleted_ = false;

// tests
expected_device_service_deleted_ = true;

// teardown
EXPECT_EQ(message_pipe_closed_, expect_device_service_deleted_);
```

vs.

```
// tests
EXPECT_TRUE(message_pipe_closed_);
```

https://codereview.chromium.org/2448713002/diff/640001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2448713002/diff/640001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:41: var
errorString = '';
var errorString = Object.keys(ConnectResult).find(function(key) { return
ConnectResult[key] === response.result});

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device.cc:27: auto* device_impl = new Device(adapter,
std::move(connection));
nit:
auto device_impl = base::WrapUnique(new Device(adapter, std::move(connection));
auto binding =  mojo::MakeStrongBinding(std::move(device_impl),
std::move(request));

It would be even better if we could use base::MakeUnique but that would require
keeping the constructor public which I don't think we should do:

https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/device.h
File device/bluetooth/device.h (right):

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device.h:27: // An instance of this class is constructed by
Adapter and strongly bound
An instance of this class is constructed by calling Device::Create with a
request and a BluetoothGattConnection. The lifetime of an instance of this class
is bound to the message pipe and the GATT connection to the device, meaning if
the device disconnects the instance is deleted.

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device.h:32: Device(scoped_refptr<device::BluetoothAdapter>
adapter,
To indicate that this class should only be created by using Create, make this
private.

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:170: SimulateGattServicesDiscovered();
You probably want to run the loop first before you do more requests. Otherwise
you are executing all requests one after the other.

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:207: TEST_F(BluetoothInterfaceDeviceTest,
Do you think it makes sense to have a test for when the pipe closes?

[mbrunson, 2016-11-17 19:33:33]

https://codereview.chromium.org/2448713002/diff/640001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2448713002/diff/640001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:41: var
errorString = '';
On 2016/11/17 03:54:45, ortuno wrote:
> var errorString = Object.keys(ConnectResult).find(function(key) { return
> ConnectResult[key] === response.result});

Done.

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device.cc:27: auto* device_impl = new Device(adapter,
std::move(connection));
On 2016/11/17 03:54:45, ortuno wrote:
> nit:
> auto device_impl = base::WrapUnique(new Device(adapter,
std::move(connection));
> auto binding =  mojo::MakeStrongBinding(std::move(device_impl),
> std::move(request));
> 
> It would be even better if we could use base::MakeUnique but that would
require
> keeping the constructor public which I don't think we should do:
> 
>
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

Doesn't doing std::move clear the device_impl unique_ptr here? I wouldn't be
able to set the binding afterwards. I stored the raw pointer but that doesn't
seem much better than what's already here :-\

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/device.h
File device/bluetooth/device.h (right):

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device.h:27: // An instance of this class is constructed by
Adapter and strongly bound
On 2016/11/17 03:54:45, ortuno wrote:
> An instance of this class is constructed by calling Device::Create with a
> request and a BluetoothGattConnection. The lifetime of an instance of this
class
> is bound to the message pipe and the GATT connection to the device, meaning if
> the device disconnects the instance is deleted.

Done.

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device.h:32: Device(scoped_refptr<device::BluetoothAdapter>
adapter,
On 2016/11/17 03:54:45, ortuno wrote:
> To indicate that this class should only be created by using Create, make this
> private.

Done.

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
File device/bluetooth/device_unittest.cc (right):

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:170: SimulateGattServicesDiscovered();
On 2016/11/17 03:54:45, ortuno wrote:
> You probably want to run the loop first before you do more requests. Otherwise
> you are executing all requests one after the other.

Done.

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
device/bluetooth/device_unittest.cc:207: TEST_F(BluetoothInterfaceDeviceTest,
On 2016/11/17 03:54:45, ortuno wrote:
> Do you think it makes sense to have a test for when the pipe closes?

I think it's unnecessary. What would this test be testing that's not already
covered by the other two disconnection tests? The side effect of a disconnection
is the closing of the pipe. 

-If a connection error occurs from the other side of the pipe, the 
 error should produce the same effect as closing the strong binding.
 This should be well defined and tested by mojo already.

-The closing of a pipe shouldn't produce any conditions 
 where we have to test whether pending requests are deleted before the 
 binding is closed. This order should be well defined by mojo already
 and should be covered by the tests already here.

Unless there's another case you're thinking of?

[mbrunson, 2016-11-17 21:28:01]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/17 01:31:47, dcheng wrote:
> On 2016/11/16 03:32:03, mbrunson wrote:
> > On 2016/11/15 07:24:02, dcheng wrote:
> > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > Let's use EnumTraits for this:
> > > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > 
> > > > Is it possible to use EnumTraits when the C++ class only maps a subset
of
> > the
> > > > Mojom enum?
> > > 
> > > Yes, that's not a problem.
> > > 
> > > (If the conversion is only one way, e.g. C++ -> mojom, it'd probably even
be
> > OK
> > > to stub out the the deserialization and assert NOTREACHED())
> > 
> > I talked to yzshen about this. It's not possible. I would have to add values
> to
> > the C++ enum to have full compatibility between the types which doesn't make
> > sense for the original enum.
> 
> Why would it need to do that? We only need to handle the values we expect to
be
> mapped, and DCHECK() / NOTREACHED() the other things.

Following our chat discussion: Has a simpler alternative for this been found or
should I just leave it as is?

[dcheng, 2016-11-17 21:35:36]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/17 21:28:01, mbrunson wrote:
> On 2016/11/17 01:31:47, dcheng wrote:
> > On 2016/11/16 03:32:03, mbrunson wrote:
> > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > Let's use EnumTraits for this:
> > > > > >
https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > 
> > > > > Is it possible to use EnumTraits when the C++ class only maps a subset
> of
> > > the
> > > > > Mojom enum?
> > > > 
> > > > Yes, that's not a problem.
> > > > 
> > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd probably
even
> be
> > > OK
> > > > to stub out the the deserialization and assert NOTREACHED())
> > > 
> > > I talked to yzshen about this. It's not possible. I would have to add
values
> > to
> > > the C++ enum to have full compatibility between the types which doesn't
make
> > > sense for the original enum.
> > 
> > Why would it need to do that? We only need to handle the values we expect to
> be
> > mapped, and DCHECK() / NOTREACHED() the other things.
> 
> Following our chat discussion: Has a simpler alternative for this been found
or
> should I just leave it as is?

Sorry for the slow responses. I really don't think this sets a good precedent.

Is there a time when we'd want to allow an Adapter to be used without an
AdapterClient? Perhaps we could add methods to AdapterClient for Success/Error
to mirror the C++ interfaces?

[ortuno, 2016-11-17 22:27:30]

On 2016/11/17 at 21:35:36, dcheng wrote:
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> File device/bluetooth/adapter.cc (right):
> 
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
> On 2016/11/17 21:28:01, mbrunson wrote:
> > On 2016/11/17 01:31:47, dcheng wrote:
> > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > Let's use EnumTraits for this:
> > > > > > >
https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > 
> > > > > > Is it possible to use EnumTraits when the C++ class only maps a
subset
> > of
> > > > the
> > > > > > Mojom enum?
> > > > > 
> > > > > Yes, that's not a problem.
> > > > > 
> > > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd probably
even
> > be
> > > > OK
> > > > > to stub out the the deserialization and assert NOTREACHED())
> > > > 
> > > > I talked to yzshen about this. It's not possible. I would have to add
values
> > > to
> > > > the C++ enum to have full compatibility between the types which doesn't
make
> > > > sense for the original enum.
> > > 
> > > Why would it need to do that? We only need to handle the values we expect
to
> > be
> > > mapped, and DCHECK() / NOTREACHED() the other things.
> > 
> > Following our chat discussion: Has a simpler alternative for this been found
or
> > should I just leave it as is?
> 
> Sorry for the slow responses. I really don't think this sets a good precedent.
> 
> Is there a time when we'd want to allow an Adapter to be used without an
AdapterClient? Perhaps we could add methods to AdapterClient for Success/Error
to mirror the C++ interfaces?

FWIW I think mbrunson's approach is OK. In the long run device/bluetooth will be
refactored to use the mojo enum directly.

[mbrunson, 2016-11-17 23:00:44]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/17 21:35:36, dcheng wrote:
> On 2016/11/17 21:28:01, mbrunson wrote:
> > On 2016/11/17 01:31:47, dcheng wrote:
> > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > Let's use EnumTraits for this:
> > > > > > >
> https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > 
> > > > > > Is it possible to use EnumTraits when the C++ class only maps a
subset
> > of
> > > > the
> > > > > > Mojom enum?
> > > > > 
> > > > > Yes, that's not a problem.
> > > > > 
> > > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd probably
> even
> > be
> > > > OK
> > > > > to stub out the the deserialization and assert NOTREACHED())
> > > > 
> > > > I talked to yzshen about this. It's not possible. I would have to add
> values
> > > to
> > > > the C++ enum to have full compatibility between the types which doesn't
> make
> > > > sense for the original enum.
> > > 
> > > Why would it need to do that? We only need to handle the values we expect
to
> > be
> > > mapped, and DCHECK() / NOTREACHED() the other things.
> > 
> > Following our chat discussion: Has a simpler alternative for this been found
> or
> > should I just leave it as is?
> 
> Sorry for the slow responses. I really don't think this sets a good precedent.
> 
> Is there a time when we'd want to allow an Adapter to be used without an
> AdapterClient? Perhaps we could add methods to AdapterClient for Success/Error
> to mirror the C++ interfaces?

Hmm. It seems really awkward to start an action in one place and then finish it
in another. Extra context will be required on the client side to map to the
right Device interface connection which introduces unnecessary complexity for
all clients.

It seems like it would be simpler to just have a success/error boolean and
typemap the error to get rid of ConnectResult.SUCCESS, but even then the
ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a ConnectErrorCode
value. That means there'd need to be at least two parameters/functions in the
client to typemap properly.

ConnectResult closely resembles the  WebBluetoothResult mojom enum[1] which
implements a switch for this conversion[2].

But I agree. This way is not...ideal.

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...

[2]
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

[dcheng, 2016-11-17 23:13:20]

On 2016/11/17 23:00:44, mbrunson wrote:
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> File device/bluetooth/adapter.cc (right):
> 
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
> error_code) {
> On 2016/11/17 21:35:36, dcheng wrote:
> > On 2016/11/17 21:28:01, mbrunson wrote:
> > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > Let's use EnumTraits for this:
> > > > > > > >
> > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > 
> > > > > > > Is it possible to use EnumTraits when the C++ class only maps a
> subset
> > > of
> > > > > the
> > > > > > > Mojom enum?
> > > > > > 
> > > > > > Yes, that's not a problem.
> > > > > > 
> > > > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd probably
> > even
> > > be
> > > > > OK
> > > > > > to stub out the the deserialization and assert NOTREACHED())
> > > > > 
> > > > > I talked to yzshen about this. It's not possible. I would have to add
> > values
> > > > to
> > > > > the C++ enum to have full compatibility between the types which
doesn't
> > make
> > > > > sense for the original enum.
> > > > 
> > > > Why would it need to do that? We only need to handle the values we
expect
> to
> > > be
> > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > 
> > > Following our chat discussion: Has a simpler alternative for this been
found
> > or
> > > should I just leave it as is?
> > 
> > Sorry for the slow responses. I really don't think this sets a good
precedent.
> > 
> > Is there a time when we'd want to allow an Adapter to be used without an
> > AdapterClient? Perhaps we could add methods to AdapterClient for
Success/Error
> > to mirror the C++ interfaces?
> 
> Hmm. It seems really awkward to start an action in one place and then finish
it
> in another. Extra context will be required on the client side to map to the
> right Device interface connection which introduces unnecessary complexity for
> all clients.

Can you help me understand why this makes it more complicated? We only have a
Device interface if it succeeded, and the Device interface itself should be
marshalled back over AdapterClient::OnDeviceConnected(Device). Any mapping that
needs to be done should be done at that time, I think. The JS closure that we're
passing to connectToDevice doesn't seem to actually capture any state that we
wouldn't have by just having an actual named method on AdapterBroker.

For the error case, there won't be a Device, so no mapping needs to be done.

> 
> It seems like it would be simpler to just have a success/error boolean and
> typemap the error to get rid of ConnectResult.SUCCESS, but even then the
> ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
ConnectErrorCode
> value. That means there'd need to be at least two parameters/functions in the
> client to typemap properly.

I'd caution against having too many optional parameters. I think having
distinctly named callbacks is actually a bit clearer; then we don't need to
explicitly specify the success value.

> 
> ConnectResult closely resembles the  WebBluetoothResult mojom enum[1] which
> implements a switch for this conversion[2].
> 
> But I agree. This way is not...ideal.
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> 
> [2]
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...



> FWIW I think mbrunson's approach is OK. In the long run device/bluetooth will
be
> refactored to use the mojo enum directly.

How long run is this? For IPC/Mojo, we really want to avoid any manual
conversion code like this. While this particular instance is fairly innocuous
(we're sending from browser to renderer), it also doesn't seem critical to
deviate from IPC best practices.

[ortuno, 2016-11-17 23:23:51]

On 2016/11/17 at 23:13:20, dcheng wrote:
> On 2016/11/17 23:00:44, mbrunson wrote:
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > File device/bluetooth/adapter.cc (right):
> > 
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
> > error_code) {
> > On 2016/11/17 21:35:36, dcheng wrote:
> > > On 2016/11/17 21:28:01, mbrunson wrote:
> > > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > > Let's use EnumTraits for this:
> > > > > > > > >
> > > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > > 
> > > > > > > > Is it possible to use EnumTraits when the C++ class only maps a
> > subset
> > > > of
> > > > > > the
> > > > > > > > Mojom enum?
> > > > > > > 
> > > > > > > Yes, that's not a problem.
> > > > > > > 
> > > > > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd
probably
> > > even
> > > > be
> > > > > > OK
> > > > > > > to stub out the the deserialization and assert NOTREACHED())
> > > > > > 
> > > > > > I talked to yzshen about this. It's not possible. I would have to
add
> > > values
> > > > > to
> > > > > > the C++ enum to have full compatibility between the types which
doesn't
> > > make
> > > > > > sense for the original enum.
> > > > > 
> > > > > Why would it need to do that? We only need to handle the values we
expect
> > to
> > > > be
> > > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > > 
> > > > Following our chat discussion: Has a simpler alternative for this been
found
> > > or
> > > > should I just leave it as is?
> > > 
> > > Sorry for the slow responses. I really don't think this sets a good
precedent.
> > > 
> > > Is there a time when we'd want to allow an Adapter to be used without an
> > > AdapterClient? Perhaps we could add methods to AdapterClient for
Success/Error
> > > to mirror the C++ interfaces?
> > 
> > Hmm. It seems really awkward to start an action in one place and then finish
it
> > in another. Extra context will be required on the client side to map to the
> > right Device interface connection which introduces unnecessary complexity
for
> > all clients.
> 
> Can you help me understand why this makes it more complicated? We only have a
Device interface if it succeeded, and the Device interface itself should be
marshalled back over AdapterClient::OnDeviceConnected(Device). Any mapping that
needs to be done should be done at that time, I think. The JS closure that we're
passing to connectToDevice doesn't seem to actually capture any state that we
wouldn't have by just having an actual named method on AdapterBroker.
> 
> For the error case, there won't be a Device, so no mapping needs to be done.
> 

Let's not do this please. This further deviates from the current interface. Yes,
it has Success/Error functions but they are callbacks not events.


> > 
> > It seems like it would be simpler to just have a success/error boolean and
> > typemap the error to get rid of ConnectResult.SUCCESS, but even then the
> > ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
ConnectErrorCode
> > value. That means there'd need to be at least two parameters/functions in
the
> > client to typemap properly.
> 
> I'd caution against having too many optional parameters. I think having
distinctly named callbacks is actually a bit clearer; then we don't need to
explicitly specify the success value.
> 
> > 
> > ConnectResult closely resembles the  WebBluetoothResult mojom enum[1] which
> > implements a switch for this conversion[2].
> > 
> > But I agree. This way is not...ideal.
> > 
> > [1]
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> > 
> > [2]
> >
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> 
> 
> 
> > FWIW I think mbrunson's approach is OK. In the long run device/bluetooth
will be
> > refactored to use the mojo enum directly.
> 
> How long run is this? For IPC/Mojo, we really want to avoid any manual
conversion code like this. While this particular instance is fairly innocuous
(we're sending from browser to renderer), it also doesn't seem critical to
deviate from IPC best practices.

Q1 2016

[ortuno, 2016-11-17 23:24:21]

On 2016/11/17 at 23:23:51, ortuno wrote:
> On 2016/11/17 at 23:13:20, dcheng wrote:
> > On 2016/11/17 23:00:44, mbrunson wrote:
> > >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > File device/bluetooth/adapter.cc (right):
> > > 
> > >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
> > > error_code) {
> > > On 2016/11/17 21:35:36, dcheng wrote:
> > > > On 2016/11/17 21:28:01, mbrunson wrote:
> > > > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > > > Let's use EnumTraits for this:
> > > > > > > > > >
> > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > > > 
> > > > > > > > > Is it possible to use EnumTraits when the C++ class only maps
a
> > > subset
> > > > > of
> > > > > > > the
> > > > > > > > > Mojom enum?
> > > > > > > > 
> > > > > > > > Yes, that's not a problem.
> > > > > > > > 
> > > > > > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd
probably
> > > > even
> > > > > be
> > > > > > > OK
> > > > > > > > to stub out the the deserialization and assert NOTREACHED())
> > > > > > > 
> > > > > > > I talked to yzshen about this. It's not possible. I would have to
add
> > > > values
> > > > > > to
> > > > > > > the C++ enum to have full compatibility between the types which
doesn't
> > > > make
> > > > > > > sense for the original enum.
> > > > > > 
> > > > > > Why would it need to do that? We only need to handle the values we
expect
> > > to
> > > > > be
> > > > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > > > 
> > > > > Following our chat discussion: Has a simpler alternative for this been
found
> > > > or
> > > > > should I just leave it as is?
> > > > 
> > > > Sorry for the slow responses. I really don't think this sets a good
precedent.
> > > > 
> > > > Is there a time when we'd want to allow an Adapter to be used without an
> > > > AdapterClient? Perhaps we could add methods to AdapterClient for
Success/Error
> > > > to mirror the C++ interfaces?
> > > 
> > > Hmm. It seems really awkward to start an action in one place and then
finish it
> > > in another. Extra context will be required on the client side to map to
the
> > > right Device interface connection which introduces unnecessary complexity
for
> > > all clients.
> > 
> > Can you help me understand why this makes it more complicated? We only have
a Device interface if it succeeded, and the Device interface itself should be
marshalled back over AdapterClient::OnDeviceConnected(Device). Any mapping that
needs to be done should be done at that time, I think. The JS closure that we're
passing to connectToDevice doesn't seem to actually capture any state that we
wouldn't have by just having an actual named method on AdapterBroker.
> > 
> > For the error case, there won't be a Device, so no mapping needs to be done.
> > 
> 
> Let's not do this please. This further deviates from the current interface.
Yes, it has Success/Error functions but they are callbacks not events.
> 
> 
> > > 
> > > It seems like it would be simpler to just have a success/error boolean and
> > > typemap the error to get rid of ConnectResult.SUCCESS, but even then the
> > > ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
ConnectErrorCode
> > > value. That means there'd need to be at least two parameters/functions in
the
> > > client to typemap properly.
> > 
> > I'd caution against having too many optional parameters. I think having
distinctly named callbacks is actually a bit clearer; then we don't need to
explicitly specify the success value.
> > 
> > > 
> > > ConnectResult closely resembles the  WebBluetoothResult mojom enum[1]
which
> > > implements a switch for this conversion[2].
> > > 
> > > But I agree. This way is not...ideal.
> > > 
> > > [1]
> > >
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> > > 
> > > [2]
> > >
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > 
> > 
> > 
> > > FWIW I think mbrunson's approach is OK. In the long run device/bluetooth
will be
> > > refactored to use the mojo enum directly.
> > 
> > How long run is this? For IPC/Mojo, we really want to avoid any manual
conversion code like this. While this particular instance is fairly innocuous
(we're sending from browser to renderer), it also doesn't seem critical to
deviate from IPC best practices.
> 
> Q1 2016

**argh, Q1 2017

[dcheng, 2016-11-17 23:27:42]

On 2016/11/17 23:24:21, ortuno wrote:
> On 2016/11/17 at 23:23:51, ortuno wrote:
> > On 2016/11/17 at 23:13:20, dcheng wrote:
> > > On 2016/11/17 23:00:44, mbrunson wrote:
> > > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > File device/bluetooth/adapter.cc (right):
> > > > 
> > > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > device/bluetooth/adapter.cc:15:
device::BluetoothDevice::ConnectErrorCode
> > > > error_code) {
> > > > On 2016/11/17 21:35:36, dcheng wrote:
> > > > > On 2016/11/17 21:28:01, mbrunson wrote:
> > > > > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > > > > Let's use EnumTraits for this:
> > > > > > > > > > >
> > > > > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > > > > 
> > > > > > > > > > Is it possible to use EnumTraits when the C++ class only
maps
> a
> > > > subset
> > > > > > of
> > > > > > > > the
> > > > > > > > > > Mojom enum?
> > > > > > > > > 
> > > > > > > > > Yes, that's not a problem.
> > > > > > > > > 
> > > > > > > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd
> probably
> > > > > even
> > > > > > be
> > > > > > > > OK
> > > > > > > > > to stub out the the deserialization and assert NOTREACHED())
> > > > > > > > 
> > > > > > > > I talked to yzshen about this. It's not possible. I would have
to
> add
> > > > > values
> > > > > > > to
> > > > > > > > the C++ enum to have full compatibility between the types which
> doesn't
> > > > > make
> > > > > > > > sense for the original enum.
> > > > > > > 
> > > > > > > Why would it need to do that? We only need to handle the values we
> expect
> > > > to
> > > > > > be
> > > > > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > > > > 
> > > > > > Following our chat discussion: Has a simpler alternative for this
been
> found
> > > > > or
> > > > > > should I just leave it as is?
> > > > > 
> > > > > Sorry for the slow responses. I really don't think this sets a good
> precedent.
> > > > > 
> > > > > Is there a time when we'd want to allow an Adapter to be used without
an
> > > > > AdapterClient? Perhaps we could add methods to AdapterClient for
> Success/Error
> > > > > to mirror the C++ interfaces?
> > > > 
> > > > Hmm. It seems really awkward to start an action in one place and then
> finish it
> > > > in another. Extra context will be required on the client side to map to
> the
> > > > right Device interface connection which introduces unnecessary
complexity
> for
> > > > all clients.
> > > 
> > > Can you help me understand why this makes it more complicated? We only
have
> a Device interface if it succeeded, and the Device interface itself should be
> marshalled back over AdapterClient::OnDeviceConnected(Device). Any mapping
that
> needs to be done should be done at that time, I think. The JS closure that
we're
> passing to connectToDevice doesn't seem to actually capture any state that we
> wouldn't have by just having an actual named method on AdapterBroker.
> > > 
> > > For the error case, there won't be a Device, so no mapping needs to be
done.
> > > 
> > 
> > Let's not do this please. This further deviates from the current interface.
> Yes, it has Success/Error functions but they are callbacks not events.

Right, but Mojo doesn't represent that well with just the default async
callback. It's possible to fudge around this, but my personal opinion is it's
usually easier to understand if it's just split up, rather than having an enum
result + 1 or more optional params. But this isn't a blocker.

> > 
> > 
> > > > 
> > > > It seems like it would be simpler to just have a success/error boolean
and
> > > > typemap the error to get rid of ConnectResult.SUCCESS, but even then the
> > > > ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
> ConnectErrorCode
> > > > value. That means there'd need to be at least two parameters/functions
in
> the
> > > > client to typemap properly.
> > > 
> > > I'd caution against having too many optional parameters. I think having
> distinctly named callbacks is actually a bit clearer; then we don't need to
> explicitly specify the success value.
> > > 
> > > > 
> > > > ConnectResult closely resembles the  WebBluetoothResult mojom enum[1]
> which
> > > > implements a switch for this conversion[2].
> > > > 
> > > > But I agree. This way is not...ideal.
> > > > 
> > > > [1]
> > > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> > > > 
> > > > [2]
> > > >
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > > 
> > > 
> > > 
> > > > FWIW I think mbrunson's approach is OK. In the long run device/bluetooth
> will be
> > > > refactored to use the mojo enum directly.
> > > 
> > > How long run is this? For IPC/Mojo, we really want to avoid any manual
> conversion code like this. While this particular instance is fairly innocuous
> (we're sending from browser to renderer), it also doesn't seem critical to
> deviate from IPC best practices.
> > 
> > Q1 2016
> 
> **argh, Q1 2017

If it's that short-term, can we just add the values to the C++ enum? Unlike the
previous comment, writing manual conversion code is a blocker.

[ortuno, 2016-11-17 23:43:30]

On 2016/11/17 at 23:27:42, dcheng wrote:
> On 2016/11/17 23:24:21, ortuno wrote:
> > On 2016/11/17 at 23:23:51, ortuno wrote:
> > > On 2016/11/17 at 23:13:20, dcheng wrote:
> > > > On 2016/11/17 23:00:44, mbrunson wrote:
> > > > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > > File device/bluetooth/adapter.cc (right):
> > > > > 
> > > > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > > device/bluetooth/adapter.cc:15:
device::BluetoothDevice::ConnectErrorCode
> > > > > error_code) {
> > > > > On 2016/11/17 21:35:36, dcheng wrote:
> > > > > > On 2016/11/17 21:28:01, mbrunson wrote:
> > > > > > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > > > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > > > > > Let's use EnumTraits for this:
> > > > > > > > > > > >
> > > > > >
https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > > > > > 
> > > > > > > > > > > Is it possible to use EnumTraits when the C++ class only
maps
> > a
> > > > > subset
> > > > > > > of
> > > > > > > > > the
> > > > > > > > > > > Mojom enum?
> > > > > > > > > > 
> > > > > > > > > > Yes, that's not a problem.
> > > > > > > > > > 
> > > > > > > > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd
> > probably
> > > > > > even
> > > > > > > be
> > > > > > > > > OK
> > > > > > > > > > to stub out the the deserialization and assert NOTREACHED())
> > > > > > > > > 
> > > > > > > > > I talked to yzshen about this. It's not possible. I would have
to
> > add
> > > > > > values
> > > > > > > > to
> > > > > > > > > the C++ enum to have full compatibility between the types
which
> > doesn't
> > > > > > make
> > > > > > > > > sense for the original enum.
> > > > > > > > 
> > > > > > > > Why would it need to do that? We only need to handle the values
we
> > expect
> > > > > to
> > > > > > > be
> > > > > > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > > > > > 
> > > > > > > Following our chat discussion: Has a simpler alternative for this
been
> > found
> > > > > > or
> > > > > > > should I just leave it as is?
> > > > > > 
> > > > > > Sorry for the slow responses. I really don't think this sets a good
> > precedent.
> > > > > > 
> > > > > > Is there a time when we'd want to allow an Adapter to be used
without an
> > > > > > AdapterClient? Perhaps we could add methods to AdapterClient for
> > Success/Error
> > > > > > to mirror the C++ interfaces?
> > > > > 
> > > > > Hmm. It seems really awkward to start an action in one place and then
> > finish it
> > > > > in another. Extra context will be required on the client side to map
to
> > the
> > > > > right Device interface connection which introduces unnecessary
complexity
> > for
> > > > > all clients.
> > > > 
> > > > Can you help me understand why this makes it more complicated? We only
have
> > a Device interface if it succeeded, and the Device interface itself should
be
> > marshalled back over AdapterClient::OnDeviceConnected(Device). Any mapping
that
> > needs to be done should be done at that time, I think. The JS closure that
we're
> > passing to connectToDevice doesn't seem to actually capture any state that
we
> > wouldn't have by just having an actual named method on AdapterBroker.
> > > > 
> > > > For the error case, there won't be a Device, so no mapping needs to be
done.
> > > > 
> > > 
> > > Let's not do this please. This further deviates from the current
interface.
> > Yes, it has Success/Error functions but they are callbacks not events.
> 
> Right, but Mojo doesn't represent that well with just the default async
callback. It's possible to fudge around this, but my personal opinion is it's
usually easier to understand if it's just split up, rather than having an enum
result + 1 or more optional params. But this isn't a blocker.
> 
> > > 
> > > 
> > > > > 
> > > > > It seems like it would be simpler to just have a success/error boolean
and
> > > > > typemap the error to get rid of ConnectResult.SUCCESS, but even then
the
> > > > > ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
> > ConnectErrorCode
> > > > > value. That means there'd need to be at least two parameters/functions
in
> > the
> > > > > client to typemap properly.
> > > > 
> > > > I'd caution against having too many optional parameters. I think having
> > distinctly named callbacks is actually a bit clearer; then we don't need to
> > explicitly specify the success value.
> > > > 
> > > > > 
> > > > > ConnectResult closely resembles the  WebBluetoothResult mojom enum[1]
> > which
> > > > > implements a switch for this conversion[2].
> > > > > 
> > > > > But I agree. This way is not...ideal.
> > > > > 
> > > > > [1]
> > > > >
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> > > > > 
> > > > > [2]
> > > > >
> >
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > > > 
> > > > 
> > > > 
> > > > > FWIW I think mbrunson's approach is OK. In the long run
device/bluetooth
> > will be
> > > > > refactored to use the mojo enum directly.
> > > > 
> > > > How long run is this? For IPC/Mojo, we really want to avoid any manual
> > conversion code like this. While this particular instance is fairly
innocuous
> > (we're sending from browser to renderer), it also doesn't seem critical to
> > deviate from IPC best practices.
> > > 
> > > Q1 2016
> > 
> > **argh, Q1 2017
> 
> If it's that short-term, can we just add the values to the C++ enum? Unlike
the previous comment, writing manual conversion code is a blocker.

That would mean changing all clients of device/bluetooth that use the enum to
also handle this case that they will never receive. Which I think its worse that
having manual conversion code.

[dcheng, 2016-11-18 00:13:38]

On 2016/11/17 23:43:30, ortuno wrote:
> On 2016/11/17 at 23:27:42, dcheng wrote:
> > On 2016/11/17 23:24:21, ortuno wrote:
> > > On 2016/11/17 at 23:23:51, ortuno wrote:
> > > > On 2016/11/17 at 23:13:20, dcheng wrote:
> > > > > On 2016/11/17 23:00:44, mbrunson wrote:
> > > > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > > > File device/bluetooth/adapter.cc (right):
> > > > > > 
> > > > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > > > device/bluetooth/adapter.cc:15:
> device::BluetoothDevice::ConnectErrorCode
> > > > > > error_code) {
> > > > > > On 2016/11/17 21:35:36, dcheng wrote:
> > > > > > > On 2016/11/17 21:28:01, mbrunson wrote:
> > > > > > > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > > > > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > > > > > > Let's use EnumTraits for this:
> > > > > > > > > > > > >
> > > > > > >
> https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > > > > > > 
> > > > > > > > > > > > Is it possible to use EnumTraits when the C++ class only
> maps
> > > a
> > > > > > subset
> > > > > > > > of
> > > > > > > > > > the
> > > > > > > > > > > > Mojom enum?
> > > > > > > > > > > 
> > > > > > > > > > > Yes, that's not a problem.
> > > > > > > > > > > 
> > > > > > > > > > > (If the conversion is only one way, e.g. C++ -> mojom,
it'd
> > > probably
> > > > > > > even
> > > > > > > > be
> > > > > > > > > > OK
> > > > > > > > > > > to stub out the the deserialization and assert
NOTREACHED())
> > > > > > > > > > 
> > > > > > > > > > I talked to yzshen about this. It's not possible. I would
have
> to
> > > add
> > > > > > > values
> > > > > > > > > to
> > > > > > > > > > the C++ enum to have full compatibility between the types
> which
> > > doesn't
> > > > > > > make
> > > > > > > > > > sense for the original enum.
> > > > > > > > > 
> > > > > > > > > Why would it need to do that? We only need to handle the
values
> we
> > > expect
> > > > > > to
> > > > > > > > be
> > > > > > > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > > > > > > 
> > > > > > > > Following our chat discussion: Has a simpler alternative for
this
> been
> > > found
> > > > > > > or
> > > > > > > > should I just leave it as is?
> > > > > > > 
> > > > > > > Sorry for the slow responses. I really don't think this sets a
good
> > > precedent.
> > > > > > > 
> > > > > > > Is there a time when we'd want to allow an Adapter to be used
> without an
> > > > > > > AdapterClient? Perhaps we could add methods to AdapterClient for
> > > Success/Error
> > > > > > > to mirror the C++ interfaces?
> > > > > > 
> > > > > > Hmm. It seems really awkward to start an action in one place and
then
> > > finish it
> > > > > > in another. Extra context will be required on the client side to map
> to
> > > the
> > > > > > right Device interface connection which introduces unnecessary
> complexity
> > > for
> > > > > > all clients.
> > > > > 
> > > > > Can you help me understand why this makes it more complicated? We only
> have
> > > a Device interface if it succeeded, and the Device interface itself should
> be
> > > marshalled back over AdapterClient::OnDeviceConnected(Device). Any mapping
> that
> > > needs to be done should be done at that time, I think. The JS closure that
> we're
> > > passing to connectToDevice doesn't seem to actually capture any state that
> we
> > > wouldn't have by just having an actual named method on AdapterBroker.
> > > > > 
> > > > > For the error case, there won't be a Device, so no mapping needs to be
> done.
> > > > > 
> > > > 
> > > > Let's not do this please. This further deviates from the current
> interface.
> > > Yes, it has Success/Error functions but they are callbacks not events.
> > 
> > Right, but Mojo doesn't represent that well with just the default async
> callback. It's possible to fudge around this, but my personal opinion is it's
> usually easier to understand if it's just split up, rather than having an enum
> result + 1 or more optional params. But this isn't a blocker.
> > 
> > > > 
> > > > 
> > > > > > 
> > > > > > It seems like it would be simpler to just have a success/error
boolean
> and
> > > > > > typemap the error to get rid of ConnectResult.SUCCESS, but even then
> the
> > > > > > ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
> > > ConnectErrorCode
> > > > > > value. That means there'd need to be at least two
parameters/functions
> in
> > > the
> > > > > > client to typemap properly.
> > > > > 
> > > > > I'd caution against having too many optional parameters. I think
having
> > > distinctly named callbacks is actually a bit clearer; then we don't need
to
> > > explicitly specify the success value.
> > > > > 
> > > > > > 
> > > > > > ConnectResult closely resembles the  WebBluetoothResult mojom
enum[1]
> > > which
> > > > > > implements a switch for this conversion[2].
> > > > > > 
> > > > > > But I agree. This way is not...ideal.
> > > > > > 
> > > > > > [1]
> > > > > >
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> > > > > > 
> > > > > > [2]
> > > > > >
> > >
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > > > > 
> > > > > 
> > > > > 
> > > > > > FWIW I think mbrunson's approach is OK. In the long run
> device/bluetooth
> > > will be
> > > > > > refactored to use the mojo enum directly.
> > > > > 
> > > > > How long run is this? For IPC/Mojo, we really want to avoid any manual
> > > conversion code like this. While this particular instance is fairly
> innocuous
> > > (we're sending from browser to renderer), it also doesn't seem critical to
> > > deviate from IPC best practices.
> > > > 
> > > > Q1 2016
> > > 
> > > **argh, Q1 2017
> > 
> > If it's that short-term, can we just add the values to the C++ enum? Unlike
> the previous comment, writing manual conversion code is a blocker.
> 
> That would mean changing all clients of device/bluetooth that use the enum to
> also handle this case that they will never receive. Which I think its worse
that
> having manual conversion code.

The problem is manual conversion code is hard to find and audit. This particular
instance is pretty innocuous, but it sets a bad precedent for other code.

While I'd prefer to see the AdapterClient interface change to better represent
the underlying primitives, as a short-term workaround, let's at least use the
TypeConverter template for this. TypeConverter is deprecated and being removed,
and will give us some deadline for fixing this eventually in the future.

[ortuno, 2016-11-18 00:15:26]

On 2016/11/18 at 00:13:38, dcheng wrote:
> On 2016/11/17 23:43:30, ortuno wrote:
> > On 2016/11/17 at 23:27:42, dcheng wrote:
> > > On 2016/11/17 23:24:21, ortuno wrote:
> > > > On 2016/11/17 at 23:23:51, ortuno wrote:
> > > > > On 2016/11/17 at 23:13:20, dcheng wrote:
> > > > > > On 2016/11/17 23:00:44, mbrunson wrote:
> > > > > > >
> > > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > > > > File device/bluetooth/adapter.cc (right):
> > > > > > > 
> > > > > > >
> > > >
> >
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > > > > device/bluetooth/adapter.cc:15:
> > device::BluetoothDevice::ConnectErrorCode
> > > > > > > error_code) {
> > > > > > > On 2016/11/17 21:35:36, dcheng wrote:
> > > > > > > > On 2016/11/17 21:28:01, mbrunson wrote:
> > > > > > > > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > > > > > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > > > > > > > Let's use EnumTraits for this:
> > > > > > > > > > > > > >
> > > > > > > >
> > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Is it possible to use EnumTraits when the C++ class
only
> > maps
> > > > a
> > > > > > > subset
> > > > > > > > > of
> > > > > > > > > > > the
> > > > > > > > > > > > > Mojom enum?
> > > > > > > > > > > > 
> > > > > > > > > > > > Yes, that's not a problem.
> > > > > > > > > > > > 
> > > > > > > > > > > > (If the conversion is only one way, e.g. C++ -> mojom,
it'd
> > > > probably
> > > > > > > > even
> > > > > > > > > be
> > > > > > > > > > > OK
> > > > > > > > > > > > to stub out the the deserialization and assert
NOTREACHED())
> > > > > > > > > > > 
> > > > > > > > > > > I talked to yzshen about this. It's not possible. I would
have
> > to
> > > > add
> > > > > > > > values
> > > > > > > > > > to
> > > > > > > > > > > the C++ enum to have full compatibility between the types
> > which
> > > > doesn't
> > > > > > > > make
> > > > > > > > > > > sense for the original enum.
> > > > > > > > > > 
> > > > > > > > > > Why would it need to do that? We only need to handle the
values
> > we
> > > > expect
> > > > > > > to
> > > > > > > > > be
> > > > > > > > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > > > > > > > 
> > > > > > > > > Following our chat discussion: Has a simpler alternative for
this
> > been
> > > > found
> > > > > > > > or
> > > > > > > > > should I just leave it as is?
> > > > > > > > 
> > > > > > > > Sorry for the slow responses. I really don't think this sets a
good
> > > > precedent.
> > > > > > > > 
> > > > > > > > Is there a time when we'd want to allow an Adapter to be used
> > without an
> > > > > > > > AdapterClient? Perhaps we could add methods to AdapterClient for
> > > > Success/Error
> > > > > > > > to mirror the C++ interfaces?
> > > > > > > 
> > > > > > > Hmm. It seems really awkward to start an action in one place and
then
> > > > finish it
> > > > > > > in another. Extra context will be required on the client side to
map
> > to
> > > > the
> > > > > > > right Device interface connection which introduces unnecessary
> > complexity
> > > > for
> > > > > > > all clients.
> > > > > > 
> > > > > > Can you help me understand why this makes it more complicated? We
only
> > have
> > > > a Device interface if it succeeded, and the Device interface itself
should
> > be
> > > > marshalled back over AdapterClient::OnDeviceConnected(Device). Any
mapping
> > that
> > > > needs to be done should be done at that time, I think. The JS closure
that
> > we're
> > > > passing to connectToDevice doesn't seem to actually capture any state
that
> > we
> > > > wouldn't have by just having an actual named method on AdapterBroker.
> > > > > > 
> > > > > > For the error case, there won't be a Device, so no mapping needs to
be
> > done.
> > > > > > 
> > > > > 
> > > > > Let's not do this please. This further deviates from the current
> > interface.
> > > > Yes, it has Success/Error functions but they are callbacks not events.
> > > 
> > > Right, but Mojo doesn't represent that well with just the default async
> > callback. It's possible to fudge around this, but my personal opinion is
it's
> > usually easier to understand if it's just split up, rather than having an
enum
> > result + 1 or more optional params. But this isn't a blocker.
> > > 
> > > > > 
> > > > > 
> > > > > > > 
> > > > > > > It seems like it would be simpler to just have a success/error
boolean
> > and
> > > > > > > typemap the error to get rid of ConnectResult.SUCCESS, but even
then
> > the
> > > > > > > ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
> > > > ConnectErrorCode
> > > > > > > value. That means there'd need to be at least two
parameters/functions
> > in
> > > > the
> > > > > > > client to typemap properly.
> > > > > > 
> > > > > > I'd caution against having too many optional parameters. I think
having
> > > > distinctly named callbacks is actually a bit clearer; then we don't need
to
> > > > explicitly specify the success value.
> > > > > > 
> > > > > > > 
> > > > > > > ConnectResult closely resembles the  WebBluetoothResult mojom
enum[1]
> > > > which
> > > > > > > implements a switch for this conversion[2].
> > > > > > > 
> > > > > > > But I agree. This way is not...ideal.
> > > > > > > 
> > > > > > > [1]
> > > > > > >
> > > >
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> > > > > > > 
> > > > > > > [2]
> > > > > > >
> > > >
> >
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > FWIW I think mbrunson's approach is OK. In the long run
> > device/bluetooth
> > > > will be
> > > > > > > refactored to use the mojo enum directly.
> > > > > > 
> > > > > > How long run is this? For IPC/Mojo, we really want to avoid any
manual
> > > > conversion code like this. While this particular instance is fairly
> > innocuous
> > > > (we're sending from browser to renderer), it also doesn't seem critical
to
> > > > deviate from IPC best practices.
> > > > > 
> > > > > Q1 2016
> > > > 
> > > > **argh, Q1 2017
> > > 
> > > If it's that short-term, can we just add the values to the C++ enum?
Unlike
> > the previous comment, writing manual conversion code is a blocker.
> > 
> > That would mean changing all clients of device/bluetooth that use the enum
to
> > also handle this case that they will never receive. Which I think its worse
that
> > having manual conversion code.
> 
> The problem is manual conversion code is hard to find and audit. This
particular instance is pretty innocuous, but it sets a bad precedent for other
code.
> 
> While I'd prefer to see the AdapterClient interface change to better represent
the underlying primitives, as a short-term workaround, let's at least use the
TypeConverter template for this. TypeConverter is deprecated and being removed,
and will give us some deadline for fixing this eventually in the future.

TypeConverter SGTM

[mbrunson, 2016-11-18 00:22:24]

On 2016/11/18 00:15:26, ortuno wrote:
> On 2016/11/18 at 00:13:38, dcheng wrote:
> > On 2016/11/17 23:43:30, ortuno wrote:
> > > On 2016/11/17 at 23:27:42, dcheng wrote:
> > > > On 2016/11/17 23:24:21, ortuno wrote:
> > > > > On 2016/11/17 at 23:23:51, ortuno wrote:
> > > > > > On 2016/11/17 at 23:13:20, dcheng wrote:
> > > > > > > On 2016/11/17 23:00:44, mbrunson wrote:
> > > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > > > > > File device/bluetooth/adapter.cc (right):
> > > > > > > > 
> > > > > > > >
> > > > >
> > >
>
https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
> > > > > > > > device/bluetooth/adapter.cc:15:
> > > device::BluetoothDevice::ConnectErrorCode
> > > > > > > > error_code) {
> > > > > > > > On 2016/11/17 21:35:36, dcheng wrote:
> > > > > > > > > On 2016/11/17 21:28:01, mbrunson wrote:
> > > > > > > > > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > > > > > > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > > > > > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > > > > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > > > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > > > > > > > > Let's use EnumTraits for this:
> > > > > > > > > > > > > > >
> > > > > > > > >
> > > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Is it possible to use EnumTraits when the C++ class
> only
> > > maps
> > > > > a
> > > > > > > > subset
> > > > > > > > > > of
> > > > > > > > > > > > the
> > > > > > > > > > > > > > Mojom enum?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Yes, that's not a problem.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > (If the conversion is only one way, e.g. C++ -> mojom,
> it'd
> > > > > probably
> > > > > > > > > even
> > > > > > > > > > be
> > > > > > > > > > > > OK
> > > > > > > > > > > > > to stub out the the deserialization and assert
> NOTREACHED())
> > > > > > > > > > > > 
> > > > > > > > > > > > I talked to yzshen about this. It's not possible. I
would
> have
> > > to
> > > > > add
> > > > > > > > > values
> > > > > > > > > > > to
> > > > > > > > > > > > the C++ enum to have full compatibility between the
types
> > > which
> > > > > doesn't
> > > > > > > > > make
> > > > > > > > > > > > sense for the original enum.
> > > > > > > > > > > 
> > > > > > > > > > > Why would it need to do that? We only need to handle the
> values
> > > we
> > > > > expect
> > > > > > > > to
> > > > > > > > > > be
> > > > > > > > > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > > > > > > > > 
> > > > > > > > > > Following our chat discussion: Has a simpler alternative for
> this
> > > been
> > > > > found
> > > > > > > > > or
> > > > > > > > > > should I just leave it as is?
> > > > > > > > > 
> > > > > > > > > Sorry for the slow responses. I really don't think this sets a
> good
> > > > > precedent.
> > > > > > > > > 
> > > > > > > > > Is there a time when we'd want to allow an Adapter to be used
> > > without an
> > > > > > > > > AdapterClient? Perhaps we could add methods to AdapterClient
for
> > > > > Success/Error
> > > > > > > > > to mirror the C++ interfaces?
> > > > > > > > 
> > > > > > > > Hmm. It seems really awkward to start an action in one place and
> then
> > > > > finish it
> > > > > > > > in another. Extra context will be required on the client side to
> map
> > > to
> > > > > the
> > > > > > > > right Device interface connection which introduces unnecessary
> > > complexity
> > > > > for
> > > > > > > > all clients.
> > > > > > > 
> > > > > > > Can you help me understand why this makes it more complicated? We
> only
> > > have
> > > > > a Device interface if it succeeded, and the Device interface itself
> should
> > > be
> > > > > marshalled back over AdapterClient::OnDeviceConnected(Device). Any
> mapping
> > > that
> > > > > needs to be done should be done at that time, I think. The JS closure
> that
> > > we're
> > > > > passing to connectToDevice doesn't seem to actually capture any state
> that
> > > we
> > > > > wouldn't have by just having an actual named method on AdapterBroker.
> > > > > > > 
> > > > > > > For the error case, there won't be a Device, so no mapping needs
to
> be
> > > done.
> > > > > > > 
> > > > > > 
> > > > > > Let's not do this please. This further deviates from the current
> > > interface.
> > > > > Yes, it has Success/Error functions but they are callbacks not events.
> > > > 
> > > > Right, but Mojo doesn't represent that well with just the default async
> > > callback. It's possible to fudge around this, but my personal opinion is
> it's
> > > usually easier to understand if it's just split up, rather than having an
> enum
> > > result + 1 or more optional params. But this isn't a blocker.
> > > > 
> > > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > It seems like it would be simpler to just have a success/error
> boolean
> > > and
> > > > > > > > typemap the error to get rid of ConnectResult.SUCCESS, but even
> then
> > > the
> > > > > > > > ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
> > > > > ConnectErrorCode
> > > > > > > > value. That means there'd need to be at least two
> parameters/functions
> > > in
> > > > > the
> > > > > > > > client to typemap properly.
> > > > > > > 
> > > > > > > I'd caution against having too many optional parameters. I think
> having
> > > > > distinctly named callbacks is actually a bit clearer; then we don't
need
> to
> > > > > explicitly specify the success value.
> > > > > > > 
> > > > > > > > 
> > > > > > > > ConnectResult closely resembles the  WebBluetoothResult mojom
> enum[1]
> > > > > which
> > > > > > > > implements a switch for this conversion[2].
> > > > > > > > 
> > > > > > > > But I agree. This way is not...ideal.
> > > > > > > > 
> > > > > > > > [1]
> > > > > > > >
> > > > >
> > >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> > > > > > > > 
> > > > > > > > [2]
> > > > > > > >
> > > > >
> > >
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > FWIW I think mbrunson's approach is OK. In the long run
> > > device/bluetooth
> > > > > will be
> > > > > > > > refactored to use the mojo enum directly.
> > > > > > > 
> > > > > > > How long run is this? For IPC/Mojo, we really want to avoid any
> manual
> > > > > conversion code like this. While this particular instance is fairly
> > > innocuous
> > > > > (we're sending from browser to renderer), it also doesn't seem
critical
> to
> > > > > deviate from IPC best practices.
> > > > > > 
> > > > > > Q1 2016
> > > > > 
> > > > > **argh, Q1 2017
> > > > 
> > > > If it's that short-term, can we just add the values to the C++ enum?
> Unlike
> > > the previous comment, writing manual conversion code is a blocker.
> > > 
> > > That would mean changing all clients of device/bluetooth that use the enum
> to
> > > also handle this case that they will never receive. Which I think its
worse
> that
> > > having manual conversion code.
> > 
> > The problem is manual conversion code is hard to find and audit. This
> particular instance is pretty innocuous, but it sets a bad precedent for other
> code.
> > 
> > While I'd prefer to see the AdapterClient interface change to better
represent
> the underlying primitives, as a short-term workaround, let's at least use the
> TypeConverter template for this. TypeConverter is deprecated and being
removed,
> and will give us some deadline for fixing this eventually in the future.
> 
> TypeConverter SGTM

SGTM. Working on it.

[mbrunson, 2016-11-18 01:53:06]

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
File device/bluetooth/adapter.cc (right):

https://codereview.chromium.org/2448713002/diff/480001/device/bluetooth/adapt...
device/bluetooth/adapter.cc:15: device::BluetoothDevice::ConnectErrorCode
error_code) {
On 2016/11/17 23:00:43, mbrunson wrote:
> On 2016/11/17 21:35:36, dcheng wrote:
> > On 2016/11/17 21:28:01, mbrunson wrote:
> > > On 2016/11/17 01:31:47, dcheng wrote:
> > > > On 2016/11/16 03:32:03, mbrunson wrote:
> > > > > On 2016/11/15 07:24:02, dcheng wrote:
> > > > > > On 2016/11/11 21:26:15, mbrunson wrote:
> > > > > > > On 2016/11/11 01:25:32, dcheng wrote:
> > > > > > > > Let's use EnumTraits for this:
> > > > > > > >
> > https://www.chromium.org/developers/design-documents/mojo/type-mapping
> > > > > > > 
> > > > > > > Is it possible to use EnumTraits when the C++ class only maps a
> subset
> > > of
> > > > > the
> > > > > > > Mojom enum?
> > > > > > 
> > > > > > Yes, that's not a problem.
> > > > > > 
> > > > > > (If the conversion is only one way, e.g. C++ -> mojom, it'd probably
> > even
> > > be
> > > > > OK
> > > > > > to stub out the the deserialization and assert NOTREACHED())
> > > > > 
> > > > > I talked to yzshen about this. It's not possible. I would have to add
> > values
> > > > to
> > > > > the C++ enum to have full compatibility between the types which
doesn't
> > make
> > > > > sense for the original enum.
> > > > 
> > > > Why would it need to do that? We only need to handle the values we
expect
> to
> > > be
> > > > mapped, and DCHECK() / NOTREACHED() the other things.
> > > 
> > > Following our chat discussion: Has a simpler alternative for this been
found
> > or
> > > should I just leave it as is?
> > 
> > Sorry for the slow responses. I really don't think this sets a good
precedent.
> > 
> > Is there a time when we'd want to allow an Adapter to be used without an
> > AdapterClient? Perhaps we could add methods to AdapterClient for
Success/Error
> > to mirror the C++ interfaces?
> 
> Hmm. It seems really awkward to start an action in one place and then finish
it
> in another. Extra context will be required on the client side to map to the
> right Device interface connection which introduces unnecessary complexity for
> all clients.
> 
> It seems like it would be simpler to just have a success/error boolean and
> typemap the error to get rid of ConnectResult.SUCCESS, but even then the
> ConnectResult.DEVICE_NO_LONGER_IN_RANGE value doesn't map to a
ConnectErrorCode
> value. That means there'd need to be at least two parameters/functions in the
> client to typemap properly.
> 
> ConnectResult closely resembles the  WebBluetoothResult mojom enum[1] which
> implements a switch for this conversion[2].
> 
> But I agree. This way is not...ideal.
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/modul...
> 
> [2]
>
https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_...

TypeConverter added. Done.

[dcheng, 2016-11-18 19:27:03]

lgtm with a nit

https://codereview.chromium.org/2448713002/diff/680001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/680001/device/bluetooth/devic...
device/bluetooth/device.cc:23: auto* device_ptr = device_impl.get();
Nit: save some lines by just doing:

auto* device = new Device(...);
device->binding = mojo::MakeStrongBinding(base::WrapUnique(device),
std::move(request));

[mbrunson, 2016-11-18 20:55:14]

ortuno and scheib:
Let me know if you want me to move the connect_result_type_converter file from
public/interfaces. I'll make another CL for that if this one lands before you
get back to me.

https://codereview.chromium.org/2448713002/diff/680001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/680001/device/bluetooth/devic...
device/bluetooth/device.cc:23: auto* device_ptr = device_impl.get();
On 2016/11/18 19:27:03, dcheng wrote:
> Nit: save some lines by just doing:
> 
> auto* device = new Device(...);
> device->binding = mojo::MakeStrongBinding(base::WrapUnique(device),
> std::move(request));

ortuno wanted me to change this earlier so I'm just going to leave it as it is
if that's fine with you.

https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...

[mbrunson, 2016-11-18 20:55:39]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 20:57:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-11-18 20:58:41]

https://codereview.chromium.org/2448713002/diff/680001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/680001/device/bluetooth/devic...
device/bluetooth/device.cc:23: auto* device_ptr = device_impl.get();
On 2016/11/18 20:55:14, mbrunson wrote:
> On 2016/11/18 19:27:03, dcheng wrote:
> > Nit: save some lines by just doing:
> > 
> > auto* device = new Device(...);
> > device->binding = mojo::MakeStrongBinding(base::WrapUnique(device),
> > std::move(request));
> 
> ortuno wanted me to change this earlier so I'm just going to leave it as it is
> if that's fine with you.
> 
>
https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...

Condensing it is probably slightly better. I'd get rid of the local |binding|
variable at least, since it doesn't add anything, and could inhibit RVO.

[mbrunson, 2016-11-18 21:39:17]

https://codereview.chromium.org/2448713002/diff/680001/device/bluetooth/devic...
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2448713002/diff/680001/device/bluetooth/devic...
device/bluetooth/device.cc:23: auto* device_ptr = device_impl.get();
On 2016/11/18 20:58:41, dcheng wrote:
> On 2016/11/18 20:55:14, mbrunson wrote:
> > On 2016/11/18 19:27:03, dcheng wrote:
> > > Nit: save some lines by just doing:
> > > 
> > > auto* device = new Device(...);
> > > device->binding = mojo::MakeStrongBinding(base::WrapUnique(device),
> > > std::move(request));
> > 
> > ortuno wanted me to change this earlier so I'm just going to leave it as it
is
> > if that's fine with you.
> > 
> >
>
https://codereview.chromium.org/2448713002/diff/640001/device/bluetooth/devic...
> 
> Condensing it is probably slightly better. I'd get rid of the local |binding|
> variable at least, since it doesn't add anything, and could inhibit RVO.

I'll remove |binding| then. Done.

[mbrunson, 2016-11-18 21:39:33]

The CQ bit was checked by mbrunson@chromium.org

[mbrunson, 2016-11-18 21:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
dpapad@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2448713002/#ps700001
(title: "Remove binding variable in Device.Create")

[commit-bot: I haz the power, 2016-11-18 21:40:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-18 23:42:50]

Description was changed from

==========
bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection and Device
  interface request.
Binds lifetime of Device to lifetime of BluetoothGattConnection and message
  pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection and Device
  interface request.
Binds lifetime of Device to lifetime of BluetoothGattConnection and message
  pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-11-18 23:42:52]

Committed patchset #36 (id:700001)

[commit-bot: I haz the power, 2016-11-18 23:44:55]

Description was changed from

==========
bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection and Device
  interface request.
Binds lifetime of Device to lifetime of BluetoothGattConnection and message
  pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
bluetooth: Add Device connection logic and accompanying user interface.

Changes Device interface to require a BluetoothGattConnection and Device
  interface request.
Binds lifetime of Device to lifetime of BluetoothGattConnection and message
  pipe.
Changes GetDevice function to ConnectToDevice.
Adds ConnectError codes for device connections.
Adds GetServices function to Device to list discovered services.
Adds associated user interface components for connecting to a listed device.

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/32391c11bdd0921e21331620fedf513b89a6a48d
Cr-Commit-Position: refs/heads/master@{#433332}
==========

[commit-bot: I haz the power, 2016-11-18 23:44:57]

Patchset 36 (id:??) landed as
https://crrev.com/32391c11bdd0921e21331620fedf513b89a6a48d
Cr-Commit-Position: refs/heads/master@{#433332}