Chromium Code Reviews
Help | Chromium Project | Sign in
(163)

Issue 2404623002: bluetooth: Add DeviceChanged logging in Device service. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 2 weeks ago by mbrunson
Modified:
5 months ago
Reviewers:
scheib, ortuno, dcheng, dpapad
CC:
Aaron Boodman, abarth-chromium, arv+watch_chromium.org, chromium-reviews, darin (slow to review), ortuno+watch_chromium.org, qsr+mojo_chromium.org, scheib+watch_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

bluetooth: Add DeviceChanged logging in Device service. Adds DeviceChanged callback for BluetoothAdapter::Observer in Device. Logs received DeviceChanged events in console. BUG=651282 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/4f24dfe2da20be7e9a9ba2f143cf5d318a5283b1 Cr-Commit-Position: refs/heads/master@{#427124}

Patch Set 1 #

Patch Set 2 : Change DeviceChanged to only report data from the specific device #

Patch Set 3 : Revert BUILD change, push to later patch #

Patch Set 4 : Merge upstream, update names, comments #

Patch Set 5 : Split JS into functions #

Total comments: 8

Patch Set 6 : Fix comments #

Patch Set 7 : Rename GattClient.DeviceChanged, refactor JS #

Patch Set 8 : Fix JS formatting #

Patch Set 9 : Remove GattClient, move PacketReceived to Adapter #

Patch Set 10 : Change PacketReceived to DeviceChanged #

Total comments: 2

Patch Set 11 : Make RSSI optional #

Patch Set 12 : Remove proxy from Device in JS #

Total comments: 8

Patch Set 13 : Fix deviceChanged comment #

Patch Set 14 : Update deviceChanged, fix formatting #

Total comments: 10

Patch Set 15 : Use Map, change Device class def #

Total comments: 8

Patch Set 16 : Move devices map, simplify initialization #

Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -42 lines) Patch
M chrome/browser/resources/bluetooth_internals/bluetooth_internals.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +47 lines, -42 lines 0 comments Download
M device/bluetooth/adapter.h View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M device/bluetooth/adapter.cc View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
M device/bluetooth/device.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -0 lines 0 comments Download
M device/bluetooth/public/interfaces/adapter.mojom View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -0 lines 0 comments Download
M device/bluetooth/public/interfaces/device.mojom View 1 2 3 4 5 6 7 8 9 10 1 chunk +6 lines, -0 lines 0 comments Download
Trybot results:  chromium_presubmit   android_arm64_dbg_recipe   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   closure_compilation   linux_chromium_clobber_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_compile_dbg_ng   chromium_presubmit   linux_chromium_asan_rel_ng   chromeos_x86-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-simulator   linux_android_rel_ng   ios-device   cast_shell_android   android_n5x_swarming_rel   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe   linux_chromium_rel_ng   chromium_presubmit   win_chromium_rel_ng   linux_chromium_rel_ng   linux_chromium_rel_ng   win_chromium_rel_ng   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-device   linux_android_rel_ng   ios-simulator   android_cronet   android_n5x_swarming_rel   cast_shell_android   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   linux_chromium_rel_ng   closure_compilation   linux_chromium_compile_dbg_ng   linux_chromium_clobber_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_compile_dbg_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_asan_rel_ng   chromium_presubmit   chromeos_x86-generic_chromium_compile_only_ng   chromeos_amd64-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   cast_shell_linux 
Commit queue not available (can’t edit this change).

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 70 (41 generated)
mbrunson
Switched around the JS a little for logging advertising packets. I didn't add every aspect ...
5 months, 1 week ago (2016-10-13 20:40:56 UTC) #10
ortuno
https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom#newcode36 device/bluetooth/public/interfaces/device.mojom:36: DeviceChanged(AdvertisingPacket packet); In the spirit of "Just a Thin ...
5 months, 1 week ago (2016-10-13 22:22:49 UTC) #13
scheib
https://codereview.chromium.org/2404623002/diff/80001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/80001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode13 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:13: // Dictionary for address->client mapping Close with . https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom ...
5 months, 1 week ago (2016-10-14 07:14:51 UTC) #14
mbrunson
https://codereview.chromium.org/2404623002/diff/80001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/80001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode13 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:13: // Dictionary for address->client mapping On 2016/10/14 07:14:51, scheib ...
5 months, 1 week ago (2016-10-14 18:25:26 UTC) #16
scheib
https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom#newcode36 device/bluetooth/public/interfaces/device.mojom:36: DeviceChanged(AdvertisingPacket packet); On 2016/10/14 18:25:25, mbrunson wrote: > On ...
5 months, 1 week ago (2016-10-14 20:25:40 UTC) #20
ortuno
https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom#newcode36 device/bluetooth/public/interfaces/device.mojom:36: DeviceChanged(AdvertisingPacket packet); On 2016/10/14 at 20:25:40, scheib wrote: > ...
5 months, 1 week ago (2016-10-17 05:03:32 UTC) #21
mbrunson
https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom#newcode36 device/bluetooth/public/interfaces/device.mojom:36: DeviceChanged(AdvertisingPacket packet); On 2016/10/17 05:03:31, ortuno wrote: > On ...
5 months, 1 week ago (2016-10-17 21:33:38 UTC) #22
scheib
On 2016/10/17 05:03:32, ortuno wrote: > https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom > File device/bluetooth/public/interfaces/device.mojom (right): > > https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom#newcode36 > ...
5 months, 1 week ago (2016-10-18 05:37:42 UTC) #27
ortuno
On 2016/10/18 at 05:37:42, scheib wrote: > On 2016/10/17 05:03:32, ortuno wrote: > > https://codereview.chromium.org/2404623002/diff/80001/device/bluetooth/public/interfaces/device.mojom ...
5 months, 1 week ago (2016-10-18 05:58:47 UTC) #28
mbrunson
Ok. Makes sense to me. Keeping things on topic, what else do I have to ...
5 months ago (2016-10-18 22:59:09 UTC) #29
mbrunson
On 2016/10/18 22:59:09, mbrunson wrote: > Ok. Makes sense to me. Keeping things on topic, ...
5 months ago (2016-10-19 18:27:26 UTC) #31
scheib
https://codereview.chromium.org/2404623002/diff/180001/device/bluetooth/public/interfaces/device.mojom File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2404623002/diff/180001/device/bluetooth/public/interfaces/device.mojom#newcode11 device/bluetooth/public/interfaces/device.mojom:11: int8 rssi; I'd prefer we not use magic values ...
5 months ago (2016-10-19 21:28:56 UTC) #32
mbrunson
https://codereview.chromium.org/2404623002/diff/180001/device/bluetooth/public/interfaces/device.mojom File device/bluetooth/public/interfaces/device.mojom (right): https://codereview.chromium.org/2404623002/diff/180001/device/bluetooth/public/interfaces/device.mojom#newcode11 device/bluetooth/public/interfaces/device.mojom:11: int8 rssi; On 2016/10/19 21:28:56, scheib wrote: > I'd ...
5 months ago (2016-10-19 22:29:22 UTC) #33
ortuno
lgtm https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode46 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:46: * Logs received advertising packet to console. caches ...
5 months ago (2016-10-19 23:19:19 UTC) #34
scheib
https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode30 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:30: deviceAdded: function(device) { function(deviceInfo) - so that we don't ...
5 months ago (2016-10-19 23:49:24 UTC) #35
mbrunson
https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/220001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode30 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:30: deviceAdded: function(device) { On 2016/10/19 23:49:24, scheib wrote: > ...
5 months ago (2016-10-20 00:33:51 UTC) #36
scheib
LGTM
5 months ago (2016-10-20 01:20:02 UTC) #37
mbrunson
OWNERS review, please: dcheng: device/bluetooth/public/interfaces/adapter.mojom device/bluetooth/public/interfaces/device.mojom dpapad: chrome/browser/resources/bluetooth_internals/bluetooth_internals.js
5 months ago (2016-10-20 01:43:51 UTC) #41
dpapad
https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode14 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:14: var devices = {}; Can you use a proper ...
5 months ago (2016-10-20 17:52:03 UTC) #44
mbrunson
https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode17 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: function Device() {}; On 2016/10/20 17:52:03, dpapad wrote: > ...
5 months ago (2016-10-20 18:15:36 UTC) #45
dpapad
https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode17 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: function Device() {}; On 2016/10/20 at 18:15:36, mbrunson wrote: ...
5 months ago (2016-10-20 18:36:22 UTC) #46
mbrunson
https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/260001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode14 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:14: var devices = {}; On 2016/10/20 17:52:03, dpapad wrote: ...
5 months ago (2016-10-20 20:09:04 UTC) #47
dpapad
https://chromiumcodereview.appspot.com/2404623002/diff/280001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://chromiumcodereview.appspot.com/2404623002/diff/280001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode17 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: var Device = function(info) { this.info = info; }; ...
5 months ago (2016-10-20 21:22:01 UTC) #48
mbrunson
https://codereview.chromium.org/2404623002/diff/280001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js File chrome/browser/resources/bluetooth_internals/bluetooth_internals.js (right): https://codereview.chromium.org/2404623002/diff/280001/chrome/browser/resources/bluetooth_internals/bluetooth_internals.js#newcode17 chrome/browser/resources/bluetooth_internals/bluetooth_internals.js:17: var Device = function(info) { this.info = info; }; ...
5 months ago (2016-10-20 22:00:36 UTC) #49
dpapad
LGTM for JS
5 months ago (2016-10-20 23:20:51 UTC) #50
dcheng
mojo lgtm
5 months ago (2016-10-21 05:43:14 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2404623002/300001
5 months ago (2016-10-24 19:29:31 UTC) #66
commit-bot: I haz the power
Committed patchset #16 (id:300001)
5 months ago (2016-10-24 19:46:44 UTC) #68
commit-bot: I haz the power
5 months ago (2016-10-24 19:55:59 UTC) #70
Message was sent while issue was closed.
Patchset 16 (id:??) landed as
https://crrev.com/4f24dfe2da20be7e9a9ba2f143cf5d318a5283b1
Cr-Commit-Position: refs/heads/master@{#427124}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62