Bluetooth: Add a "connectable" property to the BluetoothDevice.

Bluetooth: Add a "connectable" property to the BluetoothDevice.

This fix adds a new "connectable" property to the BluetoothDevice telling
whether the device normally accepts connections initiated from the adapter
when the device is paired but disconnected. Mice and keyboards are usually
not connectable under this definition, while any other device is usually
connectable.

BUG=chromium-os:38410
TEST=Run in Debug mode and watch the "connectable" property in the chrome logs. A bluetooth mouse show connectable=0.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184287

[deymo, 2013-02-21 20:21:43]

This is the first part of a series of three patches:
1. add the service providers cache and HID options parsing.
2. make the service providers cache persistent.
3. Use this information in the UI to fix the bug.

dbeam@ for OWNER review of chrome/browser/ui/webui/options/chromeos/
keybuk/bryeung for the rest.
Thanks!

[keybuk, 2013-02-21 21:08:57]

TEST= lines are mandatory in commit messages

please add youngki to Cc, since this adds new API to BluetoothDevice and
BluetoothServiceRecord so will need a BluetoothDeviceWin and
BluetoothServiceRecordWin implementation

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device.h:333: bool connectable_;
nit: should go below the other bools, to match the Is*() function order

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:72: }
One of the reasons that GetServiceRecords() always calls DiscoverServices() is
that the service list can be dynamic (e.g. services only published on Android
while the app is actually running)

This would seem to break that by caching the values and never updating. Is there
a way to alter this so that it always attempts a refresh, and if that fails,
falls back to the cache instead of giving an error?

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:430: service_records_.clear();
This gives a time period where another thread would see an empty service records
list before it's repopulated?

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:451: base::Bind(error_callback,
ERROR_SDP_FAILED));
Could failure be handled better here? This adds another user error case where we
could perhaps have a "default behavior" and just log the error.

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:462: // Update the
BluetoothDevice::connectable_ property.
nit: a comment explaining the boolean logic would be nice for future
maintainers, e.g. "in the cases of multiple HID profiles, all must be normally
connectable for the property to be true"

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_se...
File device/bluetooth/bluetooth_service_record_chromeos.cc (right):

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_se...
device/bluetooth/bluetooth_service_record_chromeos.cc:34: static const char*
kValueTrue = "true";
nit: this list would be nicer if it were ordered differently, e.g. block of
"Node" constants; blank line; block of "Uuid" constants; blank line; block of
"Id" constants

[Dan Beam, 2013-02-21 22:41:55]

$ head -3 chrome/browser/ui/webui/options/chromeos/OWNERS 
dpolukhin@chromium.org
glotov@chromium.org
nkostylev@chromium.org

you may want to ask one of them first (prefer more local owners). also, from my
experience, kevers@ is the go-to guy for bluetooth on cros.

[Dan Beam, 2013-02-21 22:42:40]

https://codereview.chromium.org/12310048/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://codereview.chromium.org/12310048/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:366:
error_name = "bluetoothConnectSdpFailed";
what does SDP mean?

https://codereview.chromium.org/12310048/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:418:
js_properties.SetBoolean("connectable", device->IsConnectable());
also, I have no idea what "connectable" means.

[deymo, 2013-02-22 23:09:09]

Hi!
Second patch uploaded, also I updated the description to add the TEST= (although
it is manual).
AFAIK, youngki was in CC in the first message.

I removed from the patch the interaction with the UI (will do that part in a
different CL). Per my discussion with keybuk, the service_records_ problems are
ok.

https://codereview.chromium.org/12310048/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc
(right):

https://codereview.chromium.org/12310048/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:366:
error_name = "bluetoothConnectSdpFailed";
On 2013/02/21 22:42:40, Dan Beam wrote:
> what does SDP mean?
SDP = Service Discovery Protocol
BTW, I'll remove this error message in the UI level.

https://codereview.chromium.org/12310048/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/chromeos/bluetooth_options_handler.cc:418:
js_properties.SetBoolean("connectable", device->IsConnectable());
On 2013/02/21 22:42:40, Dan Beam wrote:
> also, I have no idea what "connectable" means.
I chose the term "Connectable" from the "HIDNormallyConnectable" property name
from the Bluetooth HID spec. This property says if you can connect from the
adapter to the device when the device is not connected. This property is usually
false for mice and keyboards because they turn the radio off when on idle. After
all, those are only input devices, and if they have some events to send, they
will start the connection from the device to the adapter and send whatever they
need.
This property is important to the UI to (later) hide/disable the "Connect"
button for this kind of devices.

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
File device/bluetooth/bluetooth_device.h (right):

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device.h:333: bool connectable_;
On 2013/02/21 21:08:58, keybuk wrote:
> nit: should go below the other bools, to match the Is*() function order

