bluetooth: Implement simulateCentral

bluetooth: Implement simulateCentral

FakeCentral allows clients to simulate events that a device
in the Central/Observer role would receive as well as monitor the operations
performed by the device in the Central/Observer role.

BUG=569709
Review-Url: https://codereview.chromium.org/2853433002
Cr-Commit-Position: refs/heads/master@{#469289}
Committed: https://chromium.googlesource.com/chromium/src/+/3833c3878b778637ca7769f91e8a191a36379850

[ortuno, 2017-04-28 01:42:45]

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

[commit-bot: I haz the power, 2017-04-28 01:44:15]

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-04-28 02:33:19]

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

[commit-bot: I haz the power, 2017-04-28 02:33:20]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ortuno, 2017-04-28 03:24:20]

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

[commit-bot: I haz the power, 2017-04-28 03:24:54]

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-04-28 05:10:09]

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

[commit-bot: I haz the power, 2017-04-28 05:10:10]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-04-28 05:11:57]

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

[ortuno, 2017-04-28 05:11:58]

scheib: PTAL

[scheib, 2017-04-29 02:07:52]

Code seems OK. Naming ... comment below. I'm out of time just now and haven't
read the bluetooth spec section on roles yet, maybe this really is the best
name. Adapter is the wrong name?

https://codereview.chromium.org/2853433002/diff/40001/device/bluetooth/public...
File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):

https://codereview.chromium.org/2853433002/diff/40001/device/bluetooth/public...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:33: //
FakeLECentralObserverManager allows clients to simulate events that a device
This name is... hard. I don't have a better name yet, but ... we should simplify
it. Do we need both Central and Observer in the name? Maybe we use just one, and
in this comment indicate that in Bluetooth terms it is both?

[ortuno, 2017-05-01 06:09:04]

https://codereview.chromium.org/2853433002/diff/40001/device/bluetooth/public...
File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):

https://codereview.chromium.org/2853433002/diff/40001/device/bluetooth/public...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:33: //
FakeLECentralObserverManager allows clients to simulate events that a device
On 2017/04/29 at 02:07:52, scheib wrote:
> This name is... hard. I don't have a better name yet, but ... we should
simplify it. Do we need both Central and Observer in the name? Maybe we use just
one, and in this comment indicate that in Bluetooth terms it is both?

Currently we have an Adapter class that exposes all types of operations that can
be performed on the Adapter e.g. powering it on, changing it's name, scanning,
etc. But only Android and BlueZ expose many of these operations, which has led
us to have a bunch of NOTIMPLEMENTED or #if defined(...). To avoid that I think
we should split the interface into:

AdapterManager (ChromeOS only)
CentralObserverManager (All platforms) actual name tbd
PeripheralBroadcasterManager (All platforms) actual name tbd

Fake -> Because it's not the actual interface.
LE -> To make it easy to distinguish between LE and Classic interfaces. Though
if we are getting rid of classic support we might as well drop the "LE".
Central -> On the spec Central handles connection to devices
Observer -> On the spec Observer handles discovery of devices
Manager -> Because there are certain operations that aren't related to either
Central or Observer e.g. current state?

What do you think about: FakeRemoteManager? That would imply that eventually we
would have a RemoteManager and LocalManager interfaces. The former for
Central/Observer operation and the later for Peripheral/Broadcaster operations.

[scheib, 2017-05-01 18:16:59]

https://codereview.chromium.org/2853433002/diff/40001/device/bluetooth/public...
File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):

https://codereview.chromium.org/2853433002/diff/40001/device/bluetooth/public...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:33: //
FakeLECentralObserverManager allows clients to simulate events that a device
> Currently we have an Adapter class that exposes all types of operations that
can
> be performed on the Adapter e.g. powering it on, changing it's name, scanning,
> etc. But only Android and BlueZ expose many of these operations, which has led
> us to have a bunch of NOTIMPLEMENTED or #if defined(...). To avoid that I
think
> we should split the interface into:

Splitting SGTM

> 
> AdapterManager (ChromeOS only)
> CentralObserverManager (All platforms) actual name tbd
> PeripheralBroadcasterManager (All platforms) actual name tbd
> 
> Fake -> Because it's not the actual interface.
> LE -> To make it easy to distinguish between LE and Classic interfaces. Though
> if we are getting rid of classic support we might as well drop the "LE".

