bluetooth: Implement simulatePreconnectedPeripheral.

bluetooth: Implement simulatePreconnectedPeripheral.

simulatePreconnectedPeripherals allows clients to simulate peripherals
that already connected to the system but that the UA doesn't know about.

Adds a new helper method to create system-connected devices and uses
the method in a couple of tests.

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

[ortuno, 2017-05-03 01:32:10]

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

[commit-bot: I haz the power, 2017-05-03 01:33:29]

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 01:43:53]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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_...)

[ortuno, 2017-05-03 05:22:55]

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

[commit-bot: I haz the power, 2017-05-03 05:23:05]

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 05:38:23]

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

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

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 05:40:45]

Description was changed from

==========
WIP: bluetooth: Implement SimulateConnectedPeripheral.
==========

to

==========
bluetooth: Implement simulateSystemConnectedPeripheral.

simulateSystemConnectedPeripherals allows clients to simulate peripherals
that already connected to the system but that the UA doesn't know about.

Adds a new helper method to create system-connected devices and uses
the method in a couple of tests.
==========

[ortuno, 2017-05-03 05:42:24]

Description was changed from

==========
bluetooth: Implement simulateSystemConnectedPeripheral.

simulateSystemConnectedPeripherals allows clients to simulate peripherals
that already connected to the system but that the UA doesn't know about.

Adds a new helper method to create system-connected devices and uses
the method in a couple of tests.
==========

to

==========
bluetooth: Implement simulateSystemConnectedPeripheral.

simulateSystemConnectedPeripherals allows clients to simulate peripherals
that already connected to the system but that the UA doesn't know about.

Adds a new helper method to create system-connected devices and uses
the method in a couple of tests.

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

[ortuno, 2017-05-03 06:29:48]

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

[ortuno, 2017-05-03 06:29:49]

scheib: PTAL

[commit-bot: I haz the power, 2017-05-03 08:04:55]

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

[commit-bot: I haz the power, 2017-05-03 08:04:55]

Dry run: This issue passed the CQ dry run.

[scheib, 2017-05-03 20:38:49]

small bits, but one important Q regarding structure of adding peripherals to the
fake central:

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

https://codereview.chromium.org/2858803003/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:44: // connected to
the system or didn't connected through the UA e.g. a
"or weren't connected through the user agent, e.g."

https://codereview.chromium.org/2858803003/diff/80001/device/bluetooth/test/f...
File device/bluetooth/test/fake_central.cc (right):

https://codereview.chromium.org/2858803003/diff/80001/device/bluetooth/test/f...
device/bluetooth/test/fake_central.cc:29: if (device_iter == devices_.end()) {
If called with the same address but a new name this logic uses the old device
with that address but doesn't update the name. Same will be true for
simulateAdvertisementReceived but in that case we know with certainty we'll be
reusing addresses. 

Here's a proposal: We have a way to create a FakePeripheral with address, name,
but it is an error to reuse an address. Then, the ID of that peripheral can be
used with simulateAdvertisementReceived or
addConnectedLEPeripheral/SimulateSystemConnectedPeripheral

Regarding the name of the previously connected method
Perhaps:
 SimulatePreconnetedPeripheral,
 SimulatePreviouslyConnetedPeripheral,
 SimulatePeripheralAlreadyConnected

Also, should the design doc be updated as we go? OK if not, but just want to
know what the plan is.
https://docs.google.com/document/d/1Nhv_oVDCodd1pEH_jj9k8gF4rPGb_84VYaZ9IG8M_...

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/requestDevice/acceptAllDevices/device-with-name.html
(right):

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice/acceptAllDevices/device-with-name.html:9:
let device_name = generate_string(9, '\u2764' /* ❤ */);
preexisting point, but why are we using generate_string?

In this location, why not just '❤❤❤❤❤❤❤❤❤' directly?
In other locations, just '\ucode'.repeat(123).

OK to ignore in this patch.

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html
(right):

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html:13:
optionalServices: ['generic_access']}))
If possible, remove optionalServices line.

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

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:111: //
connected to the system or didn't connected through the UA e.g. a
ditto

[ortuno, 2017-05-04 04:16:21]

FYI simulatePreconnectedPeripheral in web-bluetooth-test.js changed a bit.

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

https://codereview.chromium.org/2858803003/diff/80001/device/bluetooth/public...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:44: // connected to
the system or didn't connected through the UA e.g. a
On 2017/05/03 at 20:38:48, scheib wrote:
> "or weren't connected through the user agent, e.g."

Done.

