Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
819713002:
bluetoothPrivate: Add disconnectAll function (Closed)
|
Created:
6 years ago by armansito Modified:
5 years, 11 months ago CC:
asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, joth, Tim Song Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionbluetoothPrivate: Add disconnectAll function
This CL adds the chrome.bluetoothPrivate.disconnectAll function, which can
be used by whitelisted extensions to teardown all connected data bearers
to a remote Bluetooth device.
BUG=441058
Committed: https://crrev.com/373214a5345dd65ca441dcbcca4aa9eecf276a82
Cr-Commit-Position: refs/heads/master@{#310154}
Patch Set 1 #Patch Set 2 : Ran update_extension_functions.py #
Total comments: 9
Patch Set 3 : Renamed disconnect -> disconnectAll; addressed jyasskin@'s comments #
Total comments: 5
Patch Set 4 : Fixed comment by jyasskin@ #Messages
Total messages: 26 (3 generated)
armansito@chromium.org changed reviewers: + jamuraa@chromium.org
joth@: Can you please test the CL and see that it works for you?
lgtm
armansito@chromium.org changed reviewers: + isherman@chromium.org, jyasskin@chromium.org
jyasskin@: extensions/OWNERS isherman@: histograms OWNERS
cc'd tengs@: FYI, may be useful for your use case as well.
histograms.xml lgtm, thanks
https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js (right): https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js:7: var errorDisconnectFailed = 'Failed to disconnect device'; This message has been really annoying when debugging uses of the bluetooth API. Can you report anything more precise? And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets an error description and then THROWS IT AWAY. That's really not an acceptable way to treat our developers. https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js:18: if (chrome.runtime.lastError) Isn't this redundant with the assertNoLastError() call? https://codereview.chromium.org/819713002/diff/20001/extensions/common/api/bl... File extensions/common/api/bluetooth_private.json (right): https://codereview.chromium.org/819713002/diff/20001/extensions/common/api/bl... extensions/common/api/bluetooth_private.json:41: "name": "disconnect", Can you call this something that won't be confused with bluetooth*.disconnect(), which only tear down the current app's connection? "disconnectAllConnections()"?
https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js (right): https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js:7: var errorDisconnectFailed = 'Failed to disconnect device'; On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > This message has been really annoying when debugging uses of the bluetooth API. > Can you report anything more precise? > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets an error > description and then THROWS IT AWAY. That's really not an acceptable way to > treat our developers. The only error that BlueZ returns is 'org.bluez.Error.NotConnected' (see http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). We're already returning errorNotConnected here at the API level by doing the check ourselves. https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js:18: if (chrome.runtime.lastError) On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > Isn't this redundant with the assertNoLastError() call? I'm not sure actually, as I mostly followed the other bluetoothPrivate API tests as example and they do this check. I can test this and remove it if necessary. https://codereview.chromium.org/819713002/diff/20001/extensions/common/api/bl... File extensions/common/api/bluetooth_private.json (right): https://codereview.chromium.org/819713002/diff/20001/extensions/common/api/bl... extensions/common/api/bluetooth_private.json:41: "name": "disconnect", On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > Can you call this something that won't be confused with bluetooth*.disconnect(), > which only tear down the current app's connection? "disconnectAllConnections()"? Makes sense, will do.
https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js (right): https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js:7: var errorDisconnectFailed = 'Failed to disconnect device'; On 2014/12/20 01:24:28, armansito wrote: > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > This message has been really annoying when debugging uses of the bluetooth > API. > > Can you report anything more precise? > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets an error > > description and then THROWS IT AWAY. That's really not an acceptable way to > > treat our developers. > > The only error that BlueZ returns is 'org.bluez.Error.NotConnected' (see > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > We're already returning errorNotConnected here at the API level by doing the > check ourselves. If this error case is impossible, you should probably DCHECK that it doesn't happen instead of writing code to send an incorrect message to developers if it does happen. I suspect there's a race condition during the several cross-process hops in which it does happen, though.
https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js (right): https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js:7: var errorDisconnectFailed = 'Failed to disconnect device'; On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > On 2014/12/20 01:24:28, armansito wrote: > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > This message has been really annoying when debugging uses of the bluetooth > > API. > > > Can you report anything more precise? > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets an error > > > description and then THROWS IT AWAY. That's really not an acceptable way to > > > treat our developers. > > > > The only error that BlueZ returns is 'org.bluez.Error.NotConnected' (see > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > We're already returning errorNotConnected here at the API level by doing the > > check ourselves. > > If this error case is impossible, you should probably DCHECK that it doesn't > happen instead of writing code to send an incorrect message to developers if it > does happen. I suspect there's a race condition during the several cross-process > hops in which it does happen, though. No it's not impossible. I'm not sure I'm following you here. In what case are we returning an incorrect error message from this function?
https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right): https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... extensions/browser/api/bluetooth/bluetooth_private_api.cc:329: SetError(kDisconnectError); [Moving over to the API implementation.] On 2014/12/20 01:43:38, armansito wrote: > On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > > On 2014/12/20 01:24:28, armansito wrote: > > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > > This message has been really annoying when debugging uses of the bluetooth > > > API. > > > > Can you report anything more precise? > > > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets an > error > > > > description and then THROWS IT AWAY. That's really not an acceptable way > to > > > > treat our developers. > > > > > > The only error that BlueZ returns is 'org.bluez.Error.NotConnected' (see > > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > > We're already returning errorNotConnected here at the API level by doing the > > > check ourselves. > > > > If this error case is impossible, you should probably DCHECK that it doesn't > > happen instead of writing code to send an incorrect message to developers if > it > > does happen. I suspect there's a race condition during the several > cross-process > > hops in which it does happen, though. > > No it's not impossible. I'm not sure I'm following you here. In what case are we > returning an incorrect error message from this function? Say BlueZ returns 'org.bluez.Error.NotConnected'. That'll happen after the !IsConnected() check above that returns "Device is not connected". Instead, I believe the code will wind up here and return "Failed to disconnect device", which isn't the right message.
On 2014/12/20 01:43:38, armansito wrote: > https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... > File chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js > (right): > > https://codereview.chromium.org/819713002/diff/20001/chrome/test/data/extensi... > chrome/test/data/extensions/api_test/bluetooth_private/disconnect/test.js:7: var > errorDisconnectFailed = 'Failed to disconnect device'; > On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > > On 2014/12/20 01:24:28, armansito wrote: > > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > > This message has been really annoying when debugging uses of the bluetooth > > > API. > > > > Can you report anything more precise? > > > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets an > error > > > > description and then THROWS IT AWAY. That's really not an acceptable way > to > > > > treat our developers. > > > > > > The only error that BlueZ returns is 'org.bluez.Error.NotConnected' (see > > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > > We're already returning errorNotConnected here at the API level by doing the > > > check ourselves. > > > > If this error case is impossible, you should probably DCHECK that it doesn't > > happen instead of writing code to send an incorrect message to developers if > it > > does happen. I suspect there's a race condition during the several > cross-process > > hops in which it does happen, though. > > No it's not impossible. I'm not sure I'm following you here. In what case are we > returning an incorrect error message from this function? There can be other failures that can happen due to the asynchronous nature of D-Bus, Bluetooth timeouts, etc. In general, disconnecting is a best-effort operation so it SHOULD succeed, but there are these other cases that don't necessarily apply to the platforms other than Chrome OS and are really in the sort of "Unexpected error" category. So far, the APIs report failure for these cases and I chose to be consistent here.
On 2014/12/20 01:48:56, Jeffrey Yasskin wrote: > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right): > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > extensions/browser/api/bluetooth/bluetooth_private_api.cc:329: > SetError(kDisconnectError); > [Moving over to the API implementation.] > > On 2014/12/20 01:43:38, armansito wrote: > > On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > > > On 2014/12/20 01:24:28, armansito wrote: > > > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > > > This message has been really annoying when debugging uses of the > bluetooth > > > > API. > > > > > Can you report anything more precise? > > > > > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets an > > error > > > > > description and then THROWS IT AWAY. That's really not an acceptable way > > to > > > > > treat our developers. > > > > > > > > The only error that BlueZ returns is 'org.bluez.Error.NotConnected' (see > > > > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > > > We're already returning errorNotConnected here at the API level by doing > the > > > > check ourselves. > > > > > > If this error case is impossible, you should probably DCHECK that it doesn't > > > happen instead of writing code to send an incorrect message to developers if > > it > > > does happen. I suspect there's a race condition during the several > > cross-process > > > hops in which it does happen, though. > > > > No it's not impossible. I'm not sure I'm following you here. In what case are > we > > returning an incorrect error message from this function? > > Say BlueZ returns 'org.bluez.Error.NotConnected'. That'll happen after the > !IsConnected() check above that returns "Device is not connected". Instead, I > believe the code will wind up here and return "Failed to disconnect device", > which isn't the right message. Right, I guess in the case where say Disconnect fails because the device became disconnected before the call to Disconnect reached BlueZ, we could just report success or errorNotConnected by doing the check in the error callback.
On 2014/12/20 01:53:18, armansito wrote: > On 2014/12/20 01:48:56, Jeffrey Yasskin wrote: > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right): > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > extensions/browser/api/bluetooth/bluetooth_private_api.cc:329: > > SetError(kDisconnectError); > > [Moving over to the API implementation.] > > > > On 2014/12/20 01:43:38, armansito wrote: > > > On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > > > > On 2014/12/20 01:24:28, armansito wrote: > > > > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > > > > This message has been really annoying when debugging uses of the > > bluetooth > > > > > API. > > > > > > Can you report anything more precise? > > > > > > > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets an > > > error > > > > > > description and then THROWS IT AWAY. That's really not an acceptable > way > > > to > > > > > > treat our developers. > > > > > > > > > > The only error that BlueZ returns is 'org.bluez.Error.NotConnected' (see > > > > > > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > > > > We're already returning errorNotConnected here at the API level by doing > > the > > > > > check ourselves. > > > > > > > > If this error case is impossible, you should probably DCHECK that it > doesn't > > > > happen instead of writing code to send an incorrect message to developers > if > > > it > > > > does happen. I suspect there's a race condition during the several > > > cross-process > > > > hops in which it does happen, though. > > > > > > No it's not impossible. I'm not sure I'm following you here. In what case > are > > we > > > returning an incorrect error message from this function? > > > > Say BlueZ returns 'org.bluez.Error.NotConnected'. That'll happen after the > > !IsConnected() check above that returns "Device is not connected". Instead, I > > believe the code will wind up here and return "Failed to disconnect device", > > which isn't the right message. > > Right, I guess in the case where say Disconnect fails because the device became > disconnected before the call to Disconnect reached BlueZ, we could just report > success or errorNotConnected by doing the check in the error callback. Yep, and in general in other cases, developers will appreciate being able to see why an operation failed without debugging Chrome itself, even if the reason is relatively obscure or varies by platform.
On 2014/12/20 02:03:29, Jeffrey Yasskin wrote: > On 2014/12/20 01:53:18, armansito wrote: > > On 2014/12/20 01:48:56, Jeffrey Yasskin wrote: > > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > > File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right): > > > > > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > > extensions/browser/api/bluetooth/bluetooth_private_api.cc:329: > > > SetError(kDisconnectError); > > > [Moving over to the API implementation.] > > > > > > On 2014/12/20 01:43:38, armansito wrote: > > > > On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > > > > > On 2014/12/20 01:24:28, armansito wrote: > > > > > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > > > > > This message has been really annoying when debugging uses of the > > > bluetooth > > > > > > API. > > > > > > > Can you report anything more precise? > > > > > > > > > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() gets > an > > > > error > > > > > > > description and then THROWS IT AWAY. That's really not an acceptable > > way > > > > to > > > > > > > treat our developers. > > > > > > > > > > > > The only error that BlueZ returns is 'org.bluez.Error.NotConnected' > (see > > > > > > > > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > > > > > We're already returning errorNotConnected here at the API level by > doing > > > the > > > > > > check ourselves. > > > > > > > > > > If this error case is impossible, you should probably DCHECK that it > > doesn't > > > > > happen instead of writing code to send an incorrect message to > developers > > if > > > > it > > > > > does happen. I suspect there's a race condition during the several > > > > cross-process > > > > > hops in which it does happen, though. > > > > > > > > No it's not impossible. I'm not sure I'm following you here. In what case > > are > > > we > > > > returning an incorrect error message from this function? > > > > > > Say BlueZ returns 'org.bluez.Error.NotConnected'. That'll happen after the > > > !IsConnected() check above that returns "Device is not connected". Instead, > I > > > believe the code will wind up here and return "Failed to disconnect device", > > > which isn't the right message. > > > > Right, I guess in the case where say Disconnect fails because the device > became > > disconnected before the call to Disconnect reached BlueZ, we could just report > > success or errorNotConnected by doing the check in the error callback. > > Yep, and in general in other cases, developers will appreciate being able to see > why an operation failed without debugging Chrome itself, even if the reason is > relatively obscure or varies by platform. That makes sense. We have been improving error reporting where we can, though there are cases like this one in device/bluetooth that has been around for some time. I guess the ideal thing to do here would be to have BluetoothDevice::Disconnect to report an error instead of doing checks here but I don't want that to block this private API CL from getting merged. In this case, would reporting "Not connected" as the error be preferred over returning success?
On 2014/12/20 02:12:31, armansito wrote: > On 2014/12/20 02:03:29, Jeffrey Yasskin wrote: > > On 2014/12/20 01:53:18, armansito wrote: > > > On 2014/12/20 01:48:56, Jeffrey Yasskin wrote: > > > > > > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > > > File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > > > extensions/browser/api/bluetooth/bluetooth_private_api.cc:329: > > > > SetError(kDisconnectError); > > > > [Moving over to the API implementation.] > > > > > > > > On 2014/12/20 01:43:38, armansito wrote: > > > > > On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > > > > > > On 2014/12/20 01:24:28, armansito wrote: > > > > > > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > > > > > > This message has been really annoying when debugging uses of the > > > > bluetooth > > > > > > > API. > > > > > > > > Can you report anything more precise? > > > > > > > > > > > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() > gets > > an > > > > > error > > > > > > > > description and then THROWS IT AWAY. That's really not an > acceptable > > > way > > > > > to > > > > > > > > treat our developers. > > > > > > > > > > > > > > The only error that BlueZ returns is 'org.bluez.Error.NotConnected' > > (see > > > > > > > > > > > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > > > > > > We're already returning errorNotConnected here at the API level by > > doing > > > > the > > > > > > > check ourselves. > > > > > > > > > > > > If this error case is impossible, you should probably DCHECK that it > > > doesn't > > > > > > happen instead of writing code to send an incorrect message to > > developers > > > if > > > > > it > > > > > > does happen. I suspect there's a race condition during the several > > > > > cross-process > > > > > > hops in which it does happen, though. > > > > > > > > > > No it's not impossible. I'm not sure I'm following you here. In what > case > > > are > > > > we > > > > > returning an incorrect error message from this function? > > > > > > > > Say BlueZ returns 'org.bluez.Error.NotConnected'. That'll happen after the > > > > !IsConnected() check above that returns "Device is not connected". > Instead, > > I > > > > believe the code will wind up here and return "Failed to disconnect > device", > > > > which isn't the right message. > > > > > > Right, I guess in the case where say Disconnect fails because the device > > became > > > disconnected before the call to Disconnect reached BlueZ, we could just > report > > > success or errorNotConnected by doing the check in the error callback. > > > > Yep, and in general in other cases, developers will appreciate being able to > see > > why an operation failed without debugging Chrome itself, even if the reason is > > relatively obscure or varies by platform. > > That makes sense. We have been improving error reporting where we can, though > there are cases like this one in device/bluetooth that has been around for some > time. I guess the ideal thing to do here would be to have > BluetoothDevice::Disconnect to report an error instead of doing checks here but > I don't want that to block this private API CL from getting merged. > > In this case, would reporting "Not connected" as the error be preferred over > returning success? I'm happy to let you refactor BluetoothDevice::Disconnect in a separate CL. For this one, on the one hand, I can imagine returning success for "not connected" regardless, since the device does end up non-connected. On the other, as long as the API sometimes returns not-connected, it should return the same thing regardless of when the system discovers that. It seems like returning success will be easier and somewhat more correct in this CL, so maybe do that?
On 2014/12/20 02:50:51, Jeffrey Yasskin wrote: > On 2014/12/20 02:12:31, armansito wrote: > > On 2014/12/20 02:03:29, Jeffrey Yasskin wrote: > > > On 2014/12/20 01:53:18, armansito wrote: > > > > On 2014/12/20 01:48:56, Jeffrey Yasskin wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > > > > File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > > > > extensions/browser/api/bluetooth/bluetooth_private_api.cc:329: > > > > > SetError(kDisconnectError); > > > > > [Moving over to the API implementation.] > > > > > > > > > > On 2014/12/20 01:43:38, armansito wrote: > > > > > > On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > > > > > > > On 2014/12/20 01:24:28, armansito wrote: > > > > > > > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > > > > > > > This message has been really annoying when debugging uses of the > > > > > bluetooth > > > > > > > > API. > > > > > > > > > Can you report anything more precise? > > > > > > > > > > > > > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() > > gets > > > an > > > > > > error > > > > > > > > > description and then THROWS IT AWAY. That's really not an > > acceptable > > > > way > > > > > > to > > > > > > > > > treat our developers. > > > > > > > > > > > > > > > > The only error that BlueZ returns is > 'org.bluez.Error.NotConnected' > > > (see > > > > > > > > > > > > > > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > > > > > > > We're already returning errorNotConnected here at the API level by > > > doing > > > > > the > > > > > > > > check ourselves. > > > > > > > > > > > > > > If this error case is impossible, you should probably DCHECK that it > > > > doesn't > > > > > > > happen instead of writing code to send an incorrect message to > > > developers > > > > if > > > > > > it > > > > > > > does happen. I suspect there's a race condition during the several > > > > > > cross-process > > > > > > > hops in which it does happen, though. > > > > > > > > > > > > No it's not impossible. I'm not sure I'm following you here. In what > > case > > > > are > > > > > we > > > > > > returning an incorrect error message from this function? > > > > > > > > > > Say BlueZ returns 'org.bluez.Error.NotConnected'. That'll happen after > the > > > > > !IsConnected() check above that returns "Device is not connected". > > Instead, > > > I > > > > > believe the code will wind up here and return "Failed to disconnect > > device", > > > > > which isn't the right message. > > > > > > > > Right, I guess in the case where say Disconnect fails because the device > > > became > > > > disconnected before the call to Disconnect reached BlueZ, we could just > > report > > > > success or errorNotConnected by doing the check in the error callback. > > > > > > Yep, and in general in other cases, developers will appreciate being able to > > see > > > why an operation failed without debugging Chrome itself, even if the reason > is > > > relatively obscure or varies by platform. > > > > That makes sense. We have been improving error reporting where we can, though > > there are cases like this one in device/bluetooth that has been around for > some > > time. I guess the ideal thing to do here would be to have > > BluetoothDevice::Disconnect to report an error instead of doing checks here > but > > I don't want that to block this private API CL from getting merged. > > > > In this case, would reporting "Not connected" as the error be preferred over > > returning success? > > I'm happy to let you refactor BluetoothDevice::Disconnect in a separate CL. For > this one, on the one hand, I can imagine returning success for "not connected" > regardless, since the device does end up non-connected. On the other, as long as > the API sometimes returns not-connected, it should return the same thing > regardless of when the system discovers that. It seems like returning success > will be easier and somewhat more correct in this CL, so maybe do that? Actually, probably returning "Not connected" is better especially if we refactor BluetoothDevice::Disconnect to return errors in the future and simply return the result from that. At least the current behavior will be consistent with the future behavior in that case.
On 2014/12/20 02:58:10, armansito wrote: > On 2014/12/20 02:50:51, Jeffrey Yasskin wrote: > > On 2014/12/20 02:12:31, armansito wrote: > > > On 2014/12/20 02:03:29, Jeffrey Yasskin wrote: > > > > On 2014/12/20 01:53:18, armansito wrote: > > > > > On 2014/12/20 01:48:56, Jeffrey Yasskin wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > > > > > File extensions/browser/api/bluetooth/bluetooth_private_api.cc > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/819713002/diff/20001/extensions/browser/api/b... > > > > > > extensions/browser/api/bluetooth/bluetooth_private_api.cc:329: > > > > > > SetError(kDisconnectError); > > > > > > [Moving over to the API implementation.] > > > > > > > > > > > > On 2014/12/20 01:43:38, armansito wrote: > > > > > > > On 2014/12/20 01:29:58, Jeffrey Yasskin wrote: > > > > > > > > On 2014/12/20 01:24:28, armansito wrote: > > > > > > > > > On 2014/12/20 00:53:48, Jeffrey Yasskin wrote: > > > > > > > > > > This message has been really annoying when debugging uses of > the > > > > > > bluetooth > > > > > > > > > API. > > > > > > > > > > Can you report anything more precise? > > > > > > > > > > > > > > > > > > > > And yes, you can. BluetoothDeviceChromeOS::OnDisconnectError() > > > gets > > > > an > > > > > > > error > > > > > > > > > > description and then THROWS IT AWAY. That's really not an > > > acceptable > > > > > way > > > > > > > to > > > > > > > > > > treat our developers. > > > > > > > > > > > > > > > > > > The only error that BlueZ returns is > > 'org.bluez.Error.NotConnected' > > > > (see > > > > > > > > > > > > > > > > > > http://git.kernel.org/cgit/bluetooth/bluez.git/tree/doc/device-api.txt#n29). > > > > > > > > > We're already returning errorNotConnected here at the API level > by > > > > doing > > > > > > the > > > > > > > > > check ourselves. > > > > > > > > > > > > > > > > If this error case is impossible, you should probably DCHECK that > it > > > > > doesn't > > > > > > > > happen instead of writing code to send an incorrect message to > > > > developers > > > > > if > > > > > > > it > > > > > > > > does happen. I suspect there's a race condition during the several > > > > > > > cross-process > > > > > > > > hops in which it does happen, though. > > > > > > > > > > > > > > No it's not impossible. I'm not sure I'm following you here. In what > > > case > > > > > are > > > > > > we > > > > > > > returning an incorrect error message from this function? > > > > > > > > > > > > Say BlueZ returns 'org.bluez.Error.NotConnected'. That'll happen after > > the > > > > > > !IsConnected() check above that returns "Device is not connected". > > > Instead, > > > > I > > > > > > believe the code will wind up here and return "Failed to disconnect > > > device", > > > > > > which isn't the right message. > > > > > > > > > > Right, I guess in the case where say Disconnect fails because the device > > > > became > > > > > disconnected before the call to Disconnect reached BlueZ, we could just > > > report > > > > > success or errorNotConnected by doing the check in the error callback. > > > > > > > > Yep, and in general in other cases, developers will appreciate being able > to > > > see > > > > why an operation failed without debugging Chrome itself, even if the > reason > > is > > > > relatively obscure or varies by platform. > > > > > > That makes sense. We have been improving error reporting where we can, > though > > > there are cases like this one in device/bluetooth that has been around for > > some > > > time. I guess the ideal thing to do here would be to have > > > BluetoothDevice::Disconnect to report an error instead of doing checks here > > but > > > I don't want that to block this private API CL from getting merged. > > > > > > In this case, would reporting "Not connected" as the error be preferred over > > > returning success? > > > > I'm happy to let you refactor BluetoothDevice::Disconnect in a separate CL. > For > > this one, on the one hand, I can imagine returning success for "not connected" > > regardless, since the device does end up non-connected. On the other, as long > as > > the API sometimes returns not-connected, it should return the same thing > > regardless of when the system discovers that. It seems like returning success > > will be easier and somewhat more correct in this CL, so maybe do that? > > Actually, probably returning "Not connected" is better especially if we refactor > BluetoothDevice::Disconnect to return errors in the future and simply return the > result from that. At least the current behavior will be consistent with the > future behavior in that case. ping
lgtm, sorry for the delay. https://codereview.chromium.org/819713002/diff/40001/extensions/browser/api/b... File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right): https://codereview.chromium.org/819713002/diff/40001/extensions/browser/api/b... extensions/browser/api/bluetooth/bluetooth_private_api.cc:334: // Despite the failure, the device may have become unexpectedly disconnected "Despite the failure" isn't clear here. Do you mean that "This disconnection might have failed because the device was disconnected by some other process while this disconnection was in flight. In this case, ..."? https://codereview.chromium.org/819713002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/819713002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:46121: + <int value="767" label="DELETED_SYNCEDNOTIFICATIONSPRIVATE_GETINITIALDATA"/> It looks like you have some extraneous changes to this file, maybe because you need to merge?
https://codereview.chromium.org/819713002/diff/40001/extensions/browser/api/b... File extensions/browser/api/bluetooth/bluetooth_private_api.cc (right): https://codereview.chromium.org/819713002/diff/40001/extensions/browser/api/b... extensions/browser/api/bluetooth/bluetooth_private_api.cc:334: // Despite the failure, the device may have become unexpectedly disconnected On 2015/01/06 19:22:32, Jeffrey Yasskin wrote: > "Despite the failure" isn't clear here. Do you mean that "This disconnection > might have failed because the device was disconnected by some other process > while this disconnection was in flight. In this case, ..."? I clarified the message, thanks for your comment on that. https://codereview.chromium.org/819713002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/819713002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:46121: + <int value="767" label="DELETED_SYNCEDNOTIFICATIONSPRIVATE_GETINITIALDATA"/> On 2015/01/06 19:22:32, Jeffrey Yasskin wrote: > It looks like you have some extraneous changes to this file, maybe because you > need to merge? No, these were added automatically when I ran tools/metrics/histrograms/update_extension_functions.py
lgtm https://codereview.chromium.org/819713002/diff/40001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/819713002/diff/40001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:46121: + <int value="767" label="DELETED_SYNCEDNOTIFICATIONSPRIVATE_GETINITIALDATA"/> On 2015/01/06 20:10:11, armansito wrote: > On 2015/01/06 19:22:32, Jeffrey Yasskin wrote: > > It looks like you have some extraneous changes to this file, maybe because you > > need to merge? > > No, these were added automatically when I ran > tools/metrics/histrograms/update_extension_functions.py Acknowledged.
The CQ bit was checked by armansito@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/819713002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/373214a5345dd65ca441dcbcca4aa9eecf276a82 Cr-Commit-Position: refs/heads/master@{#310154} |
