bluetooth: Implement simulateGATTConnectionResponse()

bluetooth: Implement setNextGATTConnectionResponse()

Sets the next GATT Connection request response for peripheral with
|address| to |code|. |code| could be an HCI Error Code from
BT 4.2 Vol 2 Part D 1.3 List Of Error Codes or a number outside that range
returned by specific platforms e.g. Android returns 0x101 to signal a GATT
failure

BUG=719814
Review-Url: https://codereview.chromium.org/2874873003
Cr-Commit-Position: refs/heads/master@{#476940}
Committed: https://chromium.googlesource.com/chromium/src/+/a7f7ed7b9e28bc9da80a4d36297efa9e3e31871c

[ortuno, 2017-05-11 06:46:52]

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

[commit-bot: I haz the power, 2017-05-11 06:47: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-11 09:31:22]

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

[commit-bot: I haz the power, 2017-05-11 09:31:22]

Dry run: 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:13:49]

Description was changed from

==========
bluetooth: Implement simulateGATTConnectionResponse()

BUG=
==========

to

==========
bluetooth: Implement simulateGATTConnectionResponse()

Simulates a GATT Connection request response with |code| for peripheral
with |address|. |code| could be an HCI Error Code from
BT 4.2 Vol 2 Part D 1.3 List Of Error Codes or a number outside that
range returned by specific platforms e.g. Android returns 0x101 to signal
a GATT failure
https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html#...

BUG=719814
==========

[ortuno, 2017-05-11 23:40:00]

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

[ortuno, 2017-05-11 23:40:56]

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

[ortuno, 2017-05-11 23:40:56]

scheib: PTAL. Two things to note:

1. As discussed using unsigned short for |code|.
2. No FakePeripheral mojo interface. Adding such an interface would avoid the
routing FakeCentral currently does but I think the resulting interface is less
elegant:

Current approach:
// fake_bluetooth.mojom

FakeCentral {
  SimulatePreconnectedPeripheral(...) => ();
  SimualteAdvertisementRecieved(...) => ();
}

Steps:

1. (js) simulatePreconnectedPeripheral calls
FakeCentral::SimulatePreconnectedPeripheral
2. (c++) FakeCentral::SimulatePreconnectedPeripheral just creates or update the
FakePeripheral
   corresponding to the address passed in.
3. (js) Returns a FakePeripheral, creating one if it didn't exist before.

SimulateAdvertisementReceived would follow the same pattern.

If we were to introduce a new FakePeripheral mojo interface:
// fake_bluetooth.mojom
FakeCentral {
  SimulatePreconnectedPeripheral(...) => (FakePeripheral? peripheral);
  SimualteAdvertisementRecieved(...) => (FakePeripheral? peripheral);
}

Steps:

1. (js) simulatePreconnectedPeripheral would call
FakeCentral::SimulatePreconnectedPeripheral
   (c++) FakeCentral::SimulatePreconnectedPeripheral would create or update the
FakePeripheral
   corresponding to the address passed in. If it created a new FakePeripheral it
would send
   the interface ptr over.
   (js) Returns a FakePeripheral that holds the FakePeripheral interface ptr.

SimulateAdvertisementReceived would follow the same pattern.

[commit-bot: I haz the power, 2017-05-11 23:41: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-05-11 23:47:33]

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

[commit-bot: I haz the power, 2017-05-11 23:47:34]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[ortuno, 2017-05-12 00:53:44]

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

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:457:
function getConnectedHealthThermometerDevice(
Forgot to mention this. This changes our testing pattern a bit. Before we could
set all the device's attributes and responses upfront so tests only needed to
call one function at the top. Now tests need to dispatch responses during the
test e.g. connect response after test calls gatt.connect(). Rather than trying
to simulate ON_CALL and make it so that a single call at the top would allow you
to call connect() and get a response in the test, I went with helper functions
that get you a device at the state where you want it e.g. a connected device in
this case.

The downside is that you can't see all steps in the test file. Upsides include
less complex Test API and tests are somewhat easier to read as they only contain
code relevant to the test.

[scheib, 2017-05-12 05:43:56]

https://codereview.chromium.org/2874873003/diff/60001/device/bluetooth/test/f...
File device/bluetooth/test/fake_peripheral.cc (right):

https://codereview.chromium.org/2874873003/diff/60001/device/bluetooth/test/f...
device/bluetooth/test/fake_peripheral.cc:32: CreateGattConnectionCallbacks
callbacks =
Call up to BluetoothDevice::DidConnectGatt DidFailToConnectGatt, and reuse
BluetoothDevice code.. or if not feasible, then at least flush all entries in
the queue, not just one.

if above is wrong, then at least:
  DCHECK(!create_____.empty());

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/server/connect/connection-succeeds.html
(right):

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/connection-succeeds.html:10:
return setUpHealthThermometerAndHeartRateDevices()
a) We should document what is returned by setUp....Devices methods. If we use
this one we should capture the result as something specific
'thermometer_peripheral' since we know which one it is (I think).

