Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2401193002:
bluetooth: Add Device service for chrome://bluetooth-internals. (Closed)
|
Created:
4 years, 2 months ago by mbrunson Modified:
4 years, 2 months ago CC:
Aaron Boodman, abarth-chromium, arv+watch_chromium.org, chromium-reviews, darin (slow to review), ortuno+watch_chromium.org, qsr+mojo_chromium.org, Devlin, scheib+watch_chromium.org, stevenjb, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Add Device service for chrome://bluetooth-internals.
Moves DeviceInfo struct to device.mojom and creates interface for Device.
Adds starter code for Device implementation for the Bluetooth Mojo service.
Adds GetDevice to the Adapter service to create Device interfaces.
Adds console logging for DeviceInfo on chrome://bluetooth-internals page.
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/22ff37419618d39c8ff220108a935d3d8762a4f5
Cr-Commit-Position: refs/heads/master@{#425085}
Patch Set 1 #Patch Set 2 : Update JS #Patch Set 3 : Add log function comment #
Total comments: 27
Patch Set 4 : Rename LEDevice, fix TODO, adapter ownership #Patch Set 5 : Include ref_counted #
Total comments: 8
Patch Set 6 : Mojom changes, JS formatting #
Total comments: 2
Patch Set 7 : Reorganizing, cleanup includes, add name for display #
Total comments: 16
Patch Set 8 : JS, comment fixes #
Total comments: 6
Patch Set 9 : Simplify promise #
Total comments: 7
Patch Set 10 : Fixes #
Total comments: 2
Patch Set 11 : Fully qualify mojom import #Dependent Patchsets: Messages
Total messages: 60 (28 generated)
Description was changed from ========== bluetooth: Add LEDevice service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for LEDevice. Adds starter code for LEDevice implementation for the Bluetooth Mojo service. Adds GetDeviceService to the Adapter service to create LEDevice interfaces. Adds console logging for LEDeviceInfo on chrome://bluetooth-internals page. BUG=651282 ========== to ========== bluetooth: Add LEDevice service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for LEDevice. Adds starter code for LEDevice implementation for the Bluetooth Mojo service. Adds GetDeviceService to the Adapter service to create LEDevice interfaces. Adds console logging for LEDeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mbrunson@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:37: * TODO: Move to shared location. See http://crbug.com/652361. Please follow TODO format of the style guide[1] e.g. TODO(crbug.com/652361): ... [1] https://google.github.io/styleguide/cppguide.html#TODO_Comments https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:61: ]).then(function([frameInterfaces, ...modules]) { I thought you weren't allowed to use ES6 features? Same below with destructuring. https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:70: var adapterFactory = connection.bindHandleToProxy( I think you have to rebase; I remember you making this change in another CL. https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:125: for (var i = 0; i < devices.length; i++) { nit: devices.forEach(function(deviceInfo) { logDeviceService(deviceInfo).catch(function(error) { console.log(deviceInfo.name, error); }); }); https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/adapte... device/bluetooth/adapter.cc:42: base::MakeUnique<LEDevice>(address, std::move(adapter_)), Any reason why you are using std::move here? std::move is usually used to indicate that the ownership is being transferred; in this case you are indicating Adapter will no longer own or use adapter_ and now LEDevice will own it, which I think is incorrect. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... File device/bluetooth/le_device.h (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... device/bluetooth/le_device.h:10: #include "base/macros.h" Include: base/memory/ref_counted.h for scoped_refptr. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... device/bluetooth/le_device.h:20: // and service and uses the platform abstraction of device/bluetooth. It handles requests to interact with a Bluetooth Device. Uses the platform abstraction of device/bluetooth. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... device/bluetooth/le_device.h:24: const scoped_refptr<device::BluetoothAdapter>& adapter); Don't take a const&, rather take just a scoped_refptr and let the caller decide if it wants to transfer ownership.[1] [1] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... device/bluetooth/le_device.h:36: scoped_refptr<device::BluetoothAdapter> adapter_; Comments please. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/adapter.mojom:21: GetDeviceService(string address) => (LEDevice? device); nit: I would move this below GetDevices() to keep related functions grouped. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... File device/bluetooth/public/interfaces/le_device.mojom (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/le_device.mojom:8: string? name; What about nameForDisplay? You will probably want to use that when showing devices' names. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/le_device.mojom:13: interface LEDevice { Since this is just a thin wrapper around the current implementation I don't think we have a way to distinguish between LE devices and classic devices. So I think this should just be called "Device". scheib: any thoughts? https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/le_device.mojom:15: // Returns null, If no adapter is available. nit: lower-case after comma. Also I think you mean "device" instead of "adapter.
https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:37: * TODO: Move to shared location. See http://crbug.com/652361. On 2016/10/10 00:11:20, ortuno wrote: > Please follow TODO format of the style guide[1] e.g. TODO(crbug.com/652361): ... > > > [1] https://google.github.io/styleguide/cppguide.html#TODO_Comments Done. https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:61: ]).then(function([frameInterfaces, ...modules]) { On 2016/10/10 00:11:20, ortuno wrote: > I thought you weren't allowed to use ES6 features? Same below with > destructuring. I was told destructuring is supported. https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:70: var adapterFactory = connection.bindHandleToProxy( On 2016/10/10 00:11:20, ortuno wrote: > I think you have to rebase; I remember you making this change in another CL. Yeah. My repo is in a weird state right now. I'll try setting upstream to origin/master and going from there. https://codereview.chromium.org/2401193002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:125: for (var i = 0; i < devices.length; i++) { On 2016/10/10 00:11:20, ortuno wrote: > nit: > > devices.forEach(function(deviceInfo) { > logDeviceService(deviceInfo).catch(function(error) { > console.log(deviceInfo.name, error); > }); > }); Done. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/adapte... device/bluetooth/adapter.cc:42: base::MakeUnique<LEDevice>(address, std::move(adapter_)), On 2016/10/10 00:11:20, ortuno wrote: > Any reason why you are using std::move here? std::move is usually used to > indicate that the ownership is being transferred; in this case you are > indicating Adapter will no longer own or use adapter_ and now LEDevice will own > it, which I think is incorrect. You are right. This shouldn't be here. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... File device/bluetooth/le_device.h (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... device/bluetooth/le_device.h:10: #include "base/macros.h" On 2016/10/10 00:11:20, ortuno wrote: > Include: base/memory/ref_counted.h for scoped_refptr. Done. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... device/bluetooth/le_device.h:20: // and service and uses the platform abstraction of device/bluetooth. On 2016/10/10 00:11:20, ortuno wrote: > It handles requests to interact with a Bluetooth Device. Uses the platform > abstraction of device/bluetooth. Done. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... device/bluetooth/le_device.h:24: const scoped_refptr<device::BluetoothAdapter>& adapter); On 2016/10/10 00:11:20, ortuno wrote: > Don't take a const&, rather take just a scoped_refptr and let the caller decide > if it wants to transfer ownership.[1] > > [1] > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/le_dev... device/bluetooth/le_device.h:36: scoped_refptr<device::BluetoothAdapter> adapter_; On 2016/10/10 00:11:20, ortuno wrote: > Comments please. Done. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/adapter.mojom:21: GetDeviceService(string address) => (LEDevice? device); On 2016/10/10 00:11:20, ortuno wrote: > nit: I would move this below GetDevices() to keep related functions grouped. Done. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... File device/bluetooth/public/interfaces/le_device.mojom (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/le_device.mojom:8: string? name; On 2016/10/10 00:11:20, ortuno wrote: > What about nameForDisplay? You will probably want to use that when showing > devices' names. Unless I'm missing something, GetNameForDisplay() uses UTF-16 strings which can't be transferred over Mojo. I figured converting them to UTF-8 defeats the purpose of the original encoding. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/le_device.mojom:13: interface LEDevice { On 2016/10/10 00:11:20, ortuno wrote: > Since this is just a thin wrapper around the current implementation I don't > think we have a way to distinguish between LE devices and classic devices. So I > think this should just be called "Device". > > scheib: any thoughts? I'll change it for now. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/le_device.mojom:15: // Returns null, If no adapter is available. On 2016/10/10 00:11:20, ortuno wrote: > nit: lower-case after comma. Also I think you mean "device" instead of "adapter. Done.
https://codereview.chromium.org/2401193002/diff/80001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/80001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:64: [bluetoothAdapter, bluetoothDevice, connection] = modules; Why destructure twice? Why not just all above? https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/device.h File device/bluetooth/device.h (right): https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/device... device/bluetooth/device.h:19: // device/bluetooth/public/interfaces/le_device.mojom. Drop the "le_", and check to see if it's anywhere else in patch. Also, fix up the patch description. https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/public... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/public... device/bluetooth/public/interfaces/adapter.mojom:29: GetDeviceService(string address) => (Device? device); I'm concerned about the 'Service' suffix here.. I think the whole collection of Bluetooth interfaces will in the future be a Service as a group - but not each interface. I think that 'GetDevice' is fine. BTW that's already used for AdapterFactory's GetAdapter. https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/public... File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/public... device/bluetooth/public/interfaces/device.mojom:9: string id; Let's not add the 'id' unless we need it (which I don't think we will). On all platforms other than Bluez the ID is just the Address. On Bluez I don't think we need to work with the ID.
Description was changed from ========== bluetooth: Add LEDevice service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for LEDevice. Adds starter code for LEDevice implementation for the Bluetooth Mojo service. Adds GetDeviceService to the Adapter service to create LEDevice interfaces. Adds console logging for LEDeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add Device service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for Levice. Adds starter code for Device implementation for the Bluetooth Mojo service. Adds GetDevice to the Adapter service to create Device interfaces. Adds console logging for DeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== bluetooth: Add Device service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for Levice. Adds starter code for Device implementation for the Bluetooth Mojo service. Adds GetDevice to the Adapter service to create Device interfaces. Adds console logging for DeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add Device service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for Device. Adds starter code for Device implementation for the Bluetooth Mojo service. Adds GetDevice to the Adapter service to create Device interfaces. Adds console logging for DeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2401193002/diff/80001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/80001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:64: [bluetoothAdapter, bluetoothDevice, connection] = modules; On 2016/10/10 20:23:51, scheib wrote: > Why destructure twice? Why not just all above? Destructuring in the parameter list hides the global variables. I'll add a comment since that isn't very obvious here. https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/device.h File device/bluetooth/device.h (right): https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/device... device/bluetooth/device.h:19: // device/bluetooth/public/interfaces/le_device.mojom. On 2016/10/10 20:23:51, scheib wrote: > Drop the "le_", and check to see if it's anywhere else in patch. Also, fix up > the patch description. Oh I missed one. Done. https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/public... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/public... device/bluetooth/public/interfaces/adapter.mojom:29: GetDeviceService(string address) => (Device? device); On 2016/10/10 20:23:51, scheib wrote: > I'm concerned about the 'Service' suffix here.. I think the whole collection of > Bluetooth interfaces will in the future be a Service as a group - but not each > interface. I think that 'GetDevice' is fine. BTW that's already used for > AdapterFactory's GetAdapter. Ah ok. I will change this. https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/public... File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2401193002/diff/80001/device/bluetooth/public... device/bluetooth/public/interfaces/device.mojom:9: string id; On 2016/10/10 20:23:51, scheib wrote: > Let's not add the 'id' unless we need it (which I don't think we will). On all > platforms other than Bluez the ID is just the Address. On Bluez I don't think we > need to work with the ID. Done.
lgtm bar some nits. It would be nice if you ran trybots before sending in for review, sometimes trybots can uncover issues that cause the code to change significantly. https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... File device/bluetooth/public/interfaces/le_device.mojom (right): https://codereview.chromium.org/2401193002/diff/40001/device/bluetooth/public... device/bluetooth/public/interfaces/le_device.mojom:8: string? name; On 2016/10/10 at 19:21:28, mbrunson wrote: > On 2016/10/10 00:11:20, ortuno wrote: > > What about nameForDisplay? You will probably want to use that when showing > > devices' names. > > Unless I'm missing something, GetNameForDisplay() uses UTF-16 strings which can't be transferred over Mojo. I figured converting them to UTF-8 defeats the purpose of the original encoding. The purpose of GetNameForDisplay is not only to return a string16 but also to return a name that is appropriate for displaying. If the device has no name, GetNameForDisplay will construct a name based on the available information e.g. address, appearance, etc. So for example for a mouse with no name, GetName() will return no name but GetNameForDisplay() will return "Mouse (00:00:00:00:00)" which is more appropriate for displaying. GetNameForDisplay should be used whenever the device's name is being presented to the user. https://codereview.chromium.org/2401193002/diff/100001/device/bluetooth/adapt... File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2401193002/diff/100001/device/bluetooth/adapt... device/bluetooth/adapter.h:31: void GetDevice(const std::string& address, nit: Match the order of the mojom declarations.
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
LGTM
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
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...
https://codereview.chromium.org/2401193002/diff/100001/device/bluetooth/adapt... File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2401193002/diff/100001/device/bluetooth/adapt... device/bluetooth/adapter.h:31: void GetDevice(const std::string& address, On 2016/10/11 00:45:45, ortuno wrote: > nit: Match the order of the mojom declarations. Done.
mbrunson@chromium.org changed reviewers: + dcheng@chromium.org, dpapad@chromium.org
OWNERS review, please: dpapad: chrome/browser/browser_resources.grd chrome/browser/resources/bluetooth_internals/bluetooth_internals.js chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc dcheng: device/bluetooth/public/interfaces/adapter.mojom device/bluetooth/public/interfaces/device.mojom I'm getting to the point where I won't be able to implement the Mojo service directly in the browser without a suitable user interface. dcheng can't do a Mojo security review without knowing the implementation of the service, right? I would have liked to finish the skeleton of the service, but I'm assuming this is not possible without writing a client that uses the service, right?
https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:18: * @param {!bluetoothDevice.DeviceInfo} device the device that was added Nit: Per styleguide at https://google.github.io/styleguide/javascriptguide.xml?showone=Comments#Comm..., consider either removing the description (does not add much value), or using proper punctuation. @param {!bluetoothDevice.DeviceInfo} device or @param {!bluetoothDevice.DeviceInfo} device The device that was added. https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:79: bluetoothAdapter.Adapter); Indentation off. https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:96: return adapter.getDevice(deviceInfo.address) Regarding promise callback indentation style, I know different teams have different prefs (and AFAIK the styleguide does not specify one style), but here are two different styles I have encountered extensively in Chromium (and elsewhere) // Version 1: Keep it all in one line if it fits. return adapter.getDevice(deviceInfo.address).then(function(response) { // Do stuff here. }).then(function() { // Do more stuff here. }); OR // Version 2: Break line after "then(" return adapter.getDevice(deviceInfo.address).then( functon(response) { // Do stuff here. }).then(function() { // Do more stuff here. }); I find both of these more readable than breaking line before ".then()" https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:100: if (deviceHandle) { Can you reverse this logic? I think it is more readable this way. if (!deviceHandle) throw new Error('Device cannot be found.'); // Do the normal case stuff here. ... https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:101: var device = connection.bindHandleToProxy( I don't think this is meant to be called every time logDevice is called. Isn't this a one-time only setup, similar to the adapter initialization at line 78? https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:124: .catch(function(error) { console.error(deviceInfo.name, error); }); Do you really want a dedicated error handler for each device? Could you go away with something as follows Promise.all(devices.map(logDevice)).catch(function(error) { ...}); Where there is a single error handler for all devices? https://codereview.chromium.org/2401193002/diff/120001/device/bluetooth/publi... File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2401193002/diff/120001/device/bluetooth/publi... device/bluetooth/public/interfaces/device.mojom:5: module bluetooth.mojom; Does this have to be a different file for technical reasons? I personally would prefer to keep closely related interfaces in a single mojom file until there is a need to re-use DeviceInfo and Device from places where Adapter, AdapterClient etc are not used. https://codereview.chromium.org/2401193002/diff/120001/device/bluetooth/publi... device/bluetooth/public/interfaces/device.mojom:15: // Returns null, If no device is available. Nit (optional): Maximize 80col usage before braking line? // Gets basic information about the device. Returns null, If no device is // available.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:18: * @param {!bluetoothDevice.DeviceInfo} device the device that was added On 2016/10/11 21:58:32, dpapad wrote: > Nit: Per styleguide at > https://google.github.io/styleguide/javascriptguide.xml?showone=Comments#Comm..., > consider either removing the description (does not add much value), or using > proper punctuation. > > @param {!bluetoothDevice.DeviceInfo} device > or > @param {!bluetoothDevice.DeviceInfo} device The device that was added. Done. https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:79: bluetoothAdapter.Adapter); On 2016/10/11 21:58:32, dpapad wrote: > Indentation off. Done. https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:96: return adapter.getDevice(deviceInfo.address) On 2016/10/11 21:58:32, dpapad wrote: > Regarding promise callback indentation style, I know different teams have > different prefs (and AFAIK the styleguide does not specify one style), but here > are two different styles I have encountered extensively in Chromium (and > elsewhere) > > // Version 1: Keep it all in one line if it fits. > return adapter.getDevice(deviceInfo.address).then(function(response) { > // Do stuff here. > }).then(function() { > // Do more stuff here. > }); > > OR > > // Version 2: Break line after "then(" > return adapter.getDevice(deviceInfo.address).then( > functon(response) { > // Do stuff here. > }).then(function() { > // Do more stuff here. > }); > > I find both of these more readable than breaking line before ".then()" Done. https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:100: if (deviceHandle) { On 2016/10/11 21:58:32, dpapad wrote: > Can you reverse this logic? I think it is more readable this way. > > if (!deviceHandle) > throw new Error('Device cannot be found.'); > > // Do the normal case stuff here. > ... Done. https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:101: var device = connection.bindHandleToProxy( On 2016/10/11 21:58:32, dpapad wrote: > I don't think this is meant to be called every time logDevice is called. Isn't > this a one-time only setup, similar to the adapter initialization at line 78? adapter.GetDevice creates a new handle for each request even if it's the same device. I figured the handle has to be bound to a proxy each time it's created. https://codereview.chromium.org/2401193002/diff/120001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:124: .catch(function(error) { console.error(deviceInfo.name, error); }); On 2016/10/11 21:58:32, dpapad wrote: > Do you really want a dedicated error handler for each device? Could you go away > with something as follows > > Promise.all(devices.map(logDevice)).catch(function(error) { ...}); > > Where there is a single error handler for all devices? I could do this. I'll just move the device name to the log function so I don't need it here. https://codereview.chromium.org/2401193002/diff/120001/device/bluetooth/publi... File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2401193002/diff/120001/device/bluetooth/publi... device/bluetooth/public/interfaces/device.mojom:5: module bluetooth.mojom; On 2016/10/11 21:58:32, dpapad wrote: > Does this have to be a different file for technical reasons? > > I personally would prefer to keep closely related interfaces in a single mojom > file until there is a need to re-use DeviceInfo and Device from places where > Adapter, AdapterClient etc are not used. Less of a technical reason and more for organization. The layout of the C++ implementation for the interfaces closely match the mojom declarations. We try to keep things separate conceptually unless it's necessary to keep them together. https://codereview.chromium.org/2401193002/diff/120001/device/bluetooth/publi... device/bluetooth/public/interfaces/device.mojom:15: // Returns null, If no device is available. On 2016/10/11 21:58:32, dpapad wrote: > Nit (optional): Maximize 80col usage before braking line? > > // Gets basic information about the device. Returns null, If no device is > // available. Done.
https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:114: .then(function() { return adapter.getDevices(); }) I am confused about the purpose of these calls here. Specifically: 1) adapter.getDevices() returns an array of DeviceInfo 2) We iterate over that array and call adapter.getDevice(deviceInfo.address). 3) Then we call device.getInfo() to retrieve the same info we already had at step 1? Basically I don't understand the usefulness of the new Device mojom interface. It only has one method, and the information it provides is already available from the Adapter. Am I missing something? https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:115: .then(function(response) { The error will propagate to the already existing error handler at line 123, no need for one more call to catch(). .then(function(response) { ... return Promise.all(devices.map(logDevice)); }) .catch(function(error) { console.error(error); });
https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:114: .then(function() { return adapter.getDevices(); }) On 2016/10/11 23:51:04, dpapad wrote: > I am confused about the purpose of these calls here. Specifically: > > 1) adapter.getDevices() returns an array of DeviceInfo > 2) We iterate over that array and call adapter.getDevice(deviceInfo.address). > 3) Then we call device.getInfo() to retrieve the same info we already had at > step 1? > > Basically I don't understand the usefulness of the new Device mojom interface. > It only has one method, and the information it provides is already available > from the Adapter. Am I missing something? No. You're not missing anything. This is the minimal amount of implementation to get a Device service. The main purpose of the Device service is to provide a place to list Bluetooth GATT services running on the device. Services are not populated until the device is connected to. The device cannot be connected to until a BluetoothGattConnection (which will be another mojom interface) is created. This leads to a whole set of new files and functions that must be implemented to create the connection, get service info, and manage it appropriately. In order to keep this patch a reasonable size, I didn't included that stuff. This leaves the GetInfo function which exists as a convenience for consumers of the Device service. I added it to the JS file because I needed to make sure it produced the same output in the client, and I assumed I needed some consumer of the data to allow for a security review. https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:115: .then(function(response) { On 2016/10/11 23:51:04, dpapad wrote: > The error will propagate to the already existing error handler at line 123, no > need for one more call to catch(). > > .then(function(response) { > ... > return Promise.all(devices.map(logDevice)); > }) > .catch(function(error) { console.error(error); }); Done.
https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:114: .then(function() { return adapter.getDevices(); }) > The main purpose of the Device service is to provide a place to list Bluetooth GATT services running on the device. Services are not populated until the device is connected to. The device cannot be connected to until a BluetoothGattConnection (which will be another mojom interface) is created. This leads to a whole set of new files and functions that must be implemented to create the connection, get service info, and manage it appropriately. In order to keep this patch a reasonable size, I didn't included that stuff. This leaves the GetInfo function which exists as a convenience for consumers of the Device service. > > I added it to the JS file because I needed to make sure it produced the same output in the client, and I assumed I needed some consumer of the data to allow for a security review. Thanks for the explanation. It still seems odd to me that every device requires a service. My understanding is that you are trying to mitigate the fact that Mojo services can only pass plain data objects (see DeviceInfo struct) or handles back and forth. I am guessing you are trying to achieve something as close to the following snippet, but taking into account Mojo limitations? adapter.getDevices().then(function(devices) { devices.forEach(fuction(d) { // Somehow "magically" returned |devices| are fully fledged classes with // their own public API, no need to manually create a service for each // device. d.getInfo().then(...); d.getGattServices().then(...); }); }); So given Mojo's limitations, I am not confident that spawning a "Device" service, for every device is the best path forward. When I think of a "service" I think of a singleton instance of a class that exposes some functionality, and is usually instantiated once during startup (similar to the already existing chrome.bluetooth "service", https://developer.chrome.com/apps/bluetooth). All functionality can be exposed via calls to the top-level (and only) service "adapter" (or could be renamed to bluetoothService). bluetoothService.listDeviceInfo() => (array<DeviceInfo) bluetoothService.getDeviceInfo(string address); => (DeviceInfo) bluetoothService.getDeviceGattServices(string deviceId) => (array<GattService>) bluetoothService.connectToDevice(string deviceId) => (boolean) // add more methods here as needed. ...... I also understand that you've chosen to mirror the C++ class structure in the exposed Mojo interfaces. Why is this coupling beneficial to the UI page? Let me take a step back and make a more general comment. As a WebUI OWNER, my task is not to simply make style comments on JS usage, but to understand and review the overall design. There is a complete lack of knowledge about chrome://bluetooth-internals roadmap on my part. - What is the target group? End users? Bluetooth team developers? - Are there mocks/specs for the finalized UI? - Is there an overall design doc I should read? I feel that I am discovering the design little by little on every review, and that makes useful reviewing hard. My suggestion is 1) land all Mojo interfaces at once in a single CL, so that there is a complete picture of the exposed API and all involved data structures. 2) In follow up CLs start implementing the already existing Mojo interfaces step by step. I think this would make reviewing easier, certainly for me, but perhaps for other reviewers too.
https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:114: .then(function() { return adapter.getDevices(); }) On 2016/10/12 17:39:02, dpapad wrote: > > The main purpose of the Device service is to provide a place to list Bluetooth > GATT services running on the device. Services are not populated until the device > is connected to. The device cannot be connected to until a > BluetoothGattConnection (which will be another mojom interface) is created. This > leads to a whole set of new files and functions that must be implemented to > create the connection, get service info, and manage it appropriately. In order > to keep this patch a reasonable size, I didn't included that stuff. This leaves > the GetInfo function which exists as a convenience for consumers of the Device > service. > > > > I added it to the JS file because I needed to make sure it produced the same > output in the client, and I assumed I needed some consumer of the data to allow > for a security review. > > > Thanks for the explanation. It still seems odd to me that every device requires > a service. My understanding is that you are trying to mitigate the fact that > Mojo services can only pass plain data objects (see DeviceInfo struct) or > handles back and forth. I am guessing you are trying to achieve something as > close to the following snippet, but taking into account Mojo limitations? > > adapter.getDevices().then(function(devices) { > devices.forEach(fuction(d) { > // Somehow "magically" returned |devices| are fully fledged classes with > // their own public API, no need to manually create a service for each > // device. > d.getInfo().then(...); > d.getGattServices().then(...); > }); > }); > > So given Mojo's limitations, I am not confident that spawning a "Device" > service, for every device is the best path forward. When I think of a "service" > I think of a singleton instance of a class that exposes some functionality, and > is usually instantiated once during startup (similar to the already existing > chrome.bluetooth "service", https://developer.chrome.com/apps/bluetooth). > > All functionality can be exposed via calls to the top-level (and only) service > "adapter" (or could be renamed to bluetoothService). > bluetoothService.listDeviceInfo() => (array<DeviceInfo) > bluetoothService.getDeviceInfo(string address); => (DeviceInfo) > bluetoothService.getDeviceGattServices(string deviceId) => (array<GattService>) > bluetoothService.connectToDevice(string deviceId) => (boolean) > // add more methods here as needed. > ...... > > I also understand that you've chosen to mirror the C++ class structure in the > exposed Mojo interfaces. Why is this coupling beneficial to the UI page? > > Let me take a step back and make a more general comment. As a WebUI OWNER, my > task is not to simply make style comments on JS usage, but to understand and > review the overall design. There is a complete lack of knowledge about > chrome://bluetooth-internals roadmap on my part. > > - What is the target group? End users? Bluetooth team developers? > - Are there mocks/specs for the finalized UI? > - Is there an overall design doc I should read? > > I feel that I am discovering the design little by little on every review, and > that makes useful reviewing hard. My suggestion is > > 1) land all Mojo interfaces at once in a single CL, so that there is a > complete picture of the exposed API and all involved data structures. > 2) In follow up CLs start implementing the already existing Mojo interfaces step > by step. > > I think this would make reviewing easier, certainly for me, but perhaps for > other > reviewers too. Design doc is a WIP: go/fizzbluetoothinternals. You will find a general overview and discussion of the design for this Mojo service near the end. This coupling is beneficial to the UI page because it creates a separation of concerns for parts of the UI. I'm planning to use Polymer for a web components oriented design, so I would rather have each component manage one specific layer of the Bluetooth stack. Other concerns mainly focus on adapting developers to the new Mojo interfaces. We didn't want to create a completely new structure for the transition of existing users if they wish/prefer to use the Mojo implementation. The only reason I didn't land all of the Mojo services was because I thought I needed implementation of the mojom to get a security review. If that's not necessary I will definitely do that.
On 2016/10/12 18:06:52, mbrunson wrote: > https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... > File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js > (right): > > https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... > chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:114: > .then(function() { return adapter.getDevices(); }) > On 2016/10/12 17:39:02, dpapad wrote: > > > The main purpose of the Device service is to provide a place to list > Bluetooth > > GATT services running on the device. Services are not populated until the > device > > is connected to. The device cannot be connected to until a > > BluetoothGattConnection (which will be another mojom interface) is created. > This > > leads to a whole set of new files and functions that must be implemented to > > create the connection, get service info, and manage it appropriately. In order > > to keep this patch a reasonable size, I didn't included that stuff. This > leaves > > the GetInfo function which exists as a convenience for consumers of the Device > > service. > > > > > > I added it to the JS file because I needed to make sure it produced the same > > output in the client, and I assumed I needed some consumer of the data to > allow > > for a security review. > > > > > > Thanks for the explanation. It still seems odd to me that every device > requires > > a service. My understanding is that you are trying to mitigate the fact that > > Mojo services can only pass plain data objects (see DeviceInfo struct) or > > handles back and forth. I am guessing you are trying to achieve something as > > close to the following snippet, but taking into account Mojo limitations? > > > > adapter.getDevices().then(function(devices) { > > devices.forEach(fuction(d) { > > // Somehow "magically" returned |devices| are fully fledged classes with > > // their own public API, no need to manually create a service for each > > // device. > > d.getInfo().then(...); > > d.getGattServices().then(...); > > }); > > }); > > > > So given Mojo's limitations, I am not confident that spawning a "Device" > > service, for every device is the best path forward. When I think of a > "service" > > I think of a singleton instance of a class that exposes some functionality, > and > > is usually instantiated once during startup (similar to the already existing > > chrome.bluetooth "service", https://developer.chrome.com/apps/bluetooth). > > > > All functionality can be exposed via calls to the top-level (and only) service > > "adapter" (or could be renamed to bluetoothService). > > bluetoothService.listDeviceInfo() => (array<DeviceInfo) > > bluetoothService.getDeviceInfo(string address); => (DeviceInfo) > > bluetoothService.getDeviceGattServices(string deviceId) => > (array<GattService>) > > bluetoothService.connectToDevice(string deviceId) => (boolean) > > // add more methods here as needed. > > ...... > > > > I also understand that you've chosen to mirror the C++ class structure in the > > exposed Mojo interfaces. Why is this coupling beneficial to the UI page? > > > > Let me take a step back and make a more general comment. As a WebUI OWNER, my > > task is not to simply make style comments on JS usage, but to understand and > > review the overall design. There is a complete lack of knowledge about > > chrome://bluetooth-internals roadmap on my part. > > > > - What is the target group? End users? Bluetooth team developers? > > - Are there mocks/specs for the finalized UI? > > - Is there an overall design doc I should read? > > > > I feel that I am discovering the design little by little on every review, and > > that makes useful reviewing hard. My suggestion is > > > > 1) land all Mojo interfaces at once in a single CL, so that there is a > > complete picture of the exposed API and all involved data structures. > > 2) In follow up CLs start implementing the already existing Mojo interfaces > step > > by step. > > > > I think this would make reviewing easier, certainly for me, but perhaps for > > other > > reviewers too. > > Design doc is a WIP: go/fizzbluetoothinternals. You will find a general overview > and discussion of the design for this Mojo service near the end. This coupling > is beneficial to the UI page because it creates a separation of concerns for > parts of the UI. I'm planning to use Polymer for a web components oriented > design, so I would rather have each component manage one specific layer of the > Bluetooth stack. > > Other concerns mainly focus on adapting developers to the new Mojo interfaces. > We didn't want to create a completely new structure for the transition of > existing users if they wish/prefer to use the Mojo implementation. > > The only reason I didn't land all of the Mojo services was because I thought I > needed implementation of the mojom to get a security review. If that's not > necessary I will definitely do that. This CL LGTM, but I'm not sure what this last comment means. Do you mind elaborating? =) https://codereview.chromium.org/2401193002/diff/160001/device/bluetooth/devic... File device/bluetooth/device.cc (right): https://codereview.chromium.org/2401193002/diff/160001/device/bluetooth/devic... device/bluetooth/device.cc:15: : address_(address), adapter_(adapter) {} Nit: std::move(adapter) https://codereview.chromium.org/2401193002/diff/160001/device/bluetooth/publi... File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2401193002/diff/160001/device/bluetooth/publi... device/bluetooth/public/interfaces/device.mojom:14: // Gets basic information about the device. Returns null, If no device is Nit:If=> if
On 2016/10/12 18:27:43, dcheng wrote: > On 2016/10/12 18:06:52, mbrunson wrote: > > > https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... > > File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js > > (right): > > > > > https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... > > chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:114: > > .then(function() { return adapter.getDevices(); }) > > On 2016/10/12 17:39:02, dpapad wrote: > > > > The main purpose of the Device service is to provide a place to list > > Bluetooth > > > GATT services running on the device. Services are not populated until the > > device > > > is connected to. The device cannot be connected to until a > > > BluetoothGattConnection (which will be another mojom interface) is created. > > This > > > leads to a whole set of new files and functions that must be implemented to > > > create the connection, get service info, and manage it appropriately. In > order > > > to keep this patch a reasonable size, I didn't included that stuff. This > > leaves > > > the GetInfo function which exists as a convenience for consumers of the > Device > > > service. > > > > > > > > I added it to the JS file because I needed to make sure it produced the > same > > > output in the client, and I assumed I needed some consumer of the data to > > allow > > > for a security review. > > > > > > > > > Thanks for the explanation. It still seems odd to me that every device > > requires > > > a service. My understanding is that you are trying to mitigate the fact that > > > Mojo services can only pass plain data objects (see DeviceInfo struct) or > > > handles back and forth. I am guessing you are trying to achieve something as > > > close to the following snippet, but taking into account Mojo limitations? > > > > > > adapter.getDevices().then(function(devices) { > > > devices.forEach(fuction(d) { > > > // Somehow "magically" returned |devices| are fully fledged classes with > > > // their own public API, no need to manually create a service for each > > > // device. > > > d.getInfo().then(...); > > > d.getGattServices().then(...); > > > }); > > > }); > > > > > > So given Mojo's limitations, I am not confident that spawning a "Device" > > > service, for every device is the best path forward. When I think of a > > "service" > > > I think of a singleton instance of a class that exposes some functionality, > > and > > > is usually instantiated once during startup (similar to the already existing > > > chrome.bluetooth "service", https://developer.chrome.com/apps/bluetooth). > > > > > > All functionality can be exposed via calls to the top-level (and only) > service > > > "adapter" (or could be renamed to bluetoothService). > > > bluetoothService.listDeviceInfo() => (array<DeviceInfo) > > > bluetoothService.getDeviceInfo(string address); => (DeviceInfo) > > > bluetoothService.getDeviceGattServices(string deviceId) => > > (array<GattService>) > > > bluetoothService.connectToDevice(string deviceId) => (boolean) > > > // add more methods here as needed. > > > ...... > > > > > > I also understand that you've chosen to mirror the C++ class structure in > the > > > exposed Mojo interfaces. Why is this coupling beneficial to the UI page? > > > > > > Let me take a step back and make a more general comment. As a WebUI OWNER, > my > > > task is not to simply make style comments on JS usage, but to understand and > > > review the overall design. There is a complete lack of knowledge about > > > chrome://bluetooth-internals roadmap on my part. > > > > > > - What is the target group? End users? Bluetooth team developers? > > > - Are there mocks/specs for the finalized UI? > > > - Is there an overall design doc I should read? > > > > > > I feel that I am discovering the design little by little on every review, > and > > > that makes useful reviewing hard. My suggestion is > > > > > > 1) land all Mojo interfaces at once in a single CL, so that there is a > > > complete picture of the exposed API and all involved data structures. > > > 2) In follow up CLs start implementing the already existing Mojo interfaces > > step > > > by step. > > > > > > I think this would make reviewing easier, certainly for me, but perhaps for > > > other > > > reviewers too. > > > > Design doc is a WIP: go/fizzbluetoothinternals. You will find a general > overview > > and discussion of the design for this Mojo service near the end. This coupling > > is beneficial to the UI page because it creates a separation of concerns for > > parts of the UI. I'm planning to use Polymer for a web components oriented > > design, so I would rather have each component manage one specific layer of the > > Bluetooth stack. > > > > Other concerns mainly focus on adapting developers to the new Mojo interfaces. > > We didn't want to create a completely new structure for the transition of > > existing users if they wish/prefer to use the Mojo implementation. > > > > The only reason I didn't land all of the Mojo services was because I thought I > > needed implementation of the mojom to get a security review. If that's not > > necessary I will definitely do that. > > This CL LGTM, but I'm not sure what this last comment means. Do you mind > elaborating? =) When I was first landing these Mojo interfaces, you commented that it wasn't possible to do a security review without any context of how the interface is used / implemented. I figured this referred to all future commits referring to mojom files. Is this not the case? Am I being more meticulous/careful than necessary? https://codereview.chromium.org/2357383002/#msg76
On 2016/10/12 19:06:49, mbrunson wrote: > On 2016/10/12 18:27:43, dcheng wrote: > > On 2016/10/12 18:06:52, mbrunson wrote: > > > > > > https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... > > > File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js > > > (right): > > > > > > > > > https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... > > > chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:114: > > > .then(function() { return adapter.getDevices(); }) > > > On 2016/10/12 17:39:02, dpapad wrote: > > > > > The main purpose of the Device service is to provide a place to list > > > Bluetooth > > > > GATT services running on the device. Services are not populated until the > > > device > > > > is connected to. The device cannot be connected to until a > > > > BluetoothGattConnection (which will be another mojom interface) is > created. > > > This > > > > leads to a whole set of new files and functions that must be implemented > to > > > > create the connection, get service info, and manage it appropriately. In > > order > > > > to keep this patch a reasonable size, I didn't included that stuff. This > > > leaves > > > > the GetInfo function which exists as a convenience for consumers of the > > Device > > > > service. > > > > > > > > > > I added it to the JS file because I needed to make sure it produced the > > same > > > > output in the client, and I assumed I needed some consumer of the data to > > > allow > > > > for a security review. > > > > > > > > > > > > Thanks for the explanation. It still seems odd to me that every device > > > requires > > > > a service. My understanding is that you are trying to mitigate the fact > that > > > > Mojo services can only pass plain data objects (see DeviceInfo struct) or > > > > handles back and forth. I am guessing you are trying to achieve something > as > > > > close to the following snippet, but taking into account Mojo limitations? > > > > > > > > adapter.getDevices().then(function(devices) { > > > > devices.forEach(fuction(d) { > > > > // Somehow "magically" returned |devices| are fully fledged classes > with > > > > // their own public API, no need to manually create a service for each > > > > // device. > > > > d.getInfo().then(...); > > > > d.getGattServices().then(...); > > > > }); > > > > }); > > > > > > > > So given Mojo's limitations, I am not confident that spawning a "Device" > > > > service, for every device is the best path forward. When I think of a > > > "service" > > > > I think of a singleton instance of a class that exposes some > functionality, > > > and > > > > is usually instantiated once during startup (similar to the already > existing > > > > chrome.bluetooth "service", https://developer.chrome.com/apps/bluetooth). > > > > > > > > All functionality can be exposed via calls to the top-level (and only) > > service > > > > "adapter" (or could be renamed to bluetoothService). > > > > bluetoothService.listDeviceInfo() => (array<DeviceInfo) > > > > bluetoothService.getDeviceInfo(string address); => (DeviceInfo) > > > > bluetoothService.getDeviceGattServices(string deviceId) => > > > (array<GattService>) > > > > bluetoothService.connectToDevice(string deviceId) => (boolean) > > > > // add more methods here as needed. > > > > ...... > > > > > > > > I also understand that you've chosen to mirror the C++ class structure in > > the > > > > exposed Mojo interfaces. Why is this coupling beneficial to the UI page? > > > > > > > > Let me take a step back and make a more general comment. As a WebUI OWNER, > > my > > > > task is not to simply make style comments on JS usage, but to understand > and > > > > review the overall design. There is a complete lack of knowledge about > > > > chrome://bluetooth-internals roadmap on my part. > > > > > > > > - What is the target group? End users? Bluetooth team developers? > > > > - Are there mocks/specs for the finalized UI? > > > > - Is there an overall design doc I should read? > > > > > > > > I feel that I am discovering the design little by little on every review, > > and > > > > that makes useful reviewing hard. My suggestion is > > > > > > > > 1) land all Mojo interfaces at once in a single CL, so that there is a > > > > complete picture of the exposed API and all involved data structures. > > > > 2) In follow up CLs start implementing the already existing Mojo > interfaces > > > step > > > > by step. > > > > > > > > I think this would make reviewing easier, certainly for me, but perhaps > for > > > > other > > > > reviewers too. > > > > > > Design doc is a WIP: go/fizzbluetoothinternals. You will find a general > > overview > > > and discussion of the design for this Mojo service near the end. This > coupling > > > is beneficial to the UI page because it creates a separation of concerns for > > > parts of the UI. I'm planning to use Polymer for a web components oriented > > > design, so I would rather have each component manage one specific layer of > the > > > Bluetooth stack. > > > > > > Other concerns mainly focus on adapting developers to the new Mojo > interfaces. > > > We didn't want to create a completely new structure for the transition of > > > existing users if they wish/prefer to use the Mojo implementation. > > > > > > The only reason I didn't land all of the Mojo services was because I thought > I > > > needed implementation of the mojom to get a security review. If that's not > > > necessary I will definitely do that. > > > > This CL LGTM, but I'm not sure what this last comment means. Do you mind > > elaborating? =) > > When I was first landing these Mojo interfaces, you commented that it wasn't > possible to do a security review without any context of how the interface is > used / implemented. I figured this referred to all future commits referring to > mojom files. Is this not the case? Am I being more meticulous/careful than > necessary? > > https://codereview.chromium.org/2357383002/#msg76 Chiming in on a few points: dcheng, thanks for reviews. The service will consist of many bluetooth interfaces, even in long term, because globbing everything together into one interface makes that interface far too big and moves to a style we don't want. The larger process will take several refactoring steps, and we've decided to start by keeping the mojom interfaces pretty light weight and similar to the C++ interfaces, though we are breaking up concepts a bit because the C++ interfaces already suffer from over-complexity. https://bugs.chromium.org/p/chromium/issues/detail?id=612319#c19 I think that all mojo interfaces in one CL would be too large and blind to how they are used/wired up. I advised mbrunson to keep to a series of small patches that bring up one concept at a time, and we've already seen value in seeing not only the interface but the device/bluetooth implementation as well. The design doc go/fizzbluetoothinternals is a good reference for what the full set of interfaces will look like, but we hit limits of what is useful to pre-plan there versus land and review in patches.
https://codereview.chromium.org/2401193002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2401193002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:27: "device/bluetooth/public/interfaces/device.mojom", +dbeam@chromium.org Still catching up with the design doc at go/fizzbluetoothinternals. I am noticing a discrepancy of the C++ code backing up chrome://bluetooth-internals with the rest of WebUI. Specifically C++ code handling messages from WebUI for all other WebUI pages resides at chrome/browser/ui/webui/. For the case of bluetooth-internals, the only code residing there (chrome/browser/ui/webui/bluetooth_internals) is a dummy content::WebUIController subclass that simply serves the CSS and JS. The JS files at chrome/browser/resources/bluetooth_internals/ are reaching out to device/bluetooth directly, which seems a bit odd. Notice how all other Mojo-ified WebUI pages (plugins/site-engagement,omnibox) have their *.mojom files (and their implementations of those) inside chrome/browser/ui/webui. Is it OK to a) Have WebUI handling code outside of the so far established location chrome/browser/ui/webui/? b) Mix device/bluetooth code with WebUI handling code? I am looping in more WebUI OWNERS. @dbeam: WDYT? Is this breaking existing conventions in WebUI code structure?
On 2016/10/12 19:32:05, scheib wrote: > On 2016/10/12 19:06:49, mbrunson wrote: > > On 2016/10/12 18:27:43, dcheng wrote: > > > On 2016/10/12 18:06:52, mbrunson wrote: > > > > > > > > > > https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... > > > > File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2401193002/diff/140001/chrome/browser/resourc... > > > > chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:114: > > > > .then(function() { return adapter.getDevices(); }) > > > > On 2016/10/12 17:39:02, dpapad wrote: > > > > > > The main purpose of the Device service is to provide a place to list > > > > Bluetooth > > > > > GATT services running on the device. Services are not populated until > the > > > > device > > > > > is connected to. The device cannot be connected to until a > > > > > BluetoothGattConnection (which will be another mojom interface) is > > created. > > > > This > > > > > leads to a whole set of new files and functions that must be implemented > > to > > > > > create the connection, get service info, and manage it appropriately. In > > > order > > > > > to keep this patch a reasonable size, I didn't included that stuff. This > > > > leaves > > > > > the GetInfo function which exists as a convenience for consumers of the > > > Device > > > > > service. > > > > > > > > > > > > I added it to the JS file because I needed to make sure it produced > the > > > same > > > > > output in the client, and I assumed I needed some consumer of the data > to > > > > allow > > > > > for a security review. > > > > > > > > > > > > > > > Thanks for the explanation. It still seems odd to me that every device > > > > requires > > > > > a service. My understanding is that you are trying to mitigate the fact > > that > > > > > Mojo services can only pass plain data objects (see DeviceInfo struct) > or > > > > > handles back and forth. I am guessing you are trying to achieve > something > > as > > > > > close to the following snippet, but taking into account Mojo > limitations? > > > > > > > > > > adapter.getDevices().then(function(devices) { > > > > > devices.forEach(fuction(d) { > > > > > // Somehow "magically" returned |devices| are fully fledged classes > > with > > > > > // their own public API, no need to manually create a service for > each > > > > > // device. > > > > > d.getInfo().then(...); > > > > > d.getGattServices().then(...); > > > > > }); > > > > > }); > > > > > > > > > > So given Mojo's limitations, I am not confident that spawning a "Device" > > > > > service, for every device is the best path forward. When I think of a > > > > "service" > > > > > I think of a singleton instance of a class that exposes some > > functionality, > > > > and > > > > > is usually instantiated once during startup (similar to the already > > existing > > > > > chrome.bluetooth "service", > https://developer.chrome.com/apps/bluetooth). > > > > > > > > > > All functionality can be exposed via calls to the top-level (and only) > > > service > > > > > "adapter" (or could be renamed to bluetoothService). > > > > > bluetoothService.listDeviceInfo() => (array<DeviceInfo) > > > > > bluetoothService.getDeviceInfo(string address); => (DeviceInfo) > > > > > bluetoothService.getDeviceGattServices(string deviceId) => > > > > (array<GattService>) > > > > > bluetoothService.connectToDevice(string deviceId) => (boolean) > > > > > // add more methods here as needed. > > > > > ...... > > > > > > > > > > I also understand that you've chosen to mirror the C++ class structure > in > > > the > > > > > exposed Mojo interfaces. Why is this coupling beneficial to the UI page? > > > > > > > > > > Let me take a step back and make a more general comment. As a WebUI > OWNER, > > > my > > > > > task is not to simply make style comments on JS usage, but to understand > > and > > > > > review the overall design. There is a complete lack of knowledge about > > > > > chrome://bluetooth-internals roadmap on my part. > > > > > > > > > > - What is the target group? End users? Bluetooth team developers? > > > > > - Are there mocks/specs for the finalized UI? > > > > > - Is there an overall design doc I should read? > > > > > > > > > > I feel that I am discovering the design little by little on every > review, > > > and > > > > > that makes useful reviewing hard. My suggestion is > > > > > > > > > > 1) land all Mojo interfaces at once in a single CL, so that there is a > > > > > complete picture of the exposed API and all involved data structures. > > > > > 2) In follow up CLs start implementing the already existing Mojo > > interfaces > > > > step > > > > > by step. > > > > > > > > > > I think this would make reviewing easier, certainly for me, but perhaps > > for > > > > > other > > > > > reviewers too. > > > > > > > > Design doc is a WIP: go/fizzbluetoothinternals. You will find a general > > > overview > > > > and discussion of the design for this Mojo service near the end. This > > coupling > > > > is beneficial to the UI page because it creates a separation of concerns > for > > > > parts of the UI. I'm planning to use Polymer for a web components oriented > > > > design, so I would rather have each component manage one specific layer of > > the > > > > Bluetooth stack. > > > > > > > > Other concerns mainly focus on adapting developers to the new Mojo > > interfaces. > > > > We didn't want to create a completely new structure for the transition of > > > > existing users if they wish/prefer to use the Mojo implementation. > > > > > > > > The only reason I didn't land all of the Mojo services was because I > thought > > I > > > > needed implementation of the mojom to get a security review. If that's not > > > > necessary I will definitely do that. > > > > > > This CL LGTM, but I'm not sure what this last comment means. Do you mind > > > elaborating? =) > > > > When I was first landing these Mojo interfaces, you commented that it wasn't > > possible to do a security review without any context of how the interface is > > used / implemented. I figured this referred to all future commits referring to > > mojom files. Is this not the case? Am I being more meticulous/careful than > > necessary? > > > > https://codereview.chromium.org/2357383002/#msg76 Oh, I see. This is fine, and it makes the review simpler. Thanks! > > Chiming in on a few points: > > dcheng, thanks for reviews. > > The service will consist of many bluetooth interfaces, even in long term, > because globbing everything together into one interface makes that interface far > too big and moves to a style we don't want. > > The larger process will take several refactoring steps, and we've decided to > start by keeping the mojom interfaces pretty light weight and similar to the C++ > interfaces, though we are breaking up concepts a bit because the C++ interfaces > already suffer from over-complexity. > https://bugs.chromium.org/p/chromium/issues/detail?id=612319#c19 > > I think that all mojo interfaces in one CL would be too large and blind to how > they are used/wired up. I advised mbrunson to keep to a series of small patches > that bring up one concept at a time, and we've already seen value in seeing not > only the interface but the device/bluetooth implementation as well. > > The design doc go/fizzbluetoothinternals is a good reference for what the full > set of interfaces will look like, but we hit limits of what is useful to > pre-plan there versus land and review in patches. This makes sense, thanks for the details!
LGTM https://codereview.chromium.org/2401193002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2401193002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:27: "device/bluetooth/public/interfaces/device.mojom", > I am looping in more WebUI OWNERS. @dbeam: WDYT? Is this breaking existing conventions in WebUI code structure? Talked about this with @dbeam in person. It seems that reaching outside of chrome/browser/ui/webui directly is fine, there are already some examples of doing so (some WebUI pages exist in content/ instead). Regarding mixing device/bluetooth code with WebUI handling code, it still seems odd from my perspective (Chrome's UI related code should live somewhere under chrome/, top-level directories like device/, printing/ etc are considered lower-level services not UI), but if device/bluetooth OWNERS are fine with that (already LGed), then it is OK.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
+stevenjb & rdevlin.cronin if i'm reading your design doc correctly, this UI will always be in Chrome. can we just use the existing extension API rather than making a bunch of services? we've used other extension APIs in chrome:// pages before (i.e. chrome.extensions, chrome.settingPrivate, chrome.bluetooth even maybe?). what do your new mojo services offer over the existing chrome extension APIs (chrome.bluetooth, chrome.bluetoothPrivate) and why can't we just add to that? chrome.bluetooth and chrome.bluetoothPrivate are implemented in: extensions/browser/api/bluetooth/ they depend on a BluetoothAdapter in device/bluetooth/ which seems to have access to GATT (whatever that means) devices. fwiw: it seems that extension APIs will eventually be using mojo under the covers (or at least that's the plan)
On 2016/10/12 21:06:08, Dan Beam wrote: > +stevenjb & rdevlin.cronin > > if i'm reading your design doc correctly, this UI will always be in Chrome. can > we just use the existing extension API rather than making a bunch of services? > we've used other extension APIs in chrome:// pages before (i.e. > chrome.extensions, chrome.settingPrivate, chrome.bluetooth even maybe?). what > do your new mojo services offer over the existing chrome extension APIs > (chrome.bluetooth, chrome.bluetoothPrivate) and why can't we just add to that? > > chrome.bluetooth and chrome.bluetoothPrivate are implemented in: > extensions/browser/api/bluetooth/ > > they depend on a BluetoothAdapter in device/bluetooth/ which seems to have > access to GATT (whatever that means) devices. > > fwiw: it seems that extension APIs will eventually be using mojo under the > covers (or at least that's the plan) Re: use of device/bluetooth for UI: The internals page is UI to show internal concepts. For UI specific needs in C++ the code will live in chrome/browser/ui/webui, but when dealing with lower level types and data it is appropriate to use those types directly as they're defined in device/bluetooth. There's no value in attempting to wrap them in UI directories. The code that is being added to device/bluetooth now is step 1 in a long term refactor of clients to use a bluetooth service exposed via mojo - there will be many clients of this eventually. Re: using extentions API instead: The bluetooth internals webUI is intended to show internal data as well that is not exposed to web content or via the chrome.bluetooth APIs. It makes sense for the internals page to go directly to the source of truth.
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...
https://codereview.chromium.org/2401193002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2401193002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:27: "device/bluetooth/public/interfaces/device.mojom", On 2016/10/12 21:02:32, dpapad wrote: > > I am looping in more WebUI OWNERS. @dbeam: WDYT? Is this breaking existing > conventions in WebUI code structure? > > Talked about this with @dbeam in person. It seems that reaching outside of > chrome/browser/ui/webui directly is fine, there are already some examples of > doing so (some WebUI pages exist in content/ instead). Regarding mixing > device/bluetooth code with WebUI handling code, it still seems odd from my > perspective (Chrome's UI related code should live somewhere under chrome/, > top-level directories like device/, printing/ etc are considered lower-level > services not UI), but if device/bluetooth OWNERS are fine with that (already > LGed), then it is OK. Great. scheib@ and ortuno@ are the device/bluetooth OWNERS, and I always add and get LGTM from them first on this project. https://codereview.chromium.org/2401193002/diff/160001/device/bluetooth/devic... File device/bluetooth/device.cc (right): https://codereview.chromium.org/2401193002/diff/160001/device/bluetooth/devic... device/bluetooth/device.cc:15: : address_(address), adapter_(adapter) {} On 2016/10/12 18:27:43, dcheng wrote: > Nit: std::move(adapter) Done. https://codereview.chromium.org/2401193002/diff/160001/device/bluetooth/publi... File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2401193002/diff/160001/device/bluetooth/publi... device/bluetooth/public/interfaces/device.mojom:14: // Gets basic information about the device. Returns null, If no device is On 2016/10/12 18:27:43, dcheng wrote: > Nit:If=> if Done.
On 2016/10/12 21:11:57, scheib wrote: > On 2016/10/12 21:06:08, Dan Beam wrote: > > +stevenjb & rdevlin.cronin > > > > if i'm reading your design doc correctly, this UI will always be in Chrome. > can > > we just use the existing extension API rather than making a bunch of services? > > > we've used other extension APIs in chrome:// pages before (i.e. > > chrome.extensions, chrome.settingPrivate, chrome.bluetooth even maybe?). what > > do your new mojo services offer over the existing chrome extension APIs > > (chrome.bluetooth, chrome.bluetoothPrivate) and why can't we just add to that? > > > > chrome.bluetooth and chrome.bluetoothPrivate are implemented in: > > extensions/browser/api/bluetooth/ > > > > they depend on a BluetoothAdapter in device/bluetooth/ which seems to have > > access to GATT (whatever that means) devices. > > > > fwiw: it seems that extension APIs will eventually be using mojo under the > > covers (or at least that's the plan) > > Re: use of device/bluetooth for UI: > The internals page is UI to show internal concepts. For UI specific needs in C++ > the code will live in chrome/browser/ui/webui, but when dealing with lower level > types and data it is appropriate to use those types directly as they're defined > in device/bluetooth. There's no value in attempting to wrap them in UI > directories. The code that is being added to device/bluetooth now is step 1 in a > long term refactor of clients to use a bluetooth service exposed via mojo - > there will be many clients of this eventually. > > Re: using extentions API instead: > The bluetooth internals webUI is intended to show internal data as well that is > not exposed to web content or via the chrome.bluetooth APIs. It makes sense for > the internals page to go directly to the source of truth. which information is not exposed to chrome.bluetooth APIs?
On 2016/10/12 at 23:21:35, dbeam wrote: > On 2016/10/12 21:11:57, scheib wrote: > > On 2016/10/12 21:06:08, Dan Beam wrote: > > > +stevenjb & rdevlin.cronin > > > > > > if i'm reading your design doc correctly, this UI will always be in Chrome. > > can > > > we just use the existing extension API rather than making a bunch of services? > > > > > we've used other extension APIs in chrome:// pages before (i.e. > > > chrome.extensions, chrome.settingPrivate, chrome.bluetooth even maybe?). what > > > do your new mojo services offer over the existing chrome extension APIs > > > (chrome.bluetooth, chrome.bluetoothPrivate) and why can't we just add to that? > > > > > > chrome.bluetooth and chrome.bluetoothPrivate are implemented in: > > > extensions/browser/api/bluetooth/ > > > > > > they depend on a BluetoothAdapter in device/bluetooth/ which seems to have > > > access to GATT (whatever that means) devices. > > > > > > fwiw: it seems that extension APIs will eventually be using mojo under the > > > covers (or at least that's the plan) > > > > Re: use of device/bluetooth for UI: > > The internals page is UI to show internal concepts. For UI specific needs in C++ > > the code will live in chrome/browser/ui/webui, but when dealing with lower level > > types and data it is appropriate to use those types directly as they're defined > > in device/bluetooth. There's no value in attempting to wrap them in UI > > directories. The code that is being added to device/bluetooth now is step 1 in a > > long term refactor of clients to use a bluetooth service exposed via mojo - > > there will be many clients of this eventually. > > > > Re: using extentions API instead: > > The bluetooth internals webUI is intended to show internal data as well that is > > not exposed to web content or via the chrome.bluetooth APIs. It makes sense for > > the internals page to go directly to the source of truth. > > which information is not exposed to chrome.bluetooth APIs? Also note that this is intended to be used with Bluetooth Low Energy devices and the chrome.bluetoothlowenergy API (which contains the necessary functions) is only supported on Chrome OS.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/12 23:23:49, ortuno wrote: > On 2016/10/12 at 23:21:35, dbeam wrote: > > On 2016/10/12 21:11:57, scheib wrote: > > > On 2016/10/12 21:06:08, Dan Beam wrote: > > > > +stevenjb & rdevlin.cronin > > > > > > > > if i'm reading your design doc correctly, this UI will always be in > Chrome. > > > can > > > > we just use the existing extension API rather than making a bunch of > services? > > > > > > > we've used other extension APIs in chrome:// pages before (i.e. > > > > chrome.extensions, chrome.settingPrivate, chrome.bluetooth even maybe?). > what > > > > do your new mojo services offer over the existing chrome extension APIs > > > > (chrome.bluetooth, chrome.bluetoothPrivate) and why can't we just add to > that? > > > > > > > > chrome.bluetooth and chrome.bluetoothPrivate are implemented in: > > > > extensions/browser/api/bluetooth/ > > > > > > > > they depend on a BluetoothAdapter in device/bluetooth/ which seems to have > > > > access to GATT (whatever that means) devices. > > > > > > > > fwiw: it seems that extension APIs will eventually be using mojo under the > > > > covers (or at least that's the plan) > > > > > > Re: use of device/bluetooth for UI: > > > The internals page is UI to show internal concepts. For UI specific needs in > C++ > > > the code will live in chrome/browser/ui/webui, but when dealing with lower > level > > > types and data it is appropriate to use those types directly as they're > defined > > > in device/bluetooth. There's no value in attempting to wrap them in UI > > > directories. The code that is being added to device/bluetooth now is step 1 > in a > > > long term refactor of clients to use a bluetooth service exposed via mojo - > > > there will be many clients of this eventually. > > > > > > Re: using extentions API instead: > > > The bluetooth internals webUI is intended to show internal data as well that > is > > > not exposed to web content or via the chrome.bluetooth APIs. It makes sense > for > > > the internals page to go directly to the source of truth. > > > > which information is not exposed to chrome.bluetooth APIs? > > Also note that this is intended to be used with Bluetooth Low Energy devices and > the chrome.bluetoothlowenergy API (which contains the necessary functions) is > only supported on Chrome OS. We'll also be adding diagnostic information which will be logged out as events and operating system specific status results are encountered.
rockot@chromium.org changed reviewers: + rockot@chromium.org
Not that anyone asked, but this LGTM ;) https://codereview.chromium.org/2401193002/diff/180001/device/bluetooth/publi... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2401193002/diff/180001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:7: import "device.mojom"; drive-by nit: Please fully qualify import paths as you would for includes
https://codereview.chromium.org/2401193002/diff/180001/device/bluetooth/publi... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2401193002/diff/180001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:7: import "device.mojom"; On 2016/10/13 03:26:58, Ken Rockot wrote: > drive-by nit: Please fully qualify import paths as you would for includes Done.
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...
dbeam@chromium.org changed reviewers: - dbeam@chromium.org
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 scheib@chromium.org, ortuno@chromium.org, dcheng@chromium.org, dpapad@chromium.org, rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2401193002/#ps200001 (title: "Fully qualify mojom import")
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 Device service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for Device. Adds starter code for Device implementation for the Bluetooth Mojo service. Adds GetDevice to the Adapter service to create Device interfaces. Adds console logging for DeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add Device service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for Device. Adds starter code for Device implementation for the Bluetooth Mojo service. Adds GetDevice to the Adapter service to create Device interfaces. Adds console logging for DeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add Device service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for Device. Adds starter code for Device implementation for the Bluetooth Mojo service. Adds GetDevice to the Adapter service to create Device interfaces. Adds console logging for DeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add Device service for chrome://bluetooth-internals. Moves DeviceInfo struct to device.mojom and creates interface for Device. Adds starter code for Device implementation for the Bluetooth Mojo service. Adds GetDevice to the Adapter service to create Device interfaces. Adds console logging for DeviceInfo on chrome://bluetooth-internals page. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/22ff37419618d39c8ff220108a935d3d8762a4f5 Cr-Commit-Position: refs/heads/master@{#425085} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/22ff37419618d39c8ff220108a935d3d8762a4f5 Cr-Commit-Position: refs/heads/master@{#425085} |
