Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446823002:
bluetooth: Componentize device list in chrome://bluetooth-internals. (Closed)
Descriptionbluetooth: Componentize device list in chrome://bluetooth-internals.
Separates device list HTML management code from device handling code.
Adds DeviceTable for management of device table UI.
Adds observable DeviceCollection for dynamic updates of DeviceTable.
Adds AdapterBroker to handle adapter events and simplifier Adapter
service communication.
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
TBR=dbeam@chromium.org
Committed: https://crrev.com/d5d3edafbb53f7af22875e0c274d75bc1d84b430
Cr-Commit-Position: refs/heads/master@{#430458}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Separate device.js and bluetooth_internals.js #
Total comments: 2
Patch Set 3 : Simplify interfaces #Patch Set 4 : Merge upstream, use importModules in util.js #Patch Set 5 : Remove extra table #Patch Set 6 : Add AdapterBroker, route AdapterClient events #Patch Set 7 : Fix upstream #Patch Set 8 : Clean up comments, HTML scripts #Patch Set 9 : removeRow->deleteRow #
Total comments: 4
Patch Set 10 : Add adapter_broker module #Patch Set 11 : Fix formatting #Patch Set 12 : Clean up bluetooth_internals, formatting #Patch Set 13 : Formatting changes, comments #
Total comments: 12
Patch Set 14 : Fix issues, add comments, move Adapter code to AdapterBroker #
Total comments: 8
Patch Set 15 : Rename functions, fix comments #
Total comments: 14
Patch Set 16 : Comments, exports, and CustomEvents #
Total comments: 12
Patch Set 17 : Add @private, fix comments #Patch Set 18 : Change innerText to textContent in device table #
Total comments: 42
Dependent Patchsets: Messages
Total messages: 56 (29 generated)
Description was changed from ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. BUG=651282 ========== to ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mbrunson@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
https://codereview.chromium.org/2446823002/diff/1/chrome/browser/resources/bl... File chrome/browser/resources/bluetooth_internals/device.js (right): https://codereview.chromium.org/2446823002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/device.js:10: cr.define('device', function() { Split the DeviceTable code into a separate file. We should try to keep UI code separated and give the modules more meaningful names e.g. device-collection, device-table, etc. https://codereview.chromium.org/2446823002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/device.js:164: update: function(device) { Would it make sense to move all of the device info logic here? You would have only two functions and no need to expose Device: addOrUpdate(DeviceInfo device_info) which would get the device from the list and update it's fields. remove(DeviceInfo device_info) would mark the device as removed. Also I think a Design doc would be useful here to get a sense of the overall structure of the list and how you plan implementing services, characteristics, etc.
Ping me if you'd like a review before you address ortuno's comments.
https://codereview.chromium.org/2446823002/diff/1/chrome/browser/resources/bl... File chrome/browser/resources/bluetooth_internals/device.js (right): https://codereview.chromium.org/2446823002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/device.js:10: cr.define('device', function() { On 2016/10/26 03:26:11, ortuno wrote: > Split the DeviceTable code into a separate file. We should try to keep UI code > separated and give the modules more meaningful names e.g. device-collection, > device-table, etc. Done. https://codereview.chromium.org/2446823002/diff/1/chrome/browser/resources/bl... chrome/browser/resources/bluetooth_internals/device.js:164: update: function(device) { On 2016/10/26 03:26:11, ortuno wrote: > Would it make sense to move all of the device info logic here? You would have > only two functions and no need to expose Device: > > addOrUpdate(DeviceInfo device_info) which would get the device from the list and > update it's fields. > > remove(DeviceInfo device_info) would mark the device as removed. > > Also I think a Design doc would be useful here to get a sense of the overall > structure of the list and how you plan implementing services, characteristics, > etc. Hmm...ok. A few other places could have better separation after these suggestions. I created global access to the Mojo interfaces for better separation in the future. That way I can put all the Device connection code in the device_collection.Device class[1]. [1] https://codereview.chromium.org/2448713002/diff/60001/chrome/browser/resource...
On 2016/10/28 20:22:17, scheib wrote: > Ping me if you'd like a review before you address ortuno's comments. I've made some big organizational changes which will hopefully allow better separation of concerns between code modules. So, you can review now.
LGTM, but there's little value I add for the JS review. https://codereview.chromium.org/2446823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2446823002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:10: cr.define('bluetooth_internals', function() { pardon my ignorance, but isn't part of the point of these define modules to list out the other required components? e.g. device_collection and device_table?
https://codereview.chromium.org/2446823002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2446823002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:10: cr.define('bluetooth_internals', function() { On 2016/10/31 20:24:36, scheib wrote: > pardon my ignorance, but isn't part of the point of these define modules to list > out the other required components? e.g. device_collection and device_table? There's three kinds of defines that I've seen used in WebUI: local module, ui module, and general module ("cr.define", "cr.ui.define", and just "define", respectively). cr.define just creates a global variable on the window object. cr.ui.define creates a class that represents an HTML element. And plain old define is used to import Mojom file dependencies. It can be very confusing...
mbrunson@chromium.org changed reviewers: + dpapad@chromium.org
OWNERS review for WebUI, please.
Description was changed from ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
mbrunson@chromium.org changed reviewers: - dpapad@chromium.org
Description was changed from ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. Adds AdapterBroker to handle adapter events and simplifier Adapter service communication. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2446823002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2446823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:110: adapterBroker = new AdapterBroker(adapter); Would it make sense to: 1. Put AdapterBroker in its own module. 2. Expose an getAdapterBroker() function in that module that would initialize the mojom stuff, acquire an adapter, set the client and return an AdapterBroker. 3. Roll interfaces into the adapter-broker modules since bluetooth-internals doesn't need to know anything about it. ? https://codereview.chromium.org/2446823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:129: devices /** this */); nit: /** => /*
https://codereview.chromium.org/2446823002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2446823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:110: adapterBroker = new AdapterBroker(adapter); On 2016/11/03 04:58:21, ortuno wrote: > Would it make sense to: > > 1. Put AdapterBroker in its own module. > 2. Expose an getAdapterBroker() function in that module that would initialize > the mojom stuff, acquire an adapter, set the client and return an AdapterBroker. > 3. Roll interfaces into the adapter-broker modules since bluetooth-internals > doesn't need to know anything about it. > > ? I can do 1 and 2 relatively easily. Not sure about 3 though. Eventually, there will be a point where other classes will need to know about some of the interfaces. For example, Device connection logic needs to know the ConnectResult codes to handle connection errors. If I did roll interfaces into the adapter_broker module, I'd have to put all of the Device-related interface code in the AdapterBroker instead of in the Device class. Seems like that would be giving AdapterBroker too much to do. https://codereview.chromium.org/2446823002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:129: devices /** this */); On 2016/11/03 04:58:21, ortuno wrote: > nit: /** => /* 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...
https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_collection.js (right): https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:54: this.push(device); nit: I would just do this.push(new Device(deviceInfo)); https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:65: 'Device does not exist.'); nit: Why is this on a different line? https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:82: DeviceCollection, DeviceCollection, qq: What are you returning here? Shouldn't this be: DeviceCollection: DeviceCollection? https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_table.js (right): https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:42: if (this.devices_) { Are you planning on being able to replace the devices collection? If not consider just making this an assert. https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:12: * Initializes Mojo proxies for page and Bluetooth services. Also mention that the interfaces are added to the interfaces object. https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:30: var adapterFactory = connection.bindHandleToProxy( Could you move the adapter code to the broker?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_collection.js (right): https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:54: this.push(device); On 2016/11/03 22:14:15, ortuno wrote: > nit: I would just do this.push(new Device(deviceInfo)); Done. https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:65: 'Device does not exist.'); On 2016/11/03 22:14:15, ortuno wrote: > nit: Why is this on a different line? Should be on same line. Done. https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:82: DeviceCollection, DeviceCollection, On 2016/11/03 22:14:15, ortuno wrote: > qq: What are you returning here? Shouldn't this be: DeviceCollection: > DeviceCollection? Must have hit a key by accident or something. Should return DeviceCollection. Done. https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_table.js (right): https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:42: if (this.devices_) { On 2016/11/03 22:14:15, ortuno wrote: > Are you planning on being able to replace the devices collection? If not > consider just making this an assert. I originally had this for easier testing, but I just gave the test access to the original DeviceCollection, so I'll just use assert. Done. https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:12: * Initializes Mojo proxies for page and Bluetooth services. On 2016/11/03 22:14:15, ortuno wrote: > Also mention that the interfaces are added to the interfaces object. Done. https://codereview.chromium.org/2446823002/diff/240001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:30: var adapterFactory = connection.bindHandleToProxy( On 2016/11/03 22:14:15, ortuno wrote: > Could you move the adapter code to the broker? Done.
lgtm bar some better naming https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:42: initialize: initialize Maybe initializeViews https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:12: * Initializes Mojo proxies for page and Bluetooth services. Adds Mojo Fix comment since this is no longer initializing proxies. https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:16: function initializeProxies() { nit: rename this to retrieve interfaces since you are no longer initializing proxies. https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:34: initialize: initializeProxies, Maybe retrieve since you are no longer initializing anything.
https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:42: initialize: initialize On 2016/11/04 01:44:09, ortuno wrote: > Maybe initializeViews Done. https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:12: * Initializes Mojo proxies for page and Bluetooth services. Adds Mojo On 2016/11/04 01:44:09, ortuno wrote: > Fix comment since this is no longer initializing proxies. Done. https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:16: function initializeProxies() { On 2016/11/04 01:44:10, ortuno wrote: > nit: rename this to retrieve interfaces since you are no longer initializing > proxies. Done. https://codereview.chromium.org/2446823002/diff/260001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:34: initialize: initializeProxies, On 2016/11/04 01:44:10, ortuno wrote: > Maybe retrieve since you are no longer initializing anything. Done.
mbrunson@chromium.org changed reviewers: + dpapad@chromium.org
WebUI OWNERS review, please.
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam for device_table.js, which uses some WebUI infrastructure I am not familiar with. https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right): https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:73: var event = new Event('deviceadded'); How about using a CustomEvent instead? Seems more appropriate than augmenting |deviceInfo| on a standard Event instance. See example at https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent. https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:93: console.log(new Date(), deviceInfo); Can we remove this now? https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_collection.js (right): https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:10: cr.define('device_collection', function() { Shouldn't all namespaces for code under bluetooth_internals/ have something like 'bluetooth_internal' as a prefix? What prevents this from colliding with a 'device_collection' namespace outside of bluetooth_internals/ ? https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:14: * @param {!Array} array the starting collection of devices. s/the/The https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:18: cr.ui.ArrayDataModel.call(this, array); Can you explain a bit why you need to extend ArrayDataModel? https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:24: BluetoothAdapter: bluetoothAdapter, FYI, If this code was to every be compiled by Closure Compiler, this would definitely confuse the compiler. 'interfaces' module only exports importInterfaces() as far as the compiler understands.
https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right): https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:73: var event = new Event('deviceadded'); On 2016/11/04 17:19:59, dpapad wrote: > How about using a CustomEvent instead? Seems more appropriate than augmenting > |deviceInfo| on a standard Event instance. See example at > https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent. Done. https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:93: console.log(new Date(), deviceInfo); On 2016/11/04 17:19:59, dpapad wrote: > Can we remove this now? Done. https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_collection.js (right): https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:10: cr.define('device_collection', function() { On 2016/11/04 17:20:00, dpapad wrote: > Shouldn't all namespaces for code under bluetooth_internals/ have something like > 'bluetooth_internal' as a prefix? What prevents this from colliding with a > 'device_collection' namespace outside of bluetooth_internals/ ? I mainly did this for ease of use in other files. Naming collisions seemed like a very low possiblility given how cr.define works unless I'm missing something? When I made these files I searched around and didn't find an established pattern in WebUI resources for cr.define. Some directories just use the directory name most of the time [1], some use what you suggested [2], and some just use the file name [3]. There's probably a few other styles but I figured [3] reduced the amount of typing to reference other objects. If your way is generally prefered, let me know and I'll change it. [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/hotword/trainin... [2] https://cs.chromium.org/chromium/src/chrome/browser/resources/inline_login/in... [3] https://cs.chromium.org/chromium/src/chrome/browser/resources/welcome/win10/s... https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:14: * @param {!Array} array the starting collection of devices. On 2016/11/04 17:20:00, dpapad wrote: > s/the/The Done. https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:18: cr.ui.ArrayDataModel.call(this, array); On 2016/11/04 17:20:00, dpapad wrote: > Can you explain a bit why you need to extend ArrayDataModel? ArrayDataModel provides a set of functions and events that notifies observers when the array changes. I used it to prevent a tight coupling between DeviceCollection and DeviceTable. I wanted a separation of concerns between data and UI. I'll add a comment for this. https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:24: BluetoothAdapter: bluetoothAdapter, On 2016/11/04 17:20:00, dpapad wrote: > FYI, If this code was to every be compiled by Closure Compiler, this would > definitely confuse the compiler. 'interfaces' module only exports > importInterfaces() as far as the compiler understands. I've exported some empty objects and the test passes. 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...
LGTM with nits. Please wait for @dbeam's approval too. https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_collection.js (right): https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:10: cr.define('device_collection', function() { On 2016/11/04 at 22:57:40, mbrunson wrote: > On 2016/11/04 17:20:00, dpapad wrote: > > Shouldn't all namespaces for code under bluetooth_internals/ have something like > > 'bluetooth_internal' as a prefix? What prevents this from colliding with a > > 'device_collection' namespace outside of bluetooth_internals/ ? > > I mainly did this for ease of use in other files. Naming collisions seemed like a very low possiblility given how cr.define works unless I'm missing something? > > When I made these files I searched around and didn't find an established pattern in WebUI resources for cr.define. Some directories just use the directory name most of the time [1], some use what you suggested [2], and some just use the file name [3]. There's probably a few other styles but I figured [3] reduced the amount of typing to reference other objects. If your way is generally prefered, let me know and I'll change it. > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/resources/hotword/trainin... > > [2] https://cs.chromium.org/chromium/src/chrome/browser/resources/inline_login/in... > > [3] > https://cs.chromium.org/chromium/src/chrome/browser/resources/welcome/win10/s... I see, there is 3 competing patterns for namespace naming, so either way is fine with me. Hoping that we do have a WebUI styleguide laying down the rules some time soon. FWIW, I think approach [1] is best of both worlds, minimal typing and simple, just one flat namespace bluetoth_internals for everything under that folder. https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/280001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:24: BluetoothAdapter: bluetoothAdapter, On 2016/11/04 at 22:57:41, mbrunson wrote: > On 2016/11/04 17:20:00, dpapad wrote: > > FYI, If this code was to every be compiled by Closure Compiler, this would > > definitely confuse the compiler. 'interfaces' module only exports > > importInterfaces() as far as the compiler understands. > > I've exported some empty objects and the test passes. Done. Not sure what test you are referring to. Compiler is run by executing ./third_party/closure_compiler/run_compiler and is not related to any tests. Anyway as said, I don't think this code is included in any of the compile targets, so this was more of an FYI. https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_table.js (right): https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:20: this.devices_ = null; Can you add @type annotation? https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:24: }); \n https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:62: * @param {!Event} event @private here and elsewhere. https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:79: newRowForDevice_: function(device, index) { Nit: Perhaps simply rename to insertRow_ ? https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:107: * @param {!number} index "!" is implied for string,number,boolean https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:110: var row = this.body_.rows[index]; Perhaps add an assert here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_table.js (right): https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:20: this.devices_ = null; On 2016/11/04 23:43:52, dpapad wrote: > Can you add @type annotation? Done. https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:24: }); On 2016/11/04 23:43:52, dpapad wrote: > \n Done. https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:62: * @param {!Event} event On 2016/11/04 23:43:52, dpapad wrote: > @private here and elsewhere. Done. https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:79: newRowForDevice_: function(device, index) { On 2016/11/04 23:43:51, dpapad wrote: > Nit: Perhaps simply rename to insertRow_ ? Done. https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:107: * @param {!number} index On 2016/11/04 23:43:52, dpapad wrote: > "!" is implied for string,number,boolean Done. https://codereview.chromium.org/2446823002/diff/300001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:110: var row = this.body_.rows[index]; On 2016/11/04 23:43:51, dpapad wrote: > Perhaps add an assert here? 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...
On 2016/11/04 23:43:52, dpapad wrote: > LGTM with nits. Please wait for @dbeam's approval too. dbeam is going to unavailable until Wednesday at the earliest it seems. Can I just land this and TBR him? I have a string of patches that depend on this one.
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/11/07 22:05:52, mbrunson wrote: > On 2016/11/04 23:43:52, dpapad wrote: > > LGTM with nits. Please wait for @dbeam's approval too. > > dbeam is going to unavailable until Wednesday at the earliest it seems. Can I > just land this and TBR him? I have a string of patches that depend on this one. OK.
Description was changed from ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. Adds AdapterBroker to handle adapter events and simplifier Adapter service communication. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. Adds AdapterBroker to handle adapter events and simplifier Adapter service communication. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation TBR=dbeam@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2446823002/#ps340001 (title: "Change innerText to textContent in device table")
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: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. Adds AdapterBroker to handle adapter events and simplifier Adapter service communication. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation TBR=dbeam@chromium.org ========== to ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. Adds AdapterBroker to handle adapter events and simplifier Adapter service communication. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation TBR=dbeam@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. Adds AdapterBroker to handle adapter events and simplifier Adapter service communication. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation TBR=dbeam@chromium.org ========== to ========== bluetooth: Componentize device list in chrome://bluetooth-internals. Separates device list HTML management code from device handling code. Adds DeviceTable for management of device table UI. Adds observable DeviceCollection for dynamic updates of DeviceTable. Adds AdapterBroker to handle adapter events and simplifier Adapter service communication. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation TBR=dbeam@chromium.org Committed: https://crrev.com/d5d3edafbb53f7af22875e0c274d75bc1d84b430 Cr-Commit-Position: refs/heads/master@{#430458} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/d5d3edafbb53f7af22875e0c274d75bc1d84b430 Cr-Commit-Position: refs/heads/master@{#430458}
Message was sent while issue was closed.
https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:40: * @return {Array<interfaces.BluetoothDevice.DeviceInfo>} can we add more !, i.e. !Array<!interfaces.BluetoothDevice.DeviceInfo> https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:48: * @return {interfaces.BluetoothAdapter.AdapterInfo} can you use ? or ! https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:74: detail: { don't you want just new CustomEvent('deviceadded', {deviceInfo: deviceInfo}) or just new CustomEvent('deviceadded', deviceInfo) here in all these cases? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:118: } no curlies https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:120: return interfaces.importInterfaces().then(function(adapter) { can we call this like setupInterfaces()? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:122: AdapterClient.prototype.__proto__ = why is this necessary? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:135: } no curlies https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:11: <link rel="import" href="chrome://resources/html/cr/ui.html"> is there a reason you're importing just one of these as an .html import? can we use that for all these <script>s or make new .html files if they don't exist? this is a good way to de-duplicate. followup note: this doesn't need to run on iOS, right? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_collection.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:78: this.removed = false; are these used externally? can they be @private? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_table.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:7: * chrome://bluetooth-internals/. nit: can we unwrap this? fits on one line https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:11: var REMOVED_CSS = 'removed'; can we name this REMOVED_CLASS_NAME or REMOVED_CLASS or something? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:20: // @type {Array<device_collection.Device>} ?Array https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:36: this.headers_ = this.tHead.rows[0].cells; can you mark these as @private? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:45: can you mark devices_ as @private? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:123: row.classList.remove(REMOVED_CSS); row.classList.toggle(REMOVED_CSS, device.removed); https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:124: } nit: no curlies https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:135: obj = obj[part]; can you use cr.exportPath() here? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:23: Object.assign(interfaces, { this might require compiler integration as is more lines than: interfaces.BluetoothAdapter = bluetoothAdapter; ... can we not use Object.assign() here? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:36: FrameInterfaces: {}, what's going on with these {}? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:30: IDR_BLUETOOTH_INTERNALS_ADAPTER_BROKER_JS); can we alphabetize this?
Message was sent while issue was closed.
CL fixes implemented here: https://codereview.chromium.org/2488093003/ https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:40: * @return {Array<interfaces.BluetoothDevice.DeviceInfo>} On 2016/11/09 18:00:32, Dan Beam wrote: > can we add more !, i.e. !Array<!interfaces.BluetoothDevice.DeviceInfo> Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:48: * @return {interfaces.BluetoothAdapter.AdapterInfo} On 2016/11/09 18:00:32, Dan Beam wrote: > can you use ? or ! Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:74: detail: { On 2016/11/09 18:00:31, Dan Beam wrote: > don't you want just new CustomEvent('deviceadded', {deviceInfo: deviceInfo}) or > just new CustomEvent('deviceadded', deviceInfo) here in all these cases? CustomEvent spec [1] requires second parameter dictionary to have a 'detail' key. [1] https://developer.mozilla.org/en-US/docs/Web/API/CustomEvent/CustomEvent https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:118: } On 2016/11/09 18:00:32, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:120: return interfaces.importInterfaces().then(function(adapter) { On 2016/11/09 18:00:32, Dan Beam wrote: > can we call this like setupInterfaces()? Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:122: AdapterClient.prototype.__proto__ = On 2016/11/09 18:00:32, Dan Beam wrote: > why is this necessary? The Mojo connection libraries [1] require client interfaces to be subclasses of the auto-generated stubClass in order to create a handle for that object. There are a few places where this is done [2][3]. [1] https://cs.chromium.org/chromium/src/mojo/public/js/connection.js?rcl=0&l=172 [2] https://cs.chromium.org/chromium/src/chrome/browser/resources/omnibox/omnibox... [3] https://cs.chromium.org/chromium/src/chrome/browser/resources/plugins.js?rcl=... https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:135: } On 2016/11/09 18:00:32, Dan Beam wrote: > no curlies This doesn't fit on one line. https://engdoc.corp.google.com/eng/doc/devguide/js/styleguide.shtml?cl=head#f... Unless we're using a different style guide? https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:11: <link rel="import" href="chrome://resources/html/cr/ui.html"> On 2016/11/09 18:00:32, Dan Beam wrote: > is there a reason you're importing just one of these as an .html import? can we > use that for all these <script>s or make new .html files if they don't exist? > this is a good way to de-duplicate. > > followup note: this doesn't need to run on iOS, right? It was mainly so others and myself know exactly what I'm importing. It can change these to html imports though. It doesn't need to run on iOS as far as I know. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_collection.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_collection.js:78: this.removed = false; On 2016/11/09 18:00:32, Dan Beam wrote: > are these used externally? can they be @private? Yes. These are used externally. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_table.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:7: * chrome://bluetooth-internals/. On 2016/11/09 18:00:32, Dan Beam wrote: > nit: can we unwrap this? fits on one line Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:11: var REMOVED_CSS = 'removed'; On 2016/11/09 18:00:32, Dan Beam wrote: > can we name this REMOVED_CLASS_NAME or REMOVED_CLASS or something? Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:20: // @type {Array<device_collection.Device>} On 2016/11/09 18:00:32, Dan Beam wrote: > ?Array Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:36: this.headers_ = this.tHead.rows[0].cells; On 2016/11/09 18:00:32, Dan Beam wrote: > can you mark these as @private? Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:45: On 2016/11/09 18:00:32, Dan Beam wrote: > can you mark devices_ as @private? Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:123: row.classList.remove(REMOVED_CSS); On 2016/11/09 18:00:32, Dan Beam wrote: > row.classList.toggle(REMOVED_CSS, device.removed); Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:124: } On 2016/11/09 18:00:32, Dan Beam wrote: > nit: no curlies Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:135: obj = obj[part]; On 2016/11/09 18:00:32, Dan Beam wrote: > can you use cr.exportPath() here? cr.exportPath doesn't quite do what I want. I don't want to change the object, I just want the value at the end of the path or null if it's not there. I'd still have to traverse the path if I use cr.exportPath to get the value I want. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/interfaces.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:23: Object.assign(interfaces, { On 2016/11/09 18:00:32, Dan Beam wrote: > this might require compiler integration as is more lines than: > > interfaces.BluetoothAdapter = bluetoothAdapter; > ... > > can we not use Object.assign() here? Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/interfaces.js:36: FrameInterfaces: {}, On 2016/11/09 18:00:32, Dan Beam wrote: > what's going on with these {}? It was a hint that these values were going to be exported from this module, but I can remove them. Done. https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/ui/webu... File chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/ui/webu... chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc:30: IDR_BLUETOOTH_INTERNALS_ADAPTER_BROKER_JS); On 2016/11/09 18:00:32, Dan Beam wrote: > can we alphabetize this? Done.
Message was sent while issue was closed.
https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/adapter_broker.js:135: } On 2016/11/10 01:02:13, mbrunson wrote: > On 2016/11/09 18:00:32, Dan Beam wrote: > > no curlies > > This doesn't fit on one line. > > https://engdoc.corp.google.com/eng/doc/devguide/js/styleguide.shtml?cl=head#f... > > Unless we're using a different style guide? yeah, that doc deals with ES6 (we're using ES5 in chrome still, generally) and is internal and google-specific (we have some chrome-specific changes). there are also notes on the bottom of pretty much every style guide (C++, JS, etc.) that says "be consistent". trust me when i say that most webui code (in general) omits curlies even if each separate line fits in 80 cols https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/device_table.js (right): https://codereview.chromium.org/2446823002/diff/340001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/device_table.js:135: obj = obj[part]; On 2016/11/10 01:02:13, mbrunson wrote: > On 2016/11/09 18:00:32, Dan Beam wrote: > > can you use cr.exportPath() here? > > cr.exportPath doesn't quite do what I want. I don't want to change the object, I > just want the value at the end of the path or null if it's not there. I'd still > have to traverse the path if I use cr.exportPath to get the value I want. Acknowledged. |
