bluetooth: Add basic scanning to chrome://bluetooth-internals.

bluetooth: Add basic scanning to chrome://bluetooth-internals.

Adds DiscoverySession to Adapter Mojo service to handle starting a new BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

GIF: https://goo.gl/photos/9B14sgpGNiRqeoxk6

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

Review-Url: https://codereview.chromium.org/2564113003
Cr-Commit-Position: refs/heads/master@{#442141}
Committed: https://chromium.googlesource.com/chromium/src/+/b0f0a102a61ef7682e4aac5d5587e36ac6f6b3d6

[mbrunson, 2016-12-09 19:12:50]

Description was changed from

==========
bluetooth: Add scanning to Adapter and chrome://bluetooth-internals.

Adds DiscoverySession to Adapter interface to handle BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

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

to

==========
bluetooth: Add scanning to Adapter and chrome://bluetooth-internals.

Adds DiscoverySession to Adapter interface to handle BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

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

[mbrunson, 2016-12-09 19:24:53]

Description was changed from

==========
bluetooth: Add scanning to Adapter and chrome://bluetooth-internals.

Adds DiscoverySession to Adapter interface to handle BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

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

to

==========
bluetooth: Add basic scanning to chrome://bluetooth-internals.

Adds DiscoverySession to Adapter interface to handle BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

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

[mbrunson, 2016-12-13 21:52:12]

Description was changed from

==========
bluetooth: Add basic scanning to chrome://bluetooth-internals.

Adds DiscoverySession to Adapter interface to handle BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

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

to

==========
bluetooth: Add basic scanning to chrome://bluetooth-internals.

Adds DiscoverySession to Adapter interface to handle BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

GIF: https://goo.gl/photos/9B14sgpGNiRqeoxk6

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

[mbrunson, 2016-12-14 00:33:25]

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

[commit-bot: I haz the power, 2016-12-14 00:34:06]

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, 2016-12-14 00:36:31]

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

[commit-bot: I haz the power, 2016-12-14 00:36:31]

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mbrunson, 2016-12-14 03:22:52]

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

[commit-bot: I haz the power, 2016-12-14 03:23:26]

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, 2016-12-14 03:26:06]

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

[commit-bot: I haz the power, 2016-12-14 03:26:07]

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...)

[mbrunson, 2016-12-14 04:19:10]

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

[commit-bot: I haz the power, 2016-12-14 04:19:30]

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, 2016-12-14 04:21:38]

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

[commit-bot: I haz the power, 2016-12-14 04:21:38]

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-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...)

[mbrunson, 2016-12-15 20:29:03]

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

[commit-bot: I haz the power, 2016-12-15 20:29:50]

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

[mbrunson, 2016-12-15 22:19:33]

Description was changed from

==========
bluetooth: Add basic scanning to chrome://bluetooth-internals.

Adds DiscoverySession to Adapter interface to handle BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

GIF: https://goo.gl/photos/9B14sgpGNiRqeoxk6

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

to

==========
bluetooth: Add basic scanning to chrome://bluetooth-internals.

Adds DiscoverySession to Adapter Mojo service to handle starting a new
BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

GIF: https://goo.gl/photos/9B14sgpGNiRqeoxk6

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

[mbrunson, 2016-12-16 00:44:29]

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

[commit-bot: I haz the power, 2016-12-16 00:46:04]

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, 2016-12-16 06:18:19]

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

[commit-bot: I haz the power, 2016-12-16 06:18:20]

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

[mbrunson, 2016-12-16 21:52:33]

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

[commit-bot: I haz the power, 2016-12-16 21:54:02]

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, 2016-12-16 23:48:02]

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

[commit-bot: I haz the power, 2016-12-16 23:48:03]

Dry run: This issue passed the CQ dry run.

[mbrunson, 2016-12-20 23:07:22]

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

[commit-bot: I haz the power, 2016-12-20 23:08:30]

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, 2016-12-20 23:24:47]

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

