Implement support for the OOB Pairing APIs.

Implement support for the OOB Pairing APIs.

TEST=none (though tests are included with the extension APIs)
BUG=119473
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142337

[keybuk, 2012-06-07 20:52:48]

http://codereview.chromium.org/10546010/diff/1/chromeos/dbus/bluetooth_adapte...
File chromeos/dbus/bluetooth_adapter_client.h (right):

http://codereview.chromium.org/10546010/diff/1/chromeos/dbus/bluetooth_adapte...
chromeos/dbus/bluetooth_adapter_client.h:249: const AdapterCallback& callback) =
0;
NAK, this should go in BluetoothOutOfBandDataClient or similar - the convention
is one class per interface, not object type

(which is why we have BluetoothInputClient *and* BluetoothDeviceClient

[bryeung, 2012-06-08 19:16:58]

On 2012/06/07 20:52:48, keybuk wrote:
>
http://codereview.chromium.org/10546010/diff/1/chromeos/dbus/bluetooth_adapte...
> File chromeos/dbus/bluetooth_adapter_client.h (right):
> 
>
http://codereview.chromium.org/10546010/diff/1/chromeos/dbus/bluetooth_adapte...
> chromeos/dbus/bluetooth_adapter_client.h:249: const AdapterCallback& callback)
=
> 0;
> NAK, this should go in BluetoothOutOfBandDataClient or similar - the
convention
> is one class per interface, not object type
> 
> (which is why we have BluetoothInputClient *and* BluetoothDeviceClient

I think I have this sorted out now.  PTAL.

[keybuk, 2012-06-08 20:06:38]

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc (left):

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:254: 
why removed?

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:97: typedef
base::Callback<void(bool result)> ResultCallback;
This isn't the style of these classes - they use separate ErrorCallbacks to
indicate error rather than 'bool success'

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:146: const
chromeos::BluetoothOutOfBandClient::SuccessCallback& callback);
Should this not be on BluetoothDevice if it takes an address?

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:150: const
chromeos::BluetoothOutOfBandClient::SuccessCallback& callback);
Likewise maybe this should be on BluetoothDevice?

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/bluetooth_ada...
File chromeos/dbus/bluetooth_adapter_client.cc (right):

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/bluetooth_ada...
chromeos/dbus/bluetooth_adapter_client.cc:8: #include <utility>
is this a hold-over from a previous patch?

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/bluetooth_out...
File chromeos/dbus/bluetooth_out_of_band_client.cc (right):

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.cc:33: const DataCallback& callback)
{
missing OVERRIDE (many instances of this)

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/mock_bluetoot...
File chromeos/dbus/mock_bluetooth_adapter_client.h (right):

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/mock_bluetoot...
chromeos/dbus/mock_bluetooth_adapter_client.h:10: #include
"base/memory/scoped_ptr.h"
is this a hold-over from an earlier patch?

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/mock_bluetoot...
File chromeos/dbus/mock_bluetooth_out_of_band_client.cc (right):

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/mock_bluetoot...
chromeos/dbus/mock_bluetooth_out_of_band_client.cc:11:
MockBluetoothOutOfBandClient::~MockBluetoothOutOfBandClient() {}
I am confused as to why this is a rename?

[bryeung, 2012-06-14 15:31:07]

satorux: would you be willing to take over review of this CL?  I would like it
to be included in M21 as part of the apps stuff happening for IO and Scott is on
vacation.

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc (left):

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.cc:254: 
On 2012/06/08 20:06:39, keybuk wrote:
> why removed?

not sure how that happened, fixed

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/bluetooth_adapter.h (right):

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:97: typedef
base::Callback<void(bool result)> ResultCallback;
On 2012/06/08 20:06:39, keybuk wrote:
> This isn't the style of these classes - they use separate ErrorCallbacks to
> indicate error rather than 'bool success'

Done.

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:146: const
chromeos::BluetoothOutOfBandClient::SuccessCallback& callback);
On 2012/06/08 20:06:39, keybuk wrote:
> Should this not be on BluetoothDevice if it takes an address?

Done.

http://codereview.chromium.org/10546010/diff/3001/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/bluetooth_adapter.h:150: const
chromeos::BluetoothOutOfBandClient::SuccessCallback& callback);
On 2012/06/08 20:06:39, keybuk wrote:
> Likewise maybe this should be on BluetoothDevice?

Done.

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/bluetooth_ada...
File chromeos/dbus/bluetooth_adapter_client.cc (right):

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/bluetooth_ada...
chromeos/dbus/bluetooth_adapter_client.cc:8: #include <utility>
On 2012/06/08 20:06:39, keybuk wrote:
> is this a hold-over from a previous patch?

this was to make it lint clean...I'll fix in a different CL

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/bluetooth_out...
File chromeos/dbus/bluetooth_out_of_band_client.cc (right):

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.cc:33: const DataCallback& callback)
{
On 2012/06/08 20:06:39, keybuk wrote:
> missing OVERRIDE (many instances of this)

Done.

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/mock_bluetoot...
File chromeos/dbus/mock_bluetooth_adapter_client.h (right):

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/mock_bluetoot...
chromeos/dbus/mock_bluetooth_adapter_client.h:10: #include
"base/memory/scoped_ptr.h"
On 2012/06/08 20:06:39, keybuk wrote:
> is this a hold-over from an earlier patch?

removed

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/mock_bluetoot...
File chromeos/dbus/mock_bluetooth_out_of_band_client.cc (right):

http://codereview.chromium.org/10546010/diff/3001/chromeos/dbus/mock_bluetoot...
chromeos/dbus/mock_bluetooth_out_of_band_client.cc:11:
MockBluetoothOutOfBandClient::~MockBluetoothOutOfBandClient() {}
On 2012/06/08 20:06:39, keybuk wrote:
> I am confused as to why this is a rename?

Not sure: git must be picking up the adapter as a close enough file since this
file is so trivial.

[satorux1, 2012-06-14 16:07:55]

http://codereview.chromium.org/10546010/diff/9002/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h (right):

http://codereview.chromium.org/10546010/diff/9002/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h:33: void
SetLocalOutOfBandPairingData(BluetoothOutOfBandPairingData* data);
Can you go with a regular MOCK_METHOD? I think it's not preferable to have a
concrete function in a mock class.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
File chromeos/dbus/bluetooth_out_of_band_client.cc (right):

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.cc:120: uint8_t* bytes;
please initialize this with NULL to be extra defensive.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.cc:132: }
I'd suggest to emit LOG(WARNING) if the response is invalid, like:

