Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1247553002:
bluetooth: Implement FailingConnectionsAdapter. (Closed)
|
Created:
5 years, 5 months ago by ortuno Modified:
5 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, scheib+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@bluetooth-tests-small-5 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbluetooth: Implement FailingConnectionsAdapter.
Also implement UnconnectableDevice. Used for testing that the platform handles
all connection errors successfully.
Design doc: https://docs.google.com/document/d/1_QsBzcc84SwF7oaBWbO8rBzn39MISQ3w-6QM9hk0L3E/edit?usp=sharing
Three sided patch since we have to remove the old adapter after we modify tests:
[1] This patch.
[2] Add unconnectable device tests and remove old tests. (http://crrev.com/1244973002)
[3] Remove old UnconnectableDeviceAdapter. (http://crrev.com/1243803002)
BUG=499552
Committed: https://crrev.com/dc2e5b370019a1437f4ea4deba6cf27eccaf3c0c
Cr-Commit-Position: refs/heads/master@{#339983}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Address jyasskin's comments #
Total comments: 2
Patch Set 3 : canonicalAddress -> makeMACAddress and change it's parameter type #Patch Set 4 : Use PRIx64 macro instead of 'lx' #Patch Set 5 : Use PRIx64 macro instead of 'lx' #
Depends on Patchset: Messages
Total messages: 34 (17 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
ortuno@chromium.org changed reviewers: + jyasskin@chromium.org
@jyasskin: PTAL https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:259: for (int error = BluetoothDevice::ERROR_UNKNOWN; Is this a good way of doing it? I've heard that you should generally avoid casting enums to ints.
https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:259: for (int error = BluetoothDevice::ERROR_UNKNOWN; On 2015/07/21 21:32:43, ortuno wrote: > Is this a good way of doing it? I've heard that you should generally avoid > casting enums to ints. This is fine. If the relevant enum had a COUNT member, I'd probably use "for (int error = 0; error < BluetoothDevice::COUNT; ++error)" instead, but it doesn't matter much. It's generally good to avoid casting between enums and ints (the int->enum cast is actually more dangerous due to unspecified behavior when out-of-bounds), but when you're iterating over all the enumerators, it's the only way to do it. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:366: uuids.push_back(BluetoothUUID(kPrefixConnectionErrorUUID + I think this should use StringPrintf("%08x-97e5-4cd7-b9f1-f5a427670c59", error_code) instead of string concatenation. You might write an errorUUID(unsigned) function to encapsulate this, and then you wouldn't need to extract that format string into a constant. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:371: kAddressPrefix + std::to_string(error_code))); Similarly, a format string instead of string concatenation will probably be more readable, and will deal better with 2-digit error codes if they arise in the future. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:27: // "00000000-97e5-4cd7-b9f1-f5a427670c59" with the bits of the alias. You don't need the "The algorithm consists of" wording: that's in the Web BT spec just so I can make the real definition be in the Bluetooth spec but still give developers something to hold onto. Say something like: errorUUID(alias), used below, returns a UUID with the top 32 bits of "00000000-97e5-4cd7-b9f1-f5a427670c59" replaced with the bits of |alias|. For example, errorUUID(0xDEADBEEF) returns "deadbeef-97e5-4cd7-b9f1-f5a427670c59". The bottom 96 bits of error UUIDs were generated as a type 4 (random) UUID. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:116: // of error. Each of the devices has a different UUID so that they can be "has a service with a different UUID" https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:120: // - UnconnectableDevice(BluetoothDevice::ERROR_UNKNOWN) errorUUID(0x0) This set of error codes isn't going to generalize well to other browsers. I'm not sure what we should do about that, since we do want to test all of our error codes. Maybe we just mark certain tests as not upstreamable? https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:231: // - uuid 'uuid' isn't one of the arguments.
jyasskin@chromium.org changed reviewers: + scheib@chromium.org
https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:120: // - UnconnectableDevice(BluetoothDevice::ERROR_UNKNOWN) errorUUID(0x0) On 2015/07/21 23:08:58, Jeffrey Yasskin wrote: > This set of error codes isn't going to generalize well to other browsers. I'm > not sure what we should do about that, since we do want to test all of our error > codes. Maybe we just mark certain tests as not upstreamable? @scheib, thoughts?
https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:120: // - UnconnectableDevice(BluetoothDevice::ERROR_UNKNOWN) errorUUID(0x0) On 2015/07/21 23:09:33, Jeffrey Yasskin wrote: > On 2015/07/21 23:08:58, Jeffrey Yasskin wrote: > > This set of error codes isn't going to generalize well to other browsers. I'm > > not sure what we should do about that, since we do want to test all of our > error > > codes. Maybe we just mark certain tests as not upstreamable? > > @scheib, thoughts? In thinking over this, I agree, these tests have value, we should have them, and test not only the error name but message as well. It does not seem onerous to have other implementations produce the same errors when under test. They may either hack the test suite to skip the tests, or they may have a bit of code that when under test produces these error message in a hard coded way. Imagine // Early respond to error for Bluetooth test suite devices that do not // correspond to our implementation: ... It's not so bad, especially since most of these will correspond to other implementation's details too and they will use most of them. I think go ahead with this and minimal modification. Comment in LayoutTests/bluetooth/bluetooth-helpers.js connection_errors that these devices correspond to a particular implementation's details and may not apply to all implementations.
Patchset #2 (id:100001) has been deleted
https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc (right): https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:366: uuids.push_back(BluetoothUUID(kPrefixConnectionErrorUUID + On 2015/07/21 at 23:08:58, Jeffrey Yasskin wrote: > I think this should use StringPrintf("%08x-97e5-4cd7-b9f1-f5a427670c59", error_code) instead of string concatenation. You might write an errorUUID(unsigned) function to encapsulate this, and then you wouldn't need to extract that format string into a constant. Done. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.cc:371: kAddressPrefix + std::to_string(error_code))); On 2015/07/21 at 23:08:58, Jeffrey Yasskin wrote: > Similarly, a format string instead of string concatenation will probably be more readable, and will deal better with 2-digit error codes if they arise in the future. Done. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:27: // "00000000-97e5-4cd7-b9f1-f5a427670c59" with the bits of the alias. On 2015/07/21 at 23:08:58, Jeffrey Yasskin wrote: > You don't need the "The algorithm consists of" wording: that's in the Web BT spec just so I can make the real definition be in the Bluetooth spec but still give developers something to hold onto. Say something like: > > errorUUID(alias), used below, returns a UUID with the top 32 bits of "00000000-97e5-4cd7-b9f1-f5a427670c59" replaced with the bits of |alias|. For example, errorUUID(0xDEADBEEF) returns "deadbeef-97e5-4cd7-b9f1-f5a427670c59". The bottom 96 bits of error UUIDs were generated as a type 4 (random) UUID. Done. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:116: // of error. Each of the devices has a different UUID so that they can be On 2015/07/21 at 23:08:58, Jeffrey Yasskin wrote: > "has a service with a different UUID" Done. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:120: // - UnconnectableDevice(BluetoothDevice::ERROR_UNKNOWN) errorUUID(0x0) On 2015/07/22 at 00:15:58, scheib wrote: > On 2015/07/21 23:09:33, Jeffrey Yasskin wrote: > > On 2015/07/21 23:08:58, Jeffrey Yasskin wrote: > > > This set of error codes isn't going to generalize well to other browsers. I'm > > > not sure what we should do about that, since we do want to test all of our > > error > > > codes. Maybe we just mark certain tests as not upstreamable? > > > > @scheib, thoughts? > > In thinking over this, I agree, these tests have value, we should have them, and test not only the error name but message as well. > > It does not seem onerous to have other implementations produce the same errors when under test. They may either hack the test suite to skip the tests, or they may have a bit of code that when under test produces these error message in a hard coded way. Imagine > // Early respond to error for Bluetooth test suite devices that do not > // correspond to our implementation: > ... > > It's not so bad, especially since most of these will correspond to other implementation's details too and they will use most of them. > > I think go ahead with this and minimal modification. Comment in LayoutTests/bluetooth/bluetooth-helpers.js connection_errors that these devices correspond to a particular implementation's details and may not apply to all implementations. I think we are removing connection_errors. I will add a note in the tests though. https://codereview.chromium.org/1247553002/diff/80001/content/shell/browser/l... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:231: // - uuid On 2015/07/21 at 23:08:58, Jeffrey Yasskin wrote: > 'uuid' isn't one of the arguments. Done.
lgtm https://codereview.chromium.org/1247553002/diff/120001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1247553002/diff/120001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:286: static std::string canonicalAddress(int addr); Maybe macAddress() or makeMACAddress()? The argument should probably be unsigned too, or uint64_t.
Patchset #3 (id:140001) has been deleted
Thanks! https://codereview.chromium.org/1247553002/diff/120001/content/shell/browser/... File content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h (right): https://codereview.chromium.org/1247553002/diff/120001/content/shell/browser/... content/shell/browser/layout_test/layout_test_bluetooth_adapter_provider.h:286: static std::string canonicalAddress(int addr); On 2015/07/22 at 01:32:27, Jeffrey Yasskin wrote: > Maybe macAddress() or makeMACAddress()? The argument should probably be unsigned too, or uint64_t. Done.
@scheib: PTAL
LGTM
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org Link to the patchset: https://codereview.chromium.org/1247553002/#ps160001 (title: "canonicalAddress -> makeMACAddress and change it's parameter type")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247553002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by ortuno@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jyasskin@chromium.org, scheib@chromium.org Link to the patchset: https://codereview.chromium.org/1247553002/#ps200001 (title: "Use PRIx64 macro instead of 'lx'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247553002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247553002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) linux_arm_compile on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247553002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247553002/200001
The CQ bit was unchecked by ortuno@chromium.org
The CQ bit was checked by ortuno@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1247553002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1247553002/200001
Message was sent while issue was closed.
Committed patchset #5 (id:200001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dc2e5b370019a1437f4ea4deba6cf27eccaf3c0c Cr-Commit-Position: refs/heads/master@{#339983} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