b) In this test, we don't need multiple devices, and I think we should simplify
it to the generic flow you used elsewhere 
  setUpPreconnectedDevice({}).then(device => 
  requestDevice ... acceptAllDevices

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/server/connect/garbage-collection-ran-during-error.html
(right):

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/garbage-collection-ran-during-error.html:15:
.then(runGarbageCollection)
We should file an issue if not already done on 'runGarbageCollection'

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/server/connect/get-same-gatt-server.html
(right):

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/get-same-gatt-server.html:17:
fake_peripheral.simulateGATTConnectionResponse({code: HCI_SUCCESS}),
Not OK to have simulate calls in parallel to the connect() calls. Also, just one
simulate(success) call should be needed.

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/bluetooth/server/device-same-object.html
(right):

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/device-same-object.html:15:
fake_peripheral.simulateGATTConnectionResponse({code: HCI_SUCCESS})
ditto...

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

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:465:
return requestDeviceWithKeyDown(options)
options parameter needed here ... because sometimes we'll need to set e.g.
optional services?

[ortuno, 2017-05-15 05:57:10]

Patchset #6 (id:100001) has been deleted

[ortuno, 2017-05-15 06:37:34]

Patchset #5 (id:80001) has been deleted

[ortuno, 2017-05-15 06:37:43]

Patchset #5 (id:120001) has been deleted

[ortuno, 2017-05-15 06:47:54]

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

[commit-bot: I haz the power, 2017-05-15 06:48:11]

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-15 07:01:31]

https://codereview.chromium.org/2874873003/diff/60001/device/bluetooth/test/f...
File device/bluetooth/test/fake_peripheral.cc (right):

https://codereview.chromium.org/2874873003/diff/60001/device/bluetooth/test/f...
device/bluetooth/test/fake_peripheral.cc:32: CreateGattConnectionCallbacks
callbacks =
On 2017/05/12 at 05:43:55, scheib wrote:
> Call up to BluetoothDevice::DidConnectGatt DidFailToConnectGatt, and reuse
BluetoothDevice code.. or if not feasible, then at least flush all entries in
the queue, not just one.
> 

Happy to change it but I would like to understand why we would like to flush the
queue? See reply in test.

> if above is wrong, then at least:
>   DCHECK(!create_____.empty());

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/server/connect/connection-succeeds.html
(right):

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/connection-succeeds.html:10:
return setUpHealthThermometerAndHeartRateDevices()
On 2017/05/12 at 05:43:55, scheib wrote:
> a) We should document what is returned by setUp....Devices methods. If we use
this one we should capture the result as something specific
'thermometer_peripheral' since we know which one it is (I think).
> 

Makes sense. Added docs.

