Add CPP API for BLE advertisments.

Add CPP API for BLE advertisments.

This CL adds the new classes, changes to existing classes and tests for adding
the CPP API for LE advertisements. The design for this is available at
http://go/chrome-ble-advertising.

R=armansito@chromium.org, jamuraa@chromium.org
BUG=466375
Committed: https://crrev.com/c96da18077ef4b5ab28cb8b2684cd84386075e5a
Cr-Commit-Position: refs/heads/master@{#327128}

[scheib, 2015-04-16 22:11:32]

scheib@chromium.org changed reviewers:
+ scheib@chromium.org

[scheib, 2015-04-16 22:11:32]

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter.h:377: typedef
base::Callback<void(scoped_refptr<BluetoothAdvertisement>)>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_advertisement.h:37: ADVERTISEMENT_TYPE_BROADCAST,
Doc or cite what an advertisement type means.

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_advertisement.h:80: virtual void
AdvertisementReleased(
What causes the release? Only Unregister? If other things can, then there seems
to be a race after creating a BluetoothAdvertisement object between registering
an observer, and it being released.

[rkc, 2015-04-17 19:57:08]

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter.h:377: typedef
base::Callback<void(scoped_refptr<BluetoothAdvertisement>)>
On 2015/04/16 22:11:32, scheib wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Going with consistency here. This positioning of the typedefs is followed
throughout the //device/bluetooth code.
Changed it to using to keep with the new style though.

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_advertisement.h:37: ADVERTISEMENT_TYPE_BROADCAST,
On 2015/04/16 22:11:32, scheib wrote:
> Doc or cite what an advertisement type means. 

arman, michael, is there a citation I can give for this?

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_advertisement.h:80: virtual void
AdvertisementReleased(
On 2015/04/16 22:11:32, scheib wrote:
> What causes the release? Only Unregister? If other things can, then there
seems
> to be a race after creating a BluetoothAdvertisement object between
registering
> an observer, and it being released.

IIUC, a release should only happen in circumstances like the adapter shutting
down or a major error in the stack. In case the advertisement doesn't catch the
release, we shouldn't break anything since any further operations on the
advertisement will just return errors stating that it doesn't exist anymore.

[armansito, 2015-04-17 20:15:14]

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_advertisement.h:37: ADVERTISEMENT_TYPE_BROADCAST,
On 2015/04/17 19:57:08, Rahul Chaturvedi wrote:
> On 2015/04/16 22:11:32, scheib wrote:
> > Doc or cite what an advertisement type means. 
> 
> arman, michael, is there a citation I can give for this?

Just document what these mean. These specifically refer to LE GAP roles but you
should explain the side effects of these types. These should be documented on
the corresponding D-Bus API reference (though now that I see it the
documentation there isn't really useful).

Basically, a broadcaster device is not connectable but a peripheral is. So if
you choose "broadcaster", the sent advertisement type will be ADV_NONCONN_IND.
On the other hand a peripheral is connectable, so depending on the presence of
scan response data, the sent advertisement type will be ADV_IND or ADV_SCAN_IND.

[Marie Janssen, 2015-04-21 18:15:10]

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
2015 and no (c) here and elsewhere on new files.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:31: ERROR_INVALID_LENGTH,  //
Advertisement is not of an in valid length.
nit: Comment has double negative / doesn't make sense.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:37: ADVERTISEMENT_TYPE_BROADCAST,
Add some comments here about how broadcast advertisements aren't connectable and
peripherals are, since that's the effect.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:45: // Structure that holds the data
for an advertisement.
I don't like this design.  It's basically holding a set of scoped_ptrs for
approximately zero time because all of the arguments end up being passe to
BluetoothLEAdvertisementServiceProvider::Create right away.

Using scoped_ptrs for this means that the object isn't usable more than once.
I'd rather have the BluetoothAdvertisement live as an object and be passed to
the BluetoothLEAdvertisementServiceProvider::Create instead - which also makes
it easier to add fields to the Advertisement if more things get added in the
future.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:70: scoped_ptr<ServiceData>
service_data_;
Please add the IncludeTXPower new property too.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement_chromeos_unittest.cc (right):

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement_chromeos_unittest.cc:159:
last_callback_count_++;
This should be last_callback_count_ = callback_count_ for clarity, and update
last_error_callback_count_ too. 
If we have a non-success then the other tests can attempt to run correctly.

[scheib, 2015-04-21 23:13:25]

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter.h:377: typedef
base::Callback<void(scoped_refptr<BluetoothAdvertisement>)>
On 2015/04/17 19:57:08, Rahul Chaturvedi wrote:
> On 2015/04/16 22:11:32, scheib wrote:
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> 
> Going with consistency here. This positioning of the typedefs is followed
> throughout the //device/bluetooth code.
> Changed it to using to keep with the new style though.

https://codereview.chromium.org/1100483005 is landing which will consistently
fix this. Please use chromium style.

[rkc, 2015-04-23 19:32:17]

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_adapter.h:377: typedef
base::Callback<void(scoped_refptr<BluetoothAdvertisement>)>
On 2015/04/21 23:13:25, scheib wrote:
> On 2015/04/17 19:57:08, Rahul Chaturvedi wrote:
> > On 2015/04/16 22:11:32, scheib wrote:
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> > 
> > Going with consistency here. This positioning of the typedefs is followed
> > throughout the //device/bluetooth code.
> > Changed it to using to keep with the new style though.
> 
> <crxref style="background-image:
url(chrome-extension://ppnlfijacbhfpjgkafbeiaklmgoljooo/res/type/codereview.png);">https://codereview.chromium.org/1100483005</crxref>
is landing which will consistently
> fix this. Please use chromium style.

Done.

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_advertisement.h:37: ADVERTISEMENT_TYPE_BROADCAST,
On 2015/04/17 20:15:14, armansito wrote:
> On 2015/04/17 19:57:08, Rahul Chaturvedi wrote:
> > On 2015/04/16 22:11:32, scheib wrote:
> > > Doc or cite what an advertisement type means. 
> > 
> > arman, michael, is there a citation I can give for this?
> 
> Just document what these mean. These specifically refer to LE GAP roles but
you
> should explain the side effects of these types. These should be documented on
> the corresponding D-Bus API reference (though now that I see it the
> documentation there isn't really useful).
> 
> Basically, a broadcaster device is not connectable but a peripheral is. So if
> you choose "broadcaster", the sent advertisement type will be ADV_NONCONN_IND.
> On the other hand a peripheral is connectable, so depending on the presence of
> scan response data, the sent advertisement type will be ADV_IND or
ADV_SCAN_IND.

Done.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2015/04/21 18:15:10, Michael Janssen wrote:
> 2015 and no (c) here and elsewhere on new files.

Done.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:31: ERROR_INVALID_LENGTH,  //
Advertisement is not of an in valid length.
On 2015/04/21 18:15:10, Michael Janssen wrote:
> nit: Comment has double negative / doesn't make sense.

Done.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:37: ADVERTISEMENT_TYPE_BROADCAST,
On 2015/04/21 18:15:09, Michael Janssen wrote:
> Add some comments here about how broadcast advertisements aren't connectable
and
> peripherals are, since that's the effect.

Done.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:45: // Structure that holds the data
for an advertisement.
On 2015/04/21 18:15:10, Michael Janssen wrote:
> I don't like this design.  It's basically holding a set of scoped_ptrs for
> approximately zero time because all of the arguments end up being passe to
> BluetoothLEAdvertisementServiceProvider::Create right away.
> 
> Using scoped_ptrs for this means that the object isn't usable more than once.
> I'd rather have the BluetoothAdvertisement live as an object and be passed to
> the BluetoothLEAdvertisementServiceProvider::Create instead - which also makes
> it easier to add fields to the Advertisement if more things get added in the
> future.

Done.

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:70: scoped_ptr<ServiceData>
service_data_;
On 2015/04/21 18:15:09, Michael Janssen wrote:
> Please add the IncludeTXPower new property too.

I don't see this property in the service_constants.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/cros_s...

Can we do this as a separate CL once we have the DEPS rolled to pull in this new
property?

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement_chromeos_unittest.cc (right):

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement_chromeos_unittest.cc:159:
last_callback_count_++;
On 2015/04/21 18:15:10, Michael Janssen wrote:
> This should be last_callback_count_ = callback_count_ for clarity, and update
> last_error_callback_count_ too. 
> If we have a non-success then the other tests can attempt to run correctly.

Done.

[armansito, 2015-04-23 20:13:25]

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:70: scoped_ptr<ServiceData>
service_data_;
On 2015/04/23 19:32:17, Rahul Chaturvedi wrote:
> On 2015/04/21 18:15:09, Michael Janssen wrote:
> > Please add the IncludeTXPower new property too.
> 
> I don't see this property in the service_constants.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/cros_s...
> 
> Can we do this as a separate CL once we have the DEPS rolled to pull in this
new
> property?

How is that related to what goes into the high-level API?

[rkc, 2015-04-24 20:04:59]

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:70: scoped_ptr<ServiceData>
service_data_;
On 2015/04/23 20:13:25, armansito wrote:
> On 2015/04/23 19:32:17, Rahul Chaturvedi wrote:
> > On 2015/04/21 18:15:09, Michael Janssen wrote:
> > > Please add the IncludeTXPower new property too.
> > 
> > I don't see this property in the service_constants.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/cros_s...
> > 
> > Can we do this as a separate CL once we have the DEPS rolled to pull in this
> new
> > property?
> 
> How is that related to what goes into the high-level API?

Done.

[Marie Janssen, 2015-04-24 21:43:08]

lgtm

[rkc, 2015-04-24 21:51:11]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2015-04-24 21:52:01]

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

[commit-bot: I haz the power, 2015-04-24 21:52:03]

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

[commit-bot: I haz the power, 2015-04-24 21:52:04]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[armansito, 2015-04-24 22:11:00]

Mostly nits.

https://codereview.chromium.org/1054743003/diff/80001/chromeos/dbus/bluetooth...
File chromeos/dbus/bluetooth_le_advertisement_service_provider.cc (right):

https://codereview.chromium.org/1054743003/diff/80001/chromeos/dbus/bluetooth...
chromeos/dbus/bluetooth_le_advertisement_service_provider.cc:408: new
FakeBluetoothLEAdvertisementServiceProvider(object_path, delegate));
Why do you call Pass() above and not here?

https://codereview.chromium.org/1054743003/diff/80001/chromeos/dbus/fake_blue...
File chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.cc (right):

https://codereview.chromium.org/1054743003/diff/80001/chromeos/dbus/fake_blue...
chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.cc:16:
object_path_ = object_path;
What's the reason for this change? You already did this in the initializer list.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:198:
base::Callback<void(BluetoothAdvertisement::ErrorCode)>;
Why did you do this here? I prefer that you perform refactors like this, that
don't directly relate to your CL, in separate CLs. This CL is already near 700
lines...

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:391:
std::pair<BluetoothDevice::PairingDelegate*, PairingDelegatePriority>;
Same as above. Do this typedef -> using conversion in a different CL.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:96: virtual ~Observer() {}
New line after destructor definition.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:110: typedef
base::Callback<void(ErrorCode)> ErrorCallback;
You used "using" above, so be consistent.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:117: BluetoothAdvertisement();
new line

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:119: virtual
~BluetoothAdvertisement();
Make this public, so that the owner of the object can delete it.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement_chromeos.cc (right):

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement_chromeos.cc:24: LOG(ERROR) <<
"Unregister failed with error code = " << error;
nit: say "BluetoothAdvertisementChromeOS::Unregister failed ..."

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement_chromeos.cc:58: : adapter_(adapter) {
Can you add DCHECKs for DBusThreadManager being initialized where it's used in
this file?

[rkc, 2015-04-24 22:55:25]

https://codereview.chromium.org/1054743003/diff/80001/chromeos/dbus/bluetooth...
File chromeos/dbus/bluetooth_le_advertisement_service_provider.cc (right):

https://codereview.chromium.org/1054743003/diff/80001/chromeos/dbus/bluetooth...
chromeos/dbus/bluetooth_le_advertisement_service_provider.cc:408: new
FakeBluetoothLEAdvertisementServiceProvider(object_path, delegate));
On 2015/04/24 22:11:00, armansito wrote:
> Why do you call Pass() above and not here?

Don't need to in either place.
Done.

https://codereview.chromium.org/1054743003/diff/80001/chromeos/dbus/fake_blue...
File chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.cc (right):

https://codereview.chromium.org/1054743003/diff/80001/chromeos/dbus/fake_blue...
chromeos/dbus/fake_bluetooth_le_advertisement_service_provider.cc:16:
object_path_ = object_path;
On 2015/04/24 22:11:00, armansito wrote:
> What's the reason for this change? You already did this in the initializer
list.

object_path_ is now a member of the super class; hence initializing it in the
initializer list of the current class won't work. We have to either pass it to
the superclass constructor or just initialize it like I am doing it here. This
seemed simpler.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:198:
base::Callback<void(BluetoothAdvertisement::ErrorCode)>;
On 2015/04/24 22:11:00, armansito wrote:
> Why did you do this here? I prefer that you perform refactors like this, that
> don't directly relate to your CL, in separate CLs. This CL is already near 700
> lines...

I agree in principle, but in this one case, I had to use using for my own code
anyway. It looked very incongruent to leave just my two typedefs with using and
the rest with typedef.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter.h:391:
std::pair<BluetoothDevice::PairingDelegate*, PairingDelegatePriority>;
On 2015/04/24 22:11:00, armansito wrote:
> Same as above. Do this typedef -> using conversion in a different CL.

Since this really isn't directly next to my new code, changed this back to
typedef.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:96: virtual ~Observer() {}
On 2015/04/24 22:11:00, armansito wrote:
> New line after destructor definition.

Done.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:110: typedef
base::Callback<void(ErrorCode)> ErrorCallback;
On 2015/04/24 22:11:00, armansito wrote:
> You used "using" above, so be consistent.

Done.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:117: BluetoothAdvertisement();
On 2015/04/24 22:11:00, armansito wrote:
> new line

Done.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:119: virtual
~BluetoothAdvertisement();
On 2015/04/24 22:11:00, armansito wrote:
> Make this public, so that the owner of the object can delete it.

We can't. The class is ref-counted.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement_chromeos.cc (right):

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement_chromeos.cc:24: LOG(ERROR) <<
"Unregister failed with error code = " << error;
On 2015/04/24 22:11:00, armansito wrote:
> nit: say "BluetoothAdvertisementChromeOS::Unregister failed ..."

Done.

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement_chromeos.cc:58: : adapter_(adapter) {
On 2015/04/24 22:11:00, armansito wrote:
> Can you add DCHECKs for DBusThreadManager being initialized where it's used in
> this file?

Done.

[armansito, 2015-04-24 23:13:13]

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_advertisement.h:119: virtual
~BluetoothAdvertisement();
On 2015/04/24 22:55:25, Rahul Chaturvedi wrote:
> On 2015/04/24 22:11:00, armansito wrote:
> > Make this public, so that the owner of the object can delete it.
> 
> We can't. The class is ref-counted.

Ah, didn't realize that.

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:198:
base::Callback<void(BluetoothAdvertisement::ErrorCode)>;
Now you have typedef below and using here... Were you going tho change these
back?

[rkc, 2015-04-24 23:15:04]

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:198:
base::Callback<void(BluetoothAdvertisement::ErrorCode)>;
On 2015/04/24 23:13:13, armansito wrote:
> Now you have typedef below and using here... Were you going tho change these
> back?

These would look really weird with 8 typedefs and 2 using's, which is why I
converted all of these. I changed the ones below back because they were
unrelated to this code block anyway.

[armansito, 2015-04-24 23:24:05]

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:183: using InitCallback =
base::Callback<void()>;
Use base::Closure here.

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:198:
base::Callback<void(BluetoothAdvertisement::ErrorCode)>;
On 2015/04/24 23:15:04, Rahul Chaturvedi wrote:
> On 2015/04/24 23:13:13, armansito wrote:
> > Now you have typedef below and using here... Were you going tho change these
> > back?
> 
> These would look really weird with 8 typedefs and 2 using's, which is why I
> converted all of these. I changed the ones below back because they were
> unrelated to this code block anyway.

Well now you have 10 usings here and 2 typedefs later in the file which is still
inconsistent but that's beside the point. Please do refactors like this in CLs
of their own.

[rkc, 2015-04-24 23:29:59]

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter.h (right):

https://codereview.chromium.org/1054743003/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter.h:198:
base::Callback<void(BluetoothAdvertisement::ErrorCode)>;
On 2015/04/24 23:24:05, armansito wrote:
> On 2015/04/24 23:15:04, Rahul Chaturvedi wrote:
> > On 2015/04/24 23:13:13, armansito wrote:
> > > Now you have typedef below and using here... Were you going tho change
these
> > > back?
> > 
> > These would look really weird with 8 typedefs and 2 using's, which is why I
> > converted all of these. I changed the ones below back because they were
> > unrelated to this code block anyway.
> 
> Well now you have 10 usings here and 2 typedefs later in the file which is
still
> inconsistent but that's beside the point. Please do refactors like this in CLs
> of their own.

Done.

[armansito, 2015-04-24 23:31:53]

lgtm, thanks.

[rkc, 2015-04-24 23:32:36]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-04-24 23:32:37]

The patchset sent to the CQ was uploaded after l-g-t-m from jamuraa@chromium.org
Link to the patchset: https://codereview.chromium.org/1054743003/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-04-24 23:33:15]

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

[commit-bot: I haz the power, 2015-04-24 23:47:43]

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

[commit-bot: I haz the power, 2015-04-24 23:47:44]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rkc, 2015-04-24 23:49:15]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2015-04-24 23:49:54]

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

[rkc, 2015-04-24 23:56:38]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-04-24 23:56:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org, jamuraa@chromium.org
Link to the patchset: https://codereview.chromium.org/1054743003/#ps140001
(title: "mac build fix")

[commit-bot: I haz the power, 2015-04-24 23:57:11]

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

[rkc, 2015-04-25 00:06:28]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-04-25 00:06:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org, jamuraa@chromium.org
Link to the patchset: https://codereview.chromium.org/1054743003/#ps160001
(title: "windows build fix")

[commit-bot: I haz the power, 2015-04-25 00:07:10]

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

[commit-bot: I haz the power, 2015-04-25 00:29:26]

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

[commit-bot: I haz the power, 2015-04-25 00:29:27]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[armansito, 2015-04-25 05:50:12]

You'll need to update BUILD.gn as well.

[rkc, 2015-04-27 18:19:59]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-04-27 18:19:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org, jamuraa@chromium.org
Link to the patchset: https://codereview.chromium.org/1054743003/#ps180001
(title: "moar build fixes")

[commit-bot: I haz the power, 2015-04-27 18:21:36]

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

[rkc, 2015-04-27 18:45:55]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-04-27 18:45:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org, jamuraa@chromium.org
Link to the patchset: https://codereview.chromium.org/1054743003/#ps200001
(title: "win fix, again")

[commit-bot: I haz the power, 2015-04-27 18:46:31]

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

[commit-bot: I haz the power, 2015-04-27 19:45:26]

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

[commit-bot: I haz the power, 2015-04-27 19:45:27]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[rkc, 2015-04-27 20:31:14]

The CQ bit was checked by rkc@chromium.org

[rkc, 2015-04-27 20:31:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
armansito@chromium.org, jamuraa@chromium.org
Link to the patchset: https://codereview.chromium.org/1054743003/#ps220001
(title: "tentative win fix")

[commit-bot: I haz the power, 2015-04-27 20:31:51]

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

[commit-bot: I haz the power, 2015-04-27 21:29:31]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-04-27 21:30:51]

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

[spang, 2015-04-27 22:15:56]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1111563002/ by spang@chromium.org.

The reason for reverting is: Breaks component=shared_library build (missing
export?)

../../device/bluetooth/bluetooth_adapter_chromeos.cc:321: error: undefined
reference to
'chromeos::BluetoothAdvertisementChromeOS::BluetoothAdvertisementChromeOS(scoped_ptr<device::BluetoothAdvertisement::Data,
base::DefaultDeleter<device::BluetoothAdvertisement::Data> >,
scoped_refptr<chromeos::BluetoothAdapterChromeOS>)'
../../base/memory/scoped_ptr.h:128: error: undefined reference to
'device::BluetoothAdvertisement::Data::~Data()'
../../device/bluetooth/bluetooth_adapter_chromeos.cc:322: error: undefined
reference to
'chromeos::BluetoothAdvertisementChromeOS::Register(base::Callback<void ()>
const&, base::Callback<void (device::BluetoothAdvertisement::ErrorCode)>
const&)'
obj/device/bluetooth/device_bluetooth.bluetooth_adapter_chromeos.o(.debug_addr+0x6f20):
error: undefined reference to
'chromeos::BluetoothAdvertisementChromeOS::Register(base::Callback<void ()>
const&, base::Callback<void (device::BluetoothAdvertisement::ErrorCode)>
const&)'.

