Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2357383002:
bluetooth: Add device list retrieval for chrome://bluetooth-internals (Closed)
|
Created:
4 years, 3 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, scheib+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Add device list retrieval for chrome://bluetooth-internals
Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page.
Adds mojom files for Bluetooth service definition and internals page handler.
Adds basic Bluetooth device retrieval using starter Bluetooth service
implementation for logging on web front end.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
BUG=651282
Committed: https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9
Committed: https://crrev.com/70d50e7a3e3289bfce518ae91a85759b40469642
Cr-Original-Commit-Position: refs/heads/master@{#422570}
Cr-Commit-Position: refs/heads/master@{#422855}
Patch Set 1 #
Total comments: 66
Patch Set 2 : Mojo interface and implementation fixes #
Total comments: 35
Patch Set 3 : Internals page/Mojo service implementation change #Patch Set 4 : Revert chrome browser client file #
Total comments: 55
Patch Set 5 : Remove unneccesary dependencies, limit Mojom visibility, style fixes #Patch Set 6 : Remove extra bluetooth dep in //chrome/broswer/ui #Patch Set 7 : continuation #Patch Set 8 : Update visibility #
Total comments: 5
Patch Set 9 : Visibility and dep changes #Patch Set 10 : Remove ES6 #Patch Set 11 : Fix JS syntax #Patch Set 12 : Add dependency to gen mojom for browser/resources #Patch Set 13 : Move dependency to //chrome/browser:resources target #
Total comments: 8
Patch Set 14 : Fix merge issue #Patch Set 15 : Remove scoped_refptr get calls #
Total comments: 12
Patch Set 16 : Style changes, destructuring in JS #Patch Set 17 : Fix Windows BUILD visibility #Dependent Patchsets: Messages
Total messages: 121 (75 generated)
Description was changed from ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. ========== to ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mbrunson@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
Description was changed from ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds functionality to Mojo's connection.js for sending interface request's without a callback. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
First round of comments. I didn't get to look at everything, will continue tomorrow. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:26: return new Promise(function(resolve, reject) { q: Does this need to be a function? Could it be an arrow function? Also you don't need the reject, I think. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:27: define(moduleNames, function(var_args) { var_args seems unused? https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:28: resolve(Array.prototype.slice.call(arguments, 0)); I believe you can use the spread operator here. So just: resolve(...arguments); https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:28: resolve(Array.prototype.slice.call(arguments, 0)); I believe you can use the spread operator here. So just: resolve(...arguments); https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:45: .then((modules) => { .then(([frameInterfaces, mojom, bluetooth, connection]) => { // ... That way you wouldn't need to initialize each variable :) https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:58: AdapterClientImpl.prototype = { Is it possible to use ES6 classes? https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webshare/... https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom:2: Remove new line. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_page_handler.h (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_page_handler.h:14: class BluetoothInternalsPageHandler : public mojom::InternalsPageHandler, Add comment about the class. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:41: page_handler_.reset(new BluetoothInternalsPageHandler(std::move(request))); I'm probably missing some context here but why not just bind Adapter directly here? The extra layer of indirection seems unnecessary. Somewhat related: You probably talked to Ken about this before but is there a global registry that we will eventually use for the Adapter interface? I know registries are going away but I'm not sure what's replacing them. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:15: device::BluetoothAdapterFactoryWrapper::Get().AcquireAdapter( Don't use the Wrapper. That class was added because Web Bluetooth does some weird stuff for testing. Just use BluetoothAdapterFactory. Also I would move the adapter creation to GetAdapter(). https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:27: for (const device::BluetoothDevice* device : adapter->GetDevices()) { q: Why do you do this after adapter construction rather than when clients request devices? https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:35: std::vector<bluetooth::DeviceInfoPtr> result; optional nit: "devices" is more specific. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:59: if (addresses_.insert(device_address).second && client_) { The comment says that if a *known* address was added alert the client but the code does the opposite: if a device_address is inserted (therefore unknown) then notify the client. Also when do you expect to see known addresses here? You are removing address in DeviceRemoved so all addresses would be unknown. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:78: bluetooth::DeviceInfoPtr BluetoothAdapterService::GetDeviceInfo( optional nit: ConstructDeviceInfoStruct would be more specific. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:81: device_info->name = device->GetName().value_or(""); I think you need to surround this by an if statement otherwise you will be returning an empty string for devices that have no name. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:19: class BluetoothAdapterService : public bluetooth::Adapter, Is BluetoothAdapter the right name here? I thought we were moving to calling things "Interface"? nit: Add a comment about the class. See WebBluetoothServiceImpl for example: https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_... https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/BUILD.gn:16: sources = [ nit: Shouldn't this include the uuid mojom as well? https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... File device/bluetooth/public/interfaces/bluetooth.mojom (right): https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:2: nit: remove new line https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:6: module bluetooth; scheib: Should we add a note mentioning that this is experimental and shouldn't be used by any clients that what a stable API? https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:13: uint16 device_id; nit: there was a comment about this on the design doc. I'm OK with landing this and then removing depending on the outcome of that discussion but if you can get a resolution sooner it would be better. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:19: // Retrieves the list of devices that can be detected I want to get rid of this function but since that discussion is still ongoing I think it's OK to land this but we should add more detail regarding what the function returns i.e. Retrieves the list of the devices known by the adapter. This includes Connected Devices, GATT Connected Devices, Paired Devices and Devices discovered by during a classic of low-energy scan. nit: Period at the end of sentence. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:20: GetDevices(int8 index, int8 count) => (array<DeviceInfo> devices); Could you mention the purpose of the two arguments in the comment? Also question: What's the use case for them? https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:20: GetDevices(int8 index, int8 count) => (array<DeviceInfo> devices); Could you mention the purpose of the two arguments in the comment? Also question: What's the use case for them? https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:24: // Called when a device has been detected nit: Called the first time a device is discovered. Also period at the end of sentence. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:28: DeviceRemoved(DeviceInfo device); nit: Called after the device hasn't been seen for 3 minutes. Also period at the end of sentence. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:32: // Creates pipe for Bluetooth Adapter between |client| and |adapter| nit: Period at the end of sentence.
https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:26: return new Promise(function(resolve, reject) { On 2016/09/22 08:32:53, ortuno wrote: > q: Does this need to be a function? Could it be an arrow function? Also you > don't need the reject, I think. These are good suggestions, but ortuno FYI this is 'boilerplate copy-paste' from other Javascript mojo pages -- mojo team knows that the bindings are terrible and need an update, and they'll be doing that. I think it's OK to remain consistent with the previous pattern, which will ease refactoring when the mojo team does a clean up. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/OWNERS (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/OWNERS:1: per-file *.mojom=set noparent In person chat with pam@ indicated they weren't great for up-to-date webui reviews - only rubber stamps. Because ortuno and I are new to this code we should use a more active webui reviewer. We should also ask them at which point we should add our own OWNERS here to stop bugging them. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/BUILD.gn:7: mojom("bluetooth_mojom_bluetooth_uuid") { Oops, I missed when when renaming uuid into just bluetooth.mojom.uuid. I think this should just be a single mojom("bluetooth") And, we may want to make a new one with limited GN visibility* for the new interfaces. * https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/check.md e.g. mojom("bluetooth_experimental") https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... File device/bluetooth/public/interfaces/bluetooth.mojom (right): https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:6: module bluetooth; On 2016/09/22 08:32:54, ortuno wrote: > scheib: Should we add a note mentioning that this is experimental and shouldn't > be used by any clients that what a stable API? Use: module bluetooth.mojom I think we should use gn's visibility to restrict access for now: https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/check.md https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:18: interface Adapter { ortuno: what do you think here, I think that we should keep related interfaces in individual files. e.g. this file should be renamed to adapter.mojom, and the internals interface moved out to chrome/browser/ui/webui/bluetooth/bluetooth_internals.mojom https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:31: interface InternalsPageHandler { This interface shouldn't be in device/bluetooth https://codereview.chromium.org/2357383002/diff/1/mojo/public/js/connection.js File mojo/public/js/connection.js (right): https://codereview.chromium.org/2357383002/diff/1/mojo/public/js/connection.j... mojo/public/js/connection.js:104: function bindProxy2(remoteInterface) { Let's separate out this change to be stand-alone. Update the bindProxy method above an refactor sites that call it.
https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:26: return new Promise(function(resolve, reject) { On 2016/09/22 08:32:53, ortuno wrote: > q: Does this need to be a function? Could it be an arrow function? Also you > don't need the reject, I think. The define callback has to be a function. I've run into issues trying to do it with arrow functions before. As scheib said, this is just copy-paste from some other page so I think leaving it is fine for now. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:27: define(moduleNames, function(var_args) { On 2016/09/22 08:32:53, ortuno wrote: > var_args seems unused? Done. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:28: resolve(Array.prototype.slice.call(arguments, 0)); On 2016/09/22 08:32:53, ortuno wrote: > I believe you can use the spread operator here. So just: > > resolve(...arguments); I believe the promise spec only allows a single value. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:45: .then((modules) => { On 2016/09/22 08:32:53, ortuno wrote: > .then(([frameInterfaces, mojom, bluetooth, connection]) => { > // ... > > That way you wouldn't need to initialize each variable :) In which case this has to be one variable; however, I can use destructuring for this. I still have to get used to ES6 features. :-) https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:58: AdapterClientImpl.prototype = { On 2016/09/22 08:32:53, ortuno wrote: > Is it possible to use ES6 classes? > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/webshare/... Done. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom:2: On 2016/09/22 08:32:53, ortuno wrote: > Remove new line. Done. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_page_handler.h (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_page_handler.h:14: class BluetoothInternalsPageHandler : public mojom::InternalsPageHandler, On 2016/09/22 08:32:53, ortuno wrote: > Add comment about the class. Done. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:41: page_handler_.reset(new BluetoothInternalsPageHandler(std::move(request))); On 2016/09/22 08:32:53, ortuno wrote: > I'm probably missing some context here but why not just bind Adapter directly > here? The extra layer of indirection seems unnecessary. > > Somewhat related: You probably talked to Ken about this before but is there a > global registry that we will eventually use for the Adapter interface? I know > registries are going away but I'm not sure what's replacing them. This seems to be the established pattern but the WebUIController and the WebUIHandler can definitely be merged. I will do that. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:15: device::BluetoothAdapterFactoryWrapper::Get().AcquireAdapter( On 2016/09/22 08:32:53, ortuno wrote: > Don't use the Wrapper. That class was added because Web Bluetooth does some > weird stuff for testing. Just use BluetoothAdapterFactory. Also I would move the > adapter creation to GetAdapter(). Done. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:27: for (const device::BluetoothDevice* device : adapter->GetDevices()) { On 2016/09/22 08:32:54, ortuno wrote: > q: Why do you do this after adapter construction rather than when clients > request devices? Done. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:35: std::vector<bluetooth::DeviceInfoPtr> result; On 2016/09/22 08:32:53, ortuno wrote: > optional nit: "devices" is more specific. Done. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:59: if (addresses_.insert(device_address).second && client_) { On 2016/09/22 08:32:53, ortuno wrote: > The comment says that if a *known* address was added alert the client but the > code does the opposite: if a device_address is inserted (therefore unknown) then > notify the client. > > Also when do you expect to see known addresses here? You are removing address in > DeviceRemoved so all addresses would be unknown. That's a typo. I don't expect to see known addresses, but I was checking for inconsistent state just in case. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:78: bluetooth::DeviceInfoPtr BluetoothAdapterService::GetDeviceInfo( On 2016/09/22 08:32:54, ortuno wrote: > optional nit: ConstructDeviceInfoStruct would be more specific. Done. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:81: device_info->name = device->GetName().value_or(""); On 2016/09/22 08:32:53, ortuno wrote: > I think you need to surround this by an if statement otherwise you will be > returning an empty string for devices that have no name. Removing the value_or should be enough. Name is an optional property here. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:19: class BluetoothAdapterService : public bluetooth::Adapter, On 2016/09/22 08:32:54, ortuno wrote: > Is BluetoothAdapter the right name here? I thought we were moving to calling > things "Interface"? > > nit: Add a comment about the class. > > See WebBluetoothServiceImpl for example: > > https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_... Since everything is a WIP, this file will change many, many times so it's probably OK for now. however, I haven't heard anything about using "Interface" in the service implementation. Was this in the design doc discussion? https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/BUILD.gn:7: mojom("bluetooth_mojom_bluetooth_uuid") { On 2016/09/23 20:53:51, scheib wrote: > Oops, I missed when when renaming uuid into just bluetooth.mojom.uuid. > > I think this should just be a single mojom("bluetooth") > > And, we may want to make a new one with limited GN visibility* for the new > interfaces. > * https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/check.md > > e.g. mojom("bluetooth_experimental") Done. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/BUILD.gn:16: sources = [ On 2016/09/22 08:32:54, ortuno wrote: > nit: Shouldn't this include the uuid mojom as well? Done. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... File device/bluetooth/public/interfaces/bluetooth.mojom (right): https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:2: On 2016/09/22 08:32:54, ortuno wrote: > nit: remove new line Done. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:13: uint16 device_id; On 2016/09/22 08:32:54, ortuno wrote: > nit: there was a comment about this on the design doc. I'm OK with landing this > and then removing depending on the outcome of that discussion but if you can get > a resolution sooner it would be better. I think just removing it entirely for now would be better. Many, many things will be added to this file later so I think it's OK to omit things we're not sure about for now unless it hinders manual testing. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:18: interface Adapter { On 2016/09/23 20:53:51, scheib wrote: > ortuno: what do you think here, > I think that we should keep related interfaces in individual files. e.g. this > file should be renamed to adapter.mojom, and the internals interface moved out > to > chrome/browser/ui/webui/bluetooth/bluetooth_internals.mojom The internals interface was not supposed to be in here to begin with. I must have left it in here when I copied this file to make the bluetooth_internals.mojom. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:19: // Retrieves the list of devices that can be detected On 2016/09/22 08:32:54, ortuno wrote: > I want to get rid of this function but since that discussion is still ongoing I > think it's OK to land this but we should add more detail regarding what the > function returns i.e. Retrieves the list of the devices known by the adapter. > This includes Connected Devices, GATT Connected Devices, Paired Devices and > Devices discovered by during a classic of low-energy scan. > > nit: Period at the end of sentence. Done. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:20: GetDevices(int8 index, int8 count) => (array<DeviceInfo> devices); On 2016/09/22 08:32:54, ortuno wrote: > Could you mention the purpose of the two arguments in the comment? > > Also question: What's the use case for them? These should probably be removed. The original use case was for pagination on the internals page but with the move to a more generic interface that seems like something that should be handled by the consumer of this. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:24: // Called when a device has been detected On 2016/09/22 08:32:54, ortuno wrote: > nit: Called the first time a device is discovered. > > Also period at the end of sentence. Done. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:28: DeviceRemoved(DeviceInfo device); On 2016/09/22 08:32:54, ortuno wrote: > nit: Called after the device hasn't been seen for 3 minutes. > > Also period at the end of sentence. Done. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:31: interface InternalsPageHandler { On 2016/09/23 20:53:51, scheib wrote: > This interface shouldn't be in device/bluetooth Done. https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:32: // Creates pipe for Bluetooth Adapter between |client| and |adapter| On 2016/09/22 08:32:54, ortuno wrote: > nit: Period at the end of sentence. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Have to talked to scheib about testing? https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:26: return new Promise(function(resolve, reject) { On 2016/09/24 at 01:05:46, mbrunson wrote: > On 2016/09/22 08:32:53, ortuno wrote: > > q: Does this need to be a function? Could it be an arrow function? Also you > > don't need the reject, I think. > > The define callback has to be a function. I've run into issues trying to do it with arrow functions before. As scheib said, this is just copy-paste from some other page so I think leaving it is fine for now. Ah got it. Thanks for the explanation. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:27: define(moduleNames, function(var_args) { On 2016/09/24 at 01:05:46, mbrunson wrote: > On 2016/09/22 08:32:53, ortuno wrote: > > var_args seems unused? > > Done. Did you mean to delete the argument or are you leaving it there because of the previous comment? https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:28: resolve(Array.prototype.slice.call(arguments, 0)); On 2016/09/24 at 01:05:46, mbrunson wrote: > On 2016/09/22 08:32:53, ortuno wrote: > > I believe you can use the spread operator here. So just: > > > > resolve(...arguments); > > I believe the promise spec only allows a single value. Right. In ES2015 you can use either the spread operator or Array.from to turn 'arguments' into an array. https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/argu... https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:59: if (addresses_.insert(device_address).second && client_) { On 2016/09/24 at 01:05:47, mbrunson wrote: > On 2016/09/22 08:32:53, ortuno wrote: > > The comment says that if a *known* address was added alert the client but the > > code does the opposite: if a device_address is inserted (therefore unknown) then > > notify the client. > > > > Also when do you expect to see known addresses here? You are removing address in > > DeviceRemoved so all addresses would be unknown. > > That's a typo. > > I don't expect to see known addresses, but I was checking for inconsistent state just in case. We should be able to trust the API to not mess up. That said, you could just add a DCHECK[1] to indicate that you don't expect for that to happen. bool insertion_result = addresses_.insert(device_address).second; DCHECK(insertion_result); [1] https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/ui/webui/blu... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:19: class BluetoothAdapterService : public bluetooth::Adapter, On 2016/09/24 at 01:05:47, mbrunson wrote: > On 2016/09/22 08:32:54, ortuno wrote: > > Is BluetoothAdapter the right name here? I thought we were moving to calling > > things "Interface"? > > > > nit: Add a comment about the class. > > > > See WebBluetoothServiceImpl for example: > > > > https://cs.chromium.org/chromium/src/content/browser/bluetooth/web_bluetooth_... > > Since everything is a WIP, this file will change many, many times so it's probably OK for now. however, I haven't heard anything about using "Interface" in the service implementation. Was this in the design doc discussion? This is not really a big issue but I was just wondering what the right name for this should be. I think there are a couple of issues here: 1. From the examples I've seen if the interface's name in the mojom is "Adapter" the class that implements the interface should be called "AdapterImpl". 2. Is the name "Adapter" correct? Should it include the name "Interface" or "Service" in there? https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... File device/bluetooth/public/interfaces/bluetooth.mojom (right): https://codereview.chromium.org/2357383002/diff/1/device/bluetooth/public/int... device/bluetooth/public/interfaces/bluetooth.mojom:18: interface Adapter { On 2016/09/24 at 01:05:47, mbrunson wrote: > On 2016/09/23 20:53:51, scheib wrote: > > ortuno: what do you think here, > > I think that we should keep related interfaces in individual files. e.g. this > > file should be renamed to adapter.mojom, and the internals interface moved out > > to > > chrome/browser/ui/webui/bluetooth/bluetooth_internals.mojom > > The internals interface was not supposed to be in here to begin with. I must have left it in here when I copied this file to make the bluetooth_internals.mojom. Separating in the interfaces in files sounds good. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:40: .then((modules) => { Unless I'm missing something you should be able to use destructuring to directly assign the variables: .then(([frameInterfaces, mojom, bluetooth, connection]) => { // ... https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:42: let pageHandler = connection.bindHandleToProxy( Consider moving this further down to where it is used. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:63: console.log('Device added'); Device removed :) https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:70: return new Promise((resolve) => { There is no indication that initializeProxies() will resolve with an adapter service. To improve readability you could just initialize the service in here: return new Promise((resolve) => { adapterHandle = connection.bindProxy(resolve, bluetooth.Adapter); }).then(service => { adapterService = service; // Create a message pipe and bind one end to client implementation clientHandle = connection.bindStubDerivedImpl(adapterClient); // Send interface request and other end of client message pipe to // C++ pageHandler.getAdapterService(adapterHandle, clientHandle); }); }); Alternatively you can simplify this code if you follow the pattern explained in bluetooth_internals.mojom https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom:9: interface InternalsPageHandler { This is not in the bluetooth module so I believe it should be named BluetoothInternalsPageHandler. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom:11: GetAdapterService(bluetooth.mojom.Adapter& adapter, Sorry, what you had before was ok. I should have been clearer. I was thinking that it might not be necessary to go through the internals page to get the adapter. It might be possible to register the adapter service here[1] and then just retrieve it in bluetooth_internals.js with: adapterService = connection.bindHandleToProxy( frameInterfaces.getInterface(mojom.bluetooth.Adapter.name), mojom.bluetooth.Adapter); Note that this will mean that we have to set the client separately. I think this is the right way forward given that this will end up being how every client will retrieve the adapter service but I don't think it's a big deal for now. Anyway it's just something to explore. What you had before was fine. In case I wasn't clear or if you have any questions I made a POC patch that implements this pattern https://codereview.chromium.org/2372433002 check bluetooth_internals.js, chrome_content_browser_client.cc and bluetooth_adapter_service.h/cc [1] https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl... https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h:30: // mojom::InternalsPageHandler overrides: The structure you had before was fine. Sorry! https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:19: // Implementation of Mojo BluetoothAdapter located in Should this be under the bluetooth: namespace? https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:44: bluetooth::mojom::DeviceInfoPtr ConstructDeviceInfoStruct( The order of declaration should match the order of the definition: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:51: scoped_refptr<device::BluetoothAdapter> adapter_; nit: Could you add comments for these? https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:52: std::set<std::string> addresses_; q: Why do you keep track of the devices? In other words why not just call DeviceAdded and DeviceRemoved for all devices? Also fyi std::set uses a tree whereas std::unordered_set uses a hash set. Consider using unordered_set if you don't care about the order. http://stackoverflow.com/questions/16075890/what-is-the-difference-between-se... https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... File device/bluetooth/public/interfaces/bluetooth.mojom (right): https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... device/bluetooth/public/interfaces/bluetooth.mojom:17: // during a classic low-energy scan. nit: classic or low-energy
https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:76: clientHandle = connection.bindStubDerivedImpl(adapterClient); You are creating a different pipe for the client. This means that there is no order guarantee regarding the order of the messages. So if you have this in the implementation: callback.Run(); client_.NotifyOfSomething(); It's entirely possible that the interface client receives NotifyOfSomething() first and then the callback. I don't think you need the guarantee right now, in Web Bluetooth we do, but keep it in mind in case you start seen races.
(some comments, more review to do) https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:76: clientHandle = connection.bindStubDerivedImpl(adapterClient); On 2016/09/26 07:45:17, ortuno wrote: > You are creating a different pipe for the client. This means that there is no > order guarantee regarding the order of the messages. So if you have this in the > implementation: > > callback.Run(); > client_.NotifyOfSomething(); > > It's entirely possible that the interface client receives NotifyOfSomething() > first and then the callback. > > I don't think you need the guarantee right now, in Web Bluetooth we do, but keep > it in mind in case you start seen races. https://www.chromium.org/developers/design-documents/mojo/associated-interfaces is what is designed to resolve this, but I don't know if javascript examples exist. I suspect we'll need them eventually, if we need them sooner ping rockot@ for help. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:19: // Implementation of Mojo BluetoothAdapter located in On 2016/09/26 01:57:56, ortuno wrote: > Should this be under the bluetooth: namespace? I think yes, and the bluetooth service implementations under device/bluetooth/?? (possibly another subdirectory, e.g. bluetooth/service -- look for other examples) Also, I believe we can drop 'Bluetooth' from the class names, they'll all live in the bluetooth namespace. https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:7: mojom("bluetooth") { Looking at src/services/.../BUILD.gn, I see that they always use: mojom("interfaces"), and wrap up all the mojom for that .../public/interfaces/*.mojom I think we should use those as style guide, so let's use 'interfaces' and 'experimental_interfaces'
https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:46: class AdapterClientImpl extends bluetooth.AdapterClient.stubClass { Move the AdapterClientImpl class definition to the top level of the module, unless it is closing over something here I didn't spot. In the long run will you have a class that is only a ClientImpl, or perhaps you'll simply have a single class controller for the web UI page? That is, perhaps this is just a "class Controller extends bluetooth.AdapterClient.stubClass". https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:74: if (addresses_.insert(device_address).second && client_) { Do we need this logic here? Why not pass through the DeviceAdded call without logic, and without saving to addresses_ (remove that member variable). https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:17: "bluetooth.mojom", adapter.mojom, (rename that file, and only place adapter concepts into it).
On 2016/09/26 01:57:56, ortuno wrote: > Have to talked to scheib about testing? https://bugs.chromium.org/p/chromium/issues/detail?id=612319#c19
Description was changed from ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds functionality to Mojo's connection.js for sending interface request's without a callback. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds functionality to Mojo's connection.js for sending interface request's without a callback. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG= ==========
Seems like the fixes are in and working now. We'll see what the trybots say. https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:27: define(moduleNames, function(var_args) { On 2016/09/26 01:57:55, ortuno wrote: > On 2016/09/24 at 01:05:46, mbrunson wrote: > > On 2016/09/22 08:32:53, ortuno wrote: > > > var_args seems unused? > > > > Done. > > Did you mean to delete the argument or are you leaving it there because of the > previous comment? I'm just going to leave this entire function alone for now. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:40: .then((modules) => { On 2016/09/26 01:57:56, ortuno wrote: > Unless I'm missing something you should be able to use destructuring to directly > assign the variables: > > .then(([frameInterfaces, mojom, bluetooth, connection]) => { > // ... Done. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:42: let pageHandler = connection.bindHandleToProxy( On 2016/09/26 01:57:56, ortuno wrote: > Consider moving this further down to where it is used. Done. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:46: class AdapterClientImpl extends bluetooth.AdapterClient.stubClass { On 2016/09/26 19:37:26, scheib wrote: > Move the AdapterClientImpl class definition to the top level of the module, > unless it is closing over something here I didn't spot. > > In the long run will you have a class that is only a ClientImpl, or perhaps > you'll simply have a single class controller for the web UI page? That is, > perhaps this is just a "class Controller extends > bluetooth.AdapterClient.stubClass". I've moved the declaration up and put the subclassing code here since bluetooth.AdapterClient is not known until importModules is called. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:63: console.log('Device added'); On 2016/09/26 01:57:56, ortuno wrote: > Device removed :) Yes. That makes much more sense. :) https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:76: clientHandle = connection.bindStubDerivedImpl(adapterClient); On 2016/09/26 07:45:17, ortuno wrote: > You are creating a different pipe for the client. This means that there is no > order guarantee regarding the order of the messages. So if you have this in the > implementation: > > callback.Run(); > client_.NotifyOfSomething(); > > It's entirely possible that the interface client receives NotifyOfSomething() > first and then the callback. > > I don't think you need the guarantee right now, in Web Bluetooth we do, but keep > it in mind in case you start seen races. Ok. When I was writing the adapter service, I specifically wanted to avoid that kind of pattern for this reason. I'll look into associated interfaces in JavaScript. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom:9: interface InternalsPageHandler { On 2016/09/26 01:57:56, ortuno wrote: > This is not in the bluetooth module so I believe it should be named > BluetoothInternalsPageHandler. Done. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom:11: GetAdapterService(bluetooth.mojom.Adapter& adapter, On 2016/09/26 01:57:56, ortuno wrote: > Sorry, what you had before was ok. I should have been clearer. I was thinking > that it might not be necessary to go through the internals page to get the > adapter. It might be possible to register the adapter service here[1] and then > just retrieve it in bluetooth_internals.js with: > > adapterService = connection.bindHandleToProxy( > frameInterfaces.getInterface(mojom.bluetooth.Adapter.name), > mojom.bluetooth.Adapter); > > Note that this will mean that we have to set the client separately. I think this > is the right way forward given that this will end up being how every client will > retrieve the adapter service but I don't think it's a big deal for now. > > Anyway it's just something to explore. What you had before was fine. In case I > wasn't clear or if you have any questions I made a POC patch that implements > this pattern https://codereview.chromium.org/2372433002 check > bluetooth_internals.js, chrome_content_browser_client.cc and > bluetooth_adapter_service.h/cc > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_cl... I agree. Decoupling the adapter service from the WebUI is a better way of doing it. This is the original idea I had, but I wasn't sure where the adapter service would be created, who would own it, and what its lifetime would be. Doing it your way greatly simplifies the WebUI and JS code and removes many changes from this patch. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h:30: // mojom::InternalsPageHandler overrides: On 2016/09/26 01:57:56, ortuno wrote: > The structure you had before was fine. Sorry! I'm reverting the entire WebUI to its state before this CL because these changes won't be needed if we register the adapter service like you mentioned. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.cc:74: if (addresses_.insert(device_address).second && client_) { On 2016/09/26 19:37:27, scheib wrote: > Do we need this logic here? Why not pass through the DeviceAdded call without > logic, and without saving to addresses_ (remove that member variable). Done. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h (right): https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:19: // Implementation of Mojo BluetoothAdapter located in On 2016/09/26 18:00:37, scheib wrote: > On 2016/09/26 01:57:56, ortuno wrote: > > Should this be under the bluetooth: namespace? > > I think yes, and the bluetooth service implementations under device/bluetooth/?? > (possibly another subdirectory, e.g. bluetooth/service -- look for other > examples) > > Also, I believe we can drop 'Bluetooth' from the class names, they'll all live > in the bluetooth namespace. I've renamed and moved this to device/bluetooth. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:44: bluetooth::mojom::DeviceInfoPtr ConstructDeviceInfoStruct( On 2016/09/26 01:57:56, ortuno wrote: > The order of declaration should match the order of the definition: > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:51: scoped_refptr<device::BluetoothAdapter> adapter_; On 2016/09/26 01:57:56, ortuno wrote: > nit: Could you add comments for these? Done. https://codereview.chromium.org/2357383002/diff/10001/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/services/bluetooth_adapter_service.h:52: std::set<std::string> addresses_; On 2016/09/26 01:57:56, ortuno wrote: > q: Why do you keep track of the devices? In other words why not just call > DeviceAdded and DeviceRemoved for all devices? > > Also fyi std::set uses a tree whereas std::unordered_set uses a hash set. > Consider using unordered_set if you don't care about the order. > > http://stackoverflow.com/questions/16075890/what-is-the-difference-between-se... I have removed this and simplified the adapter service to be as thin of a wrapper of device/bluetooth/bluetooth_adapter as possible. The less state, the better, and it removes the immediate need of rigorous testing for the Mojo service. https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:7: mojom("bluetooth") { On 2016/09/26 18:00:37, scheib wrote: > Looking at src/services/.../BUILD.gn, I see that they always use: > mojom("interfaces"), and wrap up all the mojom for that > .../public/interfaces/*.mojom > > I think we should use those as style guide, so let's use 'interfaces' and > 'experimental_interfaces' Done. https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:17: "bluetooth.mojom", On 2016/09/26 19:37:27, scheib wrote: > adapter.mojom, (rename that file, and only place adapter concepts into it). Done. https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... File device/bluetooth/public/interfaces/bluetooth.mojom (right): https://codereview.chromium.org/2357383002/diff/10001/device/bluetooth/public... device/bluetooth/public/interfaces/bluetooth.mojom:17: // during a classic low-energy scan. On 2016/09/26 01:57:56, ortuno wrote: > nit: classic or low-energy 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:62: .then(([frameInterfaces, bluetooth, connection]) => { The 'bluetooth' returned module will be trouble later when we import multiple mojom files all in the 'bluetooth' module - but they each only provide part of that space. A code search shows that seperate mojom files are the right path, it will just be ugly JS binding code until that's cleaned up. Likely the 'bluetooth' variable here will need to become bluetoothAdapter, and it's use below bluetoothAdapter.Adapter. ;P https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:49: // If unknown address was added, alert client Remove comment https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:60: // If known address was removed, alert client Remove comment https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapter.h File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:22: // and devices coming from the chrome://bluetooth-internals Remove the reference to internals page. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:15: mojom("experimental_interfaces") { Did you have to drop the visibility because of adding the interface in chrome_content_browser_client.cc? That's unfortunate. I'm not sure it's worth going back -- because it seems cleaner that way. ortuno, any preference?
https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/browser_... chrome/browser/browser_resources.grd:97: <include name="IDR_BLUETOOTH_INTERNALS_MOJO_JS" file="${root_gen_dir}\chrome\browser\ui\webui\bluetooth_internals\bluetooth_internals.mojom.js" use_base_dir="false" type="BINDATA"/> There is no bluetooth_internals.mojom anymore so I don't think you need this one. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:167: #include "device/bluetooth/adapter.h" I think you need to add the device/bluetooth dependency in chrome/browser/BUILD.gn https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:70: adapterClient = new AdapterClient(); nit: Move this next to where it's used. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:557: "//device/bluetooth:mojo", I don't think you need to depend on the implementation. The ui code only depends on the bindings. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:7: #include <utility> same here? https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h:8: #include <memory> I don't think you need this? https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/BUILD.gn File device/bluetooth/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/BUILD.... device/bluetooth/BUILD.gn:23: source_set("mojo") { Should this be called experimental_mojo, maybe? scheib? Also out of curiosity why is this a source_set rather than a component? I see examples of both in the codebase. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:10: #include "mojo/public/cpp/bindings/strong_binding.h" Also include: base/memory/ptr_util.h https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:31: device::BluetoothAdapterFactory::GetAdapter(base::Bind( optional: I think it might make more sense to call OnGetAdapter when you get the adapter. You can achieve this cleanly by encapsulating the call to GetDevicesImpl in a Closure, passing that Closure to OnGetAdapter and running that closure after at the end of OnGetAdapter (closures are a magical thing): Here: BluetoothAdapterFactory::GetAdapter(base::Bind(&Adapter::OnGetAdapter, weak_ptr_factory_.GetWeakPtr(), base::Bind(&Adapter::GetDevicesImpl, callback))); void Adapter::OnGetAdapter(scoped_refptr<device::BluetoothAdapter> adapter, base::Closure continue) { DCHECK(!adapter_.get()); adapter_ = adapter; adapter_->AddObserver(); continue.Run(); } void Adapter::GetDevicesImpl(const GetDevicesCallback& callback) { // ... } https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:58: std::string device_address = device->GetAddress(); I think this is unused? I'm surprised the compiler didn't complain. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:80: scoped_refptr<device::BluetoothAdapter> Adapter::GetAdapter() { nit: Seems like accessing adapter_ directly might be easier? https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapter.h File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:11: #include "base/strings/utf_string_conversions.h" I don't think you need this include in this header. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:13: #include "device/bluetooth/bluetooth_adapter_factory.h" I don't think you need this include in this header. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:15: #include "mojo/public/cpp/bindings/binding.h" I might be missing something but I don't think you need this include either. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:24: class Adapter : public mojom::Adapter, I like that the name is so short but I'm a bit nervous about calling both the implementation and the interface just "Adapter". I would prefer AdapterImpl. I remember scheib talking about naming earlier but I can't seem to find the comment. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:30: static void Create(mojom::AdapterRequest request); Please add comment mentioning what type of binding this creates. https://google.github.io/styleguide/cppguide.html#Function_Comments https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:34: nit: I would remove the line to make it obvious all belong under the "mojom::Adapter overrides:" comment. Same for the Observer overrides. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:55: // The current Bluetooth adapter nit: Period at the end of service. When I first joined the team I also always forgot to put periods :) https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:58: // The adapter client that listens to this service Same here. :) https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:15: mojom("experimental_interfaces") { On 2016/09/28 at 03:12:21, scheib wrote: > Did you have to drop the visibility because of adding the interface in chrome_content_browser_client.cc? That's unfortunate. I'm not sure it's worth going back -- because it seems cleaner that way. > > ortuno, any preference? Yeah. Having visibility would be nice. Do you get an error if you expose it only to //device/bluetooth:mojo and //chrome/browser/ui:ui? https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... device/bluetooth/public/interfaces/adapter.mojom:20: // Sets the client that listens for the adapter's events nit: Period at the end of sentences.
https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/browser_... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/browser_... chrome/browser/browser_resources.grd:97: <include name="IDR_BLUETOOTH_INTERNALS_MOJO_JS" file="${root_gen_dir}\chrome\browser\ui\webui\bluetooth_internals\bluetooth_internals.mojom.js" use_base_dir="false" type="BINDATA"/> On 2016/09/28 09:44:54, ortuno wrote: > There is no bluetooth_internals.mojom anymore so I don't think you need this > one. Done. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/chrome_c... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/chrome_c... chrome/browser/chrome_content_browser_client.cc:167: #include "device/bluetooth/adapter.h" On 2016/09/28 09:44:54, ortuno wrote: > I think you need to add the device/bluetooth dependency in > chrome/browser/BUILD.gn Done. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:62: .then(([frameInterfaces, bluetooth, connection]) => { On 2016/09/28 03:12:21, scheib wrote: > The 'bluetooth' returned module will be trouble later when we import multiple > mojom files all in the 'bluetooth' module - but they each only provide part of > that space. > > A code search shows that seperate mojom files are the right path, it will just > be ugly JS binding code until that's cleaned up. > > Likely the 'bluetooth' variable here will need to become bluetoothAdapter, and > it's use below bluetoothAdapter.Adapter. ;P Yeah. I realized this but left it the same because it's not an issue yet. I'll go ahead and change it. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:70: adapterClient = new AdapterClient(); On 2016/09/28 09:44:54, ortuno wrote: > nit: Move this next to where it's used. Done. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/BUILD.gn File chrome/browser/ui/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/BUILD... chrome/browser/ui/BUILD.gn:557: "//device/bluetooth:mojo", On 2016/09/28 09:44:54, ortuno wrote: > I don't think you need to depend on the implementation. The ui code only depends > on the bindings. Done. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:7: #include <utility> On 2016/09/28 09:44:55, ortuno wrote: > same here? Done. https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/webui... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h (right): https://codereview.chromium.org/2357383002/diff/40013/chrome/browser/ui/webui... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h:8: #include <memory> On 2016/09/28 09:44:55, ortuno wrote: > I don't think you need this? Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/BUILD.gn File device/bluetooth/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/BUILD.... device/bluetooth/BUILD.gn:23: source_set("mojo") { On 2016/09/28 09:44:55, ortuno wrote: > Should this be called experimental_mojo, maybe? scheib? > > Also out of curiosity why is this a source_set rather than a component? I see > examples of both in the codebase. I originally had it as a component, but it was giving me issues. I noticed device/usb/mojo used source_set so I changed it to that and it seemed to fix some problems I was having. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:10: #include "mojo/public/cpp/bindings/strong_binding.h" On 2016/09/28 09:44:55, ortuno wrote: > Also include: > > base/memory/ptr_util.h > Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:31: device::BluetoothAdapterFactory::GetAdapter(base::Bind( On 2016/09/28 09:44:55, ortuno wrote: > optional: I think it might make more sense to call OnGetAdapter when you get the > adapter. You can achieve this cleanly by encapsulating the call to > GetDevicesImpl in a Closure, passing that Closure to OnGetAdapter and running > that closure after at the end of OnGetAdapter (closures are a magical thing): > > Here: > > BluetoothAdapterFactory::GetAdapter(base::Bind(&Adapter::OnGetAdapter, > weak_ptr_factory_.GetWeakPtr(), base::Bind(&Adapter::GetDevicesImpl, > callback))); > > void Adapter::OnGetAdapter(scoped_refptr<device::BluetoothAdapter> adapter, > base::Closure continue) { > DCHECK(!adapter_.get()); > adapter_ = adapter; > adapter_->AddObserver(); > continue.Run(); > } > > void Adapter::GetDevicesImpl(const GetDevicesCallback& callback) { > // ... > } This is the exact feature I was looking for when I was originally coding this! Thanks! I didn't want to have to call OnGetAdapter in every function that needs access to the adapter. Unless I missed something, you're going to have to give me another example of this because I can't get your suggestion to compile. It errors out on GetAdapter(base::Bind) even though I changed the definition of the callback function. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:49: // If unknown address was added, alert client On 2016/09/28 03:12:21, scheib wrote: > Remove comment Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:58: std::string device_address = device->GetAddress(); On 2016/09/28 09:44:55, ortuno wrote: > I think this is unused? I'm surprised the compiler didn't complain. Yeah. That's unusual. Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:60: // If known address was removed, alert client On 2016/09/28 03:12:21, scheib wrote: > Remove comment Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.cc:80: scoped_refptr<device::BluetoothAdapter> Adapter::GetAdapter() { On 2016/09/28 09:44:55, ortuno wrote: > nit: Seems like accessing adapter_ directly might be easier? Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapter.h File device/bluetooth/adapter.h (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:11: #include "base/strings/utf_string_conversions.h" On 2016/09/28 09:44:55, ortuno wrote: > I don't think you need this include in this header. Yes. This should be in the cc. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:13: #include "device/bluetooth/bluetooth_adapter_factory.h" On 2016/09/28 09:44:55, ortuno wrote: > I don't think you need this include in this header. In the cc, also. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:15: #include "mojo/public/cpp/bindings/binding.h" On 2016/09/28 09:44:55, ortuno wrote: > I might be missing something but I don't think you need this include either. Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:22: // and devices coming from the chrome://bluetooth-internals On 2016/09/28 03:12:21, scheib wrote: > Remove the reference to internals page. Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:30: static void Create(mojom::AdapterRequest request); On 2016/09/28 09:44:55, ortuno wrote: > Please add comment mentioning what type of binding this creates. > > https://google.github.io/styleguide/cppguide.html#Function_Comments Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:34: On 2016/09/28 09:44:55, ortuno wrote: > nit: I would remove the line to make it obvious all belong under the > "mojom::Adapter overrides:" comment. Same for the Observer overrides. Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:55: // The current Bluetooth adapter On 2016/09/28 09:44:55, ortuno wrote: > nit: Period at the end of service. When I first joined the team I also always > forgot to put periods :) I always used to do it in Python but I haven't gotten the habit in C++ for some reason. One of these days. ¯\_(ツ)_/¯ https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/adapte... device/bluetooth/adapter.h:58: // The adapter client that listens to this service On 2016/09/28 09:44:55, ortuno wrote: > Same here. :) Done. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:15: mojom("experimental_interfaces") { On 2016/09/28 09:44:55, ortuno wrote: > On 2016/09/28 at 03:12:21, scheib wrote: > > Did you have to drop the visibility because of adding the interface in > chrome_content_browser_client.cc? That's unfortunate. I'm not sure it's worth > going back -- because it seems cleaner that way. > > > > ortuno, any preference? > > Yeah. Having visibility would be nice. Do you get an error if you expose it only > to //device/bluetooth:mojo and //chrome/browser/ui:ui? I had visibility in at one point but it probably got dropped when I pulled some changes from another branch. I should be able to get it to build with the chrome content browser client change if I specifically target the //chrome/browser:browser target. I shouldn't have to put it in //chrome/browser/ui because there's no Mojo code in there. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... device/bluetooth/public/interfaces/adapter.mojom:20: // Sets the client that listens for the adapter's events On 2016/09/28 09:44:55, ortuno wrote: > nit: Period at the end of sentences. 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...
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:15: mojom("experimental_interfaces") { On 2016/09/28 21:20:35, mbrunson wrote: > On 2016/09/28 09:44:55, ortuno wrote: > > On 2016/09/28 at 03:12:21, scheib wrote: > > > Did you have to drop the visibility because of adding the interface in > > chrome_content_browser_client.cc? That's unfortunate. I'm not sure it's worth > > going back -- because it seems cleaner that way. > > > > > > ortuno, any preference? > > > > Yeah. Having visibility would be nice. Do you get an error if you expose it > only > > to //device/bluetooth:mojo and //chrome/browser/ui:ui? > > I had visibility in at one point but it probably got dropped when I pulled some > changes from another branch. > > I should be able to get it to build with the chrome content browser client > change if I specifically target the //chrome/browser:browser target. I shouldn't > have to put it in //chrome/browser/ui because there's no Mojo code in there. Nice to have it back, sort of, but having chrome/browser:browser also sort of defeats the point a bit... it's pretty broad access. Better than nothing, though.
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...
LGTM pending ortuno's review as well. I think we can fix up additional nits as we go forward. Please add a webui owner who's been active recently - ask for help w.r.t. the JS style errors. (also study guide / other pages to see if you can sort it out).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/BUILD.gn File device/bluetooth/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/BUILD.... device/bluetooth/BUILD.gn:23: source_set("mojo") { On 2016/09/28 at 21:20:35, mbrunson wrote: > On 2016/09/28 09:44:55, ortuno wrote: > > Should this be called experimental_mojo, maybe? scheib? > > > > Also out of curiosity why is this a source_set rather than a component? I see > > examples of both in the codebase. > > I originally had it as a component, but it was giving me issues. I noticed device/usb/mojo used source_set so I changed it to that and it seemed to fix some problems I was having. Ideally this would be a component so that users wouldn't have to recompile it. But I see that all interfaces that are used by //chrome are static libraries so I think we have no choice :( https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:15: mojom("experimental_interfaces") { On 2016/09/28 at 21:20:35, mbrunson wrote: > On 2016/09/28 09:44:55, ortuno wrote: > > On 2016/09/28 at 03:12:21, scheib wrote: > > > Did you have to drop the visibility because of adding the interface in > > chrome_content_browser_client.cc? That's unfortunate. I'm not sure it's worth > > going back -- because it seems cleaner that way. > > > > > > ortuno, any preference? > > > > Yeah. Having visibility would be nice. Do you get an error if you expose it only > > to //device/bluetooth:mojo and //chrome/browser/ui:ui? > > I had visibility in at one point but it probably got dropped when I pulled some changes from another branch. > > I should be able to get it to build with the chrome content browser client change if I specifically target the //chrome/browser:browser target. I shouldn't have to put it in //chrome/browser/ui because there's no Mojo code in there. True, there is no mojo code in there, but chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc still needs the bindings generated. I don't think we want chrome/browser:browser to have direct access to the bindings. It already depends on mojo.
also lgtm bar the visibility issue.
Description was changed from ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds functionality to Mojo's connection.js for sending interface request's without a callback. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG= ========== to ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds functionality to Mojo's connection.js for sending interface request's without a callback. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 ==========
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/BUILD.gn File device/bluetooth/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/BUILD.... device/bluetooth/BUILD.gn:23: source_set("mojo") { On 2016/09/29 01:43:06, ortuno wrote: > On 2016/09/28 at 21:20:35, mbrunson wrote: > > On 2016/09/28 09:44:55, ortuno wrote: > > > Should this be called experimental_mojo, maybe? scheib? > > > > > > Also out of curiosity why is this a source_set rather than a component? I > see > > > examples of both in the codebase. > > > > I originally had it as a component, but it was giving me issues. I noticed > device/usb/mojo used source_set so I changed it to that and it seemed to fix > some problems I was having. > > Ideally this would be a component so that users wouldn't have to recompile it. > But I see that all interfaces that are used by //chrome are static libraries so > I think we have no choice :( Hmm...I changed it into a component and added the defines and flags but I'm still getting linker errors for bluetooth::mojom::Adapter::Name_. I'll look into this some more tomorrow. https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/40013/device/bluetooth/public... device/bluetooth/public/interfaces/BUILD.gn:15: mojom("experimental_interfaces") { On 2016/09/29 01:43:06, ortuno wrote: > On 2016/09/28 at 21:20:35, mbrunson wrote: > > On 2016/09/28 09:44:55, ortuno wrote: > > > On 2016/09/28 at 03:12:21, scheib wrote: > > > > Did you have to drop the visibility because of adding the interface in > > > chrome_content_browser_client.cc? That's unfortunate. I'm not sure it's > worth > > > going back -- because it seems cleaner that way. > > > > > > > > ortuno, any preference? > > > > > > Yeah. Having visibility would be nice. Do you get an error if you expose it > only > > > to //device/bluetooth:mojo and //chrome/browser/ui:ui? > > > > I had visibility in at one point but it probably got dropped when I pulled > some changes from another branch. > > > > I should be able to get it to build with the chrome content browser client > change if I specifically target the //chrome/browser:browser target. I shouldn't > have to put it in //chrome/browser/ui because there's no Mojo code in there. > > True, there is no mojo code in there, but > chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc still > needs the bindings generated. > > I don't think we want chrome/browser:browser to have direct access to the > bindings. It already depends on mojo. I've restricted the visibility to bluetooth_internals and device/bluetooth:mojo and that seems to compile. I guess it doesn't matter if //chrome/browser/BUILD.gn depends on device/bluetooth:mojo which depends on the interfaces. As long as it doesn't depend directly on it, it's fine I guess??
https://codereview.chromium.org/2357383002/diff/120001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/120001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:1404: "//device/bluetooth:mojo", You also need a separate deps for //device/bluetooth/public/interfaces:experimental_interfaces in chrome/browser/ui/BUILD.gn because that code depends on the bindings being generated. https://codereview.chromium.org/2357383002/diff/120001/device/bluetooth/publi... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/120001/device/bluetooth/publi... device/bluetooth/public/interfaces/BUILD.gn:22: "//chrome/browser/ui/webui/bluetooth_internals:*", Interesting that gn doesn't complain here. There are no targets in //chrome/browser/ui/webui/bluetooth_internals. You want to expose this only to the ui target: //chrome/browser/ui:ui
https://codereview.chromium.org/2357383002/diff/120001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/120001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:1404: "//device/bluetooth:mojo", On 2016/09/29 03:01:12, ortuno wrote: > You also need a separate deps for > //device/bluetooth/public/interfaces:experimental_interfaces in > chrome/browser/ui/BUILD.gn because that code depends on the bindings being > generated. Done. https://codereview.chromium.org/2357383002/diff/120001/device/bluetooth/publi... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/120001/device/bluetooth/publi... device/bluetooth/public/interfaces/BUILD.gn:22: "//chrome/browser/ui/webui/bluetooth_internals:*", On 2016/09/29 03:01:12, ortuno wrote: > Interesting that gn doesn't complain here. There are no targets in > //chrome/browser/ui/webui/bluetooth_internals. You want to expose this only to > the ui target: > > //chrome/browser/ui:ui > Yes. It's strange. Maybe visibility doesn't work the way we think it does?
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mbrunson@chromium.org changed reviewers: + dbeam@chromium.org, dcheng@chromium.org, jam@chromium.org
OWNERS review, please jam: chrome/browser/browser_resources.grd chrome/browser/resources/bluetooth_internals/bluetooth_internals.js chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc device/bluetooth/adapter.cc device/bluetooth/adapter.h device/bluetooth/public/interfaces/BUILD.gn third_party/WebKit/public/BUILD.gn dbeam: Can you help me figure out why bluetooth_internals.js isn't passing the presubmit style check? 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 third_party/WebKit/public/BUILD.gn
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/09/29 20:03:54, mbrunson wrote: > > dbeam: > Can you help me figure out why bluetooth_internals.js isn't passing the > presubmit style check? because you're using ES6
On 2016/09/29 22:25:50, Dan Beam wrote: > On 2016/09/29 20:03:54, mbrunson wrote: > > > > dbeam: > > Can you help me figure out why bluetooth_internals.js isn't passing the > > presubmit style check? > > because you're using ES6 So, should I not use ES6 at all?
On 2016/09/29 22:28:32, mbrunson wrote: > On 2016/09/29 22:25:50, Dan Beam wrote: > > On 2016/09/29 20:03:54, mbrunson wrote: > > > > > > dbeam: > > > Can you help me figure out why bluetooth_internals.js isn't passing the > > > presubmit style check? > > > > because you're using ES6 > > So, should I not use ES6 at all? Chrome's in a funky spot with ES6. v8 is leading the way, but our tooling (closure linter/compiler) might be a little behind AND we also run on iOS (which is just like mobile Safari). so support might be dodgy. fwiw: https://bugs.chromium.org/p/chromium/issues/detail?id=567770
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...
On 2016/09/29 22:35:59, Dan Beam wrote: > On 2016/09/29 22:28:32, mbrunson wrote: > > On 2016/09/29 22:25:50, Dan Beam wrote: > > > On 2016/09/29 20:03:54, mbrunson wrote: > > > > > > > > dbeam: > > > > Can you help me figure out why bluetooth_internals.js isn't passing the > > > > presubmit style check? > > > > > > because you're using ES6 > > > > So, should I not use ES6 at all? > > Chrome's in a funky spot with ES6. v8 is leading the way, but our tooling > (closure linter/compiler) might be a little behind AND we also run on iOS (which > is just like mobile Safari). so support might be dodgy. > > fwiw: https://bugs.chromium.org/p/chromium/issues/detail?id=567770 Ok. I've removed the ES6 stuff, so that should be good to go.
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
https://codereview.chromium.org/2357383002/diff/120001/device/bluetooth/publi... File device/bluetooth/public/interfaces/BUILD.gn (right): https://codereview.chromium.org/2357383002/diff/120001/device/bluetooth/publi... device/bluetooth/public/interfaces/BUILD.gn:22: "//chrome/browser/ui/webui/bluetooth_internals:*", On 2016/09/29 18:00:09, mbrunson wrote: > On 2016/09/29 03:01:12, ortuno wrote: > > Interesting that gn doesn't complain here. There are no targets in > > //chrome/browser/ui/webui/bluetooth_internals. You want to expose this only to > > the ui target: > > > > //chrome/browser/ui:ui > > > > Yes. It's strange. Maybe visibility doesn't work the way we think it does? Does it throw an error if the lines are removed? https://codereview.chromium.org/2357383002/diff/220001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2357383002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:1263: Profile* profile = Is this a merge glitch? git map-branches will show the branch your feature work is tracking -- did you merge from origin/master into the feature branch without first merging into the branch the feature is tracking? https://codereview.chromium.org/2357383002/diff/220001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2992: ChromeInterfaceRegistrarAndroid::ExposeInterfacesToFrame(registry, Merge glitch?
https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/adapt... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/adapt... device/bluetooth/adapter.cc:21: if (adapter_.get()) { Nit: no .get() here and elsewhere https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/adapt... device/bluetooth/adapter.cc:39: &Adapter::OnGetAdapter, weak_ptr_factory_.GetWeakPtr(), c)); Three layer deep callback wrapping, fun. https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/publi... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:18: GetDevices() => (array<DeviceInfo> devices); My main question here is whether it'd make sense to just have a Device interface. I'm not sure how AdapterClient is going to be implemented in a followup, but I'm guessing we're going to be doing lookup by the DeviceInfo? If so, would it make more sense to just transfer a mojo handle to the Device interface and eliminate/simplify the lookup? https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:21: SetClient(AdapterClient client); Can we add this (and AdapterClient) in the CL where they're actually implemented and used?
On 2016/09/29 20:03:54, mbrunson wrote: > OWNERS review, please > > jam: > chrome/browser/browser_resources.grd > chrome/browser/resources/bluetooth_internals/bluetooth_internals.js > chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc > device/bluetooth/adapter.cc > device/bluetooth/adapter.h > device/bluetooth/public/interfaces/BUILD.gn > third_party/WebKit/public/BUILD.gn please get more specific owners, i.e. someone from chrome/owners and device/bluetooth/owners and webkit/owners > > dbeam: > Can you help me figure out why bluetooth_internals.js isn't passing the > presubmit style check? > > 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 > third_party/WebKit/public/BUILD.gn
https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/publi... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:21: SetClient(AdapterClient client); On 2016/09/30 07:04:06, dcheng wrote: > Can we add this (and AdapterClient) in the CL where they're actually implemented > and used? Fair point, but at this point in review I don't think it's worth splitting out. This patch establishes the boilerplate for the Adapter interface and its client, including the client doesn't bloat it that much and was important to figure out the context for the bindings.
On 2016/09/30 16:52:42, scheib wrote: > https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/publi... > File device/bluetooth/public/interfaces/adapter.mojom (right): > > https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/publi... > device/bluetooth/public/interfaces/adapter.mojom:21: SetClient(AdapterClient > client); > On 2016/09/30 07:04:06, dcheng wrote: > > Can we add this (and AdapterClient) in the CL where they're actually > implemented > > and used? > > Fair point, but at this point in review I don't think it's worth splitting out. > This patch establishes the boilerplate for the Adapter interface and its client, > including the client doesn't bloat it that much and was important to figure out > the context for the bindings. Unfortunately, it's not really possible to do a security review without any context of how the interface is used / implemented, hence my request. We can go either way, it's just usually easier to review patches that are smaller rather than bigger.
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: - jam@chromium.org
On 2016/09/30 16:25:41, jam wrote: > On 2016/09/29 20:03:54, mbrunson wrote: > > OWNERS review, please > > > > jam: > > chrome/browser/browser_resources.grd > > chrome/browser/resources/bluetooth_internals/bluetooth_internals.js > > chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc > > device/bluetooth/adapter.cc > > device/bluetooth/adapter.h > > device/bluetooth/public/interfaces/BUILD.gn > > third_party/WebKit/public/BUILD.gn > > please get more specific owners, i.e. someone from chrome/owners and > device/bluetooth/owners and webkit/owners > > > > dbeam: > > Can you help me figure out why bluetooth_internals.js isn't passing the > > presubmit style check? > > > > 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 > > third_party/WebKit/public/BUILD.gn I have some overlap with owners here, so I didn't need to add you. Sorry about that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/adapt... File device/bluetooth/adapter.cc (right): https://codereview.chromium.org/2357383002/diff/220001/device/bluetooth/adapt... device/bluetooth/adapter.cc:21: if (adapter_.get()) { On 2016/09/30 07:04:06, dcheng wrote: > Nit: no .get() here and elsewhere Done.
Per the offline discussion, we'll stick with the current interface. For actually communicating with devices, we'll have a Device interface, so we can centralize the DeviceInfo checks on the service broker side in the browser. mojo LGTM
Description was changed from ========== Add device list retrieval using Bluetooth service for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds functionality to Mojo's connection.js for sending interface request's without a callback. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 ========== to ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds functionality to Mojo's connection.js for sending interface request's without a callback. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 ==========
The CQ bit was checked by scheib@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
dbeam@chromium.org changed reviewers: + dpapad@chromium.org - dbeam@chromium.org
+dpapad@ for mojo-based webui
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 CL description mentions changes to Mojo's connection.js, but I don't see any such changes. Is that sentence obsolete? https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:21: console.log('Device added'); Are those console.logs temporary? https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:45: function importModules(moduleNames) { This function is repeated 3 times (and now 4), across the codebase, see https://cs.chromium.org/search/?q=importModules+lang:js&sq=package:chromium&t.... We should probably put it in as shared location and re-use. Can you add a TODO and/or file a bug about this, so that we don't loose track? https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:59: 'content/public/renderer/frame_interfaces', Indentation seems quite a bit off here. Shouldn't it be as follows? return importModules([ 'content/public/renderer/frame_interfaces', ... ]).then(function(modules) { ... }); https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:64: var frameInterfaces = modules[0]; Nit: I believe that you can use destructuring assignment now (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/D...) var frameInterfaces, bluetoothAdapter, connection; [frameInterfaces, bluetoothAdapter, connection] = modules; https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:75: // implementation Nit: Comments should have proper punctuation. https://codereview.chromium.org/2357383002/diff/260001/device/bluetooth/publi... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2357383002/diff/260001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:20: // Sets the client that listens for the adapter's events. Optional: It is not clear at all that the "client" is the WebUI page here. Can we use the same terminology as plugins.mojom, meaning setClientPage()?. See https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/plugins/plugins..... Also I suggest changing the "Adapter" generic names as follows Adapter -> BluetoothPageHandler AdapterClient -> BluetoothPage I think this makes it much clearer (self-documenting) who is the C++ handler, who is the WebUI page.
Description was changed from ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds functionality to Mojo's connection.js for sending interface request's without a callback. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 ========== to ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 ==========
https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:21: console.log('Device added'); On 2016/10/03 17:51:03, dpapad wrote: > Are those console.logs temporary? Yes. They are temporary. https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:45: function importModules(moduleNames) { On 2016/10/03 17:51:03, dpapad wrote: > This function is repeated 3 times (and now 4), across the codebase, see > https://cs.chromium.org/search/?q=importModules+lang:js&sq=package:chromium&t.... > We should probably put it in as shared location and re-use. Can you add a TODO > and/or file a bug about this, so that we don't loose track? I've made a bug for this and added a TODO: crbug.com/652361 https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:59: 'content/public/renderer/frame_interfaces', On 2016/10/03 17:51:03, dpapad wrote: > Indentation seems quite a bit off here. Shouldn't it be as follows? > > return importModules([ > 'content/public/renderer/frame_interfaces', > ... > ]).then(function(modules) { > ... > }); Done. https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:64: var frameInterfaces = modules[0]; On 2016/10/03 17:51:03, dpapad wrote: > Nit: I believe that you can use destructuring assignment now > (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/D...) > > var frameInterfaces, bluetoothAdapter, connection; > [frameInterfaces, bluetoothAdapter, connection] = modules; Done. https://codereview.chromium.org/2357383002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:75: // implementation On 2016/10/03 17:51:03, dpapad wrote: > Nit: Comments should have proper punctuation. Done. https://codereview.chromium.org/2357383002/diff/260001/device/bluetooth/publi... File device/bluetooth/public/interfaces/adapter.mojom (right): https://codereview.chromium.org/2357383002/diff/260001/device/bluetooth/publi... device/bluetooth/public/interfaces/adapter.mojom:20: // Sets the client that listens for the adapter's events. On 2016/10/03 17:51:03, dpapad wrote: > Optional: It is not clear at all that the "client" is the WebUI page here. Can > we use the same terminology as plugins.mojom, meaning setClientPage()?. See > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/plugins/plugins..... > > Also I suggest changing the "Adapter" generic names as follows > > Adapter -> BluetoothPageHandler > AdapterClient -> BluetoothPage > > I think this makes it much clearer (self-documenting) who is the C++ handler, > who is the WebUI page. This Mojo interface will not be used exclusively by the WebUI. The plan is for it to become part of a set of interfaces that all developers will use to communicate with device/bluetooth. That's why it's named this way.
LGTM
The CQ bit was checked by scheib@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 Link to the patchset: https://codereview.chromium.org/2357383002/#ps280001 (title: "Style changes, destructuring in JS")
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 list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 ========== to ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 ========== to ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 Committed: https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9 Cr-Commit-Position: refs/heads/master@{#422570} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9 Cr-Commit-Position: refs/heads/master@{#422570}
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:280001) has been created in https://codereview.chromium.org/2389053002/ by horo@chromium.org. The reason for reverting is: generate_build_files failing on chromium.chrome/Google Chrome Win BUG=652494 C:\b\c\b\win_chrome\src\buildtools\win\gn.exe gen //out/Release --check -> returned 1 ERROR at //build/split_static_library.gni:27:7: Dependency not allowed. static_library(current_name) { ^----------------------------- The item //chrome/browser/ui:ui_0 can not depend on //device/bluetooth/public/interfaces:experimental_interfaces because it is not in //device/bluetooth/public/interfaces:experimental_interfaces's visibility list: [ //device/bluetooth:mojo //chrome/browser:browser //chrome/browser/ui:ui ] GN gen failed: 1 step returned non-zero exit code: 1 @@@STEP_FAILURE@@@ .
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:280001) has been created in https://codereview.chromium.org/2390933002/ by jaydasika@chromium.org. The reason for reverting is: Speculative revert for build failure : https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/b....
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 Committed: https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9 Cr-Commit-Position: refs/heads/master@{#422570} ========== to ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 Committed: https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9 Cr-Commit-Position: refs/heads/master@{#422570} ==========
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...
<•scheib> Did the speculative revert fix the build? <horo_> yes <horo_> https://uberchromegw.corp.google.com/i/chromium.linux/builders/Linux%20Tests%... <•jaydasika> https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/b...
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 Link to the patchset: https://codereview.chromium.org/2357383002/#ps300001 (title: "Fix Windows BUILD visibility")
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 list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 Committed: https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9 Cr-Commit-Position: refs/heads/master@{#422570} ========== to ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 Committed: https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9 Cr-Commit-Position: refs/heads/master@{#422570} ==========
Message was sent while issue was closed.
Committed patchset #17 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 Committed: https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9 Cr-Commit-Position: refs/heads/master@{#422570} ========== to ========== bluetooth: Add device list retrieval for chrome://bluetooth-internals Changes WebUI setup to a MojoWebUI for chrome://bluetooth-internals page. Adds mojom files for Bluetooth service definition and internals page handler. Adds basic Bluetooth device retrieval using starter Bluetooth service implementation for logging on web front end. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation BUG=651282 Committed: https://crrev.com/e619ab633a89f1c845f5fdd7584344a5914b30a9 Committed: https://crrev.com/70d50e7a3e3289bfce518ae91a85759b40469642 Cr-Original-Commit-Position: refs/heads/master@{#422570} Cr-Commit-Position: refs/heads/master@{#422855} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/70d50e7a3e3289bfce518ae91a85759b40469642 Cr-Commit-Position: refs/heads/master@{#422855} |
