Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2908673002:
device/blueooth: Add BlueZ unit test for BluetoothDevice::SetConnectionInterval. (Closed)
Descriptiondevice/blueooth: Add BlueZ unit test for BluetoothDevice::SetConnectionInterval.
BUG=721559
Review-Url: https://codereview.chromium.org/2908673002
Cr-Commit-Position: refs/heads/master@{#476539}
Committed: https://chromium.googlesource.com/chromium/src/+/e34d18f4179b201c408484c59d27bc931c7db86b
Patch Set 1 #
Total comments: 11
Patch Set 2 : fixes #
Total comments: 2
Patch Set 3 : test invalid case #
Messages
Total messages: 17 (6 generated)
tengs@chromium.org changed reviewers: + mcchou@google.com, ortuno@chromium.org
https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:604: if (properties->type.value() == kTypeBredr) { I'm not that familiar with type as is not in upstream bluez. Do we know if it's always going to be valid? What should we return if it isn't? https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:606: "BR/EDR devices not supported"); Should the call still work for non-connected devices?
https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:604: if (properties->type.value() == kTypeBredr) { On 2017/05/26 16:54:36, ortuno wrote: > I'm not that familiar with type as is not in upstream bluez. > > Do we know if it's always going to be valid? What should we return if it isn't? This property is exposed by the org.bluez.Device1 API: https://cs.corp.google.com/chromeos_public/src/third_party/bluez/doc/device-a... So I think it should be safe to assume this property is valid. https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:606: "BR/EDR devices not supported"); On 2017/05/26 16:54:36, ortuno wrote: > Should the call still work for non-connected devices? Yes. One of the principle use cases is to set the connection parameters before connecting. I just screwed up implementing this fake method the first time around =p
https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:604: if (properties->type.value() == kTypeBredr) { On 2017/05/26 at 19:56:18, Tim Song wrote: > On 2017/05/26 16:54:36, ortuno wrote: > > I'm not that familiar with type as is not in upstream bluez. > > > > Do we know if it's always going to be valid? What should we return if it isn't? > > This property is exposed by the org.bluez.Device1 API: > https://cs.corp.google.com/chromeos_public/src/third_party/bluez/doc/device-a... > > So I think it should be safe to assume this property is valid. It's optional though so it could still be invalid. mcchou do you have any insight regarding 'type'?
mcchou@chromium.org changed reviewers: + mcchou@chromium.org
https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_bluez_unittest.cc (right): https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4603: The test on BluetoothDeviceClient::kTypeDual should be added here as well. https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:604: if (properties->type.value() == kTypeBredr) { On 2017/05/26 19:59:50, ortuno wrote: > On 2017/05/26 at 19:56:18, Tim Song wrote: > > On 2017/05/26 16:54:36, ortuno wrote: > > > I'm not that familiar with type as is not in upstream bluez. > > > > > > Do we know if it's always going to be valid? What should we return if it > isn't? > > > > This property is exposed by the org.bluez.Device1 API: > > > https://cs.corp.google.com/chromeos_public/src/third_party/bluez/doc/device-a... > > > > So I think it should be safe to assume this property is valid. > > It's optional though so it could still be invalid. > > mcchou do you have any insight regarding 'type'? Device Type is a CHROMIUM specific patch that upstream didn't adopt in my last attempt, so it's not in BlueZ upstream. The existence of property Type depends on the value of device->bredr and device->le where their values are set during the creation of a device [1]. These values are set initially during the device creation and updated later when connecting to the device for the first time, searching the device based on address type, or updating the address of the device. Theoretically, the device can be invalid if none of these two value is set to true. However if looking closer in BlueZ, you will find that at least one is set, and there is no method to invalidate these two values during the lifetime of a device. In Tim's previous patch of adding SetLEConnectionParameters method [2], the underlying check on the type of a given device is based on the address type of the device, but since the address type is not presented to upper layer. Checking property Type is the closest way to mimic the actual check, and the value of property Type also depends the address type. However, I agree with ortuno on the validity of this property, so a check on the validity should be added here. [1] https://cs.corp.google.com/chromeos_public/src/third_party/bluez/src/device.c... [2] https://chromium-review.googlesource.com/c/503432/ https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:606: "BR/EDR devices not supported"); On 2017/05/26 19:56:18, Tim Song wrote: > On 2017/05/26 16:54:36, ortuno wrote: > > Should the call still work for non-connected devices? > > Yes. One of the principle use cases is to set the connection parameters before > connecting. > > I just screwed up implementing this fake method the first time around =p The connection parameters can be set BEFORE or DURING the connection, so the check on connected status is not needed.
https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/bluez/blue... File device/bluetooth/bluez/bluetooth_bluez_unittest.cc (right): https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/bluez/blue... device/bluetooth/bluez/bluetooth_bluez_unittest.cc:4603: On 2017/05/31 20:14:06, Miao wrote: > The test on BluetoothDeviceClient::kTypeDual should be added here as well. Done. https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:604: if (properties->type.value() == kTypeBredr) { On 2017/05/31 20:14:06, Miao wrote: > On 2017/05/26 19:59:50, ortuno wrote: > > On 2017/05/26 at 19:56:18, Tim Song wrote: > > > On 2017/05/26 16:54:36, ortuno wrote: > > > > I'm not that familiar with type as is not in upstream bluez. > > > > > > > > Do we know if it's always going to be valid? What should we return if it > > isn't? > > > > > > This property is exposed by the org.bluez.Device1 API: > > > > > > https://cs.corp.google.com/chromeos_public/src/third_party/bluez/doc/device-a... > > > > > > So I think it should be safe to assume this property is valid. > > > > It's optional though so it could still be invalid. > > > > mcchou do you have any insight regarding 'type'? > > Device Type is a CHROMIUM specific patch that upstream didn't adopt in my last > attempt, so it's not in BlueZ upstream. > > The existence of property Type depends on the value of device->bredr and > device->le where their values are set during the creation of a device [1]. These > values are set initially during the device creation and updated later when > connecting to the device for the first time, searching the device based on > address type, or updating the address of the device. Theoretically, the device > can be invalid if none of these two value is set to true. However if looking > closer in BlueZ, you will find that at least one is set, and there is no method > to invalidate these two values during the lifetime of a device. > > In Tim's previous patch of adding SetLEConnectionParameters method [2], the > underlying check on the type of a given device is based on the address type of > the device, but since the address type is not presented to upper layer. Checking > property Type is the closest way to mimic the actual check, and the value of > property Type also depends the address type. However, I agree with ortuno on the > validity of this property, so a check on the validity should be added here. > > [1] > https://cs.corp.google.com/chromeos_public/src/third_party/bluez/src/device.c... > [2] https://chromium-review.googlesource.com/c/503432/ Done. I added the validity check. https://codereview.chromium.org/2908673002/diff/1/device/bluetooth/dbus/fake_... device/bluetooth/dbus/fake_bluetooth_device_client.cc:606: "BR/EDR devices not supported"); On 2017/05/31 20:14:06, Miao wrote: > On 2017/05/26 19:56:18, Tim Song wrote: > > On 2017/05/26 16:54:36, ortuno wrote: > > > Should the call still work for non-connected devices? > > > > Yes. One of the principle use cases is to set the connection parameters before > > connecting. > > > > I just screwed up implementing this fake method the first time around =p > > The connection parameters can be set BEFORE or DURING the connection, so the > check on connected status is not needed. Acknowledged.
LGTM, wait for the green light from ortuno
lgtm bar one more test case. https://codereview.chromium.org/2908673002/diff/20001/device/bluetooth/dbus/f... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2908673002/diff/20001/device/bluetooth/dbus/f... device/bluetooth/dbus/fake_bluetooth_device_client.cc:604: if (!properties->type.is_valid() || properties->type.value() == kTypeBredr) { nit: Add test for invalid type
https://codereview.chromium.org/2908673002/diff/20001/device/bluetooth/dbus/f... File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right): https://codereview.chromium.org/2908673002/diff/20001/device/bluetooth/dbus/f... device/bluetooth/dbus/fake_bluetooth_device_client.cc:604: if (!properties->type.is_valid() || properties->type.value() == kTypeBredr) { On 2017/06/01 22:59:35, ortuno wrote: > nit: Add test for invalid type Done.
The CQ bit was checked by tengs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mcchou@chromium.org, ortuno@chromium.org Link to the patchset: https://codereview.chromium.org/2908673002/#ps40001 (title: "test invalid case")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496360737791410,
"parent_rev": "9e464a8bd5869da658e45ecf5be00305792b2e61", "commit_rev":
"e34d18f4179b201c408484c59d27bc931c7db86b"}
Message was sent while issue was closed.
Description was changed from ========== device/blueooth: Add BlueZ unit test for BluetoothDevice::SetConnectionInterval. BUG=721559 ========== to ========== device/blueooth: Add BlueZ unit test for BluetoothDevice::SetConnectionInterval. BUG=721559 Review-Url: https://codereview.chromium.org/2908673002 Cr-Commit-Position: refs/heads/master@{#476539} Committed: https://chromium.googlesource.com/chromium/src/+/e34d18f4179b201c408484c59d27... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e34d18f4179b201c408484c59d27... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
