bluetooth: Require context is connected for getCharacteristic(s)

bluetooth: Require context is connected for getCharacteristic(s)

Promises reject if the frame is not connected or if the frame disconnected
during the getCharacteristic(s) operation.

BUG=608538
Committed: https://crrev.com/3aecaf01516c28df06c0b29881678a4548cbdfcf
Cr-Commit-Position: refs/heads/master@{#424113}

[ortuno, 2016-10-06 03:36:16]

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

[commit-bot: I haz the power, 2016-10-06 03:36:40]

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-10-06 04:15:51]

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

[commit-bot: I haz the power, 2016-10-06 04:15:52]

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

[ortuno, 2016-10-06 04:23:24]

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

[commit-bot: I haz the power, 2016-10-06 04:23:35]

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-10-06 05:40:13]

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

[commit-bot: I haz the power, 2016-10-06 05:40:14]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-10-06 07:01:09]

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

[ortuno, 2016-10-06 07:01:10]

jyasskin: PTAL

[ortuno, 2016-10-06 07:01:14]

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

[commit-bot: I haz the power, 2016-10-06 07:02:00]

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

[ortuno, 2016-10-06 07:11:51]

Also here is the previous patch in case you want to compare:
https://codereview.chromium.org/2183903002

You might notice that there are more tests in that patch. GetPrimaryServices is
kind of async since it has to wait for the services to be discovered but
GetCharacteristics is not. Which means that there can't be a disconnect followed
by a connect during a getCharacteristics() call; by the time the connect request
runs, getCharacteristics() will have finished already.

[commit-bot: I haz the power, 2016-10-06 08:06:35]

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

[commit-bot: I haz the power, 2016-10-06 08:06:35]

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

[Jeffrey Yasskin, 2016-10-06 17:50:32]

LGTM with the two changes.

Having a cross product of tests makes me nervous, either that we're wasting time
testing things we don't need to, or that we should be building the cross product
in a more scalable way, so it's easier to tell that we've covered everything in
the same way. I don't have good suggestions for improving things (in particular,
putting loops in a single test means we can't clean up as well between elements
of the cross product), but it's something to think about.

https://codereview.chromium.org/2387293008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2387293008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:106:
m_service->device()->gatt()->RemoveFromActiveAlgorithms(m_resolver.get());
https://webbluetoothcg.github.io/web-bluetooth/#connection-checking-wrapper says
to replace the real error with a NetworkError if we've disconnected during the
operation. We get there through the
https://webbluetoothcg.github.io/web-bluetooth/#query-the-bluetooth-cache step
of https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren.

I think that's even the right behavior. ;-)

https://codereview.chromium.org/2387293008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:113:
Persistent<ScriptPromiseResolver> m_resolver;
Can you make this const, to be clear that we always add and remove the same
object from the ActiveAlgorithms set?

[ortuno, 2016-10-07 04:41:23]

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

[commit-bot: I haz the power, 2016-10-07 04:41:38]

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

[ortuno, 2016-10-07 04:46:00]

We are testing the same code path 3x times because we use the same path for all
three variants. But I  don't think all implementations will do that so I made a
version for each function. We could just test it through one of the calls or we
could have a script to generate a test for each variant similar to what fetch
does:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...

That would allow us to write one script for getPrimaryService(s), one for
getCharacteristic(s) and one for readValue/writeValue/startNotifications and
have all the variants generated. WDYT?

https://codereview.chromium.org/2387293008/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp
(right):

https://codereview.chromium.org/2387293008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:106:
m_service->device()->gatt()->RemoveFromActiveAlgorithms(m_resolver.get());
On 2016/10/06 at 17:50:32, Jeffrey Yasskin wrote:
> https://webbluetoothcg.github.io/web-bluetooth/#connection-checking-wrapper
says to replace the real error with a NetworkError if we've disconnected during
the operation. We get there through the
https://webbluetoothcg.github.io/web-bluetooth/#query-the-bluetooth-cache step
of https://webbluetoothcg.github.io/web-bluetooth/#getgattchildren.
> 
> I think that's even the right behavior. ;-)

Done.

https://codereview.chromium.org/2387293008/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothRemoteGATTService.cpp:113:
Persistent<ScriptPromiseResolver> m_resolver;
On 2016/10/06 at 17:50:32, Jeffrey Yasskin wrote:
> Can you make this const, to be clear that we always add and remove the same
object from the ActiveAlgorithms set?

Done.

[commit-bot: I haz the power, 2016-10-07 06:33:08]

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

[commit-bot: I haz the power, 2016-10-07 06:33:09]

Dry run: This issue passed the CQ dry run.

[Jeffrey Yasskin, 2016-10-07 18:50:49]

On 2016/10/07 04:46:00, ortuno wrote:
> We are testing the same code path 3x times because we use the same path for
all
> three variants. But I  don't think all implementations will do that so I made
a
> version for each function. We could just test it through one of the calls or
we
> could have a script to generate a test for each variant similar to what fetch
> does:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> 
> That would allow us to write one script for getPrimaryService(s), one for
> getCharacteristic(s) and one for readValue/writeValue/startNotifications and
> have all the variants generated. WDYT?

An actual script to generate the tests would make sure they're all the same
except for the parts we want to vary, which is what I'm hoping for. I'm worried
that either the script will turn out hard to read, or that integrating it into
the build system will be more complicated than it's worth. I wouldn't want, for
instance, to have to check in the script's output. But if you see a way to do
it, it sounds great. Probably after you commit this CL so that we don't block
the progress this CL makes?

[ortuno, 2016-10-09 23:25:33]

The CQ bit was checked by ortuno@chromium.org

[ortuno, 2016-10-09 23:25:33]

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

[commit-bot: I haz the power, 2016-10-09 23:25:45]

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-10-10 03:03:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-10 03:05:51]

Description was changed from

==========
bluetooth: Require context is connected for getCharacteristic(s)

Promises reject if the frame is not connected or if the frame disconnected
during the getCharacteristic(s) operation.

BUG=608538
==========

to

==========
bluetooth: Require context is connected for getCharacteristic(s)

Promises reject if the frame is not connected or if the frame disconnected
during the getCharacteristic(s) operation.

BUG=608538

Committed: https://crrev.com/3aecaf01516c28df06c0b29881678a4548cbdfcf
Cr-Commit-Position: refs/heads/master@{#424113}
==========

[commit-bot: I haz the power, 2016-10-10 03:05:53]

Patchset 4 (id:??) landed as
https://crrev.com/3aecaf01516c28df06c0b29881678a4548cbdfcf
Cr-Commit-Position: refs/heads/master@{#424113}