//device/bluetooth support for attribute properties and permissions.

//device/bluetooth support for attribute properties and permissions.

This CL adds the code to //device/bluetooth to correctly hand properties
and permissions.

Currently characteristic permissions are inferred by BlueZ from the
properties that are set. Once this is no longer the case, we'll start
sending permissions to BlueZ. Currently the DBus API doesn't have
support for this.

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

[scheib, 2016-05-07 01:12:45]

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

https://codereview.chromium.org/1947353002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_application_service_provider.cc:25: const
std::vector<std::string> FlagsFromProperties(
Add a compile time check that all enum values are handled here, likely be adding
a NUM_PROPERTY to the enum. EG:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://codereview.chromium.org/1947353002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_application_service_provider.cc:122:
FlagsFromProperties(characteristic.second->GetProperties()),
It would be nice if this code was covered in
bluetooth_gatt_application_service_provider_unittest.cc. However, it seems that
to do so construction code of CreateFakeAttributes would need to be called
before CreateAttributeServiceProviders, which isn't currently possible due to
being in the constructor for an object used by CreateFakeAttributes.

What sets service_providers_ in BluetoothGattApplicationServiceProviderImpl
before CreateAttributeServiceProviders is called? It would be much nicer if we
didn't have to ask that question by having it demonstrated in test.

[rkc, 2016-05-07 20:27:01]

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

https://codereview.chromium.org/1947353002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_application_service_provider.cc:25: const
std::vector<std::string> FlagsFromProperties(
On 2016/05/07 01:12:45, scheib wrote:
> Add a compile time check that all enum values are handled here, likely be
adding
> a NUM_PROPERTY to the enum. EG:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Done.

https://codereview.chromium.org/1947353002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_application_service_provider.cc:122:
FlagsFromProperties(characteristic.second->GetProperties()),
On 2016/05/07 01:12:45, scheib wrote:
> It would be nice if this code was covered in
> bluetooth_gatt_application_service_provider_unittest.cc. However, it seems
that
> to do so construction code of CreateFakeAttributes would need to be called
> before CreateAttributeServiceProviders, which isn't currently possible due to
> being in the constructor for an object used by CreateFakeAttributes.
> 
> What sets service_providers_ in BluetoothGattApplicationServiceProviderImpl
> before CreateAttributeServiceProviders is called? It would be much nicer if we
> didn't have to ask that question by having it demonstrated in test.
> 

To use this method, we'd need to have a valid set of services, to create which,
will require all the infrastructure needed for a regular BlueZ unittest
(including bringing up an adapter). That is overkill for a unit test trying to
test specifically DBus methods.

This method does get called and tested in our BluetoothLocalGatt* unit tests
though and this code does get exercised. Which is why it is a lower priority to
to try to test this in a DBus unit test too.

[scheib, 2016-05-08 03:29:57]

LGTM with:

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

https://codereview.chromium.org/1947353002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_application_service_provider.cc:25: const
std::vector<std::string> FlagsFromProperties(
On 2016/05/07 20:27:00, Rahul Chaturvedi wrote:
> On 2016/05/07 01:12:45, scheib wrote:
> > Add a compile time check that all enum values are handled here, likely be
> adding
> > a NUM_PROPERTY to the enum. EG:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> Done.

Flags too please.

https://codereview.chromium.org/1947353002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_application_service_provider.cc:122:
FlagsFromProperties(characteristic.second->GetProperties()),
On 2016/05/07 20:27:01, Rahul Chaturvedi wrote:
> On 2016/05/07 01:12:45, scheib wrote:
> > It would be nice if this code was covered in
> > bluetooth_gatt_application_service_provider_unittest.cc. However, it seems
> that
> > to do so construction code of CreateFakeAttributes would need to be called
> > before CreateAttributeServiceProviders, which isn't currently possible due
to
> > being in the constructor for an object used by CreateFakeAttributes.
> > 
> > What sets service_providers_ in BluetoothGattApplicationServiceProviderImpl
> > before CreateAttributeServiceProviders is called? It would be much nicer if
we
> > didn't have to ask that question by having it demonstrated in test.
> > 
> 
> To use this method, we'd need to have a valid set of services, to create
which,
> will require all the infrastructure needed for a regular BlueZ unittest
> (including bringing up an adapter). That is overkill for a unit test trying to
> test specifically DBus methods.
> 
> This method does get called and tested in our BluetoothLocalGatt* unit tests
> though and this code does get exercised. Which is why it is a lower priority
to
> to try to test this in a DBus unit test too.

Acknowledged.

[rkc, 2016-05-08 22:15:30]

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

https://codereview.chromium.org/1947353002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_application_service_provider.cc:25: const
std::vector<std::string> FlagsFromProperties(
On 2016/05/08 03:29:56, scheib wrote:
> On 2016/05/07 20:27:00, Rahul Chaturvedi wrote:
> > On 2016/05/07 01:12:45, scheib wrote:
> > > Add a compile time check that all enum values are handled here, likely be
> > adding
> > > a NUM_PROPERTY to the enum. EG:
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > 
> > Done.
> 
> Flags too please.

I presume you mean permissions too?
Done.

[scheib, 2016-05-08 23:43:51]

LGTM

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

https://codereview.chromium.org/1947353002/diff/1/device/bluetooth/dbus/bluet...
device/bluetooth/dbus/bluetooth_gatt_application_service_provider.cc:25: const
std::vector<std::string> FlagsFromProperties(
On 2016/05/08 22:15:30, Rahul Chaturvedi wrote:
> On 2016/05/08 03:29:56, scheib wrote:
> > On 2016/05/07 20:27:00, Rahul Chaturvedi wrote:
> > > On 2016/05/07 01:12:45, scheib wrote:
> > > > Add a compile time check that all enum values are handled here, likely
be
> > > adding
> > > > a NUM_PROPERTY to the enum. EG:
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> > > 
> > > Done.
> > 
> > Flags too please.
> 
> I presume you mean permissions too?
> Done.

Yep.

[rkc, 2016-05-09 10:07:19]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1947353002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1947353002/40001

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

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

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

Dry run: 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 17:50:14]

The CQ bit was checked by rkc@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1947353002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1947353002/40001

[rkc, 2016-05-09 18:14:16]

The CQ bit was unchecked by rkc@chromium.org

[rkc, 2016-05-09 18:14:20]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-05-09 18:14:41]

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

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

Committed patchset #3 (id:40001)

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

Description was changed from

==========
//device/bluetooth support for attribute properties and permissions.

This CL adds the code to //device/bluetooth to correctly hand properties
and permissions.

Currently characteristic permissions are inferred by BlueZ from the
properties that are set. Once this is no longer the case, we'll start
sending permissions to BlueZ. Currently the DBus API doesn't have
support for this.

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

to

==========
//device/bluetooth support for attribute properties and permissions.

This CL adds the code to //device/bluetooth to correctly hand properties
and permissions.

Currently characteristic permissions are inferred by BlueZ from the
properties that are set. Once this is no longer the case, we'll start
sending permissions to BlueZ. Currently the DBus API doesn't have
support for this.

R=scheib@chromium.org
BUG=606853

Committed: https://crrev.com/9c75aaf50cf5fd7d86a168990109b6b62f81996f
Cr-Commit-Position: refs/heads/master@{#392383}
==========

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

Patchset 3 (id:??) landed as
https://crrev.com/9c75aaf50cf5fd7d86a168990109b6b62f81996f
Cr-Commit-Position: refs/heads/master@{#392383}