|
|
Created:
4 years, 2 months ago by mbrunson Modified:
4 years, 1 month ago CC:
Aaron Boodman, abarth-chromium, arv+watch_chromium.org, chromium-reviews, darin (slow to review), ortuno+watch_chromium.org, qsr+mojo_chromium.org, scheib+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: 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}
Patch Set 1 #Patch Set 2 : Change DeviceChanged to only report data from the specific device #Patch Set 3 : Revert BUILD change, push to later patch #Patch Set 4 : Merge upstream, update names, comments #Patch Set 5 : Split JS into functions #
Total comments: 8
Patch Set 6 : Fix comments #Patch Set 7 : Rename GattClient.DeviceChanged, refactor JS #Patch Set 8 : Fix JS formatting #Patch Set 9 : Remove GattClient, move PacketReceived to Adapter #Patch Set 10 : Change PacketReceived to DeviceChanged #
Total comments: 2
Patch Set 11 : Make RSSI optional #Patch Set 12 : Remove proxy from Device in JS #
Total comments: 8
Patch Set 13 : Fix deviceChanged comment #Patch Set 14 : Update deviceChanged, fix formatting #
Total comments: 10
Patch Set 15 : Use Map, change Device class def #
Total comments: 8
Patch Set 16 : Move devices map, simplify initialization #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 70 (41 generated)
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 ==========
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 ==========
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mbrunson@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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?
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.
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
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.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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.
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?
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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.
Ok. Makes sense to me. Keeping things on topic, what else do I have to change for this CL?
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 ==========
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.
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?
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.
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 :/
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?
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.
LGTM
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mbrunson@chromium.org changed reviewers: + dcheng@chromium.org, dpapad@chromium.org
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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);
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.
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};
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.
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.
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.
LGTM for JS
mojo lgtm
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mbrunson@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/4f24dfe2da20be7e9a9ba2f143cf5d318a5283b1 Cr-Commit-Position: refs/heads/master@{#427124} |