Classic on ChromeOS is going to stay for a while, but perhaps we can move those
class names to be very profile specific, e.g. audio.

LE is OK. GATT may be more specific and better. But I do think we could omit any
tagging of things moving forward especially if we use terms that are from
GAP/GATT. We can start renaming classic types more, e.g. when needed call
something BluetoothClassicFoo, if we need differentiation.

> Central -> On the spec Central handles connection to devices
> Observer -> On the spec Observer handles discovery of devices

I think the salient part is Central/Peripheral for our APIs. Broadcaster /
Observer seem to only describe if ad packets can be sent / received.

Do we ever expect to be a Central that isn't an Observer? No.
Do we ever expect to be a Peripheral that isn't a Broadcaster? No.

The chance of being a Central + Broadcaster and NOT a Peripheral seems very low
to me.

But, maybe we should split out classes for specifically each of the Central,
Peripheral, Observer, Broadcaster roles. That would keep APIs very focused. 

But, if we have the functionality of Central & Observer merged into one
interface I think we can omit the 'Observer' from the interface name.


> Manager -> Because there are certain operations that aren't related to either
> Central or Observer e.g. current state?

Manager doesn't seem to help differentiate from other interface types - I would
omit it.

 
> What do you think about: FakeRemoteManager? That would imply that eventually
we
> would have a RemoteManager and LocalManager interfaces. The former for
> Central/Observer operation and the later for Peripheral/Broadcaster
operations.

We do have some use of {Remote, Local} terminology already. Android APIs just
have attributes already mutable and able to be used as remote or local. Core
bluetooth has subclasses that enable attributes to be configured as local. In
Web Bluetooth I think we were heading down the {Remote, Local} terminology
route. All these seem fairly fine to me right now.


What do you think of:

in a bluetooth namespace:

Adapter
Observer - scanning
Broadcaster - advertizing
Central - connections
Peripheral - registering to serve a GATT tree
Service
LocalService
RemoteService
Characteristic
Local...

Fake versions have 'Fake' prefix.

[ortuno, 2017-05-02 01:14:10]

https://codereview.chromium.org/2853433002/diff/40001/device/bluetooth/public...
File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):

https://codereview.chromium.org/2853433002/diff/40001/device/bluetooth/public...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:33: //
FakeLECentralObserverManager allows clients to simulate events that a device
On 2017/05/01 at 18:16:59, scheib wrote:
> > Currently we have an Adapter class that exposes all types of operations that
can
> > be performed on the Adapter e.g. powering it on, changing it's name,
scanning,
> > etc. But only Android and BlueZ expose many of these operations, which has
led
> > us to have a bunch of NOTIMPLEMENTED or #if defined(...). To avoid that I
think
> > we should split the interface into:
> 
> Splitting SGTM
> 
> > 
> > AdapterManager (ChromeOS only)
> > CentralObserverManager (All platforms) actual name tbd
> > PeripheralBroadcasterManager (All platforms) actual name tbd
> > 
> > Fake -> Because it's not the actual interface.
> > LE -> To make it easy to distinguish between LE and Classic interfaces.
Though
> > if we are getting rid of classic support we might as well drop the "LE".
> 
> Classic on ChromeOS is going to stay for a while, but perhaps we can move
those class names to be very profile specific, e.g. audio.
> 
> LE is OK. GATT may be more specific and better. But I do think we could omit
any tagging of things moving forward especially if we use terms that are from
GAP/GATT. We can start renaming classic types more, e.g. when needed call
something BluetoothClassicFoo, if we need differentiation.
> 
> > Central -> On the spec Central handles connection to devices
> > Observer -> On the spec Observer handles discovery of devices
> 
> I think the salient part is Central/Peripheral for our APIs. Broadcaster /
Observer seem to only describe if ad packets can be sent / received.
> 
> Do we ever expect to be a Central that isn't an Observer? No.
> Do we ever expect to be a Peripheral that isn't a Broadcaster? No.
> 
> The chance of being a Central + Broadcaster and NOT a Peripheral seems very
low to me.
> 
> But, maybe we should split out classes for specifically each of the Central,
Peripheral, Observer, Broadcaster roles. That would keep APIs very focused. 
> 
> But, if we have the functionality of Central & Observer merged into one
interface I think we can omit the 'Observer' from the interface name.
> 
> 
> > Manager -> Because there are certain operations that aren't related to
either
> > Central or Observer e.g. current state?
> 
> Manager doesn't seem to help differentiate from other interface types - I
would omit it.
> 
>  
> > What do you think about: FakeRemoteManager? That would imply that eventually
we
> > would have a RemoteManager and LocalManager interfaces. The former for
> > Central/Observer operation and the later for Peripheral/Broadcaster
operations.
> 
> We do have some use of {Remote, Local} terminology already. Android APIs just
have attributes already mutable and able to be used as remote or local. Core
bluetooth has subclasses that enable attributes to be configured as local. In
Web Bluetooth I think we were heading down the {Remote, Local} terminology
route. All these seem fairly fine to me right now.
> 
> 
> What do you think of:
> 
> in a bluetooth namespace:
> 
> Adapter
> Observer - scanning
> Broadcaster - advertizing
> Central - connections
> Peripheral - registering to serve a GATT tree
> Service
> LocalService
> RemoteService
> Characteristic
> Local...
> 
> Fake versions have 'Fake' prefix.

