Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(147)

Issue 1565773002: Clear the BLE services list on disconnect. (Closed)

Created:
4 years, 11 months ago by tommyt
Modified:
4 years, 11 months ago
Reviewers:
scheib, ortuno, armansito
CC:
chromium-reviews, darin-cc_chromium.org, jam, perja, scheib+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Clear the BLE services list on disconnect. This lets us recreate the services list correctly upon the next reconnect, which fixes some problems with a device we have, which disconnects automatically after 30 seconds. Also reset the services_discovered flag for this device on disconnect. In order to accomplish this, I had to move this flag from the dispatcher host to the bluetooth device instance. BUG=570804 Committed: https://crrev.com/11195385967983a0b5fc3865576a9fa826d51711 Cr-Commit-Position: refs/heads/master@{#369593}

Patch Set 1 #

Patch Set 2 : Add a unittest #

Total comments: 9

Patch Set 3 : Add requested improvements to the unittest #

Total comments: 1

Patch Set 4 : Rebased onto master to solve conflicts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+75 lines, -19 lines) Patch
M content/browser/bluetooth/bluetooth_dispatcher_host.h View 2 chunks +0 lines, -9 lines 0 comments Download
M content/browser/bluetooth/bluetooth_dispatcher_host.cc View 1 2 3 4 chunks +1 line, -9 lines 0 comments Download
M device/bluetooth/bluetooth_adapter_bluez.cc View 1 chunk +1 line, -0 lines 0 comments Download
M device/bluetooth/bluetooth_device.h View 1 2 3 3 chunks +9 lines, -0 lines 0 comments Download
M device/bluetooth/bluetooth_device.cc View 2 chunks +11 lines, -1 line 0 comments Download
M device/bluetooth/bluetooth_device_android.cc View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M device/bluetooth/bluetooth_device_unittest.cc View 1 2 1 chunk +49 lines, -0 lines 0 comments Download

Messages

Total messages: 53 (19 generated)
tommyt
Hi! We've been playing around with some BLE devices here in the Opera office, and ...
4 years, 11 months ago (2016-01-06 15:04:45 UTC) #3
fbeaufortchromium
On 2016/01/06 15:04:45, tommyt wrote: > Hi! > > We've been playing around with some ...
4 years, 11 months ago (2016-01-06 15:11:04 UTC) #4
tommyt
It's a very custom device that our friends at Nordic Semiconductor has hand-made for us, ...
4 years, 11 months ago (2016-01-06 15:21:13 UTC) #5
scheib
Thanks! Meta: Would be good to create issues and CC folks for heads up of ...
4 years, 11 months ago (2016-01-07 23:44:47 UTC) #6
scheib
On 2016/01/07 23:44:47, scheib wrote: > Thanks! > > Meta: Would be good to create ...
4 years, 11 months ago (2016-01-07 23:53:05 UTC) #7
tommyt
I'll look into creating a unittest for this scenario, and update my patch. On Fri, ...
4 years, 11 months ago (2016-01-08 12:20:02 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/20001
4 years, 11 months ago (2016-01-11 11:53:34 UTC) #10
tommyt
I've uploaded a new patchset that includes a unittest
4 years, 11 months ago (2016-01-11 12:48:38 UTC) #11
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-11 12:53:37 UTC) #13
scheib
https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc#newcode400 device/bluetooth/bluetooth_device_unittest.cc:400: TEST_F(BluetoothTest, BluetoothGattConnection_DisconnectGatt_Cleanup) { Thanks, would you add + Connecting ...
4 years, 11 months ago (2016-01-11 18:40:09 UTC) #15
ortuno
I added a reference to the issue to the description as well. https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc File device/bluetooth/bluetooth_device_unittest.cc ...
4 years, 11 months ago (2016-01-11 21:48:40 UTC) #17
tommyt
Add requested improvements to the unit test. I've intentionally not added the requested bluez unit ...
4 years, 11 months ago (2016-01-12 08:42:36 UTC) #18
ortuno
https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc#newcode398 device/bluetooth/bluetooth_device_unittest.cc:398: // Calls CreateGattConnection & DisconnectGatt, then checks that gatt ...
4 years, 11 months ago (2016-01-12 22:28:49 UTC) #19
tommyt
https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc#newcode398 device/bluetooth/bluetooth_device_unittest.cc:398: // Calls CreateGattConnection & DisconnectGatt, then checks that gatt ...
4 years, 11 months ago (2016-01-13 09:42:30 UTC) #20
ortuno
lgtm https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc File device/bluetooth/bluetooth_device_unittest.cc (right): https://codereview.chromium.org/1565773002/diff/20001/device/bluetooth/bluetooth_device_unittest.cc#newcode398 device/bluetooth/bluetooth_device_unittest.cc:398: // Calls CreateGattConnection & DisconnectGatt, then checks that ...
4 years, 11 months ago (2016-01-13 18:22:02 UTC) #21
scheib
LGTM, if you don't choose to take on the bluez bit would you please file ...
4 years, 11 months ago (2016-01-13 18:36:47 UTC) #22
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/40001
4 years, 11 months ago (2016-01-14 07:33:54 UTC) #24
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/145475)
4 years, 11 months ago (2016-01-14 07:36:04 UTC) #26
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/60001
4 years, 11 months ago (2016-01-14 08:00:28 UTC) #28
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-14 09:09:54 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/60001
4 years, 11 months ago (2016-01-14 09:18:46 UTC) #33
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/135482)
4 years, 11 months ago (2016-01-14 09:25:32 UTC) #35
tommyt
@armansito: Hi! I had originally added scheib and ortuno as reviewers, but it turns out ...
4 years, 11 months ago (2016-01-14 09:39:26 UTC) #38
tommyt
On 2016/01/13 18:36:47, scheib wrote: > LGTM, if you don't choose to take on the ...
4 years, 11 months ago (2016-01-14 13:24:07 UTC) #39
tommyt
4 years, 11 months ago (2016-01-14 13:24:47 UTC) #40
tommyt
On 2016/01/14 13:24:07, tommyt wrote: > On 2016/01/13 18:36:47, scheib wrote: > > LGTM, if ...
4 years, 11 months ago (2016-01-14 14:20:02 UTC) #41
scheib
On 2016/01/14 14:20:02, tommyt wrote: > On 2016/01/14 13:24:07, tommyt wrote: > > On 2016/01/13 ...
4 years, 11 months ago (2016-01-14 22:37:53 UTC) #42
armansito
lgtm
4 years, 11 months ago (2016-01-14 22:43:02 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1565773002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1565773002/60001
4 years, 11 months ago (2016-01-14 22:55:14 UTC) #45
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 11 months ago (2016-01-14 23:01:09 UTC) #47
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/11195385967983a0b5fc3865576a9fa826d51711 Cr-Commit-Position: refs/heads/master@{#369593}
4 years, 11 months ago (2016-01-14 23:02:27 UTC) #49
jsbell
This seems to have caused the bluetooth/getPrimaryService.html layout test to start timing out. I'll set ...
4 years, 11 months ago (2016-01-15 00:19:18 UTC) #50
scheib
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1587193004/ by scheib@chromium.org. ...
4 years, 11 months ago (2016-01-15 05:00:05 UTC) #51
scheib
4 years, 11 months ago (2016-01-15 05:06:44 UTC) #52
Message was sent while issue was closed.
Reproducing locally should be possible with 
ninja -C out/Release blink_tests && blink/tools/run_layout_tests.sh bluetooth

Powered by Google App Engine
This is Rietveld 408576698