Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1464443002:
bluetooth: android: Add BluetoothDevice::ConnectErrorCode enums (Closed)
Descriptionbluetooth: android: Add BluetoothDevice::ConnectErrorCode enums
increased ConnectErrorCode enum and cases in
BluetoothDeviceAndroid::OnConnectionStateChange
BUG=531058
Committed: https://crrev.com/d730ca14bd3caced4f52617146969d72304c56e9
Cr-Commit-Position: refs/heads/master@{#368649}
Patch Set 1 : I #
Total comments: 6
Patch Set 2 : II #
Total comments: 14
Patch Set 3 : III #
Total comments: 51
Patch Set 4 : IV #
Total comments: 2
Patch Set 5 : V #
Total comments: 2
Patch Set 6 : VI #
Total comments: 4
Messages
Total messages: 69 (28 generated)
jiangkai@gmail.com changed reviewers: + scheib@chromium.org
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464443002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464443002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thank you for the contribution! To see this patch complete we will need to consider and make appropriate updates to all locations that have a switch statement based on the ConnectErrorCode type. The try bots will point to some compile failures for this, but please also do a code search for ConnectErrorCode and find other use sites, likely in extensions and easy access code.
https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:99: ERROR_UNSUPPORTED_DEVICE, We should sort these values alphabetically. https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_android.cc:216: case 0x0000000f: // GATT_INSUFFICIENT_ENCRYPTION 'Insufficient' is more specific than 'failed', let's use 'ERROR_INSUFFICIENT_ENCRYPTION'. https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device_android.cc:223: return DidFailToConnectGatt(ERROR_REQUEST_FAILED); 'Failed' and 'Not supported' have different meanings, we should use 'ERROR_NOT_SUPPORTED"
The CQ bit was checked by jiangkai@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464443002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464443002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2015/11/19 22:25:06, scheib wrote: > https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... > File device/bluetooth/bluetooth_device.h (right): > > https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... > device/bluetooth/bluetooth_device.h:99: ERROR_UNSUPPORTED_DEVICE, > We should sort these values alphabetically. > > https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... > File device/bluetooth/bluetooth_device_android.cc (right): > > https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... > device/bluetooth/bluetooth_device_android.cc:216: case 0x0000000f: // > GATT_INSUFFICIENT_ENCRYPTION > 'Insufficient' is more specific than 'failed', let's use > 'ERROR_INSUFFICIENT_ENCRYPTION'. > > https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... > device/bluetooth/bluetooth_device_android.cc:223: return > DidFailToConnectGatt(ERROR_REQUEST_FAILED); > 'Failed' and 'Not supported' have different meanings, we should use > 'ERROR_NOT_SUPPORTED" Thanks for your review. I search and update again based on ConnectErrorCode type. As issue related to Android, I didn't update in /options/chromeos/bluetooth_options_handler.cc P.S Could you send email to committers@chromium.org nominating me for try job access? Thanks :)
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464443002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464443002/20001
Thanks again for patch work. Keep it up, there's some more work to do. Getting use to the code review flow: When folks add comments and you address them please use the code review tool to visit each comment and reply. There's an option for commenting just 'done' in cases where you addressed the comment in a straight forward way. Or, discuss the comment. Often if you feel the comment was misplaced you also need to add comments to code so that future readers of code aren't confused as well. Usually code reviews will come back in 24 hours. If they take too long ping again to ensure they weren't over looked. https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:99: ERROR_UNSUPPORTED_DEVICE, On 2015/11/19 22:25:05, scheib wrote: > We should sort these values alphabetically. Sorry, I mean we should sort the entire ConnectErrorCode enum. https://codereview.chromium.org/1464443002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_metrics.h (right): https://codereview.chromium.org/1464443002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_metrics.h:105: COUNT Preserve the comment above COUNT. https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:212: case 0x00000101: // GATT_FAILURE Please sort this list by the Android Enum names. (I don't know why I failed to do that originally.) https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:229: return DidFailToConnectGatt(ERROR_READ_FAILED); More instances of 'failed' and 'not supported'. https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/test/b... File device/bluetooth/test/bluetooth_test_android.cc (right): https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/test/b... device/bluetooth/test/bluetooth_test_android.cc:89: case BluetoothDevice::ERROR_FAILED: Please sort into two groups ordered BluetoothDevice::enum name. First group with supported values and the final group with the NOTREACHED error. https://codereview.chromium.org/1464443002/diff/20001/extensions/common/api/b... File extensions/common/api/bluetooth_private.idl (right): https://codereview.chromium.org/1464443002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth_private.idl:58: attributeLengthInvalid, All places we use ConnectResultType need to be updated too -- at a minimum with an assert. E.g. in chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js https://codereview.chromium.org/1464443002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1464443002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:77189: + <int value="14" label="Read Failed"/> When fixing the names for permitted and failed fix here also.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:99: ERROR_UNSUPPORTED_DEVICE, On 2015/11/19 22:25:05, scheib wrote: > We should sort these values alphabetically. Done. https://codereview.chromium.org/1464443002/diff/1/device/bluetooth/bluetooth_... device/bluetooth/bluetooth_device.h:99: ERROR_UNSUPPORTED_DEVICE, On 2015/11/24 01:30:48, scheib wrote: > On 2015/11/19 22:25:05, scheib wrote: > > We should sort these values alphabetically. > > Sorry, I mean we should sort the entire ConnectErrorCode enum. Done. https://codereview.chromium.org/1464443002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_metrics.h (right): https://codereview.chromium.org/1464443002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_metrics.h:105: COUNT On 2015/11/24 01:30:48, scheib wrote: > Preserve the comment above COUNT. Done. https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:106: ERROR_WRITE_FAILED done with sorting alphabetically according to previous patch's comment. https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:212: case 0x00000101: // GATT_FAILURE On 2015/11/24 01:30:48, scheib wrote: > Please sort this list by the Android Enum names. (I don't know why I failed to > do that originally.) Done. https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:229: return DidFailToConnectGatt(ERROR_READ_FAILED); On 2015/11/24 01:30:48, scheib wrote: > More instances of 'failed' and 'not supported'. Could you explain a little bit more? Do you mean replace 'ERROR_WRITE_FAILED' with 'ERROR_WRITE_NOT_PERMITTED' ? And Same as 'ERROR_READ_FAILED' https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/test/b... File device/bluetooth/test/bluetooth_test_android.cc (right): https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/test/b... device/bluetooth/test/bluetooth_test_android.cc:89: case BluetoothDevice::ERROR_FAILED: On 2015/11/24 01:30:48, scheib wrote: > Please sort into two groups ordered BluetoothDevice::enum name. First group with > supported values and the final group with the NOTREACHED error. Done. https://codereview.chromium.org/1464443002/diff/20001/extensions/common/api/b... File extensions/common/api/bluetooth_private.idl (right): https://codereview.chromium.org/1464443002/diff/20001/extensions/common/api/b... extensions/common/api/bluetooth_private.idl:58: attributeLengthInvalid, On 2015/11/24 01:30:48, scheib wrote: > All places we use ConnectResultType need to be updated too -- at a minimum with > an assert. E.g. in > chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js Done. https://codereview.chromium.org/1464443002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1464443002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:77189: + <int value="14" label="Read Failed"/> On 2015/11/24 01:30:48, scheib wrote: > When fixing the names for permitted and failed fix here also. Fixed. "{Read, Write} Failed" -> "{Read, Write} Not Permitted"
The CQ bit was checked by scheib@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464443002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464443002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks, keep it up. ;) Some simple requests below (helping clean up code by keeping it all sorted for consistency, and changing the 'failed' to 'not permitted' names). Also we'll need to check in with a Chrome OS UI owner to see if they want strings for errors not possible on ChromeOS. https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device_android.cc (right): https://codereview.chromium.org/1464443002/diff/20001/device/bluetooth/blueto... device/bluetooth/bluetooth_device_android.cc:229: return DidFailToConnectGatt(ERROR_READ_FAILED); On 2015/11/25 14:17:13, Kai Jiang wrote: > On 2015/11/24 01:30:48, scheib wrote: > > More instances of 'failed' and 'not supported'. > > Could you explain a little bit more? Do you mean replace 'ERROR_WRITE_FAILED' > with 'ERROR_WRITE_NOT_PERMITTED' ? And Same as 'ERROR_READ_FAILED' Android enumerations that use a suffix "_NOT_PERMITTED" should be matched with our enumerations and text also using 'permitted' and not 'failed'. https://codereview.chromium.org/1464443002/diff/40001/chrome/app/chromeos_str... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/1464443002/diff/40001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:2236: <message name="IDS_OPTIONS_SETTINGS_BLUETOOTH_CONNECT_ATTRIBUTE_LENGTH_INVALID"> Pending OWNERS feedback, we may not want these strings at all. But, if we DO end up useing strings they should follow the other strings' form for connect failures: [More specific error description] when trying to connect to ... e.g. Write operation exceeded the maximium length of the attribute when trying to connect to Insufficient authentication when trying to connect to https://codereview.chromium.org/1464443002/diff/40001/chrome/app/chromeos_str... chrome/app/chromeos_strings.grdp:2243: Insufficient authentication for a given operation on: "<ph name="DEVICE_NAME">%1<ex>Nexus 4</ex></ph>". Insufficient encryption https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc (right): https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc:1147: .Times(9) 16 times now. And src/chrome/test/data/extensions/api_test/bluetooth_low_energy/gatt_connection/runtest.js will need to be updated. https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc (right): https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:1321: } else if (error_code == BluetoothDevice::ERROR_ATTRIBUTE_LENGTH_INVALID) { This list should be alphabetically sorted too. I'm not sure why a switch() wasn't used in previous code. It would help because a switch without a "default:" will help catch any missing "case :" values at compile time. https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js (right): https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:524: case chrome.bluetoothPrivate.ConnectResultType.ALREADY_CONNECTED: Sort list alphabetically. https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:551: case chrome.bluetoothPrivate.ConnectResultType.ATTRIBUTE_LENGTH_INVALID: We should check with [OWNERS] of this UI and propose that we only use assert() here, and NOT add strings. We don't expect these errors on chromeOS code. So, the API's .idl must include the values, but we can assert here that on ChromeOS we do not encounter them. [OWNERS] https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... I will add an owner in next message. https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:564: message = 'bluetoothConnectReadFailed'; Enum and string should use 'permitted' instead of 'failed'. https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:570: message = 'bluetoothConnectWriteFailed'; Enum and string should use 'permitted' instead of 'failed'. https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc (right): https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:102: { "bluetoothConnectReadFailed", Enum and string should use 'permitted' instead of 'failed'. https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:106: { "bluetoothConnectWriteFailed", Enum and string should use 'permitted' instead of 'failed'. https://codereview.chromium.org/1464443002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1464443002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:137: case device::BluetoothDevice::ERROR_ATTRIBUTE_LENGTH_INVALID: Sort list. https://codereview.chromium.org/1464443002/diff/40001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_metrics.h (right): https://codereview.chromium.org/1464443002/diff/40001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_metrics.h:98: ATTRIBUTE_LENGTH_INVALID = 10, ;) Don't sort list - because if enums have values assigned we keep them in order. https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/blueto... File device/bluetooth/bluetooth_device.h (right): https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:102: ERROR_READ_FAILED, ERROR_READ_NOT_PERMITTED https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/blueto... device/bluetooth/bluetooth_device.h:106: ERROR_WRITE_FAILED ERROR_WRITE_NOT_PERMITTED https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/test/b... File device/bluetooth/test/bluetooth_test_android.cc (right): https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/test/b... device/bluetooth/test/bluetooth_test_android.cc:89: case BluetoothDevice::ERROR_CONNECTION_CONGESTED: Sort list alphabetically. https://codereview.chromium.org/1464443002/diff/40001/extensions/common/api/b... File extensions/common/api/bluetooth_private.idl (right): https://codereview.chromium.org/1464443002/diff/40001/extensions/common/api/b... extensions/common/api/bluetooth_private.idl:48: success, Please sort alphabetically. https://codereview.chromium.org/1464443002/diff/40001/extensions/common/api/b... extensions/common/api/bluetooth_private.idl:62: readFailed, readNotPermitted https://codereview.chromium.org/1464443002/diff/40001/extensions/common/api/b... extensions/common/api/bluetooth_private.idl:64: writeFailed writeNotPermitted https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right): https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:39: MAP_ERROR(ConnectAttributeLengthInvalid, NetworkError, "Write operation exceeds the maximum length of the attribute."); Sort alphabetically all NetworkError items. https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:43: MAP_ERROR(ConnectReadFailed, NetworkError, "GATT read operation is not permitted."); ConnectReadNotPermitted https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:45: MAP_ERROR(ConnectWriteFailed, NetworkError, "GATT write operation is not permitted."); ConnectWriteNotPermitted https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h (right): https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h:23: // NetworkError: Sort the NetworkError: block. https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h:39: ConnectReadFailed, ConnectReadNotPermitted https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h:41: ConnectWriteFailed, ConnectWriteNotPermitted https://codereview.chromium.org/1464443002/diff/40001/third_party/closure_com... File third_party/closure_compiler/externs/bluetooth_private.js (right): https://codereview.chromium.org/1464443002/diff/40001/third_party/closure_com... third_party/closure_compiler/externs/bluetooth_private.js:42: SUCCESS: 'success', Sort alphabetically. https://codereview.chromium.org/1464443002/diff/40001/third_party/closure_com... third_party/closure_compiler/externs/bluetooth_private.js:56: READ_FAILED: 'readFailed', Use 'not permitted' instead of 'failed'. https://codereview.chromium.org/1464443002/diff/40001/third_party/closure_com... third_party/closure_compiler/externs/bluetooth_private.js:58: WRITE_FAILED: 'writeFailed', Use 'not permitted' instead of 'failed'.
Description was changed from ========== bluetooth: android: Add BluetoothDevice::ConnectErrorCode enums increased ConnectErrorCode enum and cases in BluetoothDeviceAndroid::OnConnectionStateChange BUG=531058 ========== to ========== bluetooth: android: Add BluetoothDevice::ConnectErrorCode enums increased ConnectErrorCode enum and cases in BluetoothDeviceAndroid::OnConnectionStateChange BUG=531058 ==========
scheib@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb, would you comment on what we should do here: - We're adding Bluetooth connection error enumerations that are Android only at the moment. - The enumeration is used in ChromeOS settings UI to report connection errors - Should we handle enum error values that can't be produced on ChromeOS today? This would require that we add strings [con], but would make things more robust for any future code changes that cause those values to be produced on ChromeOS [pro]. > https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource... > chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:551: > case chrome.bluetoothPrivate.ConnectResultType.ATTRIBUTE_LENGTH_INVALID: > We should check with [OWNERS] of this UI and propose that we only use assert() > here, and NOT add strings. We don't expect these errors on chromeOS code. So, > the API's .idl must include the values, but we can assert here that on ChromeOS > we do not encounter them. > > [OWNERS] > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > I will add an owner in next message.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Sorry for the late update. I run the try bot. I got wrong info under {windows,
mac, linux}_chromium_rel_ng
worker/2 bluetooth/connectGATT.html output stderr lines:
13:33:05.839 3434
[9096:9096:1128/133305:5087988879:WARNING:bluetooth_dispatcher_host.cc(672)] No
Bluetooth chooser implementation; falling back to first device.
13:33:05.839 3434
[9096:9096:1128/133305:5087993370:WARNING:bluetooth_dispatcher_host.cc(672)] No
Bluetooth chooser implementation; falling back to first device.
Scheib, Could you give me more clues of how to handle this?
P.S still ping stevenjb.
https://codereview.chromium.org/1464443002/diff/40001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):
https://codereview.chromium.org/1464443002/diff/40001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:2243: Insufficient authentication for a given
operation on: "<ph name="DEVICE_NAME">%1<ex>Nexus 4</ex></ph>".
On 2015/11/26 00:02:50, scheib wrote:
> Insufficient encryption
Done.
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc
(right):
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc:1147:
.Times(9)
On 2015/11/26 00:02:50, scheib wrote:
> 16 times now. And
>
src/chrome/test/data/extensions/api_test/bluetooth_low_energy/gatt_connection/runtest.js
> will need to be updated.
Done.
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/extensio...
File
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc
(right):
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:1321:
} else if (error_code == BluetoothDevice::ERROR_ATTRIBUTE_LENGTH_INVALID) {
On 2015/11/26 00:02:51, scheib wrote:
> This list should be alphabetically sorted too. I'm not sure why a switch()
> wasn't used in previous code. It would help because a switch without a
> "default:" will help catch any missing "case :" values at compile time.
Done.
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js
(right):
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:524:
case chrome.bluetoothPrivate.ConnectResultType.ALREADY_CONNECTED:
On 2015/11/26 00:02:51, scheib wrote:
> Sort list alphabetically.
Done.
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:570:
message = 'bluetoothConnectWriteFailed';
On 2015/11/26 00:02:51, scheib wrote:
> Enum and string should use 'permitted' instead of 'failed'.
Done.
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):
https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:106: {
"bluetoothConnectWriteFailed",
On 2015/11/26 00:02:51, scheib wrote:
> Enum and string should use 'permitted' instead of 'failed'.
Done.
https://codereview.chromium.org/1464443002/diff/40001/content/browser/bluetoo...
File content/browser/bluetooth/bluetooth_metrics.h (right):
https://codereview.chromium.org/1464443002/diff/40001/content/browser/bluetoo...
content/browser/bluetooth/bluetooth_metrics.h:104: WRITE_FAILED = 16,
Changed to 'WRITE_NOT_PERMITTED'
https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.h (right):
https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.h:106: ERROR_WRITE_FAILED
On 2015/11/26 00:02:51, scheib wrote:
> ERROR_WRITE_NOT_PERMITTED
Done.
https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device_android.cc (right):
https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device_android.cc:231: return
DidFailToConnectGatt(ERROR_WRITE_FAILED);
ERROR_WRITE_NOT_PERMITTED
https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/test/b...
File device/bluetooth/test/bluetooth_test_android.cc (right):
https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_android.cc:89: case
BluetoothDevice::ERROR_CONNECTION_CONGESTED:
On 2015/11/26 00:02:51, scheib wrote:
> Sort list alphabetically.
Done.
https://codereview.chromium.org/1464443002/diff/40001/device/bluetooth/test/b...
device/bluetooth/test/bluetooth_test_android.cc:113: case
BluetoothDevice::ERROR_WRITE_FAILED:
ERROR_{READ, WRITE}_NOT_PERMITTED
https://codereview.chromium.org/1464443002/diff/40001/extensions/browser/api/...
File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right):
https://codereview.chromium.org/1464443002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:556: case
device::BluetoothDevice::ERROR_WRITE_FAILED:
ERROR_{READ, WRITE}_NOT_PERMITTED
https://codereview.chromium.org/1464443002/diff/40001/extensions/browser/api/...
extensions/browser/api/bluetooth/bluetooth_private_api.cc:558: break;
sorted above.
https://codereview.chromium.org/1464443002/diff/40001/extensions/common/api/b...
File extensions/common/api/bluetooth_private.idl (right):
https://codereview.chromium.org/1464443002/diff/40001/extensions/common/api/b...
extensions/common/api/bluetooth_private.idl:48: success,
On 2015/11/26 00:02:51, scheib wrote:
> Please sort alphabetically.
Done.
https://codereview.chromium.org/1464443002/diff/40001/extensions/common/api/b...
extensions/common/api/bluetooth_private.idl:62: readFailed,
On 2015/11/26 00:02:51, scheib wrote:
> readNotPermitted
Done.
https://codereview.chromium.org/1464443002/diff/40001/extensions/common/api/b...
extensions/common/api/bluetooth_private.idl:64: writeFailed
On 2015/11/26 00:02:51, scheib wrote:
> writeNotPermitted
Done.
https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp (right):
https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:39:
MAP_ERROR(ConnectAttributeLengthInvalid, NetworkError, "Write operation exceeds
the maximum length of the attribute.");
On 2015/11/26 00:02:51, scheib wrote:
> Sort alphabetically all NetworkError items.
Done.
https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:43:
MAP_ERROR(ConnectReadFailed, NetworkError, "GATT read operation is not
permitted.");
On 2015/11/26 00:02:51, scheib wrote:
> ConnectReadNotPermitted
Done.
https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/bluetooth/BluetoothError.cpp:45:
MAP_ERROR(ConnectWriteFailed, NetworkError, "GATT write operation is not
permitted.");
On 2015/11/26 00:02:51, scheib wrote:
> ConnectWriteNotPermitted
Done.
https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h
(right):
https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h:23: //
NetworkError:
On 2015/11/26 00:02:51, scheib wrote:
> Sort the NetworkError: block.
Done.
https://codereview.chromium.org/1464443002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/bluetooth/WebBluetoothError.h:41:
ConnectWriteFailed,
On 2015/11/26 00:02:51, scheib wrote:
> ConnectWriteNotPermitted
Done.
https://codereview.chromium.org/1464443002/diff/40001/third_party/closure_com...
File third_party/closure_compiler/externs/bluetooth_private.js (right):
https://codereview.chromium.org/1464443002/diff/40001/third_party/closure_com...
third_party/closure_compiler/externs/bluetooth_private.js:42: SUCCESS:
'success',
On 2015/11/26 00:02:51, scheib wrote:
> Sort alphabetically.
Done.
https://codereview.chromium.org/1464443002/diff/40001/third_party/closure_com...
third_party/closure_compiler/externs/bluetooth_private.js:56: READ_FAILED:
'readFailed',
On 2015/11/26 00:02:51, scheib wrote:
> Use 'not permitted' instead of 'failed'.
Done.
https://codereview.chromium.org/1464443002/diff/40001/third_party/closure_com...
third_party/closure_compiler/externs/bluetooth_private.js:58: WRITE_FAILED:
'writeFailed',
On 2015/11/26 00:02:51, scheib wrote:
> Use 'not permitted' instead of 'failed'.
Done.
We shouldn't add strings for errors that can not (currently) occur. Instead we should just and an "Unknown" error for now so that if a new error does sneak in, we can address it if/when we get a bug report without breaking the UI. On Wed, Nov 25, 2015 at 5:06 PM, <scheib@chromium.org> wrote: > stevenjb, would you comment on what we should do here: > - We're adding Bluetooth connection error enumerations that are Android > only at > the moment. > - The enumeration is used in ChromeOS settings UI to report connection > errors > - Should we handle enum error values that can't be produced on ChromeOS > today? > This would require that we add strings [con], but would make things more > robust > for any future code changes that cause those values to be produced on > ChromeOS > [pro]. > > > > https://codereview.chromium.org/1464443002/diff/40001/chrome/browser/resource... > > > chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:551: > >> case chrome.bluetoothPrivate.ConnectResultType.ATTRIBUTE_LENGTH_INVALID: >> We should check with [OWNERS] of this UI and propose that we only use >> assert() >> here, and NOT add strings. We don't expect these errors on chromeOS code. >> So, >> the API's .idl must include the values, but we can assert here that on >> > ChromeOS > >> we do not encounter them. >> > > [OWNERS] >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res... > > I will add an owner in next message. >> > > > https://codereview.chromium.org/1464443002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
stevenjb's suggestion for strings makes sense to me, so we can collapse down to a single unknown string. stevenjb, should we use 'assert' in the JavaScript for these cases? Sorry I didn't have time to look into the errors today. I hope to help suggest a path for that over the next day or two.
I think we should avoid an assert in the JS and just show the unknown error string. That way we won't accidentally break any UI by introducing an unhandled error. (If we forget to handle it in the UI, eventually someone will hopefully notice and report it). On Mon, Nov 30, 2015 at 5:33 PM, <scheib@chromium.org> wrote: > stevenjb's suggestion for strings makes sense to me, so we can collapse > down to > a single unknown string. stevenjb, should we use 'assert' in the > JavaScript for > these cases? > > Sorry I didn't have time to look into the errors today. I hope to help > suggest a > path for that over the next day or two. > > > https://codereview.chromium.org/1464443002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
After looking through the CL, I think it is better that we include the strings now, i.e. leave the CL as-is. Hopefully we can add support for those errors on Chrome OS sooner than later. lgtm with one suggestion. https://codereview.chromium.org/1464443002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc (right): https://codereview.chromium.org/1464443002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:1351: error_status = kStatusErrorFailed; Since we are using a case statement with an enum, we should handle every enum explicitly and omit the default. That way if an enum is added but not handled here, the compiler will generate an error.
Bluetooth errors: connectGATT.html tests errors by sending an errorUUID https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... LayoutTestBluetoothAdapterProvider::GetFailingConnectionsAdapter generates the fake adapter that should fail for that test. https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/brow... GetFailingConnectionsAdapter & GetUnconnectableDevice code was brittle, we need to fix it and I prefer we do so in a way that isn't as brittle: GetUnconnectableDevice should hard code the enum & enumUUID value in a switch statement that will compile fail if more enums are added (e.g. no default:). The values can either be changed to match our new order or preserve the values used in connectGATT.html currently in this patch. GetFailingConnectionsAdapter should use a for loop iterating over 0 to ERROR_MAX_VALUE (adding NUM_CONNECT_ERROR_CODES // Keep as last enum.) No specific enum values should be referenced.
Kai Jiang, thank you for your work to date on this - hoping you'll see it through to landing. If you need more assistance, or could update on when/if you may continue please write.
Sorry about pausing on updating this issue due to some school work recently. I will continue to work on it tonight. On Thu, Jan 7, 2016 at 3:19 PM <scheib@chromium.org> wrote: > Kai Jiang, thank you for your work to date on this - hoping you'll see it > through to landing. If you need more assistance, or could update on when/if > you > may continue please write. > > https://codereview.chromium.org/1464443002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/12/03 22:17:52, scheib wrote: > Bluetooth errors: > > connectGATT.html tests errors by sending an errorUUID > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > LayoutTestBluetoothAdapterProvider::GetFailingConnectionsAdapter generates the > fake adapter that should fail for that test. > https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/brow... > > GetFailingConnectionsAdapter & GetUnconnectableDevice code was brittle, we need > to fix it and I prefer we do so in a way that isn't as brittle: > > GetUnconnectableDevice should hard code the enum & enumUUID value in a switch > statement that will compile fail if more enums are added (e.g. no default:). The > values can either be changed to match our new order or preserve the values used > in connectGATT.html currently in this patch. I have addressed the issues stevenjb and you mentioned. But in this part(GetUnconnectableDevice), I can't totally understand. Could you give me more hints about this part? > > GetFailingConnectionsAdapter should use a for loop iterating over 0 to > ERROR_MAX_VALUE (adding NUM_CONNECT_ERROR_CODES // Keep as last enum.) No > specific enum values should be referenced.
On 2016/01/08 15:28:41, Kai Jiang wrote: > On 2015/12/03 22:17:52, scheib wrote: > > Bluetooth errors: > > > > connectGATT.html tests errors by sending an errorUUID > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > LayoutTestBluetoothAdapterProvider::GetFailingConnectionsAdapter generates the > > fake adapter that should fail for that test. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/brow... > > > > GetFailingConnectionsAdapter & GetUnconnectableDevice code was brittle, we > need > > to fix it and I prefer we do so in a way that isn't as brittle: > > > > GetUnconnectableDevice should hard code the enum & enumUUID value in a switch > > statement that will compile fail if more enums are added (e.g. no default:). > The > > values can either be changed to match our new order or preserve the values > used > > in connectGATT.html currently in this patch. > I have addressed the issues stevenjb and you mentioned. But in this > part(GetUnconnectableDevice), > I can't totally understand. Could you give me more hints about this part? > > > > > GetFailingConnectionsAdapter should use a for loop iterating over 0 to > > ERROR_MAX_VALUE (adding NUM_CONNECT_ERROR_CODES // Keep as last enum.) No > > specific enum values should be referenced. See this example addition to your patch: https://github.com/scheib/chromium/compare/codereview-1464443002-...coderevie... we want the UUIDs that the tests use to be stable and never need to change, even when the value of the enums change. This adds a switch statement to map the enums into stable UUID values.
On 2016/01/08 20:04:22, scheib wrote: > On 2016/01/08 15:28:41, Kai Jiang wrote: > > On 2015/12/03 22:17:52, scheib wrote: > > > Bluetooth errors: > > > > > > connectGATT.html tests errors by sending an errorUUID > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > LayoutTestBluetoothAdapterProvider::GetFailingConnectionsAdapter generates > the > > > fake adapter that should fail for that test. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/shell/brow... > > > > > > GetFailingConnectionsAdapter & GetUnconnectableDevice code was brittle, we > > need > > > to fix it and I prefer we do so in a way that isn't as brittle: > > > > > > GetUnconnectableDevice should hard code the enum & enumUUID value in a > switch > > > statement that will compile fail if more enums are added (e.g. no default:). > > The > > > values can either be changed to match our new order or preserve the values > > used > > > in connectGATT.html currently in this patch. > > I have addressed the issues stevenjb and you mentioned. But in this > > part(GetUnconnectableDevice), > > I can't totally understand. Could you give me more hints about this part? > > > > > > > > GetFailingConnectionsAdapter should use a for loop iterating over 0 to > > > ERROR_MAX_VALUE (adding NUM_CONNECT_ERROR_CODES // Keep as last enum.) No > > > specific enum values should be referenced. > > See this example addition to your patch: > > https://github.com/scheib/chromium/compare/codereview-1464443002-...coderevie... > > we want the UUIDs that the tests use to be stable and never need to change, even > when the value of the enums change. This adds a switch statement to map the > enums into stable UUID values. BTW you should also be able to apply these changes locally with: git checkout your-working-branch git remote add github-scheib https://github.com/scheib/chromium.git git fetch github-scheib git merge codereview-1464443002-suggestion-
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
Patchset #5 (id:240001) has been deleted
Patchset #6 (id:280001) has been deleted
https://codereview.chromium.org/1464443002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc (right): https://codereview.chromium.org/1464443002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc:1351: error_status = kStatusErrorFailed; On 2015/12/01 17:51:33, stevenjb wrote: > Since we are using a case statement with an enum, we should handle every enum > explicitly and omit the default. That way if an enum is added but not handled > here, the compiler will generate an error. Done.
Thanks! Looking great. Just one more change and we'll land: https://codereview.chromium.org/1464443002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc (right): https://codereview.chromium.org/1464443002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc:1150: EXPECT_CALL(*device0_, CreateGattConnection(_, _)) If the number of enums changes we need to update this code, let's make that a compile check so that developers don't overlook it. Add here: static_assert(BluetoothDevice::NUM_CONNECT_ERROR_CODES == 16, "Update required if the number of BluetoothDevice enums changes.");
scheib@chromium.org changed reviewers: + isherman@chromium.org, reillyg@chromium.org
OWNERS review please: isherman: tools/metrics/histograms/histograms.xml reillyg: chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_api.cc chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.cc chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_event_router.h device/bluetooth/bluetooth_device.h extensions/browser/api/bluetooth/bluetooth_private_api.cc extensions/common/api/bluetooth_private.idl
histograms.xml lgtm
Patchset #6 (id:300001) has been deleted
https://codereview.chromium.org/1464443002/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc (right): https://codereview.chromium.org/1464443002/diff/260001/chrome/browser/extensi... chrome/browser/extensions/api/bluetooth_low_energy/bluetooth_low_energy_apitest.cc:1150: EXPECT_CALL(*device0_, CreateGattConnection(_, _)) On 2016/01/09 23:27:13, scheib wrote: > If the number of enums changes we need to update this code, let's make that a > compile check so that developers don't overlook it. Add here: > static_assert(BluetoothDevice::NUM_CONNECT_ERROR_CODES == 16, > "Update required if the number of BluetoothDevice enums changes."); Make sense. Addressed comment.
LGTM
lgtm
The CQ bit was checked by scheib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1464443002/#ps320001 (title: "VI")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1464443002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1464443002/320001
Message was sent while issue was closed.
Description was changed from ========== bluetooth: android: Add BluetoothDevice::ConnectErrorCode enums increased ConnectErrorCode enum and cases in BluetoothDeviceAndroid::OnConnectionStateChange BUG=531058 ========== to ========== bluetooth: android: Add BluetoothDevice::ConnectErrorCode enums increased ConnectErrorCode enum and cases in BluetoothDeviceAndroid::OnConnectionStateChange BUG=531058 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== bluetooth: android: Add BluetoothDevice::ConnectErrorCode enums increased ConnectErrorCode enum and cases in BluetoothDeviceAndroid::OnConnectionStateChange BUG=531058 ========== to ========== bluetooth: android: Add BluetoothDevice::ConnectErrorCode enums increased ConnectErrorCode enum and cases in BluetoothDeviceAndroid::OnConnectionStateChange BUG=531058 Committed: https://crrev.com/d730ca14bd3caced4f52617146969d72304c56e9 Cr-Commit-Position: refs/heads/master@{#368649} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d730ca14bd3caced4f52617146969d72304c56e9 Cr-Commit-Position: refs/heads/master@{#368649}
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1464443002/diff/320001/chrome/browser/resourc... File chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js (right): https://codereview.chromium.org/1464443002/diff/320001/chrome/browser/resourc... chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:559: case chrome.bluetoothPrivate.ConnctResultType.INSUFFICIENT_ENCRYPTION: fail
Message was sent while issue was closed.
https://codereview.chromium.org/1464443002/diff/320001/chrome/browser/resourc... File chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js (right): https://codereview.chromium.org/1464443002/diff/320001/chrome/browser/resourc... chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:559: case chrome.bluetoothPrivate.ConnctResultType.INSUFFICIENT_ENCRYPTION: On 2016/01/11 20:34:05, Dan Beam wrote: > fail I'll patch now.
Message was sent while issue was closed.
this is how I found out, btw https://build.chromium.org/p/chromium.fyi/builders/Closure%20Compilation%20Li... https://codereview.chromium.org/1464443002/diff/320001/chrome/browser/resourc... File chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js (right): https://codereview.chromium.org/1464443002/diff/320001/chrome/browser/resourc... chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:559: case chrome.bluetoothPrivate.ConnctResultType.INSUFFICIENT_ENCRYPTION: On 2016/01/11 20:35:57, scheib wrote: > On 2016/01/11 20:34:05, Dan Beam wrote: > > fail > > I'll patch now. yeah, I'm just skeptical as to how much testing this got :(
Message was sent while issue was closed.
https://codereview.chromium.org/1464443002/diff/320001/chrome/browser/resourc... File chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js (right): https://codereview.chromium.org/1464443002/diff/320001/chrome/browser/resourc... chrome/browser/resources/options/chromeos/bluetooth_pair_device_overlay.js:559: case chrome.bluetoothPrivate.ConnctResultType.INSUFFICIENT_ENCRYPTION: On 2016/01/11 20:37:37, Dan Beam wrote: > On 2016/01/11 20:35:57, scheib wrote: > > On 2016/01/11 20:34:05, Dan Beam wrote: > > > fail > > > > I'll patch now. > > yeah, I'm just skeptical as to how much testing this got :( Tryjobs as run on patches here, nothing to be skeptical about - it isn't obvious that there is no automated test coverage for this. Sorry. I'll do a linux COS build and add manual TEST= line for bringing up the bluetooth pairing UI. stevenjb: Anything other than just loading the bluetooth pairing device overlay needed? |