if (!success)
  LOG(WARNING) << "Invalid response: " << response->ToString();

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
File chromeos/dbus/bluetooth_out_of_band_client.h (right):

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:22: static const size_t
kBluetoothOutOfBandPairingDataSize = 16;
please omit static. From the spec:

 Objects declared const and not explicitly declared extern have internal
linkage.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:24: struct
BluetoothOutOfBandPairingData {
Could you write some comment about the struct?

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:25: // Simple Pairing Hash C
nit: period is missing.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:28: // Simple Pairing Randomizer R
ditto

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:43: // Read the local Out Of Band
Pairing Data and return it in |callback|.
Reads

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:48: // Set the Out Of Band Pairing
Data for the device at |address| to |data|,
Sets. Please fix other places too.

[bryeung, 2012-06-14 17:56:04]

ptal

http://codereview.chromium.org/10546010/diff/9002/chrome/browser/chromeos/blu...
File chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h (right):

http://codereview.chromium.org/10546010/diff/9002/chrome/browser/chromeos/blu...
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h:33: void
SetLocalOutOfBandPairingData(BluetoothOutOfBandPairingData* data);
On 2012/06/14 16:07:55, satorux1 wrote:
> Can you go with a regular MOCK_METHOD? I think it's not preferable to have a
> concrete function in a mock class.

It's a pain to deal with returning data to callbacks.  I think I've finally
figured out a way around that though, so I've changed this.

That is why I had to add the GetDevice mocks as well.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
File chromeos/dbus/bluetooth_out_of_band_client.cc (right):

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.cc:120: uint8_t* bytes;
On 2012/06/14 16:07:55, satorux1 wrote:
> please initialize this with NULL to be extra defensive.

Done.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.cc:132: }
On 2012/06/14 16:07:55, satorux1 wrote:
> I'd suggest to emit LOG(WARNING) if the response is invalid, like:
> 
> if (!success)
>   LOG(WARNING) << "Invalid response: " << response->ToString();

I prefer to avoid logging error conditions like this:
http://dev.chromium.org/developers/coding-style#TOC-Logging

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
File chromeos/dbus/bluetooth_out_of_band_client.h (right):

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:22: static const size_t
kBluetoothOutOfBandPairingDataSize = 16;
On 2012/06/14 16:07:55, satorux1 wrote:
> please omit static. From the spec:
> 
>  Objects declared const and not explicitly declared extern have internal
> linkage.

Done.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:24: struct
BluetoothOutOfBandPairingData {
On 2012/06/14 16:07:55, satorux1 wrote:
> Could you write some comment about the struct?

Done.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:25: // Simple Pairing Hash C
On 2012/06/14 16:07:55, satorux1 wrote:
> nit: period is missing.

Seems a bit weird, considering it isn't really a sentence.  But doesn't really
matter to me.

Done.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:28: // Simple Pairing Randomizer R
On 2012/06/14 16:07:55, satorux1 wrote:
> ditto

Done.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:43: // Read the local Out Of Band
Pairing Data and return it in |callback|.
On 2012/06/14 16:07:55, satorux1 wrote:
> Reads

Done.

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.h:48: // Set the Out Of Band Pairing
Data for the device at |address| to |data|,
On 2012/06/14 16:07:55, satorux1 wrote:
> Sets. Please fix other places too.

Done.

[satorux1, 2012-06-14 20:04:57]

LGTM

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
File chromeos/dbus/bluetooth_out_of_band_client.cc (right):

http://codereview.chromium.org/10546010/diff/9002/chromeos/dbus/bluetooth_out...
chromeos/dbus/bluetooth_out_of_band_client.cc:132: }
On 2012/06/14 17:56:04, bryeung wrote:
> On 2012/06/14 16:07:55, satorux1 wrote:
> > I'd suggest to emit LOG(WARNING) if the response is invalid, like:
> > 
> > if (!success)
> >   LOG(WARNING) << "Invalid response: " << response->ToString();
> 
> I prefer to avoid logging error conditions like this:
> http://dev.chromium.org/developers/coding-style#TOC-Logging

It's up to you but LOG(WARNING) << response->ToString() is useful to detect
incompatible changes in the response messages. Without logging like this, it's
hard to detect such changes.

[commit-bot: I haz the power, 2012-06-14 20:07:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10546010/6003

[commit-bot: I haz the power, 2012-06-14 22:03:20]

Try job failure for 10546010-6003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-14 23:44:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10546010/6003

[commit-bot: I haz the power, 2012-06-14 23:44:30]

Failed to apply patch for
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h:
While running patch -p1 --forward --force;
patching file chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h
Hunk #1 FAILED at 6.
Hunk #2 FAILED at 18.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/bluetooth/test/mock_bluetooth_adapter.h.rej

[commit-bot: I haz the power, 2012-06-14 23:51:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bryeung@chromium.org/10546010/7034

[commit-bot: I haz the power, 2012-06-15 03:51:03]

Change committed as 142337