Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2418343002:
bluetooth: Add device list UI for chrome://bluetooth-internals. (Closed)
Descriptionbluetooth: Add device list UI for chrome://bluetooth-internals.
Adds mobile responsive device list to bluetooth internals page.
Lists device name, address, and latest RSSI.
Refreshes when devices are discovered, removed, or advertising packets are received.
Screenshots
Desktop: https://goo.gl/photos/bot7JsdTuf8P2A328
Mobile: https://goo.gl/photos/G9bb9s4ih5KT3PJw8
BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/eb0336053e2673a5714326520bcde9269ca0f823
Cr-Commit-Position: refs/heads/master@{#427261}
Patch Set 1 #Patch Set 2 : Merge upstream #
Total comments: 10
Patch Set 3 : Fix formatting, remove unnecessary properties from Device in JS #Patch Set 4 : Fix device list for optional rssi #Patch Set 5 : More granular device list control, merge upstream #Patch Set 6 : Add comments to new device functions #
Total comments: 13
Patch Set 7 : Merge upstream, address ortuno comments" #Patch Set 8 : Add missing deviceElement variable #Patch Set 9 : Add error for missing device #
Total comments: 12
Patch Set 10 : CSS changes, assert, importNode, textContent #
Total comments: 6
Patch Set 11 : Update comments #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 47 (24 generated)
Description was changed from ========== bluetooth: Add device list UI for chrome://bluetooth-internals. Adds mobile responsive device list to bluetooth internals page. Lists device name, address, and latest RSSI. Refreshes when devices are discovered, removed, or advertising packets are received. BUG=651282 ========== to ========== bluetooth: Add device list UI for chrome://bluetooth-internals. Adds mobile responsive device list to bluetooth internals page. Lists device name, address, and latest RSSI. Refreshes when devices are discovered, removed, or advertising packets are received. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mbrunson@chromium.org changed reviewers: + ortuno@chromium.org, scheib@chromium.org
Nifty! Mind snagging screen shots and e.g. sharing with e.g. photos.google.com so reviewers can see what things look like? https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:32: text-decoration: none; text-decoration necessary? https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:44: #device-table { You'll likely have other tables? Perhaps use a class for .table instead of being id based #device-table? https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:47: border-spacing: 0; border-spacing necessary if collapse is used? https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:5: <meta charset="utf-8"> No tabs. https://www.chromium.org/developers/web-development-style-guide https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:32: console.log('Device added', device); Remove .log lines now that there's a display?
Description was changed from ========== bluetooth: Add device list UI for chrome://bluetooth-internals. Adds mobile responsive device list to bluetooth internals page. Lists device name, address, and latest RSSI. Refreshes when devices are discovered, removed, or advertising packets are received. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add device list UI for chrome://bluetooth-internals. Adds mobile responsive device list to bluetooth internals page. Lists device name, address, and latest RSSI. Refreshes when devices are discovered, removed, or advertising packets are received. Screenshots Desktop: https://goo.gl/photos/bot7JsdTuf8P2A328 Mobile: https://goo.gl/photos/G9bb9s4ih5KT3PJw8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:32: text-decoration: none; On 2016/10/15 03:51:45, scheib wrote: > text-decoration necessary? I meant to remove this rule. This was for a menu button but I haven't added it yet. https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:44: #device-table { On 2016/10/15 03:51:45, scheib wrote: > You'll likely have other tables? Perhaps use a class for .table instead of being > id based #device-table? Done. https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:47: border-spacing: 0; On 2016/10/15 03:51:45, scheib wrote: > border-spacing necessary if collapse is used? Removed. https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:5: <meta charset="utf-8"> On 2016/10/15 03:51:45, scheib wrote: > No tabs. https://www.chromium.org/developers/web-development-style-guide Done. https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:32: console.log('Device added', device); On 2016/10/15 03:51:45, scheib wrote: > Remove .log lines now that there's a display? I'll remove them but leave the log for adapter.getInfo which I will add a display for in a different CL.
LGTM
https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:21: background-color: cornflowerblue; Should we use the colors at https://material.google.com/style/color.html ? https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:115: var deviceList = $('device-list'); nit: Move this down to where it's used. https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:117: var deviceRow = deviceRowTemplate.content.children[0].cloneNode(true); nit: cloneNode(true /* deep */); https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:135: deviceList.removeChild(deviceElement); I don't think we want to completely remove elements since it could cause the whole list to move. Could you change the background to show that the device is no longer around? That's what we do in the Android Chooser. https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:146: if (deviceRow) { When would you receive a DeviceChanged for a device that's not on the list? Seems like that would be a bug in the device API which should be logged.
Merged upstream patch so things have been rearranged a bit. https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:21: background-color: cornflowerblue; On 2016/10/21 00:56:36, ortuno wrote: > Should we use the colors at https://material.google.com/style/color.html ? Yeah. I'll just use Blue 500. https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:115: var deviceList = $('device-list'); On 2016/10/21 00:56:36, ortuno wrote: > nit: Move this down to where it's used. Done. https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:117: var deviceRow = deviceRowTemplate.content.children[0].cloneNode(true); On 2016/10/21 00:56:36, ortuno wrote: > nit: cloneNode(true /* deep */); Done. https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:135: deviceList.removeChild(deviceElement); On 2016/10/21 00:56:36, ortuno wrote: > I don't think we want to completely remove elements since it could cause the > whole list to move. Could you change the background to show that the device is > no longer around? That's what we do in the Android Chooser. Done. https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:146: if (deviceRow) { On 2016/10/21 00:56:36, ortuno wrote: > When would you receive a DeviceChanged for a device that's not on the list? > Seems like that would be a bug in the device API which should be logged. Isn't the order of messages in Mojo not guaranteed? I assumed there was a possibility of receiving deviceChanged before deviceAdded for a newly discovered device.
https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:146: if (deviceRow) { On 2016/10/21 at 02:09:33, mbrunson wrote: > On 2016/10/21 00:56:36, ortuno wrote: > > When would you receive a DeviceChanged for a device that's not on the list? > > Seems like that would be a bug in the device API which should be logged. > > Isn't the order of messages in Mojo not guaranteed? I assumed there was a possibility of receiving deviceChanged before deviceAdded for a newly discovered device. The order is not guaranteed between pipes e.g. the order of Adapter and Device messages is not guaranteed but it is guaranteed within a pipe which in this case is the AdapterClient pipe.
Also I don't want to delay your patch another day so feel free to send to owners before my lgtm.
On 2016/10/21 05:47:31, ortuno wrote: > Also I don't want to delay your patch another day so feel free to send to owners > before my lgtm. There really needs to be a specific button for approval so that doesn't happen...
https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:146: if (deviceRow) { On 2016/10/21 02:16:55, ortuno wrote: > On 2016/10/21 at 02:09:33, mbrunson wrote: > > On 2016/10/21 00:56:36, ortuno wrote: > > > When would you receive a DeviceChanged for a device that's not on the list? > > > Seems like that would be a bug in the device API which should be logged. > > > > Isn't the order of messages in Mojo not guaranteed? I assumed there was a > possibility of receiving deviceChanged before deviceAdded for a newly discovered > device. > > The order is not guaranteed between pipes e.g. the order of Adapter and Device > messages is not guaranteed but it is guaranteed within a pipe which in this case > is the AdapterClient pipe. Ah ok. I'll just throw an exception here for that.
mbrunson@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:39: .table { Why use a .table CSS class, when you can use the tag name directly? table { ... } https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:33: if (this.devices_.has(deviceInfo.address)) { How can device that was just added can already exist in devices_? https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:35: deviceElement.classList.remove('removed'); Let's put all CSS string literals used in JS in an enum, such that they don't have to be repeated (which increases the risk of typo's). var CSS_CLASSES = { REMOVED: 'removed', ... }; Or if there is only one CSS class, var REMOVED_CSS = 'removed'; https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:41: true /* deep */); The recommended way to instantiate templates is document.importNode(), see example at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template. Does that not work here? https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:67: throw new Error('Device does not exist.'); Can you use assert.js instead, see https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=18. assert(this.devices_.has(deviceInfo.address), 'Device does not exist'); https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:72: deviceRow.querySelector('.device-name').innerText = You probably want to use textContent (more context http://kellegous.com/j/2013/02/27/innertext-vs-textcontent/).
https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:39: .table { On 2016/10/21 22:46:21, dpapad wrote: > Why use a .table CSS class, when you can use the tag name directly? > > table { > ... > } Done. https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:33: if (this.devices_.has(deviceInfo.address)) { On 2016/10/21 22:46:21, dpapad wrote: > How can device that was just added can already exist in devices_? A device can be removed if it is not detected for 3 minutes. We don't remove it from the |devices_| dictionary though; Instead, we mark it as removed so the user can still see it. If it's detected again, deviceAdded is called to re-add the device, but it already exists in the dictionary, so we just remove the "removed" class. https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:35: deviceElement.classList.remove('removed'); On 2016/10/21 22:46:21, dpapad wrote: > Let's put all CSS string literals used in JS in an enum, such that they don't > have to be repeated (which increases the risk of typo's). > > var CSS_CLASSES = { > REMOVED: 'removed', > ... > }; > > Or if there is only one CSS class, > var REMOVED_CSS = 'removed'; Done. https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:41: true /* deep */); On 2016/10/21 22:46:21, dpapad wrote: > The recommended way to instantiate templates is document.importNode(), see > example at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template. > Does that not work here? Ok. I haven't used it before, but it seems to produce the same results. Done. https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:67: throw new Error('Device does not exist.'); On 2016/10/21 22:46:21, dpapad wrote: > Can you use assert.js instead, see > https://cs.chromium.org/chromium/src/ui/webui/resources/js/assert.js?l=18. > > assert(this.devices_.has(deviceInfo.address), 'Device does not exist'); Done. https://codereview.chromium.org/2418343002/diff/160001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:72: deviceRow.querySelector('.device-name').innerText = On 2016/10/21 22:46:22, dpapad wrote: > You probably want to use textContent (more context > http://kellegous.com/j/2013/02/27/innertext-vs-textcontent/). Done.
LGTM with nits. https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:24: <th>Name</th> Are you planning to localize those strings eventually? Can you add a TODO and/or file a bug to track this if so? https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:54: * Removes the cached device and updates the device list. Well, the comment says it "removes the cached device", but as you explained it only changes how the device is shown. Can you update the comment? https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:66: console.log(new Date(), deviceInfo); Now that this page starts having UI, do you still think the console.logs are valuable? When can we remove them?
https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html (right): https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:24: <th>Name</th> On 2016/10/24 18:12:26, dpapad wrote: > Are you planning to localize those strings eventually? Can you add a TODO and/or > file a bug to track this if so? Done. https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:54: * Removes the cached device and updates the device list. On 2016/10/24 18:12:26, dpapad wrote: > Well, the comment says it "removes the cached device", but as you explained it > only changes how the device is shown. Can you update the comment? Done. https://codereview.chromium.org/2418343002/diff/180001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:66: console.log(new Date(), deviceInfo); On 2016/10/24 18:12:26, dpapad wrote: > Now that this page starts having UI, do you still think the console.logs are > valuable? When can we remove them? I'll remove the logs when I have a part of the UI that shows the thing being logged. So far, logs for adapter-related information and device changes are logged because I don't have a corresponding UI component for that data. An adapter-related information screen will be added after device connections are landed. A log screen that the user can easily search will contain the device change information and various other information relating to the changes in state of the internal device/bluetooth system. That component is much farther down the line.
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/2418343002/#ps200001 (title: "Update comments")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2404623002 Patch 300001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by mbrunson@chromium.org
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mbrunson@chromium.org
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 UI for chrome://bluetooth-internals. Adds mobile responsive device list to bluetooth internals page. Lists device name, address, and latest RSSI. Refreshes when devices are discovered, removed, or advertising packets are received. Screenshots Desktop: https://goo.gl/photos/bot7JsdTuf8P2A328 Mobile: https://goo.gl/photos/G9bb9s4ih5KT3PJw8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add device list UI for chrome://bluetooth-internals. Adds mobile responsive device list to bluetooth internals page. Lists device name, address, and latest RSSI. Refreshes when devices are discovered, removed, or advertising packets are received. Screenshots Desktop: https://goo.gl/photos/bot7JsdTuf8P2A328 Mobile: https://goo.gl/photos/G9bb9s4ih5KT3PJw8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Add device list UI for chrome://bluetooth-internals. Adds mobile responsive device list to bluetooth internals page. Lists device name, address, and latest RSSI. Refreshes when devices are discovered, removed, or advertising packets are received. Screenshots Desktop: https://goo.gl/photos/bot7JsdTuf8P2A328 Mobile: https://goo.gl/photos/G9bb9s4ih5KT3PJw8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Add device list UI for chrome://bluetooth-internals. Adds mobile responsive device list to bluetooth internals page. Lists device name, address, and latest RSSI. Refreshes when devices are discovered, removed, or advertising packets are received. Screenshots Desktop: https://goo.gl/photos/bot7JsdTuf8P2A328 Mobile: https://goo.gl/photos/G9bb9s4ih5KT3PJw8 BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/eb0336053e2673a5714326520bcde9269ca0f823 Cr-Commit-Position: refs/heads/master@{#427261} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/eb0336053e2673a5714326520bcde9269ca0f823 Cr-Commit-Position: refs/heads/master@{#427261}
Message was sent while issue was closed.
I failed to send this comment earlier -- in a following patch would you at least cite the color in a comment, or perhaps with a variable name https://developer.mozilla.org/en-US/docs/Web/CSS/Using_CSS_variables https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/bluetooth_internals/bluetooth_internals.css (right): https://codereview.chromium.org/2418343002/diff/100001/chrome/browser/resourc... chrome/browser/resources/bluetooth_internals/bluetooth_internals.css:21: background-color: cornflowerblue; On 2016/10/21 02:09:33, mbrunson wrote: > On 2016/10/21 00:56:36, ortuno wrote: > > Should we use the colors at https://material.google.com/style/color.html ? > > Yeah. I'll just use Blue 500. Citing color value source would be nice. Maybe just once at top of CSS |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
