bluetooth: Add descriptor list to DeviceDetailsPage on internals page.

bluetooth: Add descriptor list to DeviceDetailsPage on internals page.

Adds descriptor list to DeviceDetailsPage for displaying basic descriptor info
    for each characteristic.
Adds GetDescriptors message to Device interface.
Adds DescriptorInfo struct to wrap id and uuid of descriptor objects.

Screenshots: https://goo.gl/photos/vqSRubWag2P8iMzm7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2640073004
Cr-Commit-Position: refs/heads/master@{#445947}
Committed: https://chromium.googlesource.com/chromium/src/+/114e4a2cdc28928c83a898d3b73e7c30dad437a1

[mbrunson, 2017-01-20 00:56:48]

Description was changed from

==========
bluetooth: Add descriptor list to DeviceDetailsPage on internals page.

Adds descriptor list to DeviceDetailsPage for displaying basic descriptor info
    for each characteristic.
Adds GetDescriptors message to Device interface.
Adds DescriptorInfo struct to wrap id and uuid of descriptor objects.

Screenshots: https://goo.gl/photos/vqSRubWag2P8iMzm7

BUG=651282
==========

to

==========
bluetooth: Add descriptor list to DeviceDetailsPage on internals page.

Adds descriptor list to DeviceDetailsPage for displaying basic descriptor info
    for each characteristic.
Adds GetDescriptors message to Device interface.
Adds DescriptorInfo struct to wrap id and uuid of descriptor objects.

Screenshots: https://goo.gl/photos/vqSRubWag2P8iMzm7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[mbrunson, 2017-01-20 01:11:02]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 01:11:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 02:47:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 02:47:34]

Dry run: This issue passed the CQ dry run.

[mbrunson, 2017-01-20 23:08:03]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 23:08:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-21 00:48:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-21 00:48:05]

Dry run: This issue passed the CQ dry run.

[mbrunson, 2017-01-21 02:18:25]

mbrunson@chromium.org changed reviewers:
+ ortuno@chromium.org, scheib@chromium.org

[scheib, 2017-01-21 02:37:57]

LGTM

[ortuno, 2017-01-22 22:43:16]

lgtm

[mbrunson, 2017-01-23 20:14:12]

OWNERS review, please:

tsepez:
  device/bluetooth/public/interfaces/device.mojom

dbeam:
  chrome/browser/browser_resources.grd
  chrome/browser/resources/bluetooth_internals/bluetooth_internals.css
  chrome/browser/resources/bluetooth_internals/bluetooth_internals.html
  chrome/browser/resources/bluetooth_internals/characteristic_list.js
  chrome/browser/resources/bluetooth_internals/descriptor_list.js
  chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc

[mbrunson, 2017-01-23 20:15:11]

mbrunson@chromium.org changed reviewers:
+ dbeam@chromium.org, tsepez@chromium.org

[mbrunson, 2017-01-23 20:15:11]

OWNERS review, please:

tsepez:
  device/bluetooth/public/interfaces/device.mojom

dbeam:
  chrome/browser/browser_resources.grd
  chrome/browser/resources/bluetooth_internals/bluetooth_internals.css
  chrome/browser/resources/bluetooth_internals/bluetooth_internals.html
  chrome/browser/resources/bluetooth_internals/characteristic_list.js
  chrome/browser/resources/bluetooth_internals/descriptor_list.js
  chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.cc

[Tom Sepez, 2017-01-23 20:24:11]

mojom lgtm

[Dan Beam, 2017-01-24 06:07:32]

this generally lgtm

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html
(right):

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:35:
<script src="descriptor_list.js"></script>
is there a particular order to these scripts?

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/characteristic_list.js
(right):

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/characteristic_list.js:57:
listItem.deviceAddress_ = deviceAddress;
can we add type definitions for these properties in the class itself (or here, I
guess) or pass these into the ctor?

it's hard to differentiate between setting private instance properties on some
far away class or one in this file.

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/descriptor_list.js (right):

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/descriptor_list.js:123: if
(this.descriptorsRequested_ || !this.isLoading())
it's kinda confusing to call load() and return if !isLoading()...

i'd expect

load: function() {
  if (isLoading())
    return;
},

can we maybe change isLoading to isSpinnerShowing()?