> b) In this test, we don't need multiple devices, and I think we should
simplify it to the generic flow you used elsewhere 
>   setUpPreconnectedDevice({}).then(device => 
>   requestDevice ... acceptAllDevices

I introduced getDiscoveredHealthThermometerDevice(). 

My current plan is to have the following functions:

getHealthThermometerDevice();

getConnectedHealthThermometerDevice();

getDiscoveredHealthThermoemeterDevice();

getHealthThermometerDevice() will be similar to 'HeartRateAdapter'. It would
return a connected BluetoothDevice where all attributes have been discovered.
The other functions are for tests that require the device in an intermediate
step e.g. getDiscoveredHealthThermometerDevice() returns a non-connected device,
getConnectedHealthThermometerDevice() would return a connected device whose
services haven't been discovered yet, etc. As I implement more features, I'll
add them to the device returned by getHealthThermometerDevice() e.g. the next
patch will add services.

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/server/connect/garbage-collection-ran-during-error.html
(right):

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/garbage-collection-ran-during-error.html:15:
.then(runGarbageCollection)
On 2017/05/12 at 05:43:56, scheib wrote:
> We should file an issue if not already done on 'runGarbageCollection'

Do we need one? I wasn't planning on usptreaming this. I think we would get
quite a bit of push back on it; I've been told in the past that we should not be
testing this through layout tests but rather with unit tests.

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/bluetooth/server/connect/get-same-gatt-server.html
(right):

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/bluetooth/server/connect/get-same-gatt-server.html:17:
fake_peripheral.simulateGATTConnectionResponse({code: HCI_SUCCESS}),
On 2017/05/12 at 05:43:56, scheib wrote:
> Not OK to have simulate calls in parallel to the connect() calls.

I don't think there is an alternative. The only alternative I can think of is
equivalent to the current approach:

.then(device => {
  let promise = Promise.all([device.gatt.connect(), device.gatt.connect()]);
  return fake_peripheral.simulateGATTConnectionResponse({code: HCI_SUCCESS})
    .then(() => promise);
});

That said, I've been putting off a change to allow FakeCentral to wait until
it's observers have processed their own Mojo messages. That would guarantee that
the connection request from connect() has arrived before we try to dispatch the
simulated response. I'm currently working on a separate patch to add such a
thing to Mojo.

> Also, just one simulate(success) call should be needed.

I'm happy to change it but I would like to understand why? The API is supposed
to fake the OS's response and if we call connect twice on the OS I would expect
two responses.

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

https://codereview.chromium.org/2874873003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/resources/bluetooth/bluetooth-helpers.js:465:
return requestDeviceWithKeyDown(options)
On 2017/05/12 at 05:43:56, scheib wrote:
> options parameter needed here ... because sometimes we'll need to set e.g.
optional services?

Yeah, there are quite a few tests that check getPrimaryService(s) returns the
correct services based on the argument passed to requestDevice.

[ortuno, 2017-05-16 07:04:56]

I've been looking at our tests and I think we are over engineering it here. We
can bikeshed on the names but I think building the "dispatch if pending or when
arrives" behavior into all functions would work for all the tests we currently
have.

We can keep our tests readable and if the need arises in the future we can build
more granular functions specifically for those tricky tests.

I'll start fixing the patch tomorrow.

[ortuno, 2017-05-26 22:22:45]

Description was changed from

==========
bluetooth: Implement simulateGATTConnectionResponse()

Simulates a GATT Connection request response with |code| for peripheral
with |address|. |code| could be an HCI Error Code from
BT 4.2 Vol 2 Part D 1.3 List Of Error Codes or a number outside that
range returned by specific platforms e.g. Android returns 0x101 to signal
a GATT failure
https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html#...

BUG=719814
==========

to

==========
bluetooth: Implement setNextGATTConnectionResponse()

Simulates a GATT Connection request response with |code| for peripheral
with |address|. |code| could be an HCI Error Code from
BT 4.2 Vol 2 Part D 1.3 List Of Error Codes or a number outside that
range returned by specific platforms e.g. Android returns 0x101 to signal
a GATT failure
https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html#...

BUG=719814
==========

[ortuno, 2017-05-26 22:23:25]

Description was changed from

==========
bluetooth: Implement setNextGATTConnectionResponse()

Simulates a GATT Connection request response with |code| for peripheral
with |address|. |code| could be an HCI Error Code from
BT 4.2 Vol 2 Part D 1.3 List Of Error Codes or a number outside that
range returned by specific platforms e.g. Android returns 0x101 to signal
a GATT failure
https://developer.android.com/reference/android/bluetooth/BluetoothGatt.html#...

BUG=719814
==========

to

==========
bluetooth: Implement setNextGATTConnectionResponse()

Sets the next GATT Connection request response for peripheral with
|address| to |code|. |code| could be an HCI Error Code from
BT 4.2 Vol 2 Part D 1.3 List Of Error Codes or a number outside that range
returned by specific platforms e.g. Android returns 0x101 to signal a GATT
failure

BUG=719814
==========

[ortuno, 2017-05-26 22:29:30]

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

[commit-bot: I haz the power, 2017-05-26 22:30: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-26 23:28:56]

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

[commit-bot: I haz the power, 2017-05-26 23:29:10]

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-27 00:46:05]

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

[commit-bot: I haz the power, 2017-05-27 00:46:07]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-05-27 00:50:27]

scheib: PTAL

[scheib, 2017-05-30 20:54:14]

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/fake_peripheral.cc (right):

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/fake_peripheral.cc:105: return system_connected_ ||
gatt_connected_;
Our Android code at least doesn't work this way. Warrants at least a comment
explaining this impl.

If this was base only on gatt_connected_ then would you need to have the
CreateGattConnection method overridden?

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/fake_peripheral.h (right):

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/fake_peripheral.h:28: // Set it to indicate if the
Peripheral is connected or not.
Member variable has better comments

[ortuno, 2017-05-31 05:47:03]

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

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

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-31 05:47:27]