I agree on most points. I think Splitting Central/Observer is too much
granularity that is not really useful: as you said there will rarely be a
Central that isn't an Observer. Went with CentralFake. The code is much more
readable :)

[ortuno, 2017-05-02 01:14:15]

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

[commit-bot: I haz the power, 2017-05-02 01:14:57]

Dry run: CQ is trying da patch.

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

[ortuno, 2017-05-02 01:38:14]

Description was changed from

==========
bluetooth: Implement simulateCentralObserverManager

FakeLECentralObserverManager allows clients to simulate events that a device
in the Central/Observer role would receive as well as monitor the operations
performed by the device in the Central/Observer role.

BUG=569709
==========

to

==========
bluetooth: Implement simulateCentral

FakeCentral allows clients to simulate events that a device
in the Central/Observer role would receive as well as monitor the operations
performed by the device in the Central/Observer role.

BUG=569709
==========

[commit-bot: I haz the power, 2017-05-02 03:18:39]

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

[commit-bot: I haz the power, 2017-05-02 03:18:39]

Dry run: This issue passed the CQ dry run.

[scheib, 2017-05-02 03:27:00]

Much better, thanks, LGTM

https://codereview.chromium.org/2853433002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js
(right):

https://codereview.chromium.org/2853433002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:72:
case 'poweredoff':
poweredOff?

[ortuno, 2017-05-03 00:16:47]

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

[commit-bot: I haz the power, 2017-05-03 00:17:14]

Dry run: CQ is trying da patch.

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

[ortuno, 2017-05-03 00:17:42]

https://codereview.chromium.org/2853433002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js
(right):

https://codereview.chromium.org/2853433002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:72:
case 'poweredoff':
On 2017/05/02 at 03:26:59, scheib wrote:
> poweredOff?

tldr; went with powered-off/powered-on

WebIDl "strongly suggests"[1] enums should be all lower case and separated with
dashes or not at all depending on other similar APIs. I looked at Presentation,
NFC and Generic Sensors. Presentation doesn't separate[2], nfc and generic
sensors do[3][4]. So I went with separation.

[1] https://heycam.github.io/webidl/#idl-enums
[2]
https://www.w3.org/TR/presentation-api/#h-interface-presentationconnectionclo...
[3] https://w3c.github.io/web-nfc/#h-the-nfcwatchoptions-dictionary
[4] https://www.w3.org/TR/generic-sensor/#example-webidl

[ortuno, 2017-05-03 00:20:53]

The CQ bit was unchecked by ortuno@chromium.org

[ortuno, 2017-05-03 00:39:28]

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

[commit-bot: I haz the power, 2017-05-03 00:40: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, 2017-05-03 02:18:44]

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

[commit-bot: I haz the power, 2017-05-03 02:18:46]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-05-03 02:47:56]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-05-03 02:47:57]

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/2853433002/#ps100001
(title: "call setLESupported in simulateCentral")

[commit-bot: I haz the power, 2017-05-03 02:48: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-05-03 02:56:23]

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

[commit-bot: I haz the power, 2017-05-03 02:56:24]

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

[ortuno, 2017-05-03 02:58:16]

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

[ortuno, 2017-05-03 02:58:17]

dcheng: PTAL at device/bluetooth/public/interfaces/test/fake_bluetooth.mojom

