Implement BluetoothDeviceBlueZ::GetServiceRecords.

Implement BluetoothDeviceBlueZ::GetServiceRecords.

This CL adds the implementation and tests for GetServiceRecords.
The DBus API used here is out for review at,
https://chromium-review.googlesource.com/#/c/354473/

R=mcchou@chromium.org, xiyuan@chromium.org
BUG=619699
Committed: https://crrev.com/004c048627a230d2a77505473892a4388c38376f
Cr-Commit-Position: refs/heads/master@{#403324}

[xiyuan, 2016-06-29 22:43:08]

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.h:47:
base::Callback<void(std::vector<BluetoothServiceRecordBlueZ>)>;
std::vector<...>&, passing vector by value feels inefficient.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez.cc:43:
attributes_.erase(it);
Why do we need to explicitly remove the existing one? Why not just

  attributes_[id] = value;

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez_unittest.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez_unittest.cc:36: LOG(ERROR)
<< "RKC: 1";
remove debugging log, here and other places?

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/bluet...
File device/bluetooth/dbus/bluetooth_device_client.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_device_client.cc:119: break;
nit: no "break" since we "return" ?

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/bluet...
File device/bluetooth/dbus/bluetooth_device_client.h (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_device_client.h:38: using ServiceRecordsCallback
= base::Callback<void(ServiceRecordList)>;
Do we want to use ServiceRecordList& or std::unique_ptr to pass the list around
more efficiently?

[Miao, 2016-06-30 04:55:59]

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez.h (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez.h:35:
BluetoothServiceRecordBlueZ();
Can we also keep the constructor taking attributes as a batch? Since there is
case where a record has been constructed and will be translated into
BluetoothServiceRecordBlueZ. Instead of adding attributes one by one, placing
attributes all at once is more preferable.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez_unittest.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez_unittest.cc:122: size_t
success = expect_success ? 1 : 0;
Instead of passing an extra |bool expect_success| for verifying the number of
callbacks, this can be placed in the test body.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/fake_...
File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/fake_...
device/bluetooth/dbus/fake_bluetooth_device_client.cc:115:
class_id_list->emplace_back(BluetoothServiceAttributeValueBlueZ(
emplace_back(...) allows in-place construction of the object to be inserted. So
instead of creating a temp BluetoothServiceAttributeValueBlueZ, this should be 
class_id_list->emplace_back(BluetoothServiceAttributeValueBlueZ::UUID, 4,
base::MakeUnique<base::StringValue>("1802"));

[rkc, 2016-06-30 20:02:24]

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_device_bluez.h (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.h:47:
base::Callback<void(std::vector<BluetoothServiceRecordBlueZ>)>;
On 2016/06/29 22:43:08, xiyuan wrote:
> std::vector<...>&, passing vector by value feels inefficient.

Done.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez.cc:43:
attributes_.erase(it);
On 2016/06/29 22:43:08, xiyuan wrote:
> Why do we need to explicitly remove the existing one? Why not just
> 
>   attributes_[id] = value;

This is a painful part of std::map. This apparently always first creates a
BluetoothServiceAttributeValueBlueZ() first, and then tries to copy over it.
Since we don't have (nor want) a constructor for
BluetoothServiceAttributeValueBlueZ(), the only way around this is to use
insert.

Using insert has the side-effect of requiring us to explicitly erase first.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez.h (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez.h:35:
BluetoothServiceRecordBlueZ();
On 2016/06/30 04:55:58, Miao wrote:
> Can we also keep the constructor taking attributes as a batch? Since there is
> case where a record has been constructed and will be translated into
> BluetoothServiceRecordBlueZ. Instead of adding attributes one by one, placing
> attributes all at once is more preferable.

Discussed offline.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez_unittest.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez_unittest.cc:36: LOG(ERROR)
<< "RKC: 1";
On 2016/06/29 22:43:08, xiyuan wrote:
> remove debugging log, here and other places?

Whoops.
Done.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez_unittest.cc:122: size_t
success = expect_success ? 1 : 0;
On 2016/06/30 04:55:59, Miao wrote:
> Instead of passing an extra |bool expect_success| for verifying the number of
> callbacks, this can be placed in the test body.

I expect us to have more tests :)

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/bluet...
File device/bluetooth/dbus/bluetooth_device_client.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_device_client.cc:119: break;
On 2016/06/29 22:43:08, xiyuan wrote:
> nit: no "break" since we "return" ?

Done.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/bluet...
File device/bluetooth/dbus/bluetooth_device_client.h (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_device_client.h:38: using ServiceRecordsCallback
= base::Callback<void(ServiceRecordList)>;
On 2016/06/29 22:43:08, xiyuan wrote:
> Do we want to use ServiceRecordList& or std::unique_ptr to pass the list
around
> more efficiently?

Done.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/fake_...
File device/bluetooth/dbus/fake_bluetooth_device_client.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/dbus/fake_...
device/bluetooth/dbus/fake_bluetooth_device_client.cc:115:
class_id_list->emplace_back(BluetoothServiceAttributeValueBlueZ(
On 2016/06/30 04:55:59, Miao wrote:
> emplace_back(...) allows in-place construction of the object to be inserted.
So
> instead of creating a temp BluetoothServiceAttributeValueBlueZ, this should be

> class_id_list->emplace_back(BluetoothServiceAttributeValueBlueZ::UUID, 4,
> base::MakeUnique<base::StringValue>("1802"));

Done.

[xiyuan, 2016-06-30 20:07:34]

lgtm

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez.cc:43:
attributes_.erase(it);
On 2016/06/30 20:02:24, rkc wrote:
> On 2016/06/29 22:43:08, xiyuan wrote:
> > Why do we need to explicitly remove the existing one? Why not just
> > 
> >   attributes_[id] = value;
> 
> This is a painful part of std::map. This apparently always first creates a
> BluetoothServiceAttributeValueBlueZ() first, and then tries to copy over it.
> Since we don't have (nor want) a constructor for
> BluetoothServiceAttributeValueBlueZ(), the only way around this is to use
> insert.
> 
> Using insert has the side-effect of requiring us to explicitly erase first.

Acknowledged.

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez.cc:44: attributes_.insert(
nit: could this be

  attributes_.emplace(id, value);

?

[rkc, 2016-06-30 20:15:55]

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_service_record_bluez.cc (right):

https://codereview.chromium.org/2102093003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_service_record_bluez.cc:44: attributes_.insert(
On 2016/06/30 20:07:34, xiyuan wrote:
> nit: could this be
> 
>   attributes_.emplace(id, value);
> 
> ?

For some reason, map::emplace breaks some bots, while vector::emplace_back
doesn't.
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.linux...

[rkc, 2016-06-30 20:16:00]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-06-30 20:16:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 20:26:49]

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

[commit-bot: I haz the power, 2016-06-30 20:26:50]

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

[ortuno, 2016-06-30 20:37:52]

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

[ortuno, 2016-06-30 20:37:53]

device/bluetooth/test/bluetooth_test_bluez.cc lgtm, didn't look at the rest.

[rkc, 2016-06-30 20:38:26]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-06-30 20:39:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 21:33:56]

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

[commit-bot: I haz the power, 2016-06-30 21:33:57]

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

[rkc, 2016-06-30 21:39:22]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-06-30 21:39:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 23:07:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-30 23:07:23]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 23:09:22]

Description was changed from

==========
Implement BluetoothDeviceBlueZ::GetServiceRecords.

This CL adds the implementation and tests for GetServiceRecords.
The DBus API used here is out for review at,
https://chromium-review.googlesource.com/#/c/354473/

R=mcchou@chromium.org, xiyuan@chromium.org
BUG=619699
==========

to

==========
Implement BluetoothDeviceBlueZ::GetServiceRecords.

This CL adds the implementation and tests for GetServiceRecords.
The DBus API used here is out for review at,
https://chromium-review.googlesource.com/#/c/354473/

R=mcchou@chromium.org, xiyuan@chromium.org
BUG=619699

Committed: https://crrev.com/004c048627a230d2a77505473892a4388c38376f
Cr-Commit-Position: refs/heads/master@{#403324}
==========

[commit-bot: I haz the power, 2016-06-30 23:09:23]

Patchset 2 (id:??) landed as
https://crrev.com/004c048627a230d2a77505473892a4388c38376f
Cr-Commit-Position: refs/heads/master@{#403324}