[Nico, 2016-04-08 21:36:47]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-04-08 21:36:48]

https://codereview.chromium.org/1054743003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_advertisement.h:64: scoped_ptr<UUIDList>
solicit_uuids() { return solicit_uuids_.Pass(); }
Having regularly named accessors return scoped_ptrs seems very surprising. I
wouldn't expect that if I do

  f(my_obj.solicit_uuids())

that my_obj's uuids have now been cleared out. Why was this design chosen?

[rkc, 2016-04-08 21:57:27]

https://codereview.chromium.org/1054743003/diff/220001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_advertisement.h (right):

https://codereview.chromium.org/1054743003/diff/220001/device/bluetooth/bluet...
device/bluetooth/bluetooth_advertisement.h:64: scoped_ptr<UUIDList>
solicit_uuids() { return solicit_uuids_.Pass(); }
On 2016/04/08 21:36:48, Nico (hiding until Fri) wrote:
> Having regularly named accessors return scoped_ptrs seems very surprising. I
> wouldn't expect that if I do
> 
>   f(my_obj.solicit_uuids())
> 
> that my_obj's uuids have now been cleared out. Why was this design chosen?

This was a while ago but IIRC, all of these values were scopers we wanted to
pass to the DBus class and give it ownership. Instead of having 4-5 different
parameters to the method that was going to pass these forward, it seemed a bit
cleaner to put all the scopers in one structure and pass the structure along.
See //device/bluetooth/bluetooth_advertisement_bluez.cc:92

We would have liked to passed the entire structure, but at that time the DBus
code was not in //device/bluetooth but rather in //chromeos/dbus and due to
dependency rules (not sure which way it was, //cros/dbus couldn't depend on
//device/bt or the other way) made it so we couldn't share the structure between
the two classes.

Now that the DBus clients for Bluetooth are under //device/bluetooth/dbus, this
code can be made much simpler by simply sharing the structure and passing a
scoper to it rather than do this ugly hack.