https://codereview.chromium.org/2858803003/diff/80001/device/bluetooth/test/f...
File device/bluetooth/test/fake_central.cc (right):

https://codereview.chromium.org/2858803003/diff/80001/device/bluetooth/test/f...
device/bluetooth/test/fake_central.cc:29: if (device_iter == devices_.end()) {
On 2017/05/03 at 20:38:49, scheib wrote:
> If called with the same address but a new name this logic uses the old device
with that address but doesn't update the name. Same will be true for
simulateAdvertisementReceived but in that case we know with certainty we'll be
reusing addresses. 
> 
> Here's a proposal: We have a way to create a FakePeripheral with address,
name, but it is an error to reuse an address. Then, the ID of that peripheral
can be used with simulateAdvertisementReceived or
addConnectedLEPeripheral/SimulateSystemConnectedPeripheral
> 

Good catch. This is at the JS API level right? A previous iteration of the API
actually had something similar. The issue was that for addConnectedLEPeripheral
the name needs to be defined whereas for simulateAdvertisementReceived one can
omit the name. jyasskin[1] suggested splitting the dictionary needed for both
functions instead of defining the behavior in prose which lead to the current
API.

I planned to revisit having the name in the constructor once other functions
would need to create peripherals with no name. I overlooked the case you
mentioned. Since we'll need it eventually anyway I added a setter for the name.

[1]
https://docs.google.com/a/google.com/document/d/16HJ15evg_QYrWDMTIy1Ucafp2qgw...

> Regarding the name of the previously connected method
> Perhaps:
>  SimulatePreconnetedPeripheral,
>  SimulatePreviouslyConnetedPeripheral,
>  SimulatePeripheralAlreadyConnected
> 

Went with simulatePreconnetedPeripheral.

> Also, should the design doc be updated as we go? OK if not, but just want to
know what the plan is.
>
https://docs.google.com/document/d/1Nhv_oVDCodd1pEH_jj9k8gF4rPGb_84VYaZ9IG8M_...

That is my intention. I took another look and updated relevant parts. I'll try
to do a better job once CLs land.

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/requestDevice/acceptAllDevices/device-with-name.html
(right):

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice/acceptAllDevices/device-with-name.html:9:
let device_name = generate_string(9, '\u2764' /* ❤ */);
On 2017/05/03 at 20:38:49, scheib wrote:
> preexisting point, but why are we using generate_string?
> 
> In this location, why not just '❤❤❤❤❤❤❤❤❤' directly?

Changed it to 'LE Device' since there is no need for the unicode characters.

> In other locations, just '\ucode'.repeat(123).
> 
> OK to ignore in this patch.

Not sure why we added that function O.o http://crrev.com/2858183002

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html
(right):

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/requestDevice/name-empty-device-from-name-empty-filter.html:13:
optionalServices: ['generic_access']}))
On 2017/05/03 at 20:38:49, scheib wrote:
> If possible, remove optionalServices line.

Done.

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

https://codereview.chromium.org/2858803003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:111: //
connected to the system or didn't connected through the UA e.g. a
On 2017/05/03 at 20:38:49, scheib wrote:
> ditto

Done.

[ortuno, 2017-05-04 04:16:27]

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

[commit-bot: I haz the power, 2017-05-04 04:16:39]

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-04 04:19:45]

Description was changed from

==========
bluetooth: Implement simulateSystemConnectedPeripheral.

simulateSystemConnectedPeripherals allows clients to simulate peripherals
that already connected to the system but that the UA doesn't know about.

Adds a new helper method to create system-connected devices and uses
the method in a couple of tests.

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

to

==========
bluetooth: Implement simulatePreconnectedPeripheral.

simulatePreconnectedPeripherals allows clients to simulate peripherals
that already connected to the system but that the UA doesn't know about.

Adds a new helper method to create system-connected devices and uses
the method in a couple of tests.

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

[scheib, 2017-05-04 04:54:21]

https://codereview.chromium.org/2858803003/diff/80001/device/bluetooth/test/f...
File device/bluetooth/test/fake_central.cc (right):

