Bluetooth: Implement new socket API for Chrome OS

Bluetooth: Implement new socket API for Chrome OS

Remove the old BluetoothProfileChromeOS implementation and instead
extend BluetoothSocketChromeOS to handle making outgoing connections
and accepting incoming connections through the D-Bus API.

BUG=372493
TEST=device_unittests --gtest_filter=BluetoothSocketChromeOSTest.*

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

[keybuk, 2014-05-12 23:18:44]

armansito: for /bluetooth/
rpaquay: general expert review
others: FYI & extra eyes

[armansito, 2014-05-13 19:44:59]

https://codereview.chromium.org/276573004/diff/1/chromeos/dbus/bluetooth_prof...
File chromeos/dbus/bluetooth_profile_manager_client.cc (right):

https://codereview.chromium.org/276573004/diff/1/chromeos/dbus/bluetooth_prof...
chromeos/dbus/bluetooth_profile_manager_client.cc:56: if (options.name.get() !=
NULL) {
Does it make sense to check for empty string here or is that ok?

https://codereview.chromium.org/276573004/diff/1/chromeos/dbus/bluetooth_prof...
chromeos/dbus/bluetooth_profile_manager_client.cc:64: if (options.service.get()
!= NULL) {
Check for empty string?

https://codereview.chromium.org/276573004/diff/1/chromeos/dbus/bluetooth_prof...
chromeos/dbus/bluetooth_profile_manager_client.cc:134: if
(options.service_record.get() != NULL) {
Check for empty string?

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

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_chromeos.cc:441: error_callback.Run("Removed.
Use chrome.bluetoothSocket.connect() instead.");
Does it make sense to refer to the JS API in an error message from the C++ API?
Should we say "Use BluetoothDevice::ConnectToService instead"?

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_chromeos.cc (right):

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_chromeos.cc:129: DCHECK (!profile_.get());
nit: no space after DCHECK

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_chromeos.cc:306: UnregisterProfile();
Is the call to UnregisterProfile here intended?

[keybuk, 2014-05-13 20:03:53]

https://codereview.chromium.org/276573004/diff/1/chromeos/dbus/bluetooth_prof...
File chromeos/dbus/bluetooth_profile_manager_client.cc (right):

https://codereview.chromium.org/276573004/diff/1/chromeos/dbus/bluetooth_prof...
chromeos/dbus/bluetooth_profile_manager_client.cc:56: if (options.name.get() !=
NULL) {
On 2014/05/13 19:44:59, armansito wrote:
> Does it make sense to check for empty string here or is that ok?

If the caller explicitly sets empty string, then we should pass that forward to
BlueZ

https://codereview.chromium.org/276573004/diff/1/chromeos/dbus/bluetooth_prof...
chromeos/dbus/bluetooth_profile_manager_client.cc:64: if (options.service.get()
!= NULL) {
On 2014/05/13 19:44:59, armansito wrote:
> Check for empty string?

As above, while this is usually a UUID, BlueZ might interpret "" differently to
"property not in dict"

https://codereview.chromium.org/276573004/diff/1/chromeos/dbus/bluetooth_prof...
chromeos/dbus/bluetooth_profile_manager_client.cc:134: if
(options.service_record.get() != NULL) {
On 2014/05/13 19:44:59, armansito wrote:
> Check for empty string?

"" is a valid service record, while "property not present" means BlueZ will
create a service record

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

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_d...
device/bluetooth/bluetooth_device_chromeos.cc:441: error_callback.Run("Removed.
Use chrome.bluetoothSocket.connect() instead.");
On 2014/05/13 19:44:59, armansito wrote:
> Does it make sense to refer to the JS API in an error message from the C++
API?
> Should we say "Use BluetoothDevice::ConnectToService instead"?

Since the only thing using this method is the JS API, and this is a short term
error that's going away RSN, I think it's fine for the C++ API to just refer to
the JS API

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_chromeos.cc (right):

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_chromeos.cc:306: UnregisterProfile();
On 2014/05/13 19:44:59, armansito wrote:
> Is the call to UnregisterProfile here intended?

Yes, the connection has been accepted, there is no need for the profile to
remain registered

[armansito, 2014-05-13 20:06:36]

lgtm

[keybuk, 2014-05-13 20:14:52]

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_s...
File device/bluetooth/bluetooth_socket_chromeos.cc (right):

https://codereview.chromium.org/276573004/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_chromeos.cc:129: DCHECK (!profile_.get());
On 2014/05/13 19:44:59, armansito wrote:
> nit: no space after DCHECK

Done.

[rpaquay, 2014-05-13 20:17:45]

lgtm

[keybuk, 2014-05-13 20:18:22]

The CQ bit was checked by keybuk@chromium.org

[commit-bot: I haz the power, 2014-05-13 20:19:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/keybuk@chromium.org/276573004/20001

[commit-bot: I haz the power, 2014-05-13 23:33:42]

Change committed as 270245