Done.

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:72: }
On 2013/02/21 21:08:58, keybuk wrote:
> One of the reasons that GetServiceRecords() always calls DiscoverServices() is
> that the service list can be dynamic (e.g. services only published on Android
> while the app is actually running)
> 
> This would seem to break that by caching the values and never updating. Is
there
> a way to alter this so that it always attempts a refresh, and if that fails,
> falls back to the cache instead of giving an error?

Done.

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:430: service_records_.clear();
On 2013/02/21 21:08:58, keybuk wrote:
> This gives a time period where another thread would see an empty service
records
> list before it's repopulated?

As discussed, this is ok.

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:451: base::Bind(error_callback,
ERROR_SDP_FAILED));
On 2013/02/21 21:08:58, keybuk wrote:
> Could failure be handled better here? This adds another user error case where
we
> could perhaps have a "default behavior" and just log the error.
Not having the SDP information is not a "fatal" connect error. Changed this to
call callback anyway.

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:462: // Update the
BluetoothDevice::connectable_ property.
On 2013/02/21 21:08:58, keybuk wrote:
> nit: a comment explaining the boolean logic would be nice for future
> maintainers, e.g. "in the cases of multiple HID profiles, all must be normally
> connectable for the property to be true"
We have a simplification of the device here. We have only one device object with
its connected and connectable status, but many profiles (and we just try to
connect to all of them). I added a note about this.

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_se...
File device/bluetooth/bluetooth_service_record_chromeos.cc (right):

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_se...
device/bluetooth/bluetooth_service_record_chromeos.cc:34: static const char*
kValueTrue = "true";
On 2013/02/21 21:08:58, keybuk wrote:
> nit: this list would be nicer if it were ordered differently, e.g. block of
> "Node" constants; blank line; block of "Uuid" constants; blank line; block of
> "Id" constants

Done.

[deymo, 2013-02-22 23:39:58]

Third patch uploaded. This is just a bugfix about the callback or error_callback
being called twice:
* once upon success/failure of trusted.Set(true)
* once upon success/failure of ConnectApplications.

The result trusted.Set(true) is now ignored (we just log that in case it fails).
Happy review!

[keybuk, 2013-02-22 23:50:02]

lgtm

with a few nits to correct before committing

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12310048/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_chromeos.cc:430: service_records_.clear();
after discussion, this seems okay - since all BDCOs code is run on the same
thread

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.cc:475: }
nit: use LOG_IF(WARNING, !success) instead

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device_chromeos.h (right):

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:128: // Called by
CollectServiceRecordsCallback() every time the |service_records_|
nit: drop the "the", it's cleaner

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:129: // is updated.
|OnServiceRecordsChanged| updates the derivated properties that
nit: "derived" ... "derivated" is not a real word ;-)

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:138: // Called by
BluetoothAdapterClient when a call to DiscoverServices()
nit: GetServiceRecords()

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:140: // GetServiceRecords.
nit: needs a ()

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:396: // This says wether the
|service_records_| cache is initialized. Note that an
nit: whether

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_service_record.h (right):

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_service_record.h:32: // Indicates if this service
supports HIDP.
nit: HID not HIDP

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_service_record_chromeos.cc (right):

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_service_record_chromeos.cc:35: static const char*
kHidReconnectInitiate = "0x0205";
These should be named "Id" to match the others, no?

[deymo, 2013-02-23 00:43:16]

New patch with keybuk's nits.

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device_chromeos.cc (right):

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.cc:475: }
On 2013/02/22 23:50:03, keybuk wrote:
> nit: use LOG_IF(WARNING, !success) instead

Done.

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_device_chromeos.h (right):

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:128: // Called by
CollectServiceRecordsCallback() every time the |service_records_|
On 2013/02/22 23:50:03, keybuk wrote:
> nit: drop the "the", it's cleaner

Done.

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:129: // is updated.
|OnServiceRecordsChanged| updates the derivated properties that
On 2013/02/22 23:50:03, keybuk wrote:
> nit: "derived" ... "derivated" is not a real word ;-)

Done.

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:138: // Called by
BluetoothAdapterClient when a call to DiscoverServices()
On 2013/02/22 23:50:03, keybuk wrote:
> nit: GetServiceRecords()

Done.

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:140: // GetServiceRecords.
On 2013/02/22 23:50:03, keybuk wrote:
> nit: needs a ()

Done.

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_device_chromeos.h:396: // This says wether the
|service_records_| cache is initialized. Note that an
On 2013/02/22 23:50:03, keybuk wrote:
> nit: whether

Done.

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
File device/bluetooth/bluetooth_service_record.h (right):

https://codereview.chromium.org/12310048/diff/9001/device/bluetooth/bluetooth...
device/bluetooth/bluetooth_service_record.h:32: // Indicates if this service
supports HIDP.
On 2013/02/22 23:50:03, keybuk wrote:
> nit: HID not HIDP

Done.

[commit-bot: I haz the power, 2013-02-23 01:51:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/deymo@chromium.org/12310048/4010

[commit-bot: I haz the power, 2013-02-23 05:56:54]

Change committed as 184287