DBus support for attribute properties and permissions.

DBus support for attribute properties and permissions.

This CL adds the DBus code needed to handle characteristic and descriptor
properties and permissions.

We will process the props/perms into the flags expected by BlueZ in the
BluetoothLocalGattXXXBlueZ classes. The DBus layer is simply responsible
for getting the flags over to BlueZ.

R=scheib@chromium.org
BUG=606853
Committed: https://crrev.com/60a8ee95f6bbe75057aeeb33bc6ccb58a3dd182b
Cr-Commit-Position: refs/heads/master@{#392359}

[rkc, 2016-05-06 17:10:33]

rkc@chromium.org changed reviewers:
+ stevenjb@chromium.org

[rkc, 2016-05-06 17:10:34]

Adding stevenjb to help review.

[stevenjb, 2016-05-06 17:12:08]

stevenjb@chromium.org changed reviewers:
+ hashimoto@chromium.org, satorux@chromium.org

[stevenjb, 2016-05-06 17:12:08]

I'll try to look at this when I have a chance. +hashimoto@, +satorux@ in case
they have some cycles.

[scheib, 2016-05-07 00:25:22]

LGTM

[hashimoto, 2016-05-07 04:06:14]

I have no expertise in this code, so pointing out some general things.

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

https://codereview.chromium.org/1954643002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_characteristic_service_provider.h:53: //
|flags| is the list of GATT characteristic properties, |permissions| is the
Please revise the comment.

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

https://codereview.chromium.org/1954643002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_characteristic_service_provider_impl.cc:45:
// test, hence we shouldn't do any other setup.
This behavior can be a bit surprising for developers.
How about passing a dbus::MockBus to this ctor in the test instead?
The same goes for BluetoothGattDescriptorServiceProviderImpl and
BluetoothGattServiceServiceProviderImpl.

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

https://codereview.chromium.org/1954643002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_descriptor_service_provider.h:52: // the
object onto, |uuid| is the 128-bit GATT descriptor UUID, |permissions|
Please revise the comment.

[rkc, 2016-05-07 19:55:43]

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

https://codereview.chromium.org/1954643002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_characteristic_service_provider.h:53: //
|flags| is the list of GATT characteristic properties, |permissions| is the
On 2016/05/07 04:06:13, hashimoto wrote:
> Please revise the comment.

Done.

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

https://codereview.chromium.org/1954643002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_characteristic_service_provider_impl.cc:45:
// test, hence we shouldn't do any other setup.
On 2016/05/07 04:06:13, hashimoto wrote:
> This behavior can be a bit surprising for developers.
> How about passing a dbus::MockBus to this ctor in the test instead?
> The same goes for BluetoothGattDescriptorServiceProviderImpl and
> BluetoothGattServiceServiceProviderImpl.

Passing a MockBus means that the rest of this code will expect functional
behavior from the bus, which breaks in several places.

For example, the mock bus will return a null pointer from GetExportedObject.
This means that any calls on that object will then cause a crash.

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

https://codereview.chromium.org/1954643002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_descriptor_service_provider.h:52: // the
object onto, |uuid| is the 128-bit GATT descriptor UUID, |permissions|
On 2016/05/07 04:06:13, hashimoto wrote:
> Please revise the comment.

Done.

[rkc, 2016-05-07 19:55:48]

The CQ bit was checked by rkc@chromium.org

[rkc, 2016-05-07 19:55:49]

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

[commit-bot: I haz the power, 2016-05-07 19:55:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954643002/20001

[commit-bot: I haz the power, 2016-05-07 20:43:20]

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

[commit-bot: I haz the power, 2016-05-07 20:43:21]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rkc, 2016-05-07 20:44:15]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-05-07 20:44:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954643002/20001

[commit-bot: I haz the power, 2016-05-07 21:35:09]

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

[commit-bot: I haz the power, 2016-05-07 21:35:10]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rkc, 2016-05-08 22:07:02]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-05-08 22:07:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954643002/20001

[commit-bot: I haz the power, 2016-05-08 23:08:23]

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

[commit-bot: I haz the power, 2016-05-08 23:08:24]

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

[rkc, 2016-05-09 01:28:49]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-05-09 01:29:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954643002/20001

[commit-bot: I haz the power, 2016-05-09 02:18:33]

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

[commit-bot: I haz the power, 2016-05-09 02:18:34]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rkc, 2016-05-09 10:06:50]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-05-09 10:07:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954643002/20001

[commit-bot: I haz the power, 2016-05-09 11:05:54]

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

[commit-bot: I haz the power, 2016-05-09 11:05:55]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rkc, 2016-05-09 16:55:48]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-05-09 16:56:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1954643002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1954643002/20001

[stevenjb, 2016-05-09 17:31:38]

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

https://codereview.chromium.org/1954643002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_characteristic_service_provider_impl.cc:45:
// test, hence we shouldn't do any other setup.
On 2016/05/07 19:55:43, Rahul Chaturvedi wrote:
> On 2016/05/07 04:06:13, hashimoto wrote:
> > This behavior can be a bit surprising for developers.
> > How about passing a dbus::MockBus to this ctor in the test instead?
> > The same goes for BluetoothGattDescriptorServiceProviderImpl and
> > BluetoothGattServiceServiceProviderImpl.
> 
> Passing a MockBus means that the rest of this code will expect functional
> behavior from the bus, which breaks in several places.
> 
> For example, the mock bus will return a null pointer from GetExportedObject.
> This means that any calls on that object will then cause a crash.
I agree that passing MockBus() and fixing the code would probably be best, but
I'm not sure how messy that would actually be.

Otherwise I would prefer to see the majority of the members initialized to
nullptr or uninitialized, and an Initialize() and InitializeForTest() method
added for all of these implementations.

That said, this is an improvement over multiple constructors, but we should at
least document each of these in the header with something like "|bus| can be
null for tests".

[commit-bot: I haz the power, 2016-05-09 17:57:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-09 17:59:16]

Description was changed from

==========
DBus support for attribute properties and permissions.

This CL adds the DBus code needed to handle characteristic and descriptor
properties and permissions.

We will process the props/perms into the flags expected by BlueZ in the
BluetoothLocalGattXXXBlueZ classes. The DBus layer is simply responsible
for getting the flags over to BlueZ.

R=scheib@chromium.org
BUG=606853
==========

to

==========
DBus support for attribute properties and permissions.

This CL adds the DBus code needed to handle characteristic and descriptor
properties and permissions.

We will process the props/perms into the flags expected by BlueZ in the
BluetoothLocalGattXXXBlueZ classes. The DBus layer is simply responsible
for getting the flags over to BlueZ.

R=scheib@chromium.org
BUG=606853

Committed: https://crrev.com/60a8ee95f6bbe75057aeeb33bc6ccb58a3dd182b
Cr-Commit-Position: refs/heads/master@{#392359}
==========

[commit-bot: I haz the power, 2016-05-09 17:59:17]

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