https://codereview.chromium.org/2858803003/diff/80001/device/bluetooth/test/f...
device/bluetooth/test/fake_central.cc:29: if (device_iter == devices_.end()) {
On 2017/05/04 04:16:21, ortuno wrote:
> On 2017/05/03 at 20:38:49, scheib wrote:
> > If called with the same address but a new name this logic uses the old
device
> with that address but doesn't update the name. Same will be true for
> simulateAdvertisementReceived but in that case we know with certainty we'll be
> reusing addresses. 
> > 
> > Here's a proposal: We have a way to create a FakePeripheral with address,
> name, but it is an error to reuse an address. Then, the ID of that peripheral
> can be used with simulateAdvertisementReceived or
> addConnectedLEPeripheral/SimulateSystemConnectedPeripheral
> > 
> 
> Good catch. This is at the JS API level right? A previous iteration of the API
> actually had something similar. The issue was that for
addConnectedLEPeripheral
> the name needs to be defined whereas for simulateAdvertisementReceived one can
> omit the name. jyasskin[1] suggested splitting the dictionary needed for both
> functions instead of defining the behavior in prose which lead to the current
> API.
> 
> I planned to revisit having the name in the constructor once other functions
> would need to create peripherals with no name. I overlooked the case you
> mentioned. Since we'll need it eventually anyway I added a setter for the
name.
> 
> [1]
>
https://docs.google.com/a/google.com/document/d/16HJ15evg_QYrWDMTIy1Ucafp2qgw...
> 

Even with your update the logic sets the name if the peripheral hasn't been
created already. But doesn't set the name if the device_iter found a peripheral
already with that address.

I think I go back to my suggestion:

Have the API allow a peripheral to be created, but not known to the central yet.
When doing so, it gets an address and a name.

Have two methods to inform the central of the peripheral:
address = createFakePeripheral({address, name})
  // either error if an address is reused in a test, or just update the name (is
that a thing real devices can do)?
simulatePreconnectedPeripheral(address)
simulateAdvertisementReceived({address, rssi, ...})

https://codereview.chromium.org/2858803003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js
(right):

https://codereview.chromium.org/2858803003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:115:
let peripheral_id = await this.fake_central_ptr_
Do we have reasons to use 'id' and not just address?

[scheib, 2017-05-04 05:14:45]

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

https://codereview.chromium.org/2858803003/diff/140001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:40: fake_peripheral->SetName(name);
Here it is, I missed it.

[ortuno, 2017-05-04 07:02:21]

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

https://codereview.chromium.org/2858803003/diff/140001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:40: fake_peripheral->SetName(name);
On 2017/05/04 at 05:14:45, scheib wrote:
> Here it is, I missed it.

:)

As discussed leaving the logic as is. I don't think the JS API is set in stone
and I'm very open to changing it as we implement it and find what patterns work
better for our tests.

https://codereview.chromium.org/2858803003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js
(right):

https://codereview.chromium.org/2858803003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/resources/bluetooth/web-bluetooth-test.js:115:
let peripheral_id = await this.fake_central_ptr_
On 2017/05/04 at 04:54:20, scheib wrote:
> Do we have reasons to use 'id' and not just address?

I was trying to get away from our reliance on address since some platforms don't
support it. Now that I think about it, it doesn't make much sense. Changed to
just address.

[scheib, 2017-05-04 21:42:31]

lgtm

[ortuno, 2017-05-04 21:50:02]

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

[ortuno, 2017-05-04 21:50:03]

dcheng: PTAL

[dcheng, 2017-05-05 06:18:48]

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

https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/publi...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:49:
SimulatePreconnectedPeripheral(string address, string name) => ();
Is there any structure to either of these fields? The ARC++ Bluetooth code
passes address as a fixed-length byte buffer IIRC. Can we/should we be doing
that here?

IIRC, using more structured identifiers was a request from palmer@ as well in
other parts of the web blutooth mojom interface.

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

https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:28: base::MakeUnique<FakePeripheral>(this,
address);
Nit: it's OK to omit the variable type and just use 'auto' if we're using
MakeUnique

[ortuno, 2017-05-08 01:08:13]

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

https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/publi...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:49:
SimulatePreconnectedPeripheral(string address, string name) => ();
On 2017/05/05 at 06:18:48, dcheng wrote:
> Is there any structure to either of these fields?

They are MAC Address so "00:00:00:00:00:00".

> The ARC++ Bluetooth code passes address as a fixed-length byte buffer IIRC.
Can we/should we be doing that here?
> 

It's unclear to me why we pass the address as an array for Arc++. Most users of
the device/bluetooth need the address in form of a string e.g. extensions,
system tray, etc.

We could add a struct and validate the format but I think the benefit,
validating test addresses, might not be worth it.

> IIRC, using more structured identifiers was a request from palmer@ as well in
other parts of the web blutooth mojom interface.

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

https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:28: base::MakeUnique<FakePeripheral>(this,
address);
On 2017/05/05 at 06:18:48, dcheng wrote:
> Nit: it's OK to omit the variable type and just use 'auto' if we're using
MakeUnique

Good catch. Done.

[dcheng, 2017-05-09 04:20:07]

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

