bluetooth: Add DeviceChanged logging in Device service.

bluetooth: Add DeviceChanged logging in Device service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in Device.
Logs received DeviceChanged events in console.

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

Committed: https://crrev.com/4f24dfe2da20be7e9a9ba2f143cf5d318a5283b1
Cr-Commit-Position: refs/heads/master@{#427124}

[mbrunson, 2016-10-07 23:39:41]

Description was changed from

==========
bluetooth: Add advertising packet logging in LEDevice service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in LEDevice.
Converts DeviceChanged data to AdvertisingPacket and sends to client.
Logs received advertising packets in console during Web Bluetooth scanning.

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

to

==========
bluetooth: Add advertising packet logging in LEDevice service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in LEDevice.
Converts DeviceChanged data to AdvertisingPacket and sends to client.
Logs received advertising packets in console during Web Bluetooth scanning.

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

[mbrunson, 2016-10-10 22:04:43]

Description was changed from

==========
bluetooth: Add advertising packet logging in LEDevice service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in LEDevice.
Converts DeviceChanged data to AdvertisingPacket and sends to client.
Logs received advertising packets in console during Web Bluetooth scanning.

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

to

==========
bluetooth: Add advertising packet logging in Device service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in Device.
Converts DeviceChanged data to AdvertisingPacket and sends to client.
Logs received advertising packets in console during Web Bluetooth scanning.

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

[mbrunson, 2016-10-13 17:55:01]

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

[commit-bot: I haz the power, 2016-10-13 17:55:26]

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-10-13 19:36:17]

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

[commit-bot: I haz the power, 2016-10-13 19:36:18]

Dry run: This issue passed the CQ dry run.

[mbrunson, 2016-10-13 20:24:58]

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

[commit-bot: I haz the power, 2016-10-13 20:25:32]

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

[mbrunson, 2016-10-13 20:40:56]

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

[mbrunson, 2016-10-13 20:40:56]

Switched around the JS a little for logging advertising packets. I didn't add
every aspect of the advertising packet and will probably reserve that for a
later patch once I have a minimal UI up.

[commit-bot: I haz the power, 2016-10-13 22:06:52]

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

[commit-bot: I haz the power, 2016-10-13 22:06:54]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-10-13 22:22:49]

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