[dcheng, 2017-05-04 00:55:04]

mostly rubberstamp lgtm as this is a testing mojo interface only installed for
layout tests

(Unrelated: I find Bluetooth super jargon-y, I had to go look at the giant 2800+
page Bluetooth spec to understand what a "Central" is)

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:37: // Central. See
Bluetooth 4.2 Vol 3 Part C 2.2.2 "Roles when Operation over an
Operation => Operating?

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/fake_bluetooth.cc (right):

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/fake_bluetooth.cc:38: fake_central_ = new
FakeCentral(state, mojo::MakeRequest(&fake_central_ptr));
Nit: maybe use base::MakeShared<FakeCentral> to make it clear that this isn't
just assigning to a raw pointer

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/fake_central.cc (right):

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:14: : state_(state), binding_(this,
std::move(request)) {}
Nit: #include <utility>

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:20: return "";
Nit: Prefer std::string() over implicit construction from ""

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/fake_central.h (right):

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/fake_central.h:58: const base::TimeDelta& max,
It would be nice to fix this in a followup, but we prefer to pass base::Time* by
value rather than by reference.

[ortuno, 2017-05-04 05:28:25]

Thanks!

On 2017/05/04 at 00:55:04, dcheng wrote:
> mostly rubberstamp lgtm as this is a testing mojo interface only installed for
layout tests
> 
> (Unrelated: I find Bluetooth super jargon-y, I had to go look at the giant
2800+ page Bluetooth spec to understand what a "Central" is)
> 

Oh noes :( I added a short description of the roles to make it easier to
understand.

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/publi...
File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/publi...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:37: // Central. See
Bluetooth 4.2 Vol 3 Part C 2.2.2 "Roles when Operation over an
On 2017/05/04 at 00:55:04, dcheng wrote:
> Operation => Operating?

Done.

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/fake_bluetooth.cc (right):

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/fake_bluetooth.cc:38: fake_central_ = new
FakeCentral(state, mojo::MakeRequest(&fake_central_ptr));
On 2017/05/04 at 00:55:04, dcheng wrote:
> Nit: maybe use base::MakeShared<FakeCentral> to make it clear that this isn't
just assigning to a raw pointer

Done.

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/fake_central.cc (right):

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:14: : state_(state), binding_(this,
std::move(request)) {}
On 2017/05/04 at 00:55:04, dcheng wrote:
> Nit: #include <utility>

Done. Also added a couple missing ones here and in other files.

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:20: return "";
On 2017/05/04 at 00:55:04, dcheng wrote:
> Nit: Prefer std::string() over implicit construction from ""

Done.

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
File device/bluetooth/test/fake_central.h (right):

https://codereview.chromium.org/2853433002/diff/100001/device/bluetooth/test/...
device/bluetooth/test/fake_central.h:58: const base::TimeDelta& max,
On 2017/05/04 at 00:55:04, dcheng wrote:
> It would be nice to fix this in a followup, but we prefer to pass base::Time*
by value rather than by reference.

Good catch: http://crbug.com/718308

[ortuno, 2017-05-04 05:28:30]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-05-04 05:28:30]

The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2853433002/#ps120001
(title: "Address dcheng's comments")

[commit-bot: I haz the power, 2017-05-04 05:28:36]

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-05-04 07:12:05]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493875710265630,
"parent_rev": "265b2b33274e914c1a54d292f3164dfd96840b4e", "commit_rev":
"3833c3878b778637ca7769f91e8a191a36379850"}

[commit-bot: I haz the power, 2017-05-04 07:13:12]

Description was changed from

==========
bluetooth: Implement simulateCentral

FakeCentral allows clients to simulate events that a device
in the Central/Observer role would receive as well as monitor the operations
performed by the device in the Central/Observer role.

BUG=569709
==========

to

==========
bluetooth: Implement simulateCentral

FakeCentral allows clients to simulate events that a device
in the Central/Observer role would receive as well as monitor the operations
performed by the device in the Central/Observer role.

BUG=569709

Review-Url: https://codereview.chromium.org/2853433002
Cr-Commit-Position: refs/heads/master@{#469289}
Committed:
https://chromium.googlesource.com/chromium/src/+/3833c3878b778637ca7769f91e8a...
==========

[commit-bot: I haz the power, 2017-05-04 07:13:13]

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