Manage profiles in BluetoothAdapter on ChromeOS

Manage profiles in BluetoothAdapter on ChromeOS

Profiles in bluez are not specific to a device.
When client code tries to connect to two different devices, only
one profile is created, managed by BluetoothAdapterChromeOS.

dbus messages are multiplexed based on the device desired using new
class BluetoothAdapterProfileChromeOS.

BUG=chromium:421207
Committed: https://crrev.com/c56186ac2901c99349cbb4b4a6b8aa0ff8088c4d
Cr-Commit-Position: refs/heads/master@{#313837}

[Marie Janssen, 2015-01-15 17:23:32]

jamuraa@chromium.org changed reviewers:
+ armansito@chromium.org, keybuk@chromium.org

[armansito, 2015-01-21 01:44:48]

Initial pass, sorry for the delay, this somehow remained off my radar.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:887: const
ErrorCompletionCallback& error_callback) {
Add a DCHECK for delegate.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:894: } else {
No need for else since you have an early return already.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:902: device_path, delegate,
success_callback, error_callback),
I don't think it's safe to pass a raw pointer to |delegate| to the callback,
since it might be freed before the callback executes. I guess we're leaving
ownership of delegates to the upper-layer here, so I don't know what the best
solution is.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:935: } else {
No need for else, since you have an early return already.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:945: const std::string&
error_message) {
Since registering the profile failed, should it be cleaned up?

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:122: // Returns the associated
profile object.  When the profile is successfully
I would add a note here that the returned object is owned by the
BluetoothAdapterChromeOS.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:125: // called.
nit: "// will be called." instead of having "// called." in its own line.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:126:
BluetoothAdapterProfileChromeOS* UseProfile(
Well we are returning the profile here but technically registering it may fail.
So maybe it's better to return a pointer to the profile in the success callback
instead of the return value.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:128: const dbus::ObjectPath&
device_path,
Will local (listening) services also use this function? In that case, are they
expected to pass in an empty string for |device_path|?

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:129: const
BluetoothProfileManagerClient::Options* options,
Should |options| be passed by const reference instead of a pointer?

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:31: VLOG(1) <<
profile->object_path().value() << ": Registering profile.";
nit: I'd turn this log line around: "Registering profile: " + path.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:60:
BluetoothProfileServiceProvider::Delegate* delegate) {
DCHECK(delegate);

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.h (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:22: // profile for custom
bluetooth services managed by a BluetoothAdapter.
nit: s/bluetooth/Bluetooth/

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:34: const
BluetoothProfileManagerClient::Options* options,
I'd pass |options| as a const reference rather than a raw pointer.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:56: //
BluetoothProfileServiceProvider::Delegate:
I'd define these as private methods.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:66: virtual void Cancel()
override;
I'd define these as private methods.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:66: virtual void Cancel()
override;
nit: Remove "virtual" when using "override".

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:83: // Registered dbus
object path for this profile.
nit: s/dbus/D-Bus/

Or just be generally consistent (e.g. "D-Bus" is used below)

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:88: virtual void
Released() override {
nit: Don't add "virtual" if you have "override", here and below.

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

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_chromeos.cc:270: }
I'd turn this if statement around with an early return:

if (device_path_.value().empty()) {
  VLOG(1) << "foo";
  success_callback.Run();
  return;
}

VLOG(1) << "bla bla";
DBusThreadManager::Get()->...

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

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_chromeos.h:168: // The profile registered with
the adatper for this socket.
nit: s/adatper/adapter/

[Marie Janssen, 2015-01-22 21:55:33]

Slightly reworked the lifetime of the profile to place the lifetime clearly
under control of BluetoothAdapterChromeOS with callbacks.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_chromeos.cc (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:887: const
ErrorCompletionCallback& error_callback) {
On 2015/01/21 01:44:47, armansito wrote:
> Add a DCHECK for delegate.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:894: } else {
On 2015/01/21 01:44:47, armansito wrote:
> No need for else since you have an early return already.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:902: device_path, delegate,
success_callback, error_callback),
On 2015/01/21 01:44:47, armansito wrote:
> I don't think it's safe to pass a raw pointer to |delegate| to the callback,
> since it might be freed before the callback executes. I guess we're leaving
> ownership of delegates to the upper-layer here, so I don't know what the best
> solution is.

This is consistent with the passing of |delegate| to the previously used
callback in the BluetoothSocketChromeOS implementation - it's just as safe as it
was before.  I can switch it over to a weak_ptr with a little effort if that is
preferred.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:935: } else {
On 2015/01/21 01:44:47, armansito wrote:
> No need for else, since you have an early return already.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.cc:945: const std::string&
error_message) {
On 2015/01/21 01:44:47, armansito wrote:
> Since registering the profile failed, should it be cleaned up?

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_chromeos.h (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:122: // Returns the associated
profile object.  When the profile is successfully
On 2015/01/21 01:44:47, armansito wrote:
> I would add a note here that the returned object is owned by the
> BluetoothAdapterChromeOS.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:125: // called.
On 2015/01/21 01:44:47, armansito wrote:
> nit: "// will be called." instead of having "// called." in its own line.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:126:
BluetoothAdapterProfileChromeOS* UseProfile(
On 2015/01/21 01:44:47, armansito wrote:
> Well we are returning the profile here but technically registering it may
fail.
> So maybe it's better to return a pointer to the profile in the success
callback
> instead of the return value.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:128: const dbus::ObjectPath&
device_path,
On 2015/01/21 01:44:47, armansito wrote:
> Will local (listening) services also use this function? In that case, are they
> expected to pass in an empty string for |device_path|?

Yes. Empty string for |device_path| is a listening socket. Added a clarifying
comment.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_chromeos.h:129: const
BluetoothProfileManagerClient::Options* options,
On 2015/01/21 01:44:47, armansito wrote:
> Should |options| be passed by const reference instead of a pointer?

Using a pointer is because of consistency with previous use in
BluetoothSocketChromeOS.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:31: VLOG(1) <<
profile->object_path().value() << ": Registering profile.";
On 2015/01/21 01:44:47, armansito wrote:
> nit: I'd turn this log line around: "Registering profile: " + path.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:60:
BluetoothProfileServiceProvider::Delegate* delegate) {
On 2015/01/21 01:44:47, armansito wrote:
> DCHECK(delegate);

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.h (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:22: // profile for custom
bluetooth services managed by a BluetoothAdapter.
On 2015/01/21 01:44:47, armansito wrote:
> nit: s/bluetooth/Bluetooth/

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:34: const
BluetoothProfileManagerClient::Options* options,
On 2015/01/21 01:44:47, armansito wrote:
> I'd pass |options| as a const reference rather than a raw pointer.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:56: //
BluetoothProfileServiceProvider::Delegate:
On 2015/01/21 01:44:47, armansito wrote:
> I'd define these as private methods.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:66: virtual void Cancel()
override;
On 2015/01/21 01:44:47, armansito wrote:
> I'd define these as private methods.

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:66: virtual void Cancel()
override;
On 2015/01/21 01:44:47, armansito wrote:
> nit: Remove "virtual" when using "override".

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.h:83: // Registered dbus
object path for this profile.
On 2015/01/21 01:44:47, armansito wrote:
> nit: s/dbus/D-Bus/
> 
> Or just be generally consistent (e.g. "D-Bus" is used below)

Done.

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right):

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:88: virtual void
Released() override {
On 2015/01/21 01:44:47, armansito wrote:
> nit: Don't add "virtual" if you have "override", here and below.

Done.

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

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_chromeos.cc:270: }
On 2015/01/21 01:44:48, armansito wrote:
> I'd turn this if statement around with an early return:
> 
> if (device_path_.value().empty()) {
>   VLOG(1) << "foo";
>   success_callback.Run();
>   return;
> }
> 
> VLOG(1) << "bla bla";
> DBusThreadManager::Get()->...

Done.

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

https://codereview.chromium.org/851123002/diff/1/device/bluetooth/bluetooth_s...
device/bluetooth/bluetooth_socket_chromeos.h:168: // The profile registered with
the adatper for this socket.
On 2015/01/21 01:44:48, armansito wrote:
> nit: s/adatper/adapter/

Done.

[Marie Janssen, 2015-01-28 17:33:08]

Ping,  PTAL.

[armansito, 2015-01-29 04:44:18]

lgtm with one nit

https://codereview.chromium.org/851123002/diff/20001/device/bluetooth/bluetoo...
File device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc (right):

https://codereview.chromium.org/851123002/diff/20001/device/bluetooth/bluetoo...
device/bluetooth/bluetooth_adapter_profile_chromeos_unittest.cc:39:
profile_(NULL),
nit: Use nullptr instead NULL whenever you can.

[Marie Janssen, 2015-01-29 18:30:07]

The CQ bit was checked by jamuraa@chromium.org

[commit-bot: I haz the power, 2015-01-29 18:30:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/851123002/40001

[commit-bot: I haz the power, 2015-01-29 18:37:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-29 18:37:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Marie Janssen, 2015-01-29 21:21:19]

On 2015/01/29 18:37:29, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Needs a LGTM from keybuk for device/device_tests.gyp

[Marie Janssen, 2015-01-29 23:01:00]

jamuraa@chromium.org changed reviewers:
+ rpaquay@chromium.org

[Marie Janssen, 2015-01-29 23:01:01]

Renaud: 

keybuk is out, can you review this CL as owner of device/device_tests.gyp

[Marie Janssen, 2015-01-29 23:34:17]

The CQ bit was checked by jamuraa@chromium.org

[commit-bot: I haz the power, 2015-01-29 23:34:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/851123002/80001

[commit-bot: I haz the power, 2015-01-30 00:32:40]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-30 00:34:14]

Patchset 5 (id:??) landed as
https://crrev.com/c56186ac2901c99349cbb4b4a6b8aa0ff8088c4d
Cr-Commit-Position: refs/heads/master@{#313837}

[perkj_chrome, 2015-01-30 08:32:35]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/868753006/ by perkj@chromium.org.

The reason for reverting is: This causes a leak on Chrome Os ASan. 

https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%2....