https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/device.mojom:36:
DeviceChanged(AdvertisingPacket packet);
In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the adapter
client and it should match DeviceChanged more closely i.e. just call with the
address :(

Alternatively we could call it with a DeviceInfo struct since that would save
clients a round trip but that might be too much.

scheib: Do you have any thoughts?

[scheib, 2016-10-14 07:14:51]

https://codereview.chromium.org/2404623002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2404623002/diff/80001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:13: //
Dictionary for address->client mapping
Close with .

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

https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/device.mojom:36:
DeviceChanged(AdvertisingPacket packet);
On 2016/10/13 22:22:49, ortuno wrote:
> In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the
adapter
> client and it should match DeviceChanged more closely i.e. just call with the
> address :(
> 
> Alternatively we could call it with a DeviceInfo struct since that would save
> clients a round trip but that might be too much.
> 
> scheib: Do you have any thoughts?

We've deviated a bit already with DeviceInfo aggregating all the accessors for
e.g. GetName, GetAddress. If we continue that route and stick to the
DeviceChanged only replying with an address then the client will still need to
call GetInfo. At that point we'll need to include GetInquiryRSSI in the
DeviceInfo packet as well.

One could argue that populating those packet members will happen either in a
call to GetInfo or if we do it directly in a response to DeviceChanged. So, even
in our "just a thin wrapper" we'll have some untested accessors populating a
struct. If we know those only have meaning in a DeviceChanged event that I think
we may as well make the calls in that event and save the DeviceChanged ->
address -> GetInfo step and just return the AdvertisingPacket as written here.

In other words, I think it's OK to clean up naming, and move functionality into
better clusters of interfaces, when the work is doing the same thing regardless
(populating struct members with accessor calls).

What wouldn't be OK is when we start adding logic, caching state, or changing
the way algorithms work.

[mbrunson, 2016-10-14 18:25:16]

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

[mbrunson, 2016-10-14 18:25:26]

https://codereview.chromium.org/2404623002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2404623002/diff/80001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:13: //
Dictionary for address->client mapping
On 2016/10/14 07:14:51, scheib wrote:
> Close with .

Done.

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

https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/device.mojom:36:
DeviceChanged(AdvertisingPacket packet);
On 2016/10/14 07:14:51, scheib wrote:
> On 2016/10/13 22:22:49, ortuno wrote:
> > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the
> adapter
> > client and it should match DeviceChanged more closely i.e. just call with
the
> > address :(
> > 
> > Alternatively we could call it with a DeviceInfo struct since that would
save
> > clients a round trip but that might be too much.
> > 
> > scheib: Do you have any thoughts?
> 
> We've deviated a bit already with DeviceInfo aggregating all the accessors for
> e.g. GetName, GetAddress. If we continue that route and stick to the
> DeviceChanged only replying with an address then the client will still need to
> call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> DeviceInfo packet as well.
> 
> One could argue that populating those packet members will happen either in a
> call to GetInfo or if we do it directly in a response to DeviceChanged. So,
even
> in our "just a thin wrapper" we'll have some untested accessors populating a
> struct. If we know those only have meaning in a DeviceChanged event that I
think
> we may as well make the calls in that event and save the DeviceChanged ->
> address -> GetInfo step and just return the AdvertisingPacket as written here.
> 
> In other words, I think it's OK to clean up naming, and move functionality
into
> better clusters of interfaces, when the work is doing the same thing
regardless
> (populating struct members with accessor calls).
> 
> What wouldn't be OK is when we start adding logic, caching state, or changing
> the way algorithms work.

I remember ortuno recommending the Mojo clients to change, noting that the way
Mac or Android does it seems better. I was planning to follow the BluetoothGatt
pattern from Android where a single client attached to the device interface
handles the GATT-related callbacks for that device. Let me know if you think
this is a good idea because GattConnection and the Service interface will be my
next CLs.

[commit-bot: I haz the power, 2016-10-14 18:26:08]

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-10-14 20:17:24]

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

[commit-bot: I haz the power, 2016-10-14 20:17:25]

Dry run: This issue passed the CQ dry run.

[scheib, 2016-10-14 20:25:40]

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

https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/device.mojom:36:
DeviceChanged(AdvertisingPacket packet);
On 2016/10/14 18:25:25, mbrunson wrote:
> On 2016/10/14 07:14:51, scheib wrote:
> > On 2016/10/13 22:22:49, ortuno wrote:
> > > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the
> > adapter
> > > client and it should match DeviceChanged more closely i.e. just call with
> the
> > > address :(
> > > 
> > > Alternatively we could call it with a DeviceInfo struct since that would
> save
> > > clients a round trip but that might be too much.
> > > 
> > > scheib: Do you have any thoughts?
> > 
> > We've deviated a bit already with DeviceInfo aggregating all the accessors
for
> > e.g. GetName, GetAddress. If we continue that route and stick to the
> > DeviceChanged only replying with an address then the client will still need
to
> > call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> > DeviceInfo packet as well.
> > 
> > One could argue that populating those packet members will happen either in a
> > call to GetInfo or if we do it directly in a response to DeviceChanged. So,
> even
> > in our "just a thin wrapper" we'll have some untested accessors populating a
> > struct. If we know those only have meaning in a DeviceChanged event that I
> think
> > we may as well make the calls in that event and save the DeviceChanged ->
> > address -> GetInfo step and just return the AdvertisingPacket as written
here.
> > 
> > In other words, I think it's OK to clean up naming, and move functionality
> into
> > better clusters of interfaces, when the work is doing the same thing
> regardless
> > (populating struct members with accessor calls).
> > 
> > What wouldn't be OK is when we start adding logic, caching state, or
changing
> > the way algorithms work.
> 
> I remember ortuno recommending the Mojo clients to change, noting that the way
> Mac or Android does it seems better. I was planning to follow the
BluetoothGatt
> pattern from Android where a single client attached to the device interface
> handles the GATT-related callbacks for that device. Let me know if you think
> this is a good idea because GattConnection and the Service interface will be
my
> next CLs.

I dislike the Android and Core Bluetooth way of getting results from e.g.
characteristic reads or notifications on a single client interface -- and then
needing to interpret / route the request. 

Mojo will make this easier for us, as we have an asynchronous response that can
provide a value. So, we should be able to characteristic.Read().then(status,
value); That's what is sketched out on the go/fizzbluetoothinternals doc.
Perhaps update that doc with GattClient (as is here) and other similar changes
and we can look again quickly to verify it makes sense.

Notifications look like the one thing that will need a client, but I'd prefer to
keep the current design doc style of a CharacteristicClient that can be set
per-characteristic and receive notifications just for that object. (versus
having the GattClient receive notifications)

I'm curious ortuno's thoughts.

[ortuno, 2016-10-17 05:03:32]

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

https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/device.mojom:36:
DeviceChanged(AdvertisingPacket packet);
On 2016/10/14 at 20:25:40, scheib wrote:
> On 2016/10/14 18:25:25, mbrunson wrote:
> > On 2016/10/14 07:14:51, scheib wrote:
> > > On 2016/10/13 22:22:49, ortuno wrote:
> > > > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in the
> > > adapter
> > > > client and it should match DeviceChanged more closely i.e. just call
with
> > the
> > > > address :(
> > > > 
> > > > Alternatively we could call it with a DeviceInfo struct since that would
> > save
> > > > clients a round trip but that might be too much.
> > > > 
> > > > scheib: Do you have any thoughts?
> > > 
> > > We've deviated a bit already with DeviceInfo aggregating all the accessors
for
> > > e.g. GetName, GetAddress. If we continue that route and stick to the
> > > DeviceChanged only replying with an address then the client will still
need to
> > > call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> > > DeviceInfo packet as well.
> > > 
> > > One could argue that populating those packet members will happen either in
a
> > > call to GetInfo or if we do it directly in a response to DeviceChanged.
So,
> > even
> > > in our "just a thin wrapper" we'll have some untested accessors populating
a
> > > struct. If we know those only have meaning in a DeviceChanged event that I
> > think
> > > we may as well make the calls in that event and save the DeviceChanged ->
> > > address -> GetInfo step and just return the AdvertisingPacket as written
here.
> > > 
> > > In other words, I think it's OK to clean up naming, and move functionality
> > into
> > > better clusters of interfaces, when the work is doing the same thing
> > regardless
> > > (populating struct members with accessor calls).
> > > 
> > > What wouldn't be OK is when we start adding logic, caching state, or
changing
> > > the way algorithms work.
> > 

If we are going to go ahead with this pattern I have a couple of concerns:

 1. We are adding logic since now Device has to check if the event corresponds
to that specific Bluetooth Device.
 2. We will be adding a ton of observers. Say we have 100 devices each of those
device will receive an event and will have to check if the event corresponds
that device.
 3. Most importantly though, I don't think this event should go in Device. Say
you are interested in all Advertisement Packets. Where would you get them? Would
you have to get a Device interface for each device so you can get all
advertisements?

I'm not opposed to the event but I think it should live in AdapterClient.

> > I remember ortuno recommending the Mojo clients to change, noting that the
way
> > Mac or Android does it seems better. I was planning to follow the
BluetoothGatt
> > pattern from Android where a single client attached to the device interface
> > handles the GATT-related callbacks for that device. Let me know if you think
> > this is a good idea because GattConnection and the Service interface will be
my
> > next CLs.
> 
> I dislike the Android and Core Bluetooth way of getting results from e.g.
characteristic reads or notifications on a single client interface -- and then
needing to interpret / route the request. 
> 
> Mojo will make this easier for us, as we have an asynchronous response that
can provide a value. So, we should be able to characteristic.Read().then(status,
value); That's what is sketched out on the go/fizzbluetoothinternals doc.
Perhaps update that doc with GattClient (as is here) and other similar changes
and we can look again quickly to verify it makes sense.
> 
> Notifications look like the one thing that will need a client, but I'd prefer
to keep the current design doc style of a CharacteristicClient that can be set
per-characteristic and receive notifications just for that object. (versus
having the GattClient receive notifications)
> 
> I'm curious ortuno's thoughts.

(I remember a while ago, I was really excited about Mojo and how I was going to
be able to have an interface for services and another one for characteristics,
etc. But then scheib was like: "But why?"[1] so I started going in the other
direction of just having one interface haha)

My suggestion was more regarding how we perform read/writes rather than where
the events arrive. The asynchronous nature of Mojo means that we will need two
callbacks to get a characteristic or a service which I think makes the code much
harder to follow[2]. Anyway I think we should go for the interface that leads to
the easiest implementation on top of device/bluetooth.

I would personally go for the following due to the ease of implementation and
lack of extra interfaces:

interface Device { // Note that everything is in the device.
  GetServices() => (array<ServiceInfo> services);
  GetCharacteristics(string service_id) => (array<CharacteristicInfo>
characteristics);
  GetDescriptors(string service_id, string characteristic_id) =>
(array<DescriptorInfo> descriptors);

  ReadValueForCharacteristic(string service_id, string characteristic_id) =>
(array<uint8_t> value);
  WriteValueForCharacteristic(string service_id, string characteristic_id,
array<uint8_t>) => ();

  StartNotificationsForCharacteristic(string service_id, string
characteristic_id);
  SetNotificationsClient(string service_id, string characteristic_id, client);
  
}


// Device.cpp

ReadValueForCharacteristic(string service_id, string characteristic_id,
callback) {
  auto* device = adapter_->GetDevice(device_address_);
  if (device == nullptr)
    callback.Run(/* some error */);
  
  auto* service = device->GetService(service_id);
  if (service == nullptr)
    callback.Run(/* some error */);
  
  auto* characteristic = service->GetCharacteristic(characteristic_id);
  if (characteristic == nullptr)
    callback.Run(/* some error */);

  characteristic->ReadValue(base::Bind(..., callback));
}

I don't think we can get around the routing since we are not allowed to keep
pointers to any objects :(

[1] http://i.imgur.com/Br00TCn.gif?noredirect

[2]
Interface per object:

interface Service {
  GetCharacteristic(CharacteristicId id);
  GetCharacteristics() => (array<CharacteristicInfo> characteristics);
}

How Web Bluetooth would use it:

// BluetoothRemoteGATTService.cpp
GetCharacteristic(String uuid) {
  service_->GetCharacteristics(base::Bind(&OnGetCharacteristics, this, uuid,
...));
});

OnGetCharacteristics(uuid, characteristics, promise) {
  for (characteristic : characteristics) {
    if (characteristic->uuid == uuid) {
      service_->GetCharacteristic(base::Bind(&OnGetCharacteristic, this, uuid,
characteristic->id, promise));
      return;
    }
  }
}

OnGetCharacteristic(uuid, id, characteristic, promise) {
  promise.resolve(new BluetoothRemoteGATTCharacteristic(id, uuid));
}

As opposed to:

interface GattRemoteServer {
  GetServices() => (array<ServiceInfo> services);
  GetCharacteristicsForService(ServiceId id) => (array<CharacteristicInfo>
characteristics);
}

How Web Bluetooth would use it:

// BluetoothRemoteGATTService.cpp
GetCharacteristic(String uuid) {
  gatt_->GetCharacteristicsForService(base::Bind(&OnGetCharacteristics, this,
uuid, ...));
}

OnGetCharacteristics(uuid, promise, characteristics) {
  for (characteristic : characteristics) {
    if (characteristic->uuid == uuid) {
      promise.resolve(new BluetoothRemoteGATTCharacteristic(characteristic));
    }
  }
  promise.reject("Not Found Error");
}

Regarding notifications, I think we can still have a client just for
notifications:

interface RemoteGATTServer {
  SetCharacteristicNotificationsClient(CharacteristicId id,
CharacteristicNotificationClient client)
}

interface CharacteristicNotificationClient {
  NotificationReceived(type, value, ...);
}

or something like that.

[mbrunson, 2016-10-17 21:33:38]

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

https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/device.mojom:36:
DeviceChanged(AdvertisingPacket packet);
On 2016/10/17 05:03:31, ortuno wrote:
> On 2016/10/14 at 20:25:40, scheib wrote:
> > On 2016/10/14 18:25:25, mbrunson wrote:
> > > On 2016/10/14 07:14:51, scheib wrote:
> > > > On 2016/10/13 22:22:49, ortuno wrote:
> > > > > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in
the
> > > > adapter
> > > > > client and it should match DeviceChanged more closely i.e. just call
> with
> > > the
> > > > > address :(
> > > > > 
> > > > > Alternatively we could call it with a DeviceInfo struct since that
would
> > > save
> > > > > clients a round trip but that might be too much.
> > > > > 
> > > > > scheib: Do you have any thoughts?
> > > > 
> > > > We've deviated a bit already with DeviceInfo aggregating all the
accessors
> for
> > > > e.g. GetName, GetAddress. If we continue that route and stick to the
> > > > DeviceChanged only replying with an address then the client will still
> need to
> > > > call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> > > > DeviceInfo packet as well.
> > > > 
> > > > One could argue that populating those packet members will happen either
in
> a
> > > > call to GetInfo or if we do it directly in a response to DeviceChanged.
> So,
> > > even
> > > > in our "just a thin wrapper" we'll have some untested accessors
populating
> a
> > > > struct. If we know those only have meaning in a DeviceChanged event that
I
> > > think
> > > > we may as well make the calls in that event and save the DeviceChanged
->
> > > > address -> GetInfo step and just return the AdvertisingPacket as written
> here.
> > > > 
> > > > In other words, I think it's OK to clean up naming, and move
functionality
> > > into
> > > > better clusters of interfaces, when the work is doing the same thing
> > > regardless
> > > > (populating struct members with accessor calls).
> > > > 
> > > > What wouldn't be OK is when we start adding logic, caching state, or
> changing
> > > > the way algorithms work.
> > > 
> 
> If we are going to go ahead with this pattern I have a couple of concerns:
> 
>  1. We are adding logic since now Device has to check if the event corresponds
> to that specific Bluetooth Device.
>  2. We will be adding a ton of observers. Say we have 100 devices each of
those
> device will receive an event and will have to check if the event corresponds
> that device.
>  3. Most importantly though, I don't think this event should go in Device. Say
> you are interested in all Advertisement Packets. Where would you get them?
Would
> you have to get a Device interface for each device so you can get all
> advertisements?
> 
> I'm not opposed to the event but I think it should live in AdapterClient.
> 
> > > I remember ortuno recommending the Mojo clients to change, noting that the
> way
> > > Mac or Android does it seems better. I was planning to follow the
> BluetoothGatt
> > > pattern from Android where a single client attached to the device
interface
> > > handles the GATT-related callbacks for that device. Let me know if you
think
> > > this is a good idea because GattConnection and the Service interface will
be
> my
> > > next CLs.
> > 
> > I dislike the Android and Core Bluetooth way of getting results from e.g.
> characteristic reads or notifications on a single client interface -- and then
> needing to interpret / route the request. 
> > 
> > Mojo will make this easier for us, as we have an asynchronous response that
> can provide a value. So, we should be able to
characteristic.Read().then(status,
> value); That's what is sketched out on the go/fizzbluetoothinternals doc.
> Perhaps update that doc with GattClient (as is here) and other similar changes
> and we can look again quickly to verify it makes sense.
> > 
> > Notifications look like the one thing that will need a client, but I'd
prefer
> to keep the current design doc style of a CharacteristicClient that can be set
> per-characteristic and receive notifications just for that object. (versus
> having the GattClient receive notifications)
> > 
> > I'm curious ortuno's thoughts.
> 
> (I remember a while ago, I was really excited about Mojo and how I was going
to
> be able to have an interface for services and another one for characteristics,
> etc. But then scheib was like: "But why?"[1] so I started going in the other
> direction of just having one interface haha)
> 
> My suggestion was more regarding how we perform read/writes rather than where
> the events arrive. The asynchronous nature of Mojo means that we will need two
> callbacks to get a characteristic or a service which I think makes the code
much
> harder to follow[2]. Anyway I think we should go for the interface that leads
to
> the easiest implementation on top of device/bluetooth.
> 
> I would personally go for the following due to the ease of implementation and
> lack of extra interfaces:
> 
> interface Device { // Note that everything is in the device.
>   GetServices() => (array<ServiceInfo> services);
>   GetCharacteristics(string service_id) => (array<CharacteristicInfo>
> characteristics);
>   GetDescriptors(string service_id, string characteristic_id) =>
> (array<DescriptorInfo> descriptors);
> 
>   ReadValueForCharacteristic(string service_id, string characteristic_id) =>
> (array<uint8_t> value);
>   WriteValueForCharacteristic(string service_id, string characteristic_id,
> array<uint8_t>) => ();
> 
>   StartNotificationsForCharacteristic(string service_id, string
> characteristic_id);
>   SetNotificationsClient(string service_id, string characteristic_id, client);
>   
> }
> 
> 
> // Device.cpp
> 
> ReadValueForCharacteristic(string service_id, string characteristic_id,
> callback) {
>   auto* device = adapter_->GetDevice(device_address_);
>   if (device == nullptr)
>     callback.Run(/* some error */);
>   
>   auto* service = device->GetService(service_id);
>   if (service == nullptr)
>     callback.Run(/* some error */);
>   
>   auto* characteristic = service->GetCharacteristic(characteristic_id);
>   if (characteristic == nullptr)
>     callback.Run(/* some error */);
> 
>   characteristic->ReadValue(base::Bind(..., callback));
> }
> 
> I don't think we can get around the routing since we are not allowed to keep
> pointers to any objects :(
> 
> [1] http://i.imgur.com/Br00TCn.gif?noredirect
> 
> [2]
> Interface per object:
> 
> interface Service {
>   GetCharacteristic(CharacteristicId id);
>   GetCharacteristics() => (array<CharacteristicInfo> characteristics);
> }
> 
> How Web Bluetooth would use it:
> 
> // BluetoothRemoteGATTService.cpp
> GetCharacteristic(String uuid) {
>   service_->GetCharacteristics(base::Bind(&OnGetCharacteristics, this, uuid,
> ...));
> });
> 
> OnGetCharacteristics(uuid, characteristics, promise) {
>   for (characteristic : characteristics) {
>     if (characteristic->uuid == uuid) {
>       service_->GetCharacteristic(base::Bind(&OnGetCharacteristic, this, uuid,
> characteristic->id, promise));
>       return;
>     }
>   }
> }
> 
> OnGetCharacteristic(uuid, id, characteristic, promise) {
>   promise.resolve(new BluetoothRemoteGATTCharacteristic(id, uuid));
> }
> 
> As opposed to:
> 
> interface GattRemoteServer {
>   GetServices() => (array<ServiceInfo> services);
>   GetCharacteristicsForService(ServiceId id) => (array<CharacteristicInfo>
> characteristics);
> }
> 
> How Web Bluetooth would use it:
> 
> // BluetoothRemoteGATTService.cpp
> GetCharacteristic(String uuid) {
>   gatt_->GetCharacteristicsForService(base::Bind(&OnGetCharacteristics, this,
> uuid, ...));
> }
> 
> OnGetCharacteristics(uuid, promise, characteristics) {
>   for (characteristic : characteristics) {
>     if (characteristic->uuid == uuid) {
>       promise.resolve(new BluetoothRemoteGATTCharacteristic(characteristic));
>     }
>   }
>   promise.reject("Not Found Error");
> }
> 
> Regarding notifications, I think we can still have a client just for
> notifications:
> 
> interface RemoteGATTServer {
>   SetCharacteristicNotificationsClient(CharacteristicId id,
> CharacteristicNotificationClient client)
> }
> 
> interface CharacteristicNotificationClient {
>   NotificationReceived(type, value, ...);
> }
> 
> or something like that.

I've moved PacketReceived to Adapter. 

Given your suggestion, where would the GATT connection logic go?  Instead of
having the functions in Device, putting them in RemoteGattServer and having the
Device call "ConnectGatt" that returns a RemoteGattServer seems like a good idea
(I've noted these changes in the go/fizzbluetoothinternals design doc). This
leaves room for other Bluetooth device logic (pairing, other types of
connections, etc.) and keeps the GATT connection's lifetime tied to the lifetime
of the RemoteGattServer instance. Thoughts?

[mbrunson, 2016-10-17 21:34:23]

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

[commit-bot: I haz the power, 2016-10-17 21:34:55]

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-10-17 23:13:44]

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

[commit-bot: I haz the power, 2016-10-17 23:13:45]

Dry run: This issue passed the CQ dry run.

[scheib, 2016-10-18 05:37:42]

On 2016/10/17 05:03:32, ortuno wrote:
>
https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
> File device/bluetooth/public/interfaces/device.mojom (right):
> 
>
https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
> device/bluetooth/public/interfaces/device.mojom:36:
> DeviceChanged(AdvertisingPacket packet);
> On 2016/10/14 at 20:25:40, scheib wrote:
> > On 2016/10/14 18:25:25, mbrunson wrote:
> > > On 2016/10/14 07:14:51, scheib wrote:
> > > > On 2016/10/13 22:22:49, ortuno wrote:
> > > > > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in
the
> > > > adapter
> > > > > client and it should match DeviceChanged more closely i.e. just call
> with
> > > the
> > > > > address :(
> > > > > 
> > > > > Alternatively we could call it with a DeviceInfo struct since that
would
> > > save
> > > > > clients a round trip but that might be too much.
> > > > > 
> > > > > scheib: Do you have any thoughts?
> > > > 
> > > > We've deviated a bit already with DeviceInfo aggregating all the
accessors
> for
> > > > e.g. GetName, GetAddress. If we continue that route and stick to the
> > > > DeviceChanged only replying with an address then the client will still
> need to
> > > > call GetInfo. At that point we'll need to include GetInquiryRSSI in the
> > > > DeviceInfo packet as well.
> > > > 
> > > > One could argue that populating those packet members will happen either
in
> a
> > > > call to GetInfo or if we do it directly in a response to DeviceChanged.
> So,
> > > even
> > > > in our "just a thin wrapper" we'll have some untested accessors
populating
> a
> > > > struct. If we know those only have meaning in a DeviceChanged event that
I
> > > think
> > > > we may as well make the calls in that event and save the DeviceChanged
->
> > > > address -> GetInfo step and just return the AdvertisingPacket as written
> here.
> > > > 
> > > > In other words, I think it's OK to clean up naming, and move
functionality
> > > into
> > > > better clusters of interfaces, when the work is doing the same thing
> > > regardless
> > > > (populating struct members with accessor calls).
> > > > 
> > > > What wouldn't be OK is when we start adding logic, caching state, or
> changing
> > > > the way algorithms work.
> > > 
> 
> If we are going to go ahead with this pattern I have a couple of concerns:
> 
>  1. We are adding logic since now Device has to check if the event corresponds
> to that specific Bluetooth Device.
>  2. We will be adding a ton of observers. Say we have 100 devices each of
those
> device will receive an event and will have to check if the event corresponds
> that device.
>  3. Most importantly though, I don't think this event should go in Device. Say
> you are interested in all Advertisement Packets. Where would you get them?
Would
> you have to get a Device interface for each device so you can get all
> advertisements?
> 
> I'm not opposed to the event but I think it should live in AdapterClient.
> 
> > > I remember ortuno recommending the Mojo clients to change, noting that the
> way
> > > Mac or Android does it seems better. I was planning to follow the
> BluetoothGatt
> > > pattern from Android where a single client attached to the device
interface
> > > handles the GATT-related callbacks for that device. Let me know if you
think
> > > this is a good idea because GattConnection and the Service interface will
be
> my
> > > next CLs.
> > 
> > I dislike the Android and Core Bluetooth way of getting results from e.g.
> characteristic reads or notifications on a single client interface -- and then
> needing to interpret / route the request. 
> > 
> > Mojo will make this easier for us, as we have an asynchronous response that
> can provide a value. So, we should be able to
characteristic.Read().then(status,
> value); That's what is sketched out on the go/fizzbluetoothinternals doc.
> Perhaps update that doc with GattClient (as is here) and other similar changes
> and we can look again quickly to verify it makes sense.
> > 
> > Notifications look like the one thing that will need a client, but I'd
prefer
> to keep the current design doc style of a CharacteristicClient that can be set
> per-characteristic and receive notifications just for that object. (versus
> having the GattClient receive notifications)
> > 
> > I'm curious ortuno's thoughts.
> 
> (I remember a while ago, I was really excited about Mojo and how I was going
to
> be able to have an interface for services and another one for characteristics,
> etc. But then scheib was like: "But why?"[1] so I started going in the other
> direction of just having one interface haha)
> 
> My suggestion was more regarding how we perform read/writes rather than where
> the events arrive. The asynchronous nature of Mojo means that we will need two
> callbacks to get a characteristic or a service which I think makes the code
much
> harder to follow[2].

I didn't follow why the examples had different numbers of callbacks. I tried
to organize them into a diff https://www.diffchecker.com/CZXH7koo
Both interfaces get all the characteristic infos for a service,
then check all the UUIDs in a for loop. Why is one able to then directly
construct a charactertistic, but the other needs to make a call?

> Anyway I think we should go for the interface that leads to
> the easiest implementation on top of device/bluetooth.

Ok, perhaps so. But, as mentioned elsewhere, perhaps it's better to 
at least partition out the Gatt interface.

> 
> I would personally go for the following due to the ease of implementation and
> lack of extra interfaces:
> 
> interface Device { // Note that everything is in the device.
>   GetServices() => (array<ServiceInfo> services);
>   GetCharacteristics(string service_id) => (array<CharacteristicInfo>
> characteristics);
>   GetDescriptors(string service_id, string characteristic_id) =>
> (array<DescriptorInfo> descriptors);
> 
>   ReadValueForCharacteristic(string service_id, string characteristic_id) =>
> (array<uint8_t> value);
>   WriteValueForCharacteristic(string service_id, string characteristic_id,
> array<uint8_t>) => ();
> 
>   StartNotificationsForCharacteristic(string service_id, string
> characteristic_id);
>   SetNotificationsClient(string service_id, string characteristic_id, client);
>   
> }
> 
> 
> // Device.cpp
> 
> ReadValueForCharacteristic(string service_id, string characteristic_id,
> callback) {
>   auto* device = adapter_->GetDevice(device_address_);
>   if (device == nullptr)
>     callback.Run(/* some error */);
>   
>   auto* service = device->GetService(service_id);
>   if (service == nullptr)
>     callback.Run(/* some error */);
>   
>   auto* characteristic = service->GetCharacteristic(characteristic_id);
>   if (characteristic == nullptr)
>     callback.Run(/* some error */);
> 
>   characteristic->ReadValue(base::Bind(..., callback));
> }
> 
> I don't think we can get around the routing since we are not allowed to keep
> pointers to any objects :(
> 
> [1] http://i.imgur.com/Br00TCn.gif?noredirect
> 
> [2]
> Interface per object:
> 
> interface Service {
>   GetCharacteristic(CharacteristicId id);
>   GetCharacteristics() => (array<CharacteristicInfo> characteristics);
> }
> 
> How Web Bluetooth would use it:
> 
> // BluetoothRemoteGATTService.cpp
> GetCharacteristic(String uuid) {
>   service_->GetCharacteristics(base::Bind(&OnGetCharacteristics, this, uuid,
> ...));
> });
> 
> OnGetCharacteristics(uuid, characteristics, promise) {
>   for (characteristic : characteristics) {
>     if (characteristic->uuid == uuid) {
>       service_->GetCharacteristic(base::Bind(&OnGetCharacteristic, this, uuid,
> characteristic->id, promise));
>       return;
>     }
>   }
> }
> 
> OnGetCharacteristic(uuid, id, characteristic, promise) {
>   promise.resolve(new BluetoothRemoteGATTCharacteristic(id, uuid));
> }
> 
> As opposed to:
> 
> interface GattRemoteServer {
>   GetServices() => (array<ServiceInfo> services);
>   GetCharacteristicsForService(ServiceId id) => (array<CharacteristicInfo>
> characteristics);
> }
> 
> How Web Bluetooth would use it:
> 
> // BluetoothRemoteGATTService.cpp
> GetCharacteristic(String uuid) {
>   gatt_->GetCharacteristicsForService(base::Bind(&OnGetCharacteristics, this,
> uuid, ...));
> }
> 
> OnGetCharacteristics(uuid, promise, characteristics) {
>   for (characteristic : characteristics) {
>     if (characteristic->uuid == uuid) {
>       promise.resolve(new BluetoothRemoteGATTCharacteristic(characteristic));
>     }
>   }
>   promise.reject("Not Found Error");
> }
> 
> Regarding notifications, I think we can still have a client just for
> notifications:
> 
> interface RemoteGATTServer {
>   SetCharacteristicNotificationsClient(CharacteristicId id,
> CharacteristicNotificationClient client)
> }
> 
> interface CharacteristicNotificationClient {
>   NotificationReceived(type, value, ...);
> }
> 
> or something like that.

[ortuno, 2016-10-18 05:58:47]

On 2016/10/18 at 05:37:42, scheib wrote:
> On 2016/10/17 05:03:32, ortuno wrote:
> >
https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
> > File device/bluetooth/public/interfaces/device.mojom (right):
> > 
> >
https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public...
> > device/bluetooth/public/interfaces/device.mojom:36:
> > DeviceChanged(AdvertisingPacket packet);
> > On 2016/10/14 at 20:25:40, scheib wrote:
> > > On 2016/10/14 18:25:25, mbrunson wrote:
> > > > On 2016/10/14 07:14:51, scheib wrote:
> > > > > On 2016/10/13 22:22:49, ortuno wrote:
> > > > > > In the spirit of "Just a Thin Wrapper™️" I think this needs to go in
the
> > > > > adapter
> > > > > > client and it should match DeviceChanged more closely i.e. just call
> > with
> > > > the
> > > > > > address :(
> > > > > > 
> > > > > > Alternatively we could call it with a DeviceInfo struct since that
would
> > > > save
> > > > > > clients a round trip but that might be too much.
> > > > > > 
> > > > > > scheib: Do you have any thoughts?
> > > > > 
> > > > > We've deviated a bit already with DeviceInfo aggregating all the
accessors
> > for
> > > > > e.g. GetName, GetAddress. If we continue that route and stick to the
> > > > > DeviceChanged only replying with an address then the client will still
> > need to
> > > > > call GetInfo. At that point we'll need to include GetInquiryRSSI in
the
> > > > > DeviceInfo packet as well.
> > > > > 
> > > > > One could argue that populating those packet members will happen
either in
> > a
> > > > > call to GetInfo or if we do it directly in a response to
DeviceChanged.
> > So,
> > > > even
> > > > > in our "just a thin wrapper" we'll have some untested accessors
populating
> > a
> > > > > struct. If we know those only have meaning in a DeviceChanged event
that I
> > > > think
> > > > > we may as well make the calls in that event and save the DeviceChanged
->
> > > > > address -> GetInfo step and just return the AdvertisingPacket as
written
> > here.
> > > > > 
> > > > > In other words, I think it's OK to clean up naming, and move
functionality
> > > > into
> > > > > better clusters of interfaces, when the work is doing the same thing
> > > > regardless
> > > > > (populating struct members with accessor calls).
> > > > > 
> > > > > What wouldn't be OK is when we start adding logic, caching state, or
> > changing
> > > > > the way algorithms work.
> > > > 
> > 
> > If we are going to go ahead with this pattern I have a couple of concerns:
> > 
> >  1. We are adding logic since now Device has to check if the event
corresponds
> > to that specific Bluetooth Device.
> >  2. We will be adding a ton of observers. Say we have 100 devices each of
those
> > device will receive an event and will have to check if the event corresponds
> > that device.
> >  3. Most importantly though, I don't think this event should go in Device.
Say
> > you are interested in all Advertisement Packets. Where would you get them?
Would
> > you have to get a Device interface for each device so you can get all
> > advertisements?
> > 
> > I'm not opposed to the event but I think it should live in AdapterClient.
> > 
> > > > I remember ortuno recommending the Mojo clients to change, noting that
the
> > way
> > > > Mac or Android does it seems better. I was planning to follow the
> > BluetoothGatt
> > > > pattern from Android where a single client attached to the device
interface
> > > > handles the GATT-related callbacks for that device. Let me know if you
think
> > > > this is a good idea because GattConnection and the Service interface
will be
> > my
> > > > next CLs.
> > > 
> > > I dislike the Android and Core Bluetooth way of getting results from e.g.
> > characteristic reads or notifications on a single client interface -- and
then
> > needing to interpret / route the request. 
> > > 
> > > Mojo will make this easier for us, as we have an asynchronous response
that
> > can provide a value. So, we should be able to
characteristic.Read().then(status,
> > value); That's what is sketched out on the go/fizzbluetoothinternals doc.
> > Perhaps update that doc with GattClient (as is here) and other similar
changes
> > and we can look again quickly to verify it makes sense.
> > > 
> > > Notifications look like the one thing that will need a client, but I'd
prefer
> > to keep the current design doc style of a CharacteristicClient that can be
set
> > per-characteristic and receive notifications just for that object. (versus
> > having the GattClient receive notifications)
> > > 
> > > I'm curious ortuno's thoughts.
> > 
> > (I remember a while ago, I was really excited about Mojo and how I was going
to
> > be able to have an interface for services and another one for
characteristics,
> > etc. But then scheib was like: "But why?"[1] so I started going in the other
> > direction of just having one interface haha)
> > 
> > My suggestion was more regarding how we perform read/writes rather than
where
> > the events arrive. The asynchronous nature of Mojo means that we will need
two
> > callbacks to get a characteristic or a service which I think makes the code
much
> > harder to follow[2].
> 
> I didn't follow why the examples had different numbers of callbacks. I tried
> to organize them into a diff https://www.diffchecker.com/CZXH7koo
> Both interfaces get all the characteristic infos for a service,
> then check all the UUIDs in a for loop. Why is one able to then directly
> construct a charactertistic, but the other needs to make a call?
> 
> > Anyway I think we should go for the interface that leads to
> > the easiest implementation on top of device/bluetooth.
> 
> Ok, perhaps so. But, as mentioned elsewhere, perhaps it's better to 
> at least partition out the Gatt interface.
> 
> > 
> > I would personally go for the following due to the ease of implementation
and
> > lack of extra interfaces:
> > 
> > interface Device { // Note that everything is in the device.
> >   GetServices() => (array<ServiceInfo> services);
> >   GetCharacteristics(string service_id) => (array<CharacteristicInfo>
> > characteristics);
> >   GetDescriptors(string service_id, string characteristic_id) =>
> > (array<DescriptorInfo> descriptors);
> > 
> >   ReadValueForCharacteristic(string service_id, string characteristic_id) =>
> > (array<uint8_t> value);
> >   WriteValueForCharacteristic(string service_id, string characteristic_id,
> > array<uint8_t>) => ();
> > 
> >   StartNotificationsForCharacteristic(string service_id, string
> > characteristic_id);
> >   SetNotificationsClient(string service_id, string characteristic_id,
client);
> >   
> > }
> > 
> > 
> > // Device.cpp
> > 
> > ReadValueForCharacteristic(string service_id, string characteristic_id,
> > callback) {
> >   auto* device = adapter_->GetDevice(device_address_);
> >   if (device == nullptr)
> >     callback.Run(/* some error */);
> >   
> >   auto* service = device->GetService(service_id);
> >   if (service == nullptr)
> >     callback.Run(/* some error */);
> >   
> >   auto* characteristic = service->GetCharacteristic(characteristic_id);
> >   if (characteristic == nullptr)
> >     callback.Run(/* some error */);
> > 
> >   characteristic->ReadValue(base::Bind(..., callback));
> > }
> > 
> > I don't think we can get around the routing since we are not allowed to keep
> > pointers to any objects :(
> > 
> > [1] http://i.imgur.com/Br00TCn.gif?noredirect
> > 
> > [2]
> > Interface per object:
> > 
> > interface Service {
> >   GetCharacteristic(CharacteristicId id);
> >   GetCharacteristics() => (array<CharacteristicInfo> characteristics);
> > }
> > 
> > How Web Bluetooth would use it:
> > 
> > // BluetoothRemoteGATTService.cpp
> > GetCharacteristic(String uuid) {
> >   service_->GetCharacteristics(base::Bind(&OnGetCharacteristics, this, uuid,
> > ...));
> > });
> > 
> > OnGetCharacteristics(uuid, characteristics, promise) {
> >   for (characteristic : characteristics) {
> >     if (characteristic->uuid == uuid) {
> >       service_->GetCharacteristic(base::Bind(&OnGetCharacteristic, this,
uuid,
> > characteristic->id, promise));
> >       return;
> >     }
> >   }
> > }
> > 
> > OnGetCharacteristic(uuid, id, characteristic, promise) {
> >   promise.resolve(new BluetoothRemoteGATTCharacteristic(id, uuid));
> > }
> > 
> > As opposed to:
> > 
> > interface GattRemoteServer {
> >   GetServices() => (array<ServiceInfo> services);
> >   GetCharacteristicsForService(ServiceId id) => (array<CharacteristicInfo>
> > characteristics);
> > }
> > 
> > How Web Bluetooth would use it:
> > 
> > // BluetoothRemoteGATTService.cpp
> > GetCharacteristic(String uuid) {
> >   gatt_->GetCharacteristicsForService(base::Bind(&OnGetCharacteristics,
this,
> > uuid, ...));
> > }
> > 
> > OnGetCharacteristics(uuid, promise, characteristics) {
> >   for (characteristic : characteristics) {
> >     if (characteristic->uuid == uuid) {
> >       promise.resolve(new
BluetoothRemoteGATTCharacteristic(characteristic));
> >     }
> >   }
> >   promise.reject("Not Found Error");
> > }
> > 
> > Regarding notifications, I think we can still have a client just for
> > notifications:
> > 
> > interface RemoteGATTServer {
> >   SetCharacteristicNotificationsClient(CharacteristicId id,
> > CharacteristicNotificationClient client)
> > }
> > 
> > interface CharacteristicNotificationClient {
> >   NotificationReceived(type, value, ...);
> > }
> > 
> > or something like that.

In the first example you need an interface pointer to interact with a service,
that's where GetCharacteristics() would be. In the second example there are no
Interfaces for Services or Characteristics, only Ids so you don't need to
acquire an interface pointer.

[mbrunson, 2016-10-18 22:59:09]

Ok. Makes sense to me. Keeping things on topic, what else do I have to change
for this CL?

[mbrunson, 2016-10-19 01:09:24]

Description was changed from

==========
bluetooth: Add advertising packet logging in Device service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in Device.
Converts DeviceChanged data to AdvertisingPacket and sends to client.
Logs received advertising packets in console during Web Bluetooth scanning.

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

to

==========
bluetooth: Add DeviceChanged logging in Device service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in Device.
Logs received DeviceChanged events in console.

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

[mbrunson, 2016-10-19 18:27:26]

On 2016/10/18 22:59:09, mbrunson wrote:
> Ok. Makes sense to me. Keeping things on topic, what else do I have to change
> for this CL?

As discussed in the daily meeting, I have changed PacketReceived to
DeviceChanged and added an RSSI value to DeviceInfo. Let me know what else needs
to be changed.

[scheib, 2016-10-19 21:28:56]

https://codereview.chromium.org/2404623002/diff/180001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2404623002/diff/180001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:11: int8 rssi;
I'd prefer we not use magic values to indicate the value isn't there. Sounds
like we can't use int8? rssi, and we discussed in meeting filing a bug on mojo
and working around this and leaving TODOs by having an optional struct, e.g. 

struct RSSIWrapper { int8 value; } 

and then in DeviceInfo { RSSIWrapper? rssi; }. 

Does that work?

[mbrunson, 2016-10-19 22:29:22]

https://codereview.chromium.org/2404623002/diff/180001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2404623002/diff/180001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:11: int8 rssi;
On 2016/10/19 21:28:56, scheib wrote:
> I'd prefer we not use magic values to indicate the value isn't there. Sounds
> like we can't use int8? rssi, and we discussed in meeting filing a bug on mojo
> and working around this and leaving TODOs by having an optional struct, e.g. 
> 
> struct RSSIWrapper { int8 value; } 
> 
> and then in DeviceInfo { RSSIWrapper? rssi; }. 
> 
> Does that work?

Done.

[ortuno, 2016-10-19 23:19:19]

lgtm

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

https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:46: * Logs
received advertising packet to console. caches the last valid
nit: s/caches/Caches/

https://codereview.chromium.org/2404623002/diff/220001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2404623002/diff/220001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:8: struct RSSIWrapper {
I wish we had an Optional<> in javascript :/

[scheib, 2016-10-19 23:49:24]

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

https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:30:
deviceAdded: function(device) {
function(deviceInfo) - so that we don't confuse this return as a Device.

https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:54: if
(deviceInfo.rssi) {
Shouldn't this cache all the deviceInfo, always?

[mbrunson, 2016-10-20 00:33:51]

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

https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:30:
deviceAdded: function(device) {
On 2016/10/19 23:49:24, scheib wrote:
> function(deviceInfo) - so that we don't confuse this return as a Device.

Done.

https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:46: * Logs
received advertising packet to console. caches the last valid
On 2016/10/19 23:19:19, ortuno wrote:
> nit: s/caches/Caches/

This whole comment needs to be fixed. Done.

https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:54: if
(deviceInfo.rssi) {
On 2016/10/19 23:49:24, scheib wrote:
> Shouldn't this cache all the deviceInfo, always?

I think it's fine if the data model has the updated RSSI, but I want the UI to
display the last valid RSSI for debugging purposes.

The last DeviceInfo received from deviceChanged when the chooser dialog closes
always lacks the RSSI value which overwrites whatever was there with null. I
need to update the logic for the UI so this doesn't happen but that will be in a
different set of patches.

https://codereview.chromium.org/2404623002/diff/220001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/device.mojom (right):

https://codereview.chromium.org/2404623002/diff/220001/device/bluetooth/publi...
device/bluetooth/public/interfaces/device.mojom:8: struct RSSIWrapper {
On 2016/10/19 23:19:19, ortuno wrote:
> I wish we had an Optional<> in javascript :/

Yeah... I think that's planned for ES8. I think the closest we get now is
overloading valueOf or toString mixed with funky getters and setters...eww.

[scheib, 2016-10-20 01:20:02]

LGTM

[mbrunson, 2016-10-20 01:36:35]

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

[commit-bot: I haz the power, 2016-10-20 01:37:05]

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

[mbrunson, 2016-10-20 01:43:50]

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

[mbrunson, 2016-10-20 01:43:51]

OWNERS review, please:

dcheng:
  device/bluetooth/public/interfaces/adapter.mojom
  device/bluetooth/public/interfaces/device.mojom
                                                                                
                                        
dpapad:                                                                         
                                                                                
        
  chrome/browser/resources/bluetooth_internals/bluetooth_internals.js

[commit-bot: I haz the power, 2016-10-20 04:05:18]

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

[commit-bot: I haz the power, 2016-10-20 04:05:19]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-10-20 17:52:03]

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

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:14: var
devices = {};
Can you use a proper Javascript Map instance instead?

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: function
Device() {};
Why is this a class with a prototype? It has no methods only one data field.

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:32:
devices[deviceInfo.address] = new Device();
Again, don't fully understand what you are trying to do here. Seems like the
following would be sufficient, without the need for the additional wrapper
Device instance.

devices[deviceInfo.address] = deviceInfo;

Or if you use a Map
devices.set(deviceInfo.address, deviceInfo);

[mbrunson, 2016-10-20 18:15:36]

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

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: function
Device() {};
On 2016/10/20 17:52:03, dpapad wrote:
> Why is this a class with a prototype? It has no methods only one data field.

I'm using it as a simple data container right now. Do you recommend  putting the
data fields in the constructor or using a raw object and using Object.create?

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:32:
devices[deviceInfo.address] = new Device();
On 2016/10/20 17:52:03, dpapad wrote:
> Again, don't fully understand what you are trying to do here. Seems like the
> following would be sufficient, without the need for the additional wrapper
> Device instance.
> 
> devices[deviceInfo.address] = deviceInfo;
> 
> Or if you use a Map
> devices.set(deviceInfo.address, deviceInfo);

The Device instance is just a data container to group the two parts of a Device:
DeviceInfo and the Device remote interface Mojo proxy. At the moment, I'm not
using the proxy, so I didn't include it in the object yet.

[dpapad, 2016-10-20 18:36:22]

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

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: function
Device() {};
On 2016/10/20 at 18:15:36, mbrunson wrote:
> On 2016/10/20 17:52:03, dpapad wrote:
> > Why is this a class with a prototype? It has no methods only one data field.
> 
> I'm using it as a simple data container right now. Do you recommend  putting
the data fields in the constructor or using a raw object and using
Object.create?

Yes, data fields should be in the constructor. Weird things can happen when you
put them in the prototype. Quick example (you can try in dev console).

var D = function() {};
D.prototype.foo = [1,2,3];
var d1 = new D();
var d2 = new D();
d1.foo.push(4);
console.log(d2.foo);
// What do you expect here? |foo| is the same array instance for all D
instances, in other words it is 'static'.

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:32:
devices[deviceInfo.address] = new Device();
On 2016/10/20 at 18:15:36, mbrunson wrote:
> On 2016/10/20 17:52:03, dpapad wrote:
> > Again, don't fully understand what you are trying to do here. Seems like the
> > following would be sufficient, without the need for the additional wrapper
> > Device instance.
> > 
> > devices[deviceInfo.address] = deviceInfo;
> > 
> > Or if you use a Map
> > devices.set(deviceInfo.address, deviceInfo);
> 
> The Device instance is just a data container to group the two parts of a
Device: DeviceInfo and the Device remote interface Mojo proxy. At the moment,
I'm not using the proxy, so I didn't include it in the object yet.

If you want to go the class route instead of a simple object, put the member
variables in the constructor.

var Device = function() {
  this.proxy = null;
  this.info = null;
};

If you are fine with simple on-the-fly object see my previous suggestion. Then,
when you do add the proxy you can update the code as follows (no need to use
Object.create).

- devices[deviceInfo.address] = deviceInfo;
+ devices[deviceInfo.address] = {proxy: someProxy, info: deviceInfo};

[mbrunson, 2016-10-20 20:09:04]

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

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:14: var
devices = {};
On 2016/10/20 17:52:03, dpapad wrote:
> Can you use a proper Javascript Map instance instead?

Done.

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: function
Device() {};
On 2016/10/20 18:36:21, dpapad wrote:
> On 2016/10/20 at 18:15:36, mbrunson wrote:
> > On 2016/10/20 17:52:03, dpapad wrote:
> > > Why is this a class with a prototype? It has no methods only one data
field.
> > 
> > I'm using it as a simple data container right now. Do you recommend  putting
> the data fields in the constructor or using a raw object and using
> Object.create?
> 
> Yes, data fields should be in the constructor. Weird things can happen when
you
> put them in the prototype. Quick example (you can try in dev console).
> 
> var D = function() {};
> D.prototype.foo = [1,2,3];
> var d1 = new D();
> var d2 = new D();
> d1.foo.push(4);
> console.log(d2.foo);
> // What do you expect here? |foo| is the same array instance for all D
> instances, in other words it is 'static'.

Ah ok. I forgot about the shallow copy for Object types in the prototype.

https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:32:
devices[deviceInfo.address] = new Device();
On 2016/10/20 18:36:21, dpapad wrote:
> On 2016/10/20 at 18:15:36, mbrunson wrote:
> > On 2016/10/20 17:52:03, dpapad wrote:
> > > Again, don't fully understand what you are trying to do here. Seems like
the
> > > following would be sufficient, without the need for the additional wrapper
> > > Device instance.
> > > 
> > > devices[deviceInfo.address] = deviceInfo;
> > > 
> > > Or if you use a Map
> > > devices.set(deviceInfo.address, deviceInfo);
> > 
> > The Device instance is just a data container to group the two parts of a
> Device: DeviceInfo and the Device remote interface Mojo proxy. At the moment,
> I'm not using the proxy, so I didn't include it in the object yet.
> 
> If you want to go the class route instead of a simple object, put the member
> variables in the constructor.
> 
> var Device = function() {
>   this.proxy = null;
>   this.info = null;
> };
> 
> If you are fine with simple on-the-fly object see my previous suggestion.
Then,
> when you do add the proxy you can update the code as follows (no need to use
> Object.create).
> 
> - devices[deviceInfo.address] = deviceInfo;
> + devices[deviceInfo.address] = {proxy: someProxy, info: deviceInfo};

I'll stick with the class route. Done.

[dpapad, 2016-10-20 21:22:01]

https://chromiumcodereview.appspot.com/2404623002/diff/280001/chrome/browser/...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://chromiumcodereview.appspot.com/2404623002/diff/280001/chrome/browser/...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: var
Device = function(info) { this.info = info; };
@constructor
@param {!bluetoothDevice.DeviceInfo} info

https://chromiumcodereview.appspot.com/2404623002/diff/280001/chrome/browser/...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:23: var
AdapterClient = function() {};
Is there a good reason that the |devices| map is not a member of AdapterClient?

var AdapterClient = function() {
  this.devices = new Map();
};

If you also follow my suggestion at the end of this file, you can make this
field private (by renaming to devices_).

https://chromiumcodereview.appspot.com/2404623002/diff/280001/chrome/browser/...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:49:
devices.get(deviceInfo.address).info = deviceInfo;
Should probably assert that the item was found in the map before refering to
.info.

https://chromiumcodereview.appspot.com/2404623002/diff/280001/chrome/browser/...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:118:
devices.set(deviceInfo.address, new Device(deviceInfo));
Is this necessary to initialize local state? If so, can we simply call
adapter.deviceAdded(deviceInfo) here.

[mbrunson, 2016-10-20 22:00:36]

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

https://codereview.chromium.org/2404623002/diff/280001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: var
Device = function(info) { this.info = info; };
On 2016/10/20 21:22:01, dpapad wrote:
> @constructor
> @param {!bluetoothDevice.DeviceInfo} info

Done.

https://codereview.chromium.org/2404623002/diff/280001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:23: var
AdapterClient = function() {};
On 2016/10/20 21:22:01, dpapad wrote:
> Is there a good reason that the |devices| map is not a member of
AdapterClient?
> 
> var AdapterClient = function() {
>   this.devices = new Map();
> };
> 
> If you also follow my suggestion at the end of this file, you can make this
> field private (by renaming to devices_).

Originally, I was thinking to just have the UI-related code access the |devices|
map directly.

I can make it a private member of AdapterClient, and I could just make a
function that generates the needed HTML. That sounds more organized to me.
Thanks.

https://codereview.chromium.org/2404623002/diff/280001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:49:
devices.get(deviceInfo.address).info = deviceInfo;
On 2016/10/20 21:22:01, dpapad wrote:
> Should probably assert that the item was found in the map before refering to
> .info.

Done.

https://codereview.chromium.org/2404623002/diff/280001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:118:
devices.set(deviceInfo.address, new Device(deviceInfo));
On 2016/10/20 21:22:01, dpapad wrote:
> Is this necessary to initialize local state? If so, can we simply call
> adapter.deviceAdded(deviceInfo) here.

Done.

[dpapad, 2016-10-20 23:20:51]

LGTM for JS

[dcheng, 2016-10-21 05:43:14]

mojo lgtm

[mbrunson, 2016-10-21 18:20:08]

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

[commit-bot: I haz the power, 2016-10-21 18:20:36]

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-10-21 22:23:53]

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

[commit-bot: I haz the power, 2016-10-21 22:23:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mbrunson, 2016-10-21 22:33:12]

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

[commit-bot: I haz the power, 2016-10-21 22:33:55]

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-10-22 02:39:01]

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

[commit-bot: I haz the power, 2016-10-22 02:39:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mbrunson, 2016-10-24 16:46:43]

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

[commit-bot: I haz the power, 2016-10-24 16:47:08]

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-10-24 19:06:00]

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

[commit-bot: I haz the power, 2016-10-24 19:06:01]

Dry run: This issue passed the CQ dry run.

[mbrunson, 2016-10-24 19:28:28]

The CQ bit was checked by mbrunson@chromium.org

[mbrunson, 2016-10-24 19:28:29]

The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org,
scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/2404623002/#ps300001
(title: "Move devices map, simplify initialization")

[commit-bot: I haz the power, 2016-10-24 19:29:31]

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-10-24 19:46:42]

Description was changed from

==========
bluetooth: Add DeviceChanged logging in Device service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in Device.
Logs received DeviceChanged events in console.

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

to

==========
bluetooth: Add DeviceChanged logging in Device service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in Device.
Logs received DeviceChanged events in console.

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

[commit-bot: I haz the power, 2016-10-24 19:46:44]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2016-10-24 19:55:57]

Description was changed from

==========
bluetooth: Add DeviceChanged logging in Device service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in Device.
Logs received DeviceChanged events in console.

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

to

==========
bluetooth: Add DeviceChanged logging in Device service.

Adds DeviceChanged callback for BluetoothAdapter::Observer in Device.
Logs received DeviceChanged events in console.

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

Committed: https://crrev.com/4f24dfe2da20be7e9a9ba2f143cf5d318a5283b1
Cr-Commit-Position: refs/heads/master@{#427124}
==========

[commit-bot: I haz the power, 2016-10-24 19:55:59]

Patchset 16 (id:??) landed as
https://crrev.com/4f24dfe2da20be7e9a9ba2f143cf5d318a5283b1
Cr-Commit-Position: refs/heads/master@{#427124}