Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1229473005:
BluetoothAdapterMac::LowEnergyDeviceUpdated() Implemented (Closed)
|
Created:
5 years, 5 months ago by krstnmnlsn Modified:
5 years, 5 months ago CC:
chromium-reviews, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@hash Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLow energy devices are now inserted into the adapter's |devices_| list when
found (ie when the didDiscoverPeripheral message is sent to the
CBCentralManagerDelegate). Devices are also updated when new
information is returned.
BUG=496987
Committed: https://crrev.com/bbc9fafbff50e2f7a02044e84c022d46ee526157
Cr-Commit-Position: refs/heads/master@{#339321}
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Scheib's comments #
Total comments: 2
Patch Set 3 : moving 'return' #
Total comments: 2
Patch Set 4 : now with readable logic #
Total comments: 4
Patch Set 5 : Arman comments #Patch Set 6 : pulling in changes from prev CL #Patch Set 7 : pulling in changes from hash CL #Patch Set 8 : pulling in origin/master #Patch Set 9 : s/advertisementData/advertisement_data #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
krstnmnlsn@chromium.org changed reviewers: + scheib@chromium.org
Hey, finally implemented actually adding low energy devices to |devices_| :)
https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:398: if (devices_.count(device_address)) { Push back if you like, but here's my take on testing for the device existence and then inserting or updating it without searching through the map again. https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... BluetoothDevice*& device = devices_[address]; if (!device) { device = // new BluetoothDevice... } else { // use device->member Your current code does a search for the address 3 times for an update, or 2 times for a new device. https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:403: // simultaneously present is 1e-6, so we simply return. Cite the source for those numbers. Also explain handling it a bit more. ", so we ignore the device found later by returning." https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:410: FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_, I think only call DeviceChanged if data about the device did change. (old_device->Update could return a bool to let us know).
Patchset #2 (id:80001) has been deleted
Thanks, comments addressed. Also made (and copied over) some changes so that the code will be able to compile on 10.6. https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:398: if (devices_.count(device_address)) { On 2015/07/08 05:20:36, scheib wrote: > Push back if you like, but here's my take on testing for the device existence > and then inserting or updating it without searching through the map again. > https://codereview.chromium.org/1215303006/diff/60001/device/bluetooth/blueto... > > BluetoothDevice*& device = devices_[address]; > if (!device) { > device = // new BluetoothDevice... > } else { > // use device->member > > Your current code does a search for the address 3 times for an update, or 2 > times for a new device. Done. https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:403: // simultaneously present is 1e-6, so we simply return. On 2015/07/08 05:20:36, scheib wrote: > Cite the source for those numbers. Also explain handling it a bit more. ", so we > ignore the device found later by returning." Done. https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:410: FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_, On 2015/07/08 05:20:36, scheib wrote: > I think only call DeviceChanged if data about the device did change. > (old_device->Update could return a bool to let us know). (after offline discussion) leaving this for now, at least until it becomes clear which changes to the peripheral we do and do not care about.
https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/60001/device/bluetooth/blueto... device/bluetooth/bluetooth_adapter_mac.mm:410: FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_, On 2015/07/13 17:26:23, krstnmnlsn wrote: > On 2015/07/08 05:20:36, scheib wrote: > > I think only call DeviceChanged if data about the device did change. > > (old_device->Update could return a bool to let us know). > > (after offline discussion) leaving this for now, at least until it becomes clear > which changes to the peripheral we do and do not care about. Acknowledged. https://codereview.chromium.org/1229473005/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:417: FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_, Only one of DeviceAdded or DeviceChanged should be called. Either add a return to this block, or adjust the following blocks to be else if {...} else {...}
thanks! https://codereview.chromium.org/1229473005/diff/100001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/100001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:417: FOR_EACH_OBSERVER(BluetoothAdapter::Observer, observers_, On 2015/07/13 18:24:39, scheib wrote: > Only one of DeviceAdded or DeviceChanged should be called. Either add a return > to this block, or adjust the following blocks to be else if {...} else {...} Right, thanks. This is what I get for cutting and pasting code around...
https://codereview.chromium.org/1229473005/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:421: } else { OK, if you're going with early returns then we don't need } else { if () {} } Change it to just if () { ... return; } if () { ... return; } ... (Also, no blank line after return;)
now with reader-friendly logic https://codereview.chromium.org/1229473005/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:421: } else { On 2015/07/13 20:47:21, scheib wrote: > OK, if you're going with early returns then we don't need > } else { > if () {} > } > > Change it to just > if () { > ... > return; > } > > if () { > ... > return; > } > > ... > > > (Also, no blank line after return;) Done.
LGTM
krstnmnlsn@chromium.org changed reviewers: + armansito@chromium.org
Hey Arman, with this CL BLE devices are finally added to the |devices_| list as they are discovered. This CL depends (indirectly) on a previous CLs which you may want to review first https://codereview.chromium.org/1228863002/
lgtm with nits https://codereview.chromium.org/1229473005/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:411: BluetoothDevice*& device_reference = devices_[device_address]; Please add a comment here explaining why you're using a reference to a pointer and why it's OK to use the std::map [] operator before checking if the element doesn't exist (since the operator will create the map entry if it doesn't exist). Just explain the assignment done below. https://codereview.chromium.org/1229473005/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:412: Remove extra space here.
Thanks Arman x2 :) https://codereview.chromium.org/1229473005/diff/140001/device/bluetooth/bluet... File device/bluetooth/bluetooth_adapter_mac.mm (right): https://codereview.chromium.org/1229473005/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:411: BluetoothDevice*& device_reference = devices_[device_address]; On 2015/07/15 18:14:42, armansito wrote: > Please add a comment here explaining why you're using a reference to a pointer > and why it's OK to use the std::map [] operator before checking if the element > doesn't exist (since the operator will create the map entry if it doesn't > exist). Just explain the assignment done below. Done. https://codereview.chromium.org/1229473005/diff/140001/device/bluetooth/bluet... device/bluetooth/bluetooth_adapter_mac.mm:412: On 2015/07/15 18:14:42, armansito wrote: > Remove extra space here. Done.
Patchset #6 (id:180001) has been deleted
Patchset #7 (id:220001) has been deleted
The CQ bit was checked by krstnmnlsn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, armansito@chromium.org Link to the patchset: https://codereview.chromium.org/1229473005/#ps280001 (title: "s/advertisementData/advertisement_data")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1229473005/280001
Message was sent while issue was closed.
Committed patchset #9 (id:280001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/bbc9fafbff50e2f7a02044e84c022d46ee526157 Cr-Commit-Position: refs/heads/master@{#339321} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