Thanks!

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/fake_peripheral.cc (right):

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/fake_peripheral.cc:105: return system_connected_ ||
gatt_connected_;
On 2017/05/30 at 20:54:13, scheib wrote:
> Our Android code at least doesn't work this way. Warrants at least a comment
explaining this impl.
> 

Unsure what you mean by Android doesn't work this way? Neither Android nor macOS
support system connected devices, only Chrome OS does which is what we based our
code in for the Web Bluetooth Service.

Added comment to IsGattConnected() explaining the problem in more detail.

> If this was base only on gatt_connected_ then would you need to have the
CreateGattConnection method overridden?

Correct.

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/fake_peripheral.h (right):

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/fake_peripheral.h:28: // Set it to indicate if the
Peripheral is connected or not.
On 2017/05/30 at 20:54:13, scheib wrote:
> Member variable has better comments

Done.

[commit-bot: I haz the power, 2017-05-31 07:19:21]

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

[commit-bot: I haz the power, 2017-05-31 07:19:22]

Dry run: This issue passed the CQ dry run.

[scheib, 2017-05-31 17:22:21]

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/fake_peripheral.cc (right):

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/fake_peripheral.cc:105: return system_connected_ ||
gatt_connected_;
On 2017/05/31 05:47:27, ortuno wrote:
> On 2017/05/30 at 20:54:13, scheib wrote:
> > Our Android code at least doesn't work this way. Warrants at least a comment
> explaining this impl.
> > 
> 
> Unsure what you mean by Android doesn't work this way? Neither Android nor
macOS
> support system connected devices, only Chrome OS does which is what we based
our
> code in for the Web Bluetooth Service.
> 
> Added comment to IsGattConnected() explaining the problem in more detail.
> 
> > If this was base only on gatt_connected_ then would you need to have the
> CreateGattConnection method overridden?
> 
> Correct.

OK. We discussed this some earlier. I think this is too tricky to leave in the
fake this way -- acting in a way the actual systems don't. Then having the rest
of the testing framework rely on this not-how-it-really-is behavior. IIRC You
pointed out earlier that if we make it work the way bluez does then we need to
iterate over all devices to find the pre-connected devices. Where would we need
to do that? At the JS implementation of the framework? I'd like to understand
what things would look like if the fake acted the way bluez does. Maybe best to
do over VC.

[scheib, 2017-06-01 05:03:30]

Thanks for chat. LGTM but please add doc:

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
File device/bluetooth/test/fake_peripheral.cc (right):

https://codereview.chromium.org/2874873003/diff/240001/device/bluetooth/test/...
device/bluetooth/test/fake_peripheral.cc:105: return system_connected_ ||
gatt_connected_;
On 2017/05/31 17:22:21, scheib wrote:
> On 2017/05/31 05:47:27, ortuno wrote:
> > On 2017/05/30 at 20:54:13, scheib wrote:
> > > Our Android code at least doesn't work this way. Warrants at least a
comment
> > explaining this impl.
> > > 
> > 
> > Unsure what you mean by Android doesn't work this way? Neither Android nor
> macOS
> > support system connected devices, only Chrome OS does which is what we based
> our
> > code in for the Web Bluetooth Service.
> > 
> > Added comment to IsGattConnected() explaining the problem in more detail.
> > 
> > > If this was base only on gatt_connected_ then would you need to have the
> > CreateGattConnection method overridden?
> > 
> > Correct.
> 
> OK. We discussed this some earlier. I think this is too tricky to leave in the
> fake this way -- acting in a way the actual systems don't. Then having the
rest
> of the testing framework rely on this not-how-it-really-is behavior. IIRC You
> pointed out earlier that if we make it work the way bluez does then we need to
> iterate over all devices to find the pre-connected devices. Where would we
need
> to do that? At the JS implementation of the framework? I'd like to understand
> what things would look like if the fake acted the way bluez does. Maybe best
to
> do over VC.

We've just had a VC chat. I understand more context now regarding bluez's
implementation and what our web bluetooth service impl is doing. I think the
comments could be made much clearer by referring to a document that describes
the current state of 'system connected' devices. 

The issue linked (630581) isn't easy to understand the current status of, and
doesn't do a clean job at introducing this concept. 

I suggest that issue be updated to indicate the suggested next steps for that
issue.

Here I think the comment should refer either an issue or a README.md section
describing the overall problem of system connected devices and suggested future
ways to clean things up.

[ortuno, 2017-06-02 06:52:35]

I opened two[1][2] new issue to better explain the problems and referred to them
in code. The issues also were changed based on your feedback over chat.

[ortuno, 2017-06-02 06:52:53]

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