[commit-bot: I haz the power, 2016-12-20 23:24:48]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mbrunson, 2016-12-21 02:09:07]

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

[commit-bot: I haz the power, 2016-12-21 02:09:26]

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, 2016-12-21 06:12:20]

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

[commit-bot: I haz the power, 2016-12-21 06:12:21]

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

[mbrunson, 2017-01-03 20:55:05]

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

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

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-03 20:57:22]

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

[commit-bot: I haz the power, 2017-01-03 20:57:22]

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...)

[mbrunson, 2017-01-03 21:50:56]

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

[commit-bot: I haz the power, 2017-01-03 21:51:26]

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-03 21:53:30]

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

[commit-bot: I haz the power, 2017-01-03 21:53:32]

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...)

[mbrunson, 2017-01-04 03:18:32]

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

[commit-bot: I haz the power, 2017-01-04 03:18:45]

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-04 04:53:21]

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

[commit-bot: I haz the power, 2017-01-04 04:53:22]

Dry run: This issue passed the CQ dry run.

[mbrunson, 2017-01-04 19:29:00]

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

[scheib, 2017-01-05 01:02:20]

LGTM with some comment fixes.

https://codereview.chromium.org/2564113003/diff/300001/device/bluetooth/disco...
File device/bluetooth/discovery_session.h (right):

https://codereview.chromium.org/2564113003/diff/300001/device/bluetooth/disco...
device/bluetooth/discovery_session.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
2017 copyrights

https://codereview.chromium.org/2564113003/diff/300001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2564113003/diff/300001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:53: Stop() => (bool success);
Copy more of the comment from bluetooth_discovery_session.h noting that stopping
a session only stops discovery if it was the only session. 

The result of an error isn't well defined, is it really just telling us if
IsActive was true? In that case, we don't need the return value. But, if there
are other errors in stopping then the documentation here is a bit misleading as
it currently implies the return value is only about weather the session was
active.

[mbrunson, 2017-01-05 01:19:14]

https://codereview.chromium.org/2564113003/diff/300001/device/bluetooth/disco...
File device/bluetooth/discovery_session.h (right):

https://codereview.chromium.org/2564113003/diff/300001/device/bluetooth/disco...
device/bluetooth/discovery_session.h:1: // Copyright 2016 The Chromium Authors.
All rights reserved.
On 2017/01/05 01:02:20, scheib wrote:
> 2017 copyrights

Done.

https://codereview.chromium.org/2564113003/diff/300001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2564113003/diff/300001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:53: Stop() => (bool success);
On 2017/01/05 01:02:20, scheib wrote:
> Copy more of the comment from bluetooth_discovery_session.h noting that
stopping
> a session only stops discovery if it was the only session. 
> 
> The result of an error isn't well defined, is it really just telling us if
> IsActive was true? In that case, we don't need the return value. But, if there
> are other errors in stopping then the documentation here is a bit misleading
as
> it currently implies the return value is only about weather the session was
> active.

Ok. I've copied the original comment from bluetooth_discovery_session and
tweaked it a little. Done.

[mbrunson, 2017-01-05 02:01:40]

mbrunson@chromium.org changed reviewers:
+ dcheng@chromium.org

[mbrunson, 2017-01-05 02:01:41]

OWNERS review:
  device/bluetooth/public/interfaces/adapter.mojom

[mbrunson, 2017-01-05 18:39:39]

mbrunson@chromium.org changed reviewers:
+ dpapad@chromium.org

[mbrunson, 2017-01-05 18:39:40]