https://codereview.chromium.org/2640073004/diff/60001/device/bluetooth/device.cc
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2640073004/diff/60001/device/bluetooth/device...
device/bluetooth/device.cc:119: if (device == nullptr) {
is this preferable to just if (!device) { ?

[mbrunson, 2017-01-24 20:51:56]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 20:52:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 20:55:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 20:55:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mbrunson, 2017-01-24 21:29:40]

The CQ bit was checked by mbrunson@chromium.org to run a CQ dry run

[mbrunson, 2017-01-24 21:29:46]

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.html
(right):

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.html:35:
<script src="descriptor_list.js"></script>
On 2017/01/24 06:07:29, Dan Beam wrote:
> is there a particular order to these scripts?

There's no particular order to most of these scripts, but I try to group them by
feature. The only thing that really matters here is that bluetooth_internals.js
is loaded after everything else since execution of all the other scripts starts
there.

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/characteristic_list.js
(right):

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/characteristic_list.js:57:
listItem.deviceAddress_ = deviceAddress;
On 2017/01/24 06:07:29, Dan Beam wrote:
> can we add type definitions for these properties in the class itself (or here,
I
> guess) or pass these into the ctor?
> 
> it's hard to differentiate between setting private instance properties on some
> far away class or one in this file.

Done.

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
File chrome/browser/resources/bluetooth_internals/descriptor_list.js (right):

https://codereview.chromium.org/2640073004/diff/60001/chrome/browser/resource...
chrome/browser/resources/bluetooth_internals/descriptor_list.js:123: if
(this.descriptorsRequested_ || !this.isLoading())
On 2017/01/24 06:07:29, Dan Beam wrote:
> it's kinda confusing to call load() and return if !isLoading()...
> 
> i'd expect
> 
> load: function() {
>   if (isLoading())
>     return;
> },
> 
> can we maybe change isLoading to isSpinnerShowing()?

Done.

https://codereview.chromium.org/2640073004/diff/60001/device/bluetooth/device.cc
File device/bluetooth/device.cc (right):

https://codereview.chromium.org/2640073004/diff/60001/device/bluetooth/device...
device/bluetooth/device.cc:119: if (device == nullptr) {
On 2017/01/24 06:07:29, Dan Beam wrote:
> is this preferable to just if (!device) { ?

I've seen it both ways, but the other files here use the short form so I'll
switch it. Done.

[commit-bot: I haz the power, 2017-01-24 21:30:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 21:33:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 21:33:39]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[mbrunson, 2017-01-24 21:58:46]

The CQ bit was checked by mbrunson@chromium.org

[mbrunson, 2017-01-24 21:58:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
scheib@chromium.org, ortuno@chromium.org, dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2640073004/#ps120001
(title: "Fix merge issue of bluetooth_internals.css")

[commit-bot: I haz the power, 2017-01-24 21:59:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 03:20:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 03:20:59]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mbrunson, 2017-01-25 03:48:03]

The CQ bit was checked by mbrunson@chromium.org

[commit-bot: I haz the power, 2017-01-25 03:48:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 04:40:36]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485316083191210,
"parent_rev": "4384d25d7adc524779598ebe783aec0a6324bdfc", "commit_rev":
"114e4a2cdc28928c83a898d3b73e7c30dad437a1"}

[commit-bot: I haz the power, 2017-01-25 04:41:32]

Description was changed from

==========
bluetooth: Add descriptor list to DeviceDetailsPage on internals page.

Adds descriptor list to DeviceDetailsPage for displaying basic descriptor info
    for each characteristic.
Adds GetDescriptors message to Device interface.
Adds DescriptorInfo struct to wrap id and uuid of descriptor objects.

Screenshots: https://goo.gl/photos/vqSRubWag2P8iMzm7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
bluetooth: Add descriptor list to DeviceDetailsPage on internals page.

Adds descriptor list to DeviceDetailsPage for displaying basic descriptor info
    for each characteristic.
Adds GetDescriptors message to Device interface.
Adds DescriptorInfo struct to wrap id and uuid of descriptor objects.

Screenshots: https://goo.gl/photos/vqSRubWag2P8iMzm7

BUG=651282
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2640073004
Cr-Commit-Position: refs/heads/master@{#445947}
Committed:
https://chromium.googlesource.com/chromium/src/+/114e4a2cdc28928c83a898d3b73e...
==========

[commit-bot: I haz the power, 2017-01-25 04:41:34]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/114e4a2cdc28928c83a898d3b73e...