Bluetooth: implement BlueZ 5 backend for Chrome OS

Bluetooth: implement BlueZ 5 backend for Chrome OS

Provides an implementation for adapter control, device discovery,
pairing, connection, disconnection and unpairing using the BlueZ 5.x
backend on Chrome OS.

Uses Fake* classes for testing instead of Mocks.

TBR=youngki@chromium.org
BUG=220951
TEST=device_unittests

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

[keybuk, 2013-04-16 00:57:04]

satorux: for chromeos/ OWNERS
youngki: for device/bluetooth/ OWNERS
haruki: please see if you're happy with the Fake* usage per Issue #223061

[Haruki Sato, 2013-04-16 04:59:45]

Thank you for the helpful change!
I did only some sanity review and the fakes look good with nits.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
File chromeos/dbus/fake_bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_adapter_client.cc:25: const dbus::ObjectPath
FakeBluetoothAdapterClient::kAdapterPath(
could you avoid having ObjectPath and use const char[] instead?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_adapter_client.cc:32: const dbus::ObjectPath
FakeBluetoothAdapterClient::kSecondAdapterPath(
ditto.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
File chromeos/dbus/fake_bluetooth_device_client.h (right):

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:84: static const dbus::ObjectPath
kPairedDevicePath;
Use const char[] ?

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:89: static const dbus::ObjectPath
kAppleMousePath;
ditto.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:94: static const dbus::ObjectPath
kAppleKeyboardPath;
ditto.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:99: static const dbus::ObjectPath
kVanishingDevicePath;
ditto.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:104: static const dbus::ObjectPath
kMicrosoftMousePath;
ditto.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:109: static const dbus::ObjectPath
kMotorolaKeyboardPath;
ditto.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:114: static const dbus::ObjectPath
kSonyHeadphonesPath;
ditto.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:119: static const dbus::ObjectPath
kPhonePath;
ditto.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:124: static const dbus::ObjectPath
kWeirdDevicePath;
ditto.

[satorux1, 2013-04-16 06:30:45]

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
File chromeos/dbus/fake_bluetooth_profile_service_provider.h (right):

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_profile_service_provider.h:25: Delegate *delegate);
nit: indent

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
File device/bluetooth/bluetooth_adapter_experimental_chromeos.cc (right):

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:168: if
(object_path_.value().empty())
Did you mean:

if (!object_path_.value().empty())

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:353: {
nit: move { to the previous line

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:369: {
ditto

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:384: {
ditto

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

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:25: const
dbus::ObjectPath kAgentPath("/org/chromium/bluetooth_agent");
like haruki mentioned, please use const char

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:28: const char*
kAgentCapability =
Can this be defined at a compilation time? otherwise, a static initializer will
be created.

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

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.h:153: int
connecting_calls_;
maybe: num_connecting_calls_

[youngki, 2013-04-16 15:26:56]

will take a look at the test later.

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

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:91: if
(object_path_.value().empty())
Would it be more elegant if we reuse IsPresent() here?

if (!IsPresent())
  return false;

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:110: callback,
error_callback));
style nits: one argument per line.

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:114: if
(object_path_.value().empty())
Can we reuse IsPresent()?

if (!IsPresent())
  return false;

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:204: 
Can we put DCHECK here that devices_[device_chromeos->GetAddress()] should not
already exist?

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:247:
DeviceChanged(this, static_cast<BluetoothDevice*>(device_chromeos)));
I don't think this cast is necessary since BluetoothDevicexperimentalChromOS is
a subclass of BluetoothDevice.

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:279: if
(properties->powered.value() != false)
change it to if (properties->powered.value())

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:280:
PoweredChanged(properties->powered.value());
Also I think we should just make it PoweredChanged(true).

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:281: if
(properties->discovering.value() != false)
change it to if (properties->discovering.value())

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:282:
DiscoveringChanged(properties->discovering.value());
Change it to DiscoveringChanged(true) also.

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:302:
DCHECK(!object_path_.value().empty());
DCHECK(IsPresent());

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:311: if
(properties->powered.value() != false)
change it to if (properties->powered.value())

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:312:
PoweredChanged(false);
Are you sure this logic is right? This looks to me that:

if |properties| is powered:
  Tell observers that power is off;

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:313: if
(properties->discovering.value() != false)
change it to if (properties->discovering.value())

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

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:22: // The agent path
is relativel meaningless since BlueZ only supports one
s/relativel/relatively/

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:23: // at time and
will fail an attempt to register another with "Already Exists"
s/fail an attempt/fail in an attempt/

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:149: 
Remove the extra line. Or, we could early-return here and get rid of else {}.

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:172: callback,
error_callback),
one argument per line:
http://dev.chromium.org/developers/coding-style#Code_Formatting

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:225: if
(agent_.get()) {
The next 15 lines look very similar to RejectPairing(). Can we reuse the code
here? Maybe we could have RunPairingCallback(Status) helper method that contains
the next 15 lines and RejectPairing() can call RunPairingcallback(REJECTED) and
this method can call RunPairingCallback(CANCELLED)?

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:438:
--connecting_calls_;
Can we put DCHECK(connecting_calls >= 0)?

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:449:
--connecting_calls_;
Can we put DCHECK(connecting_calls >= 0)?

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:475: object_path_,
I think it still fits to put this into the above line too.

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:490:
--connecting_calls_;
Can we put DCHECK(connecting_calls >= 0)?

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:500: if (error_name
== bluetooth_adapter::kErrorAlreadyExists) {
Remove the braces since this is one-liner if block.

https://chromiumcodereview.appspot.com/13927010/diff/1/device/bluetooth/bluet...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:533:
--connecting_calls_;
Can we put DCHECK(connecting_calls >= 0)?

[keybuk, 2013-04-16 23:19:13]

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
File chromeos/dbus/fake_bluetooth_adapter_client.cc (right):

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_adapter_client.cc:25: const dbus::ObjectPath
FakeBluetoothAdapterClient::kAdapterPath(
On 2013/04/16 04:59:45, Haruki Sato wrote:
> could you avoid having ObjectPath and use const char[] instead?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static...

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_adapter_client.cc:32: const dbus::ObjectPath
FakeBluetoothAdapterClient::kSecondAdapterPath(
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
File chromeos/dbus/fake_bluetooth_device_client.h (right):

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:84: static const dbus::ObjectPath
kPairedDevicePath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> Use const char[] ?

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:89: static const dbus::ObjectPath
kAppleMousePath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:94: static const dbus::ObjectPath
kAppleKeyboardPath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:99: static const dbus::ObjectPath
kVanishingDevicePath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:104: static const dbus::ObjectPath
kMicrosoftMousePath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:109: static const dbus::ObjectPath
kMotorolaKeyboardPath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:114: static const dbus::ObjectPath
kSonyHeadphonesPath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:119: static const dbus::ObjectPath
kPhonePath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_device_client.h:124: static const dbus::ObjectPath
kWeirdDevicePath;
On 2013/04/16 04:59:45, Haruki Sato wrote:
> ditto.

Done.

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
File chromeos/dbus/fake_bluetooth_profile_service_provider.h (right):

https://codereview.chromium.org/13927010/diff/1/chromeos/dbus/fake_bluetooth_...
chromeos/dbus/fake_bluetooth_profile_service_provider.h:25: Delegate *delegate);
On 2013/04/16 06:30:45, satorux1 wrote:
> nit: indent

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
File device/bluetooth/bluetooth_adapter_experimental_chromeos.cc (right):

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:91: if
(object_path_.value().empty())
On 2013/04/16 15:26:56, youngki wrote:
> Would it be more elegant if we reuse IsPresent() here?
> 
> if (!IsPresent())
>   return false;

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:110: callback,
error_callback));
On 2013/04/16 15:26:56, youngki wrote:
> style nits: one argument per line.

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:114: if
(object_path_.value().empty())
On 2013/04/16 15:26:56, youngki wrote:
> Can we reuse IsPresent()?
> 
> if (!IsPresent())
>   return false;

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:168: if
(object_path_.value().empty())
On 2013/04/16 06:30:45, satorux1 wrote:
> Did you mean:
> 
> if (!object_path_.value().empty())

No, this only sets the adapter if it *isn't* already tracking one - will change
and add comment to clarify

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:204: 
On 2013/04/16 15:26:56, youngki wrote:
> Can we put DCHECK here that devices_[device_chromeos->GetAddress()] should not
> already exist?

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:247:
DeviceChanged(this, static_cast<BluetoothDevice*>(device_chromeos)));
On 2013/04/16 15:26:56, youngki wrote:
> I don't think this cast is necessary since BluetoothDevicexperimentalChromOS
is
> a subclass of BluetoothDevice.

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:279: if
(properties->powered.value() != false)
On 2013/04/16 15:26:56, youngki wrote:
> change it to if (properties->powered.value())

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:280:
PoweredChanged(properties->powered.value());
On 2013/04/16 15:26:56, youngki wrote:
> Also I think we should just make it PoweredChanged(true).

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:281: if
(properties->discovering.value() != false)
On 2013/04/16 15:26:56, youngki wrote:
> change it to if (properties->discovering.value())

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:282:
DiscoveringChanged(properties->discovering.value());
On 2013/04/16 15:26:56, youngki wrote:
> Change it to DiscoveringChanged(true) also.

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:302:
DCHECK(!object_path_.value().empty());
On 2013/04/16 15:26:56, youngki wrote:
> DCHECK(IsPresent());

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:311: if
(properties->powered.value() != false)
On 2013/04/16 15:26:56, youngki wrote:
> change it to if (properties->powered.value())

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:312:
PoweredChanged(false);
On 2013/04/16 15:26:56, youngki wrote:
> Are you sure this logic is right? This looks to me that:
> 
> if |properties| is powered:
>   Tell observers that power is off;

"if powered was on, the adapter is going buh-bye, so tell everyone the property
changed to off in case all they were tracking is AdapterPoweredChanged"

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:313: if
(properties->discovering.value() != false)
On 2013/04/16 15:26:56, youngki wrote:
> change it to if (properties->discovering.value())

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:353: {
On 2013/04/16 06:30:45, satorux1 wrote:
> nit: move { to the previous line

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:369: {
On 2013/04/16 06:30:45, satorux1 wrote:
> ditto

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_ad...
device/bluetooth/bluetooth_adapter_experimental_chromeos.cc:384: {
On 2013/04/16 06:30:45, satorux1 wrote:
> ditto

Done.

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

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:22: // The agent path
is relativel meaningless since BlueZ only supports one
On 2013/04/16 15:26:56, youngki wrote:
> s/relativel/relatively/

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:23: // at time and
will fail an attempt to register another with "Already Exists"
On 2013/04/16 15:26:56, youngki wrote:
> s/fail an attempt/fail in an attempt/

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:25: const
dbus::ObjectPath kAgentPath("/org/chromium/bluetooth_agent");
On 2013/04/16 06:30:45, satorux1 wrote:
> like haruki mentioned, please use const char

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:28: const char*
kAgentCapability =
On 2013/04/16 06:30:45, satorux1 wrote:
> Can this be defined at a compilation time? otherwise, a static initializer
will
> be created.

No

I'll drop this variable entirely and use the other constant

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:149: 
On 2013/04/16 15:26:56, youngki wrote:
> Remove the extra line. Or, we could early-return here and get rid of else {}.

Done.

Leaving the if/else because we've had three options here in the past and may do
in the future again

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:172: callback,
error_callback),
On 2013/04/16 15:26:56, youngki wrote:
> one argument per line:
> http://dev.chromium.org/developers/coding-style#Code_Formatting

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:225: if
(agent_.get()) {
On 2013/04/16 15:26:56, youngki wrote:
> The next 15 lines look very similar to RejectPairing(). Can we reuse the code
> here? Maybe we could have RunPairingCallback(Status) helper method that
contains
> the next 15 lines and RejectPairing() can call RunPairingcallback(REJECTED)
and
> this method can call RunPairingCallback(CANCELLED)?

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:438:
--connecting_calls_;
On 2013/04/16 15:26:56, youngki wrote:
> Can we put DCHECK(connecting_calls >= 0)?

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:449:
--connecting_calls_;
On 2013/04/16 15:26:56, youngki wrote:
> Can we put DCHECK(connecting_calls >= 0)?

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:475: object_path_,
On 2013/04/16 15:26:56, youngki wrote:
> I think it still fits to put this into the above line too.

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:490:
--connecting_calls_;
On 2013/04/16 15:26:56, youngki wrote:
> Can we put DCHECK(connecting_calls >= 0)?

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:500: if (error_name
== bluetooth_adapter::kErrorAlreadyExists) {
On 2013/04/16 15:26:56, youngki wrote:
> Remove the braces since this is one-liner if block.

Done.

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:533:
--connecting_calls_;
On 2013/04/16 15:26:56, youngki wrote:
> Can we put DCHECK(connecting_calls >= 0)?

Done.

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

https://codereview.chromium.org/13927010/diff/1/device/bluetooth/bluetooth_de...
device/bluetooth/bluetooth_device_experimental_chromeos.h:153: int
connecting_calls_;
On 2013/04/16 06:30:45, satorux1 wrote:
> maybe: num_connecting_calls_

Done.

[keybuk, 2013-04-16 23:21:45]

ptal

[Haruki Sato, 2013-04-17 14:46:11]

https://codereview.chromium.org/13927010/diff/13001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_agent_manager_client.cc (right):

https://codereview.chromium.org/13927010/diff/13001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_agent_manager_client.cc:76: 
Extra blank line?

[youngki, 2013-04-17 16:23:45]

https://chromiumcodereview.appspot.com/13927010/diff/13001/chromeos/dbus/fake...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://chromiumcodereview.appspot.com/13927010/diff/13001/chromeos/dbus/fake...
chromeos/dbus/fake_bluetooth_device_client.cc:294:
MessageLoop::current()->PostDelayedTask(
I suggest we use base::SequencedTaskRunner instead of MessageLoop::current().
You could pass that to the constructor or init method and use that to post tasks
instead. And in the test you could pass base::TestSimpleTaskRunner to control
the tasks queued. I think that could simplify
bluetooth_experimental_chromeos_unittest.cc.

https://chromiumcodereview.appspot.com/13927010/diff/13001/device/bluetooth/b...
File device/bluetooth/bluetooth_experimental_chromeos_unittest.cc (right):

https://chromiumcodereview.appspot.com/13927010/diff/13001/device/bluetooth/b...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:66:
QuitMessageLoop();
Why does AdapterDiscoveringChanged break out of loop when AdapterPresentChanged
and AdapterDiscoveringChanged don't?

https://chromiumcodereview.appspot.com/13927010/diff/13001/device/bluetooth/b...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:241:
BluetoothAdapterFactory::GetAdapter(
Why don't you just create adapter directly by new
BluetoothAdapterExperimentalChromeOS() ? You could friend this test class to the
adapter.

https://chromiumcodereview.appspot.com/13927010/diff/13001/device/bluetooth/b...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:254:
ASSERT_TRUE(adapter_ != NULL);
Can we take out all the ASSERT* and EXPECT* and put them into a testing case?
There is no need to test those every time DiscoverDevice is called.

https://chromiumcodereview.appspot.com/13927010/diff/13001/device/bluetooth/b...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:305:
DiscoverDevice("does not exist");
why do we pass "does not exist"? And how does this method discover devices?

https://chromiumcodereview.appspot.com/13927010/diff/13001/device/bluetooth/b...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:542:
message_loop.RunUntilIdle();
This method has been deprecated:
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop....

[keybuk, 2013-04-17 17:46:31]

https://codereview.chromium.org/13927010/diff/13001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_agent_manager_client.cc (right):

https://codereview.chromium.org/13927010/diff/13001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_agent_manager_client.cc:76: 
On 2013/04/17 14:46:11, Haruki Sato wrote:
> Extra blank line?

Done.

https://codereview.chromium.org/13927010/diff/13001/chromeos/dbus/fake_blueto...
File chromeos/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/13927010/diff/13001/chromeos/dbus/fake_blueto...
chromeos/dbus/fake_bluetooth_device_client.cc:294:
MessageLoop::current()->PostDelayedTask(
On 2013/04/17 16:23:45, youngki wrote:
> I suggest we use base::SequencedTaskRunner instead of MessageLoop::current().
> You could pass that to the constructor or init method and use that to post
tasks
> instead. And in the test you could pass base::TestSimpleTaskRunner to control
> the tasks queued. I think that could simplify
> bluetooth_experimental_chromeos_unittest.cc.

Do you have an example of this? No existing test case uses it

How would we get the "sequenced task runner" of the different clients to match?

How would this work when the fakes are being used by a "live" chrome browser?

https://codereview.chromium.org/13927010/diff/13001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_experimental_chromeos_unittest.cc (right):

https://codereview.chromium.org/13927010/diff/13001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:66:
QuitMessageLoop();
On 2013/04/17 16:23:45, youngki wrote:
> Why does AdapterDiscoveringChanged break out of loop when
AdapterPresentChanged
> and AdapterDiscoveringChanged don't?

mistake: deleted

https://codereview.chromium.org/13927010/diff/13001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:241:
BluetoothAdapterFactory::GetAdapter(
On 2013/04/17 16:23:45, youngki wrote:
> Why don't you just create adapter directly by new
> BluetoothAdapterExperimentalChromeOS() ? You could friend this test class to
the
> adapter.

Done.

https://codereview.chromium.org/13927010/diff/13001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:254:
ASSERT_TRUE(adapter_ != NULL);
On 2013/04/17 16:23:45, youngki wrote:
> Can we take out all the ASSERT* and EXPECT* and put them into a testing case?
> There is no need to test those every time DiscoverDevice is called.

There already is a separate testing case for these -
BluetoothExperimentalChromeOSTest.Discovery

The asserts are here more as a "make sure the conditions this function causes
actually happen, since if they don't, any test case using this function would be
invalidated"

Though the expects should change to asserts - shall fix that

https://codereview.chromium.org/13927010/diff/13001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:305:
DiscoverDevice("does not exist");
On 2013/04/17 16:23:45, youngki wrote:
> why do we pass "does not exist"? And how does this method discover devices?

will add comment

https://codereview.chromium.org/13927010/diff/13001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_experimental_chromeos_unittest.cc:542:
message_loop.RunUntilIdle();
On 2013/04/17 16:23:45, youngki wrote:
> This method has been deprecated:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop....

Not needed anymore with the current test code - dropped them all

[satorux1, 2013-04-18 00:36:28]

LGTM with a nit.

https://codereview.chromium.org/13927010/diff/30001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_device_experimental_chromeos.cc (right):

https://codereview.chromium.org/13927010/diff/30001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:587: bool
have_callback = false;
I found it slightly confusing as we'll reset callbacks which are run. Maybe
any_callbacks_run?

[keybuk, 2013-04-18 00:40:55]

https://codereview.chromium.org/13927010/diff/30001/device/bluetooth/bluetoot...
File device/bluetooth/bluetooth_device_experimental_chromeos.cc (right):

https://codereview.chromium.org/13927010/diff/30001/device/bluetooth/bluetoot...
device/bluetooth/bluetooth_device_experimental_chromeos.cc:587: bool
have_callback = false;
On 2013/04/18 00:36:28, satorux1 wrote:
> I found it slightly confusing as we'll reset callbacks which are run. Maybe
> any_callbacks_run?
> 

Done.
artifact of consolidating into one function - decided on "callback_run"

[commit-bot: I haz the power, 2013-04-18 00:42:11]

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

[satorux1, 2013-04-18 00:42:12]

LGTM

[commit-bot: I haz the power, 2013-04-18 04:41:43]

Change committed as 194760

[youngki, 2013-04-19 14:17:47]

lgtm