dpapad, WebUI OWNERS review please:
  chrome/browser/resources/bluetooth_internals/*

[dcheng, 2017-01-05 22:28:23]

This CL looks largely OK to me, but I have some questions that came up when I
was looking at the current mojo interfaces.

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/disco...
File device/bluetooth/discovery_session.h (right):

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/disco...
device/bluetooth/discovery_session.h:25:
DiscoverySession(std::unique_ptr<device::BluetoothDiscoverySession> session);
Nit: explicit

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:77: SetClient(AdapterClient
client);
Btw, is there ever a time when we'd want to create an Adapter interface with no
AdapterClient? It seems like these should be 1:1 right?

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:94: //   Trustable state.
What properties of DeviceInfo *can't* change? IOW, how does the AdapterClient
know which device's info changed?

[dpapad, 2017-01-05 23:30:34]

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:121: var event =
new CustomEvent('adapterchanged', {
Is this the only case where an 'adapterchanged' event is fired? If so can we
simplify the payload to simply include the current value? Do we need the
'property' field? Or are you planning to fire more such events in future CLs?

If you still need the 'property' field, can we put all the possible property
values in an enum and re-use instead of repeating string literals across this
file and bluetooth_internals.js?

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:148: var event =
new CustomEvent('devicechanged', {
Nit (optional): I think is more readable if you separate words with a dash,
inside event names, like device-changed, device-removed etc.

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:127: if
(event.detail.property === 'discovering') {
Is there a need for two separate if conditions? How about the following?

if (event.detail.property === 'discovering' &&
    !event.detail.value && !userRequestedScanStop && discoverySession) {
  ...
}

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:161:
updateStoppedDiscoverySession();
Is the |userRequestedScanStop| really necessary? My understanding is the
following:

calling discoverySession.stop() will trigger an 'adapterchanged' event, which
already calls updateStoppedDiscoverySession(). Here we use the boolean to
prevent that codepath, but also we call that method manually. Can't we just
always rely on 'adapterchanged' listener to call updateStoppedDiscoverySession?

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:220:
.then(function() { return adapterBroker.getInfo(); })
The info returned by this call is never used by setupAdapterSystem(). Can this
call be removed?

[mbrunson, 2017-01-06 01:18:57]

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/adapter_broker.js (right):

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:121: var event =
new CustomEvent('adapterchanged', {
On 2017/01/05 23:30:33, dpapad wrote:
> Is this the only case where an 'adapterchanged' event is fired? If so can we
> simplify the payload to simply include the current value? Do we need the
> 'property' field? Or are you planning to fire more such events in future CLs?
> 
> If you still need the 'property' field, can we put all the possible property
> values in an enum and re-use instead of repeating string literals across this
> file and bluetooth_internals.js?

There are a batch of these change events concerning the adapter[1]. So, the
property field will stay, but I've added the enum here. Done.

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?rcl...

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/adapter_broker.js:148: var event =
new CustomEvent('devicechanged', {
On 2017/01/05 23:30:33, dpapad wrote:
> Nit (optional): I think is more readable if you separate words with a dash,
> inside event names, like device-changed, device-removed etc.

If I do this I'd want to do this for all the custom events in one change, so I
probably won't do it here. The naming convention for event names is all over the
place.

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
(right):

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:127: if
(event.detail.property === 'discovering') {
On 2017/01/05 23:30:33, dpapad wrote:
> Is there a need for two separate if conditions? How about the following?
> 
> if (event.detail.property === 'discovering' &&
>     !event.detail.value && !userRequestedScanStop && discoverySession) {
>   ...
> }

Done.

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:161:
updateStoppedDiscoverySession();
On 2017/01/05 23:30:33, dpapad wrote:
> Is the |userRequestedScanStop| really necessary? My understanding is the
> following:
> 
> calling discoverySession.stop() will trigger an 'adapterchanged' event, which
> already calls updateStoppedDiscoverySession(). Here we use the boolean to
> prevent that codepath, but also we call that method manually. Can't we just
> always rely on 'adapterchanged' listener to call
updateStoppedDiscoverySession?

It's because of the 'adapterchanged' event that I added the
|userRequestedScanStop|. In every case where the scan status of the adapter
changes, the 'adapterchanged' event is fired.

This includes cases where the user did not explicitly request that scanning
should be stopped (e.g. there's some outside program that stopped it or some
hardware malfunction). I wanted a way to differentiate between these two cases
to alert the user that their discovery session was killed without their consent.

https://codereview.chromium.org/2564113003/diff/320001/chrome/browser/resourc...
chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:220:
.then(function() { return adapterBroker.getInfo(); })
On 2017/01/05 23:30:33, dpapad wrote:
> The info returned by this call is never used by setupAdapterSystem(). Can this
> call be removed?

The info returned is currently logged to console because there's no associated
UI for this piece of data. It will be used in the next patch [1][2], so I don't
want to remove this just to add it right back.

[1] https://codereview.chromium.org/2567983007/
[2]
https://codereview.chromium.org/2567983007/diff/180001/chrome/browser/resourc...

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/disco...
File device/bluetooth/discovery_session.h (right):

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/disco...
device/bluetooth/discovery_session.h:25:
DiscoverySession(std::unique_ptr<device::BluetoothDiscoverySession> session);
On 2017/01/05 22:28:23, dcheng wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:77: SetClient(AdapterClient
client);
On 2017/01/05 22:28:23, dcheng wrote:
> Btw, is there ever a time when we'd want to create an Adapter interface with
no
> AdapterClient? It seems like these should be 1:1 right?

The bluetooth-internals is currently the only user of this service. In the
future, other clients may not care to receive events from AdapterClient, so I
thought it would be better to keep it optional in this case.

This mimics the current observer model of the BluetoothAdapter and its observer
interface where subscribing to these events are optional. Keeping it similar
will minimize the headache of clients who decide to switch to the Mojo-based
service.

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:94: //   Trustable state.
On 2017/01/05 22:28:23, dcheng wrote:
> What properties of DeviceInfo *can't* change? IOW, how does the AdapterClient
> know which device's info changed?

Almost all properties of a device could change except for its id. The
recommended way to identify a device is through its address[1] and this is how
the bluetooth-internals page differentiates between devices and updates the
correct DeviceInfo. 

At the moment, I have not implemented the feature in the Mojo service to track
explicit changes in the address to make sure this identifier is consistent
although this functionality is available[2]. Devices that change address are
currently considered new devices on the bluetooth-internals page.

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?rcl=...
[2]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?rcl...

[mbrunson, 2017-01-06 01:19:09]

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

[commit-bot: I haz the power, 2017-01-06 01:20:10]

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

[dcheng, 2017-01-06 02:45:50]

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:77: SetClient(AdapterClient
client);
On 2017/01/06 01:18:56, mbrunson wrote:
> On 2017/01/05 22:28:23, dcheng wrote:
> > Btw, is there ever a time when we'd want to create an Adapter interface with
> no
> > AdapterClient? It seems like these should be 1:1 right?
> 
> The bluetooth-internals is currently the only user of this service. In the
> future, other clients may not care to receive events from AdapterClient, so I
> thought it would be better to keep it optional in this case.
> 
> This mimics the current observer model of the BluetoothAdapter and its
observer
> interface where subscribing to these events are optional. Keeping it similar
> will minimize the headache of clients who decide to switch to the Mojo-based
> service.

Would we create multiple BluetoothAdapter mojo impls? Or would all the things
connect to the same BluetoothAdapter mojo impl?

I'm just trying to understand the long-term plan here: in general, the
SetClient() pattern is one I try to discourage in IPC reviews if there is a
strong 1:1 relationship, because it requires a lot more checking (all the newly
added code needs to check for a non-null client), and it can make the control
flow harder to follow.

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:94: //   Trustable state.
On 2017/01/06 01:18:56, mbrunson wrote:
> On 2017/01/05 22:28:23, dcheng wrote:
> > What properties of DeviceInfo *can't* change? IOW, how does the
AdapterClient
> > know which device's info changed?
> 
> Almost all properties of a device could change except for its id. The
> recommended way to identify a device is through its address[1] and this is how
> the bluetooth-internals page differentiates between devices and updates the
> correct DeviceInfo. 
> 
> At the moment, I have not implemented the feature in the Mojo service to track
> explicit changes in the address to make sure this identifier is consistent
> although this functionality is available[2]. Devices that change address are
> currently considered new devices on the bluetooth-internals page.
> 
> [1]
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?rcl=...
> [2]
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?rcl...

It's not necessary in this CL, but I would prefer if Device itself was
encapsulated as a Mojo interface. That would make the identity invariants easier
to understand, because things like device changed notifications would be
broadcast to the Device interface representing that device. Is this something we
can consider doing in a followup?

[mbrunson, 2017-01-06 03:09:42]

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:77: SetClient(AdapterClient
client);
On 2017/01/06 02:45:50, dcheng wrote:
> On 2017/01/06 01:18:56, mbrunson wrote:
> > On 2017/01/05 22:28:23, dcheng wrote:
> > > Btw, is there ever a time when we'd want to create an Adapter interface
with
> > no
> > > AdapterClient? It seems like these should be 1:1 right?
> > 
> > The bluetooth-internals is currently the only user of this service. In the
> > future, other clients may not care to receive events from AdapterClient, so
I
> > thought it would be better to keep it optional in this case.
> > 
> > This mimics the current observer model of the BluetoothAdapter and its
> observer
> > interface where subscribing to these events are optional. Keeping it similar
> > will minimize the headache of clients who decide to switch to the Mojo-based
> > service.
> 
> Would we create multiple BluetoothAdapter mojo impls? Or would all the things
> connect to the same BluetoothAdapter mojo impl?
> 
> I'm just trying to understand the long-term plan here: in general, the
> SetClient() pattern is one I try to discourage in IPC reviews if there is a
> strong 1:1 relationship, because it requires a lot more checking (all the
newly
> added code needs to check for a non-null client), and it can make the control
> flow harder to follow.

Every Adapter that is requested is a new strongly bound mojo impl[1]. They will
share the same underlying device::BluetoothAdapter instance though since the
device/bluetooth system uses the default bluetooth adapter hardware. So, there
could potentially be multiple impls with different configurations or no client
at all depending on the needs of the user.

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/adapter_factory.cc?rcl=...

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:94: //   Trustable state.
On 2017/01/06 02:45:50, dcheng wrote:
> On 2017/01/06 01:18:56, mbrunson wrote:
> > On 2017/01/05 22:28:23, dcheng wrote:
> > > What properties of DeviceInfo *can't* change? IOW, how does the
> AdapterClient
> > > know which device's info changed?
> > 
> > Almost all properties of a device could change except for its id. The
> > recommended way to identify a device is through its address[1] and this is
how
> > the bluetooth-internals page differentiates between devices and updates the
> > correct DeviceInfo. 
> > 
> > At the moment, I have not implemented the feature in the Mojo service to
track
> > explicit changes in the address to make sure this identifier is consistent
> > although this functionality is available[2]. Devices that change address are
> > currently considered new devices on the bluetooth-internals page.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?rcl=...
> > [2]
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?rcl...
> 
> It's not necessary in this CL, but I would prefer if Device itself was
> encapsulated as a Mojo interface. That would make the identity invariants
easier
> to understand, because things like device changed notifications would be
> broadcast to the Device interface representing that device. Is this something
we
> can consider doing in a followup?

Hmm. My main concern is that Device and DeviceInfo are related but used in
different situations. A Device interface impl is only created when a user
connects to a Bluetooth device and is strongly bond to the message pipe and its
Bluetooth connection. DeviceInfo is available at all times from Adapter to
describe the bluetooth devices that the bluetooth adapter can see. These values
may be updated without a connection and are added, removed, or changed over time
whether a Device impl exists or not.

[dpapad, 2017-01-06 22:03:44]

LGTM for JS.

[dcheng, 2017-01-06 22:45:31]

mojo lgtm, but I would like us to consider how to evolve these interfaces to
make full use of mojo's advantages over legacy ipc

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:77: SetClient(AdapterClient
client);
On 2017/01/06 03:09:41, mbrunson wrote:
> On 2017/01/06 02:45:50, dcheng wrote:
> > On 2017/01/06 01:18:56, mbrunson wrote:
> > > On 2017/01/05 22:28:23, dcheng wrote:
> > > > Btw, is there ever a time when we'd want to create an Adapter interface
> with
> > > no
> > > > AdapterClient? It seems like these should be 1:1 right?
> > > 
> > > The bluetooth-internals is currently the only user of this service. In the
> > > future, other clients may not care to receive events from AdapterClient,
so
> I
> > > thought it would be better to keep it optional in this case.
> > > 
> > > This mimics the current observer model of the BluetoothAdapter and its
> > observer
> > > interface where subscribing to these events are optional. Keeping it
similar
> > > will minimize the headache of clients who decide to switch to the
Mojo-based
> > > service.
> > 
> > Would we create multiple BluetoothAdapter mojo impls? Or would all the
things
> > connect to the same BluetoothAdapter mojo impl?
> > 
> > I'm just trying to understand the long-term plan here: in general, the
> > SetClient() pattern is one I try to discourage in IPC reviews if there is a
> > strong 1:1 relationship, because it requires a lot more checking (all the
> newly
> > added code needs to check for a non-null client), and it can make the
control
> > flow harder to follow.
> 
> Every Adapter that is requested is a new strongly bound mojo impl[1]. They
will
> share the same underlying device::BluetoothAdapter instance though since the
> device/bluetooth system uses the default bluetooth adapter hardware. So, there
> could potentially be multiple impls with different configurations or no client
> at all depending on the needs of the user.
> 
> [1]
>
https://cs.chromium.org/chromium/src/device/bluetooth/adapter_factory.cc?rcl=...

Hmm. I'm not really sure what the best long-term approach here. Let's keep an
eye on this, and if it looks like it's going to be mostly used in a 1:1 context,
can we revisit the decision to have SetClient()? It makes all the methods more
complicated due to the null checking.

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:94: //   Trustable state.
On 2017/01/06 03:09:41, mbrunson wrote:
> On 2017/01/06 02:45:50, dcheng wrote:
> > On 2017/01/06 01:18:56, mbrunson wrote:
> > > On 2017/01/05 22:28:23, dcheng wrote:
> > > > What properties of DeviceInfo *can't* change? IOW, how does the
> > AdapterClient
> > > > know which device's info changed?
> > > 
> > > Almost all properties of a device could change except for its id. The
> > > recommended way to identify a device is through its address[1] and this is
> how
> > > the bluetooth-internals page differentiates between devices and updates
the
> > > correct DeviceInfo. 
> > > 
> > > At the moment, I have not implemented the feature in the Mojo service to
> track
> > > explicit changes in the address to make sure this identifier is consistent
> > > although this functionality is available[2]. Devices that change address
are
> > > currently considered new devices on the bluetooth-internals page.
> > > 
> > > [1]
> > >
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?rcl=...
> > > [2]
> > >
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?rcl...
> > 
> > It's not necessary in this CL, but I would prefer if Device itself was
> > encapsulated as a Mojo interface. That would make the identity invariants
> easier
> > to understand, because things like device changed notifications would be
> > broadcast to the Device interface representing that device. Is this
something
> we
> > can consider doing in a followup?
> 
> Hmm. My main concern is that Device and DeviceInfo are related but used in
> different situations. A Device interface impl is only created when a user
> connects to a Bluetooth device and is strongly bond to the message pipe and
its
> Bluetooth connection. DeviceInfo is available at all times from Adapter to
> describe the bluetooth devices that the bluetooth adapter can see. These
values
> may be updated without a connection and are added, removed, or changed over
time
> whether a Device impl exists or not.

This isn't a blocker for this CL, but one of the nice things about Mojo is that
things can be interface-associated and have an implicit identity. This is what
lets us (eventually) kill IPC routing IDs and perform many other
simplifications. It also makes code audits easier, since identity is associated
with the pipe, so you don't need to validate the name of the thing is valid,
etc.

It sounds like we'd need a separate interface for tracking device status (to
keep it distinct from the connecting to device use case), but I think it might
be worthwhile to keep this in mind, especially as it becomes clearer how this
interface will be used.

[mbrunson, 2017-01-06 23:14:37]

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/adapter.mojom (right):

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:77: SetClient(AdapterClient
client);
On 2017/01/06 22:45:31, dcheng wrote:
> On 2017/01/06 03:09:41, mbrunson wrote:
> > On 2017/01/06 02:45:50, dcheng wrote:
> > > On 2017/01/06 01:18:56, mbrunson wrote:
> > > > On 2017/01/05 22:28:23, dcheng wrote:
> > > > > Btw, is there ever a time when we'd want to create an Adapter
interface
> > with
> > > > no
> > > > > AdapterClient? It seems like these should be 1:1 right?
> > > > 
> > > > The bluetooth-internals is currently the only user of this service. In
the
> > > > future, other clients may not care to receive events from AdapterClient,
> so
> > I
> > > > thought it would be better to keep it optional in this case.
> > > > 
> > > > This mimics the current observer model of the BluetoothAdapter and its
> > > observer
> > > > interface where subscribing to these events are optional. Keeping it
> similar
> > > > will minimize the headache of clients who decide to switch to the
> Mojo-based
> > > > service.
> > > 
> > > Would we create multiple BluetoothAdapter mojo impls? Or would all the
> things
> > > connect to the same BluetoothAdapter mojo impl?
> > > 
> > > I'm just trying to understand the long-term plan here: in general, the
> > > SetClient() pattern is one I try to discourage in IPC reviews if there is
a
> > > strong 1:1 relationship, because it requires a lot more checking (all the
> > newly
> > > added code needs to check for a non-null client), and it can make the
> control
> > > flow harder to follow.
> > 
> > Every Adapter that is requested is a new strongly bound mojo impl[1]. They
> will
> > share the same underlying device::BluetoothAdapter instance though since the
> > device/bluetooth system uses the default bluetooth adapter hardware. So,
there
> > could potentially be multiple impls with different configurations or no
client
> > at all depending on the needs of the user.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/adapter_factory.cc?rcl=...
> 
> Hmm. I'm not really sure what the best long-term approach here. Let's keep an
> eye on this, and if it looks like it's going to be mostly used in a 1:1
context,
> can we revisit the decision to have SetClient()? It makes all the methods more
> complicated due to the null checking.

SGTM

https://codereview.chromium.org/2564113003/diff/320001/device/bluetooth/publi...
device/bluetooth/public/interfaces/adapter.mojom:94: //   Trustable state.
On 2017/01/06 22:45:31, dcheng wrote:
> On 2017/01/06 03:09:41, mbrunson wrote:
> > On 2017/01/06 02:45:50, dcheng wrote:
> > > On 2017/01/06 01:18:56, mbrunson wrote:
> > > > On 2017/01/05 22:28:23, dcheng wrote:
> > > > > What properties of DeviceInfo *can't* change? IOW, how does the
> > > AdapterClient
> > > > > know which device's info changed?
> > > > 
> > > > Almost all properties of a device could change except for its id. The
> > > > recommended way to identify a device is through its address[1] and this
is
> > how
> > > > the bluetooth-internals page differentiates between devices and updates
> the
> > > > correct DeviceInfo. 
> > > > 
> > > > At the moment, I have not implemented the feature in the Mojo service to
> > track
> > > > explicit changes in the address to make sure this identifier is
consistent
> > > > although this functionality is available[2]. Devices that change address
> are
> > > > currently considered new devices on the bluetooth-internals page.
> > > > 
> > > > [1]
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.h?rcl=...
> > > > [2]
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_adapter.h?rcl...
> > > 
> > > It's not necessary in this CL, but I would prefer if Device itself was
> > > encapsulated as a Mojo interface. That would make the identity invariants
> > easier
> > > to understand, because things like device changed notifications would be
> > > broadcast to the Device interface representing that device. Is this
> something
> > we
> > > can consider doing in a followup?
> > 
> > Hmm. My main concern is that Device and DeviceInfo are related but used in
> > different situations. A Device interface impl is only created when a user
> > connects to a Bluetooth device and is strongly bond to the message pipe and
> its
> > Bluetooth connection. DeviceInfo is available at all times from Adapter to
> > describe the bluetooth devices that the bluetooth adapter can see. These
> values
> > may be updated without a connection and are added, removed, or changed over
> time
> > whether a Device impl exists or not.
> 
> This isn't a blocker for this CL, but one of the nice things about Mojo is
that
> things can be interface-associated and have an implicit identity. This is what
> lets us (eventually) kill IPC routing IDs and perform many other
> simplifications. It also makes code audits easier, since identity is
associated
> with the pipe, so you don't need to validate the name of the thing is valid,
> etc.
> 
> It sounds like we'd need a separate interface for tracking device status (to
> keep it distinct from the connecting to device use case), but I think it might
> be worthwhile to keep this in mind, especially as it becomes clearer how this
> interface will be used.

So, there would be some kind of message in the Adapter interface that vends a
"DeviceListener" impl that only receive messages for a specific device? I'd have
to figure out the relationship between Device and DeviceListener, but I'll keep
this in mind. I can see this working.

[mbrunson, 2017-01-06 23:16:58]

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

[commit-bot: I haz the power, 2017-01-06 23:17:27]

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-07 01:51:59]

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

[commit-bot: I haz the power, 2017-01-07 01:52:00]

Dry run: This issue passed the CQ dry run.

[mbrunson, 2017-01-07 02:21:04]

The CQ bit was checked by mbrunson@chromium.org

[mbrunson, 2017-01-07 02:21:04]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org
Link to the patchset: https://codereview.chromium.org/2564113003/#ps380001
(title: "Replace discovering string in bluetooth_internals.js")

[commit-bot: I haz the power, 2017-01-07 02:21:23]

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-07 02:56:46]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1483755664381570,
"parent_rev": "cff406a34f98ae2381c519010795eb99768eecd7", "commit_rev":
"b0f0a102a61ef7682e4aac5d5587e36ac6f6b3d6"}

[commit-bot: I haz the power, 2017-01-07 02:57:15]

Description was changed from

==========
bluetooth: Add basic scanning to chrome://bluetooth-internals.

Adds DiscoverySession to Adapter Mojo service to handle starting a new
BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

GIF: https://goo.gl/photos/9B14sgpGNiRqeoxk6

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

to

==========
bluetooth: Add basic scanning to chrome://bluetooth-internals.

Adds DiscoverySession to Adapter Mojo service to handle starting a new
BluetoothDiscoverySession.
Adds Adapter::StartDiscoverySession to create a new DiscoverySession.
Adds Scan button to chrome://bluetooth-internals to activate scanning.

GIF: https://goo.gl/photos/9B14sgpGNiRqeoxk6

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

Review-Url: https://codereview.chromium.org/2564113003
Cr-Commit-Position: refs/heads/master@{#442141}
Committed:
https://chromium.googlesource.com/chromium/src/+/b0f0a102a61ef7682e4aac5d5587...
==========

[commit-bot: I haz the power, 2017-01-07 02:57:17]

Committed patchset #20 (id:380001) as
https://chromium.googlesource.com/chromium/src/+/b0f0a102a61ef7682e4aac5d5587...