https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/publi...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:49:
SimulatePreconnectedPeripheral(string address, string name) => ();
On 2017/05/08 01:08:13, ortuno wrote:
> On 2017/05/05 at 06:18:48, dcheng wrote:
> > Is there any structure to either of these fields?
> 
> They are MAC Address so "00:00:00:00:00:00".
> 
> > The ARC++ Bluetooth code passes address as a fixed-length byte buffer IIRC.
> Can we/should we be doing that here?
> > 
> 
> It's unclear to me why we pass the address as an array for Arc++. Most users
of
> the device/bluetooth need the address in form of a string e.g. extensions,
> system tray, etc.
> 
> We could add a struct and validate the format but I think the benefit,
> validating test addresses, might not be worth it.

Does this show up in other parts of the mojo interface? It's more about using a
consistent type across both non-test and test interfaces, and making sure that
we're handling them uniformly.

> 
> > IIRC, using more structured identifiers was a request from palmer@ as well
in
> other parts of the web blutooth mojom interface.

[ortuno, 2017-05-09 04:25:12]

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

https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/publi...
device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:49:
SimulatePreconnectedPeripheral(string address, string name) => ();
On 2017/05/09 at 04:20:07, dcheng wrote:
> On 2017/05/08 01:08:13, ortuno wrote:
> > On 2017/05/05 at 06:18:48, dcheng wrote:
> > > Is there any structure to either of these fields?
> > 
> > They are MAC Address so "00:00:00:00:00:00".
> > 
> > > The ARC++ Bluetooth code passes address as a fixed-length byte buffer
IIRC.
> > Can we/should we be doing that here?
> > > 
> > 
> > It's unclear to me why we pass the address as an array for Arc++. Most users
of
> > the device/bluetooth need the address in form of a string e.g. extensions,
> > system tray, etc.
> > 
> > We could add a struct and validate the format but I think the benefit,
> > validating test addresses, might not be worth it.
> 
> Does this show up in other parts of the mojo interface? It's more about using
a consistent type across both non-test and test interfaces, and making sure that
we're handling them uniformly.
> 

Besides Arc++, the only Bluetooth related Mojo interface we currently have is
web_bluetooth.mojom which does not use the address (it uses WebBluetoothDeviceId
which as you point out palmer@ requested). 

There is a chance the eventual BT Mojo interface/service would use a type for
it.

> > 
> > > IIRC, using more structured identifiers was a request from palmer@ as well
in
> > other parts of the web blutooth mojom interface.

[dcheng, 2017-05-10 05:04:54]

On 2017/05/09 04:25:12, ortuno wrote:
>
https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/publi...
> File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):
> 
>
https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/publi...
> device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:49:
> SimulatePreconnectedPeripheral(string address, string name) => ();
> On 2017/05/09 at 04:20:07, dcheng wrote:
> > On 2017/05/08 01:08:13, ortuno wrote:
> > > On 2017/05/05 at 06:18:48, dcheng wrote:
> > > > Is there any structure to either of these fields?
> > > 
> > > They are MAC Address so "00:00:00:00:00:00".
> > > 
> > > > The ARC++ Bluetooth code passes address as a fixed-length byte buffer
> IIRC.
> > > Can we/should we be doing that here?
> > > > 
> > > 
> > > It's unclear to me why we pass the address as an array for Arc++. Most
users
> of
> > > the device/bluetooth need the address in form of a string e.g. extensions,
> > > system tray, etc.
> > > 
> > > We could add a struct and validate the format but I think the benefit,
> > > validating test addresses, might not be worth it.
> > 
> > Does this show up in other parts of the mojo interface? It's more about
using
> a consistent type across both non-test and test interfaces, and making sure
that
> we're handling them uniformly.
> > 
> 
> Besides Arc++, the only Bluetooth related Mojo interface we currently have is
> web_bluetooth.mojom which does not use the address (it uses
WebBluetoothDeviceId
> which as you point out palmer@ requested). 

From an IPC perspective, it would be preferable to send a binary device ID (the
"128bit string"), which I assume can be any arbitrary binary data, as opposed to
serializing it as a base-64 encoded string and then attempting to enforce
structure on the string.

> 
> There is a chance the eventual BT Mojo interface/service would use a type for
> it.
> 
> > > 
> > > > IIRC, using more structured identifiers was a request from palmer@ as
well
> in
> > > other parts of the web blutooth mojom interface.

LGTM

[ortuno, 2017-05-10 07:10:21]

