Bluetooth: Profile support for Chrome OS

Bluetooth: Profile support for Chrome OS

Implement support for the BluetoothProfile API for the BlueZ 5.x
stack on Chrome OS, including BluetoothSocket support.

BUG=229636
TEST=device_unittests

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

[keybuk, 2013-04-24 23:30:18]

youngki: here's a first stab for the new Chrome OS stack

I fixed BluetoothProfile's constructor and destructor to be protected - you were
quite right in the review of that! My C++ was rusty

[youngki, 2013-04-25 14:34:34]

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.cc (right):

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.cc:56: #endif
We should run callback.Run(NULL) for #else.

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile_experimental_chromeos.cc (right):

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:110: void
BluetoothProfileExperimentalChromeOS::Release() {
Add TODO to implement this method.

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:136: void
BluetoothProfileExperimentalChromeOS::RequestDisconnection(
Add TODO to implement this method.

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:143: void
BluetoothProfileExperimentalChromeOS::Cancel() {
Add TODO to implement this method.

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile_experimental_chromeos.h (right):

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile_experimental_chromeos.h:15: #include
"dbus/file_descriptor.h"
We could remove this include and forward-declare FileDescriptor class below.

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_socket_experimental_chromeos.h (right):

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_socket_experimental_chromeos.h:13: #include
"dbus/file_descriptor.h"
I think FileDescriptor class can be forward-declared.

[keybuk, 2013-04-25 17:16:41]

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile.cc (right):

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile.cc:56: #endif
On 2013/04/25 14:34:34, youngki wrote:
> We should run callback.Run(NULL) for #else.

Done.

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile_experimental_chromeos.cc (right):

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:110: void
BluetoothProfileExperimentalChromeOS::Release() {
On 2013/04/25 14:34:34, youngki wrote:
> Add TODO to implement this method.

This IS the implementation ;) while we have to provide a method call for this
for BlueZ, we don't need to do anything special when the profile object is
released

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:136: void
BluetoothProfileExperimentalChromeOS::RequestDisconnection(
On 2013/04/25 14:34:34, youngki wrote:
> Add TODO to implement this method.

As above, this IS the implementation - we don't have anything in our JS API to
"request disconnection" because I didn't think it made any real sense for our
usage model

So returning a positive response to BlueZ seems all that is required here

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:143: void
BluetoothProfileExperimentalChromeOS::Cancel() {
On 2013/04/25 14:34:34, youngki wrote:
> Add TODO to implement this method.

Also this IS the implementation, the method is required for BlueZ but we don't
have to do anything here because it's just called before the error callback to
the ConnectToProfile() method and that's where we expect the error to be handled

(and yes, the implementation of ConnectToProfile() is still pending, along with
unit tests)

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_profile_experimental_chromeos.h (right):

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_profile_experimental_chromeos.h:15: #include
"dbus/file_descriptor.h"
On 2013/04/25 14:34:34, youngki wrote:
> We could remove this include and forward-declare FileDescriptor class below.

Done.

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
File device/bluetooth/bluetooth_socket_experimental_chromeos.h (right):

https://chromiumcodereview.appspot.com/14487002/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_socket_experimental_chromeos.h:13: #include
"dbus/file_descriptor.h"
On 2013/04/25 14:34:34, youngki wrote:
> I think FileDescriptor class can be forward-declared.

Done.

[youngki, 2013-04-26 03:51:18]

lgtm

[keybuk, 2013-04-30 18:08:46]

This latest patch set dragged in 14311007 - so ignore those pieces - that CL has
to be committed before this one

[satorux1, 2013-05-02 01:33:01]

https://codereview.chromium.org/14487002/diff/27001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_profile_experimental_chromeos.cc (right):

https://codereview.chromium.org/14487002/diff/27001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:158:
base::Owned(fd),
Instead of base::Owned(), you can probably make fd a scoped_ptr, and then do:

base::WorkerPool::GetTaskRunner()->PostTaskAndReplyWithResult(
  base::Bind(weakptr, CheckValidity, base::Passed(&fd)),
  base::Bind(weakptr, OnCheckValidity, ...));

scoped_ptr<dbus::FileDescriptor> CheckValidity(scoped_ptr<dbus::FileDescriptor>
fd) {
  fd->CheckValidity();
  return fd.Pass();
}

void OnCheckValidity(..., scoped_ptr<dbus::FileDescriptor> fd) {
   ...
}


This may be a bit more verbose, but the ownership is clearer.

https://codereview.chromium.org/14487002/diff/27001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:211: }
you might want to make this a free function in anonymous namespace. this doesn't
have to belong to this class.

[keybuk, 2013-05-02 20:17:14]

https://codereview.chromium.org/14487002/diff/27001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_profile_experimental_chromeos.cc (right):

https://codereview.chromium.org/14487002/diff/27001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:158:
base::Owned(fd),
On 2013/05/02 01:33:01, satorux1 wrote:
> Instead of base::Owned(), you can probably make fd a scoped_ptr, and then do:
> 

Awesome! This was exactly the kind of advice I was hoping you'd give - I didn't
know how to make scoped_ptr work for this kind of crazy ownership passing, now I
do!

Thanks!

https://codereview.chromium.org/14487002/diff/27001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:211: }
On 2013/05/02 01:33:01, satorux1 wrote:
> you might want to make this a free function in anonymous namespace. this
doesn't
> have to belong to this class.

Done.

[youngki, 2013-05-02 22:02:55]

lgtm

https://chromiumcodereview.appspot.com/14487002/diff/34001/device/bluetooth/b...
File device/bluetooth/bluetooth_profile_experimental_chromeos.cc (right):

https://chromiumcodereview.appspot.com/14487002/diff/34001/device/bluetooth/b...
device/bluetooth/bluetooth_profile_experimental_chromeos.cc:42: // Check the
validity of a file descriptor recieved from D-Bus. Must be run
s/recieved/received/

[satorux1, 2013-05-07 08:07:16]

LGTM with requests:

https://codereview.chromium.org/14487002/diff/42001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_profile_experimental_chromeos.h (right):

https://codereview.chromium.org/14487002/diff/42001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_profile_experimental_chromeos.h:33: class
CHROMEOS_EXPORT BluetoothProfileExperimentalChromeOS
class comment is missing. please add some.

https://codereview.chromium.org/14487002/diff/42001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_socket_experimental_chromeos.cc (right):

https://codereview.chromium.org/14487002/diff/42001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_socket_experimental_chromeos.cc:75: if
(HANDLE_EINTR(poll(&pollfd, 1, 0)) < 0) {
I need to check the manual to confirm that this won't block. Maybe add something
like

// Timeout parameter set to 0 so it won't block.

https://codereview.chromium.org/14487002/diff/42001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_socket_experimental_chromeos.h (right):

https://codereview.chromium.org/14487002/diff/42001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_socket_experimental_chromeos.h:30: class
CHROMEOS_EXPORT BluetoothSocketExperimentalChromeOS
class comment is missing. please add some.

[keybuk, 2013-05-07 23:19:39]

Have added some unit tests!

Please take one last look, this is now "ready" for submission once you both are
happy with it.

[youngki, 2013-05-08 20:17:46]

lgtm

[commit-bot: I haz the power, 2013-05-08 20:21:30]

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

[commit-bot: I haz the power, 2013-05-08 22:39:51]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-08 22:49:34]

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

[commit-bot: I haz the power, 2013-05-09 03:49:19]

Change committed as 199095