Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2907913002:
Fix ManufacturerData and ServiceData DBus data type to match with bluez. (Closed)
|
Created:
3 years, 7 months ago by Sonny Sasaka Modified:
3 years, 6 months ago CC:
Ryan Hansberry, rkc, chromium-reviews, scheib+watch_chromium.org, ortuno+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ManufacturerData and ServiceData DBus data type to match with bluez.
Bluez recent changes corresponding to this CL:
https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725ed7a1138d848aad940a73bbf5d659
https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9a1aba2cca871b79618e3b28a5ec2
BUG=725349
TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with
advertisement data containing both manufacturerData and serviceData, the
operation succeeded.
Review-Url: https://codereview.chromium.org/2907913002
Cr-Commit-Position: refs/heads/master@{#476433}
Committed: https://chromium.googlesource.com/chromium/src/+/02bf4db5faf5a62c0b373e1ea66109bea4ef4b6a
Patch Set 1 #Patch Set 2 : Fix ManufacturerData and ServiceData DBus data type to match with bluez. #
Total comments: 3
Patch Set 3 : Fix ManufacturerData and ServiceData DBus data type to match with bluez. #
Messages
Total messages: 51 (33 generated)
Description was changed from ========== Fix ManufacturerData and ServiceData DBus data type to match with bluez. This CL also fixes the misleading error code of RegisterAdvertisement: bluez's "Failed" error code shouldn't be interpreted as "already exists". BUG=725349 TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with advertisement data containing both manufacturerData and serviceData, the operation succeeded. ========== to ========== Fix ManufacturerData and ServiceData DBus data type to match with bluez. Bluez recent changes related to this CL: https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725e... https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9... This CL also fixes the misleading error code of RegisterAdvertisement: bluez's "Failed" error code shouldn't be interpreted as "already exists". BUG=725349 TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with advertisement data containing both manufacturerData and serviceData, the operation succeeded. ==========
sonnysasaka@chromium.org changed reviewers: + josephsih@chromium.org, mcchou@google.com
Description was changed from ========== Fix ManufacturerData and ServiceData DBus data type to match with bluez. Bluez recent changes related to this CL: https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725e... https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9... This CL also fixes the misleading error code of RegisterAdvertisement: bluez's "Failed" error code shouldn't be interpreted as "already exists". BUG=725349 TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with advertisement data containing both manufacturerData and serviceData, the operation succeeded. ========== to ========== Fix ManufacturerData and ServiceData DBus data type to match with bluez. Bluez recent changes corresponding to this CL: https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725e... https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9... This CL also fixes the misleading error code of RegisterAdvertisement: bluez's "Failed" error code shouldn't be interpreted as "already exists". BUG=725349 TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with advertisement data containing both manufacturerData and serviceData, the operation succeeded. ==========
The CQ bit was checked by sonnysasaka@chromium.org
The CQ bit was unchecked by sonnysasaka@chromium.org
The CQ bit was checked by sonnysasaka@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by ortuno@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
josephsih@google.com changed reviewers: + josephsih@google.com
lgtm
The CQ bit was checked by sonnysasaka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by sonnysasaka@chromium.org to run a CQ dry run
hansberry@chromium.org changed reviewers: + hansberry@chromium.org
Any news on this CL? Would be great this lands soon :) Thanks!
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sonnysasaka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from josephsih@google.com Link to the patchset: https://codereview.chromium.org/2907913002/#ps20001 (title: "Fix ManufacturerData and ServiceData DBus data type to match with bluez.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
On 2017/06/01 04:20:26, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started once the patch has received an L-G-T-M from a full > committer. > Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a > full super star committer. > Committers are members of the group "project-chromium-committers". > Note that this has nothing to do with OWNERS files. I modified the CL after LGTM. Joseph/Miao, can you please take a look again?
lgtm
The CQ bit was checked by sonnysasaka@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Looks like there are issues with committer status -- is it because Joseph LG'd from his @google.com account?
sonnysasaka@chromium.org changed reviewers: + rkc@chromium.org
https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_advertisement.h (right): https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_advertisement.h:34: ERROR_ADVERTISEMENT_ALREADY_EXISTS, // An advertisement is already This error doesn't really make sense anymore since we support multiple advertisements. Maybe rename this? If not in the scope of this CL, at least add a TODO. https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_advertisement.h:44: ERROR_ADVERTISEMENT_FAILED, Description for this error? https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/bluez/... File device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc (right): https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/bluez/... device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc:237: ExpectError(BluetoothAdvertisement::ERROR_ADVERTISEMENT_FAILED); What's the difference between ALREADY_EXISTS and FAILED now? Documenting the error codes should clarify this question.
On 2017/06/01 18:10:51, rkc wrote: > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_advertisement.h (right): > > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... > device/bluetooth/bluetooth_advertisement.h:34: > ERROR_ADVERTISEMENT_ALREADY_EXISTS, // An advertisement is already > This error doesn't really make sense anymore since we support multiple > advertisements. Maybe rename this? > If not in the scope of this CL, at least add a TODO. > > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... > device/bluetooth/bluetooth_advertisement.h:44: ERROR_ADVERTISEMENT_FAILED, > Description for this error? > > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/bluez/... > File device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc (right): > > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/bluez/... > device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc:237: > ExpectError(BluetoothAdvertisement::ERROR_ADVERTISEMENT_FAILED); > What's the difference between ALREADY_EXISTS and FAILED now? > Documenting the error codes should clarify this question. I will split the error handling fix to another CL as it is not as urgent as the main problem this CL intends to fix. PTAL.
On 2017/06/01 18:10:51, rkc wrote: > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... > File device/bluetooth/bluetooth_advertisement.h (right): > > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... > device/bluetooth/bluetooth_advertisement.h:34: > ERROR_ADVERTISEMENT_ALREADY_EXISTS, // An advertisement is already > This error doesn't really make sense anymore since we support multiple > advertisements. Maybe rename this? > If not in the scope of this CL, at least add a TODO. > > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/blueto... > device/bluetooth/bluetooth_advertisement.h:44: ERROR_ADVERTISEMENT_FAILED, > Description for this error? > > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/bluez/... > File device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc (right): > > https://codereview.chromium.org/2907913002/diff/20001/device/bluetooth/bluez/... > device/bluetooth/bluez/bluetooth_advertisement_bluez_unittest.cc:237: > ExpectError(BluetoothAdvertisement::ERROR_ADVERTISEMENT_FAILED); > What's the difference between ALREADY_EXISTS and FAILED now? > Documenting the error codes should clarify this question. I will split the error handling fix to another CL as it is not as urgent as the main problem this CL intends to fix. PTAL.
Description was changed from ========== Fix ManufacturerData and ServiceData DBus data type to match with bluez. Bluez recent changes corresponding to this CL: https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725e... https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9... This CL also fixes the misleading error code of RegisterAdvertisement: bluez's "Failed" error code shouldn't be interpreted as "already exists". BUG=725349 TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with advertisement data containing both manufacturerData and serviceData, the operation succeeded. ========== to ========== Fix ManufacturerData and ServiceData DBus data type to match with bluez. Bluez recent changes corresponding to this CL: https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725e... https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9... BUG=725349 TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with advertisement data containing both manufacturerData and serviceData, the operation succeeded. ==========
lgtm
The CQ bit was checked by sonnysasaka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from josephsih@google.com Link to the patchset: https://codereview.chromium.org/2907913002/#ps40001 (title: "Fix ManufacturerData and ServiceData DBus data type to match with bluez.")
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": 1496344217225570,
"parent_rev": "7b9f43c8110d6b72646b96e285c9e422f0546d6e", "commit_rev":
"0db66eca8c0649f8dcccf096ceee153129dce09b"}
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496344217225570,
"parent_rev": "253f8c677519fd6e6700047a2b08a09bb438c694", "commit_rev":
"02bf4db5faf5a62c0b373e1ea66109bea4ef4b6a"}
Message was sent while issue was closed.
Description was changed from ========== Fix ManufacturerData and ServiceData DBus data type to match with bluez. Bluez recent changes corresponding to this CL: https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725e... https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9... BUG=725349 TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with advertisement data containing both manufacturerData and serviceData, the operation succeeded. ========== to ========== Fix ManufacturerData and ServiceData DBus data type to match with bluez. Bluez recent changes corresponding to this CL: https://chromium.googlesource.com/chromiumos/third_party/bluez/+/758dae03725e... https://chromium.googlesource.com/chromiumos/third_party/bluez/+/d356a6242be9... BUG=725349 TEST=Called chrome.bluetoothLowEnergy.registerAdvertisement with advertisement data containing both manufacturerData and serviceData, the operation succeeded. Review-Url: https://codereview.chromium.org/2907913002 Cr-Commit-Position: refs/heads/master@{#476433} Committed: https://chromium.googlesource.com/chromium/src/+/02bf4db5faf5a62c0b373e1ea661... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/02bf4db5faf5a62c0b373e1ea661... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