On 2017/05/10 at 05:04:54, dcheng wrote:
> On 2017/05/09 04:25:12, ortuno wrote:
> >
https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/publi...
> > File device/bluetooth/public/interfaces/test/fake_bluetooth.mojom (right):
> > 
> >
https://codereview.chromium.org/2858803003/diff/160001/device/bluetooth/publi...
> > device/bluetooth/public/interfaces/test/fake_bluetooth.mojom:49:
> > SimulatePreconnectedPeripheral(string address, string name) => ();
> > On 2017/05/09 at 04:20:07, dcheng wrote:
> > > On 2017/05/08 01:08:13, ortuno wrote:
> > > > On 2017/05/05 at 06:18:48, dcheng wrote:
> > > > > Is there any structure to either of these fields?
> > > > 
> > > > They are MAC Address so "00:00:00:00:00:00".
> > > > 
> > > > > The ARC++ Bluetooth code passes address as a fixed-length byte buffer
> > IIRC.
> > > > Can we/should we be doing that here?
> > > > > 
> > > > 
> > > > It's unclear to me why we pass the address as an array for Arc++. Most
users
> > of
> > > > the device/bluetooth need the address in form of a string e.g.
extensions,
> > > > system tray, etc.
> > > > 
> > > > We could add a struct and validate the format but I think the benefit,
> > > > validating test addresses, might not be worth it.
> > > 
> > > Does this show up in other parts of the mojo interface? It's more about
using
> > a consistent type across both non-test and test interfaces, and making sure
that
> > we're handling them uniformly.
> > > 
> > 
> > Besides Arc++, the only Bluetooth related Mojo interface we currently have
is
> > web_bluetooth.mojom which does not use the address (it uses
WebBluetoothDeviceId
> > which as you point out palmer@ requested). 
> 
> From an IPC perspective, it would be preferable to send a binary device ID
(the "128bit string"), which I assume can be any arbitrary binary data, as
opposed to serializing it as a base-64 encoded string and then attempting to
enforce structure on the string.
> 

That makes sense. Opened http://crbug.com/720244

> > 
> > There is a chance the eventual BT Mojo interface/service would use a type
for
> > it.
> > 
> > > > 
> > > > > IIRC, using more structured identifiers was a request from palmer@ as
well
> > in
> > > > other parts of the web blutooth mojom interface.
> 
> LGTM

[ortuno, 2017-05-10 07:10:31]

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

[commit-bot: I haz the power, 2017-05-10 07:11: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, 2017-05-10 08:14:53]

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

[commit-bot: I haz the power, 2017-05-10 08:14:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ortuno, 2017-05-10 21:05:23]

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

[commit-bot: I haz the power, 2017-05-10 21:06:37]

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-10 23:38:20]

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

[commit-bot: I haz the power, 2017-05-10 23:38:21]

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

[ortuno, 2017-05-11 21:08:56]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-05-11 21:08:57]

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/2858803003/#ps220001
(title: "Use once callback")

[commit-bot: I haz the power, 2017-05-11 21:09:41]

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-11 23:43:23]

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

[commit-bot: I haz the power, 2017-05-11 23:43:24]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ortuno, 2017-05-11 23:46:53]

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

[commit-bot: I haz the power, 2017-05-11 23:48:13]

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-11 23:52:19]

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

[commit-bot: I haz the power, 2017-05-11 23:53:07]

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-12 01:56:18]

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

[commit-bot: I haz the power, 2017-05-12 01:56:19]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-05-12 02:30:55]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-05-12 02:30:55]

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/2858803003/#ps240001
(title: "Merge branch 'master' into bluetooth-simulate-preconnected")

[commit-bot: I haz the power, 2017-05-12 02:31:25]

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-12 02:37:27]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1494556255087900,
"parent_rev": "b26c3a43ba0a153e42639400d53ded106f5791a9", "commit_rev":
"db0faa445ba5669a0cba507ea02308832d1cb2f1"}

[commit-bot: I haz the power, 2017-05-12 02:37:37]

Description was changed from

==========
bluetooth: Implement simulatePreconnectedPeripheral.

simulatePreconnectedPeripherals allows clients to simulate peripherals
that already connected to the system but that the UA doesn't know about.

Adds a new helper method to create system-connected devices and uses
the method in a couple of tests.

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

to

==========
bluetooth: Implement simulatePreconnectedPeripheral.

simulatePreconnectedPeripherals allows clients to simulate peripherals
that already connected to the system but that the UA doesn't know about.

Adds a new helper method to create system-connected devices and uses
the method in a couple of tests.

BUG=569709

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

[commit-bot: I haz the power, 2017-05-12 02:37:39]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/db0faa445ba5669a0cba507ea023...