[ortuno, 2017-06-02 06:52:55]

dcheng: PTAL at mojom.

[ortuno, 2017-06-02 06:53:02]

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

[commit-bot: I haz the power, 2017-06-02 06:53:13]

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-06-02 08:33:54]

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

[commit-bot: I haz the power, 2017-06-02 08:33:54]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-06-02 16:07:35]

ipc lgtm

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

https://codereview.chromium.org/2874873003/diff/300001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:52: DCHECK(device_iter != devices_.end());
Might be worth handling this: otherwise, I suspect Clusterfuzz might find this
and start filing bugs on you, even though it's test-only.

https://codereview.chromium.org/2874873003/diff/300001/device/bluetooth/test/...
File device/bluetooth/test/fake_peripheral.cc (right):

https://codereview.chromium.org/2874873003/diff/300001/device/bluetooth/test/...
device/bluetooth/test/fake_peripheral.cc:221:
DCHECK(!!next_connection_response_);
Random: To me, !! reads a bit odd, since it isn't needed for a boolean test of
Optional. *shrug*

I don't feel strongly about changing this though.

[ortuno, 2017-06-05 01:36:08]

The CQ bit was checked by ortuno@chromium.org

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

Thanks! fyi: I ended up changing SetNextGattResponse's callback to take a bool
to indicate failure.

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

https://codereview.chromium.org/2874873003/diff/300001/device/bluetooth/test/...
device/bluetooth/test/fake_central.cc:52: DCHECK(device_iter != devices_.end());
On 2017/06/02 at 16:07:35, dcheng wrote:
> Might be worth handling this: otherwise, I suspect Clusterfuzz might find this
and start filing bugs on you, even though it's test-only.

Ah good catch. Done.

https://codereview.chromium.org/2874873003/diff/300001/device/bluetooth/test/...
File device/bluetooth/test/fake_peripheral.cc (right):

https://codereview.chromium.org/2874873003/diff/300001/device/bluetooth/test/...
device/bluetooth/test/fake_peripheral.cc:221:
DCHECK(!!next_connection_response_);
On 2017/06/02 at 16:07:35, dcheng wrote:
> Random: To me, !! reads a bit odd, since it isn't needed for a boolean test of
Optional. *shrug*
> 
> I don't feel strongly about changing this though.

Done.

[ortuno, 2017-06-05 01:36:10]

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/2874873003/#ps320001
(title: "Address dcheng's comments")

[commit-bot: I haz the power, 2017-06-05 01:36:23]

CQ is trying da patch.

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

[ortuno, 2017-06-05 02:44:48]

The CQ bit was unchecked by ortuno@chromium.org

[ortuno, 2017-06-05 02:49:18]

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

[commit-bot: I haz the power, 2017-06-05 02:49: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, 2017-06-05 04:33:56]

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

[commit-bot: I haz the power, 2017-06-05 04:33:58]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-06-05 04:34:30]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2017-06-05 04:34:31]

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/2874873003/#ps340001
(title: "fix")

[commit-bot: I haz the power, 2017-06-05 04:34: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-06-05 04:40:22]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1496637270649450,
"parent_rev": "2e5cae49a7f640721bf81c4c68ff9b4dc8b24efa", "commit_rev":
"a7f7ed7b9e28bc9da80a4d36297efa9e3e31871c"}

[commit-bot: I haz the power, 2017-06-05 04:40:30]

Description was changed from

==========
bluetooth: Implement setNextGATTConnectionResponse()

Sets the next GATT Connection request response for peripheral with
|address| to |code|. |code| could be an HCI Error Code from
BT 4.2 Vol 2 Part D 1.3 List Of Error Codes or a number outside that range
returned by specific platforms e.g. Android returns 0x101 to signal a GATT
failure

BUG=719814
==========

to

==========
bluetooth: Implement setNextGATTConnectionResponse()

Sets the next GATT Connection request response for peripheral with
|address| to |code|. |code| could be an HCI Error Code from
BT 4.2 Vol 2 Part D 1.3 List Of Error Codes or a number outside that range
returned by specific platforms e.g. Android returns 0x101 to signal a GATT
failure

BUG=719814

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

[commit-bot: I haz the power, 2017-06-05 04:40:32]

Committed patchset #15 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/a7f7ed7b9e28bc9da80a4d36297e...

[blundell, 2017-06-05 09:16:38]

A revert of this CL (patchset #15 id:340001) has been created in
https://codereview.chromium.org/2921233002/ by blundell@chromium.org.

The reason for reverting is: Seems to cause device-same-object test to fail
consistently on Android, e.g.:

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit...
.