Add support for local services/characteristics/descriptors.

Add support for local services/characteristics/descriptors.

We currently only support remote services, characteristics and descriptors.
This prevents us from implementing GATT server support. This CL refactors
the current code to allow us to have remote and local versions of these
attributes.

BluetoothGattDescriptors wasn't refactored into multiple classes since the
only difference between local and remote was one flag. In the future, we
might even remove this flag (at the moment it doesn't seem to have any
purpose).

R=scheib@chromium.org
BUG=601935
Committed: https://crrev.com/4e63c0523e9e45a715f00371bbee0e93b95516c0
Cr-Commit-Position: refs/heads/master@{#387516}

[scheib, 2016-04-10 04:15:46]

I'll give this a closer look Monday -- but want to raise a testing topic. So far
no new unittests are introduced here, but they will be soon as you start
implementing the Local versions. I'd very much prefer we implement those in
cross platform BluetoothTest style -- but bluez code hasn't been ported to that
yet. I want that on your radar, as we'll need it soon.

[scheib, 2016-04-11 23:36:06]

On 2016/04/10 04:15:46, scheib wrote:
> I'll give this a closer look Monday -- but want to raise a testing topic. So
far
> no new unittests are introduced here, but they will be soon as you start
> implementing the Local versions. I'd very much prefer we implement those in
> cross platform BluetoothTest style -- but bluez code hasn't been ported to
that
> yet. I want that on your radar, as we'll need it soon.

I made it part way through only -- it's time consuming to review with the moved
code.

[ortuno, 2016-04-12 15:03:37]

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

[commit-bot: I haz the power, 2016-04-12 15:04:11]

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

[commit-bot: I haz the power, 2016-04-12 15:06:26]

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

[commit-bot: I haz the power, 2016-04-12 15:06:27]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[scheib, 2016-04-12 18:05:46]

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

https://codereview.chromium.org/1872943002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_gatt_descriptor_bluez.h:30: //
BluetoothGattDescriptor for remote GATT characteristic descriptors for
remote and local

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

https://codereview.chromium.org/1872943002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_gatt_service.h:146: using ErrorCallback =
base::Callback<void(const std::string& error_name,
error_name should not be a string. We should not be exposing DBUS details in the
cross platform interface. This interface should explicitly document what may be
returned. A descriptive string for logging/output to console is fine, but any
code that program logic may depend on should be an enumeration (or nothing -
just the implicit error of the error callback).

[ortuno, 2016-04-13 14:40:27]

Also please run git cl try before sending out for review.

https://codereview.chromium.org/1872943002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_gatt_characteristic_bluez.cc (right):

https://codereview.chromium.org/1872943002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_gatt_characteristic_bluez.cc:50:
device::BluetoothUUID BluetoothGattCharacteristicBlueZ::GetUUID() const {
It looks like these implementation are specific to remote characteristic i.e.
you shouldn't need to get the Characteristic client to get the UUID of a local
characteristic. Why do you add them to the general implementation?

[rkc, 2016-04-13 22:47:25]

@Giovanni: I was going to, but someone beat me to it! :)

https://codereview.chromium.org/1872943002/diff/1/device/bluetooth/bluetooth_...
File device/bluetooth/bluetooth_gatt_characteristic_bluez.cc (right):

https://codereview.chromium.org/1872943002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_gatt_characteristic_bluez.cc:50:
device::BluetoothUUID BluetoothGattCharacteristicBlueZ::GetUUID() const {
On 2016/04/13 14:40:27, ortuno wrote:
> It looks like these implementation are specific to remote characteristic i.e.
> you shouldn't need to get the Characteristic client to get the UUID of a local
> characteristic. Why do you add them to the general implementation?

Done.

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

https://codereview.chromium.org/1872943002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_gatt_descriptor_bluez.h:30: //
BluetoothGattDescriptor for remote GATT characteristic descriptors for
On 2016/04/12 18:05:46, scheib wrote:
> remote and local

Done.

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

https://codereview.chromium.org/1872943002/diff/1/device/bluetooth/bluetooth_...
device/bluetooth/bluetooth_gatt_service.h:146: using ErrorCallback =
base::Callback<void(const std::string& error_name,
On 2016/04/12 18:05:46, scheib wrote:
> error_name should not be a string. We should not be exposing DBUS details in
the
> cross platform interface. This interface should explicitly document what may
be
> returned. A descriptive string for logging/output to console is fine, but any
> code that program logic may depend on should be an enumeration (or nothing -
> just the implicit error of the error callback).

Done.

[rkc, 2016-04-13 22:47:41]

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

[commit-bot: I haz the power, 2016-04-13 22:48:20]

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

[commit-bot: I haz the power, 2016-04-13 23:03:28]

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

[commit-bot: I haz the power, 2016-04-13 23:03:29]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[scheib, 2016-04-13 23:06:28]

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

https://codereview.chromium.org/1872943002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:67: friend class
BluetoothRemoteGattServiceBlueZ;
Instead of friending, just make the members protected?

[rkc, 2016-04-14 00:23:49]

Fixed the build errors, I think :)

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

https://codereview.chromium.org/1872943002/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:67: friend class
BluetoothRemoteGattServiceBlueZ;
On 2016/04/13 23:06:28, scheib wrote:
> Instead of friending, just make the members protected?

The classes aren't in this class's hierarchy.
I did realize that we don't need all three classes actually, since only the
remote class needs to directly delete characteristics (which requires this
friending).

Another option is to make the destructor public; but I'm trying to not change
too much of how things are done with this refactor. There is already enough
moved code :)


Removed the unneeded friend classes.

[rkc, 2016-04-14 00:23:58]

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

[commit-bot: I haz the power, 2016-04-14 00:24:34]

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

[scheib, 2016-04-14 00:58:11]

LGTM

[chromium-reviews, 2016-04-14 01:06:02]

Please wait for my review

On Wed, Apr 13, 2016, 5:58 PM <scheib@chromium.org> wrote:

> LGTM
>
> https://codereview.chromium.org/1872943002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-04-14 01:15:11]

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

[commit-bot: I haz the power, 2016-04-14 01:15:12]

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

[rkc, 2016-04-14 03:31:19]

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

[commit-bot: I haz the power, 2016-04-14 03:31:45]

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

[commit-bot: I haz the power, 2016-04-14 05:48:53]

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

[commit-bot: I haz the power, 2016-04-14 05:48:54]

Dry run: This issue passed the CQ dry run.

[ortuno, 2016-04-14 17:42:36]

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth.gyp (right):

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth.gyp:143: 'bluetooth_gatt_characteristic_bluez.cc',
The indentation seems wrong? Or is it just the review tool?

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.cc:40: for
(DescriptorMap::const_iterator iter = descriptors_.begin();
Optional if you want to do some clean up:

for (const auto& pair : descriptors_) {
  descriptors.push_back(pair.second); // pair.second.get() if you use a
unique_ptr map above.
}

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
It's 2016!

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:8: #include <stddef.h>
Don't see any NULLs or size_ts.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:9: #include <stdint.h>
Same here no ints.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:11: #include <map>
Some of these are no longer needed as well. Please clean up.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:21: #include
"device/bluetooth/bluetooth_uuid.h"
No UUIDs.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:37: // that use BlueZ.
Could you mention that this class implements common methods between Remote And
Local Characteristics.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:62:
std::map<dbus::ObjectPath, BluetoothGattDescriptorBlueZ*>;
optional: Any reason why you leave the deletion of descriptors up to the local
and remote implementations? If not please consider using unique_ptr here.

If end up not using unique_ptrs please document that any classes that add
objects to this map
are in charge of manually freeing them.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:65: // The GATT service
this GATT characteristic belongs to.
nit: The GATT service that owns this characteristic.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:76:
base::WeakPtrFactory<BluetoothGattCharacteristicBlueZ> weak_ptr_factory_;
Do you still need this for the general implementation? You also have one in each
implementation.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_bluez.h:44: void
WriteRemoteDescriptor(const std::vector<uint8_t>& new_value,
I think you need a separate class for local and remote. I count 4 methods that
will have different implementations (5 if you include GetPermissions which will
also have a different implementation).

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service.h:219: virtual void Register(const
base::Closure& callback,
I thought these were going to be moved to the adapter?

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
It's 2016!

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.cc:15: #include
"device/bluetooth/dbus/bluez_dbus_manager.h"
Please clean up includes.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.cc:44: for
(CharacteristicMap::const_iterator iter = characteristics_.begin();
optional: use for(const auto& pair : characteristics_)

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
It's 2016!

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:8: #include <stdint.h>
Not necessary.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:31: // for GATT services on
platforms that use BlueZ.
Please mention that this class implements methods common to Local and Remote
Services.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:44: const dbus::ObjectPath&
object_path() const { return object_path_; }
hmm there are a lot of variables called "object_path", maybe this should be
called GetObjectPath to make it less confusing.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:59: std::map<dbus::ObjectPath,
BluetoothGattCharacteristicBlueZ*>;
optional: Consider using unique_ptrs. If not then document that classes need to
manually free the objects.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
It's 2016!

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:40: static
std::vector<uint8_t> temp;
I thought we tried to avoid static because of the destruction overhead?

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:70: // Doesn't
apply for a local characteristic.
Please choose a pattern a follow it throughout:

1. Call NOTIMLPEMENTED with a comment on top and not run the callbacks. (This
what you do here.)
or 2. Log that the function is not supported on local objects and run callbacks.
See Register RemoteService.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
It's 2016!

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:8: #include
<stddef.h>
I don't think you need this one.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:32: //
BluetoothGattCharacteristic for remote GATT characteristics for platforms
Fix description.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:43: bool
AddDescriptor(device::BluetoothGattDescriptor* descriptor) override;
I'm guessing this will be removed base on the design doc.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:62:
base::WeakPtrFactory<BluetoothLocalGattCharacteristicBlueZ> weak_ptr_factory_;
You don't seem to use this.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
2016

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:13: #include
"device/bluetooth/dbus/bluez_dbus_manager.h"
Clean up includes here as well.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:36: bool
BluetoothLocalGattServiceBlueZ::IsPrimary() const {
This should be true right? There is no way to construct a secondary service.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:45: bool
BluetoothLocalGattServiceBlueZ::AddCharacteristic(
I'm guessing these Add functions will be removed based on the design doc?

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:57: void
BluetoothLocalGattServiceBlueZ::Register(
I thought you were moving this to the adapter?

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
It's 2016!

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.h:8: #include <stdint.h>
I don't see any ints.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.h:29: class
BluetoothLocalGattCharacteristicBlueZ;
I don't think you are using this anywhere?

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.h:30: class
BluetoothLocalGattDescriptorBlueZ;
Same here.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_bluez.h:15: #include
<vector>
Please clean up includes.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_service_bluez.h:12: #include <vector>
Clean up headers.

[rkc, 2016-04-14 19:27:28]

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth.gyp (right):

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth.gyp:143: 'bluetooth_gatt_characteristic_bluez.cc',
On 2016/04/14 17:42:34, ortuno wrote:
> The indentation seems wrong? Or is it just the review tool?

Seems fine here.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/04/14 17:42:34, ortuno wrote:
> 2016

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.cc:40: for
(DescriptorMap::const_iterator iter = descriptors_.begin();
On 2016/04/14 17:42:34, ortuno wrote:
> Optional if you want to do some clean up:
> 
> for (const auto& pair : descriptors_) {
>   descriptors.push_back(pair.second); // pair.second.get() if you use a
> unique_ptr map above.
> }

I would rather do C++11 cleanups only on new code. Doing cleanups on old code is
a never-ending task.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/04/14 17:42:34, ortuno wrote:
> It's 2016!

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:8: #include <stddef.h>
On 2016/04/14 17:42:34, ortuno wrote:
> Don't see any NULLs or size_ts.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:9: #include <stdint.h>
On 2016/04/14 17:42:35, ortuno wrote:
> Same here no ints.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:11: #include <map>
On 2016/04/14 17:42:34, ortuno wrote:
> Some of these are no longer needed as well. Please clean up.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:21: #include
"device/bluetooth/bluetooth_uuid.h"
On 2016/04/14 17:42:34, ortuno wrote:
> No UUIDs.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:37: // that use BlueZ.
On 2016/04/14 17:42:35, ortuno wrote:
> Could you mention that this class implements common methods between Remote And
> Local Characteristics.

Seems like a superfluous comment. To implement common functionality between all
derived classes is one of the main purposes of a base class.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:62:
std::map<dbus::ObjectPath, BluetoothGattDescriptorBlueZ*>;
On 2016/04/14 17:42:34, ortuno wrote:
> optional: Any reason why you leave the deletion of descriptors up to the local
> and remote implementations? If not please consider using unique_ptr here.
> 
> If end up not using unique_ptrs please document that any classes that add
> objects to this map
> are in charge of manually freeing them.

This is a refactor. The purpose is to try to move code around and only change
what really needs to be.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:65: // The GATT service
this GATT characteristic belongs to.
On 2016/04/14 17:42:34, ortuno wrote:
> nit: The GATT service that owns this characteristic.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:76:
base::WeakPtrFactory<BluetoothGattCharacteristicBlueZ> weak_ptr_factory_;
On 2016/04/14 17:42:34, ortuno wrote:
> Do you still need this for the general implementation? You also have one in
each
> implementation.

I do not, thanks for catching this.
Done.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_bluez.h:44: void
WriteRemoteDescriptor(const std::vector<uint8_t>& new_value,
On 2016/04/14 17:42:35, ortuno wrote:
> I think you need a separate class for local and remote. I count 4 methods that
> will have different implementations (5 if you include GetPermissions which
will
> also have a different implementation).

This class is small enough that leaving some remote functionality in the base
class seems fine. If the functionality does start to get quite a bit, I'll
refactor it then.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service.h:219: virtual void Register(const
base::Closure& callback,
On 2016/04/14 17:42:35, ortuno wrote:
> I thought these were going to be moved to the adapter?

All in good time. This is a refactor. The purpose is to try to move code around
and only change what really needs to be.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/04/14 17:42:35, ortuno wrote:
> It's 2016!

Indeed it is. Which is why I have changed this in all the files, including the
few places you forgot to point it out! :)
Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.cc:15: #include
"device/bluetooth/dbus/bluez_dbus_manager.h"
On 2016/04/14 17:42:35, ortuno wrote:
> Please clean up includes.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.cc:44: for
(CharacteristicMap::const_iterator iter = characteristics_.begin();
On 2016/04/14 17:42:35, ortuno wrote:
> optional: use for(const auto& pair : characteristics_)

Please see my comments elsewhere about this being a refactor.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/04/14 17:42:35, ortuno wrote:
> It's 2016!

It sure is.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:8: #include <stdint.h>
On 2016/04/14 17:42:35, ortuno wrote:
> Not necessary.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:31: // for GATT services on
platforms that use BlueZ.
On 2016/04/14 17:42:35, ortuno wrote:
> Please mention that this class implements methods common to Local and Remote
> Services.

Please see my comment elsewhere regarding this.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:44: const dbus::ObjectPath&
object_path() const { return object_path_; }
On 2016/04/14 17:42:35, ortuno wrote:
> hmm there are a lot of variables called "object_path", maybe this should be
> called GetObjectPath to make it less confusing.

Please see my other comments about this being a refactor.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:59: std::map<dbus::ObjectPath,
BluetoothGattCharacteristicBlueZ*>;
On 2016/04/14 17:42:35, ortuno wrote:
> optional: Consider using unique_ptrs. If not then document that classes need
to
> manually free the objects.

Please see my other comments about this being a refactor.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2016/04/14 17:42:35, ortuno wrote:
> It's 2016!

That is true. A whole two years after 2014. Gosh, how does time fly!

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:40: static
std::vector<uint8_t> temp;
On 2016/04/14 17:42:35, ortuno wrote:
> I thought we tried to avoid static because of the destruction overhead?

This class is not being constructed, let alone destructed.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:70: // Doesn't
apply for a local characteristic.
On 2016/04/14 17:42:35, ortuno wrote:
> Please choose a pattern a follow it throughout:
> 
> 1. Call NOTIMLPEMENTED with a comment on top and not run the callbacks. (This
> what you do here.)
> or 2. Log that the function is not supported on local objects and run
callbacks.
> See Register RemoteService.

I didn't change the code in the remote service Register method since that is
what was already there. Please see my comments about this being a refactor.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2016/04/14 17:42:36, ortuno wrote:
> It's 2016!

..and, 2016 is the year that comes after 2015.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:8: #include
<stddef.h>
On 2016/04/14 17:42:36, ortuno wrote:
> I don't think you need this one.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:32: //
BluetoothGattCharacteristic for remote GATT characteristics for platforms
On 2016/04/14 17:42:35, ortuno wrote:
> Fix description.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:43: bool
AddDescriptor(device::BluetoothGattDescriptor* descriptor) override;
On 2016/04/14 17:42:36, ortuno wrote:
> I'm guessing this will be removed base on the design doc.

Yes, it will.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.h:62:
base::WeakPtrFactory<BluetoothLocalGattCharacteristicBlueZ> weak_ptr_factory_;
On 2016/04/14 17:42:36, ortuno wrote:
> You don't seem to use this.

Done.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/04/14 17:42:36, ortuno wrote:
> 2016

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:13: #include
"device/bluetooth/dbus/bluez_dbus_manager.h"
On 2016/04/14 17:42:36, ortuno wrote:
> Clean up includes here as well.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:36: bool
BluetoothLocalGattServiceBlueZ::IsPrimary() const {
On 2016/04/14 17:42:36, ortuno wrote:
> This should be true right? There is no way to construct a secondary service.

This method doesn't apply to local characteristics.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:45: bool
BluetoothLocalGattServiceBlueZ::AddCharacteristic(
On 2016/04/14 17:42:36, ortuno wrote:
> I'm guessing these Add functions will be removed based on the design doc?

Acknowledged.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:57: void
BluetoothLocalGattServiceBlueZ::Register(
On 2016/04/14 17:42:36, ortuno wrote:
> I thought you were moving this to the adapter?

Please see my other comments regarding this being a refactor.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2016/04/14 17:42:36, ortuno wrote:
> It's 2016!

..and next year it will be 2017!

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.h:8: #include <stdint.h>
On 2016/04/14 17:42:36, ortuno wrote:
> I don't see any ints.

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.h:29: class
BluetoothLocalGattCharacteristicBlueZ;
On 2016/04/14 17:42:36, ortuno wrote:
> I don't think you are using this anywhere?

Done.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.h:30: class
BluetoothLocalGattDescriptorBlueZ;
On 2016/04/14 17:42:36, ortuno wrote:
> Same here.

Done.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_bluez.h:15: #include
<vector>
On 2016/04/14 17:42:36, ortuno wrote:
> Please clean up includes.

Done.
Also cleaned them up in the few places you missed pointing them out!:)
Done.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_service_bluez.h:12: #include <vector>
On 2016/04/14 17:42:36, ortuno wrote:
> Clean up headers.

Done.

[ortuno, 2016-04-14 21:17:38]

lgtm bar a couple of comments.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth.gyp (right):

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth.gyp:143: 'bluetooth_gatt_characteristic_bluez.cc',
On 2016/04/14 at 19:27:26, Rahul Chaturvedi wrote:
> On 2016/04/14 17:42:34, ortuno wrote:
> > The indentation seems wrong? Or is it just the review tool?
> 
> Seems fine here.

You used tabs instead of spaces. Seems fixed in latests PS though.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:62:
std::map<dbus::ObjectPath, BluetoothGattDescriptorBlueZ*>;
On 2016/04/14 at 19:27:26, Rahul Chaturvedi wrote:
> On 2016/04/14 17:42:34, ortuno wrote:
> > optional: Any reason why you leave the deletion of descriptors up to the
local
> > and remote implementations? If not please consider using unique_ptr here.
> > 
> > If end up not using unique_ptrs please document that any classes that add
> > objects to this map
> > are in charge of manually freeing them.
> 
> This is a refactor. The purpose is to try to move code around and only change
what really needs to be.

OK. I still think a comment that points out that if classes add to this map they
need to free them manually is needed, but up to you.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_bluez.h:44: void
WriteRemoteDescriptor(const std::vector<uint8_t>& new_value,
On 2016/04/14 at 19:27:27, Rahul Chaturvedi wrote:
> On 2016/04/14 17:42:35, ortuno wrote:
> > I think you need a separate class for local and remote. I count 4 methods
that
> > will have different implementations (5 if you include GetPermissions which
will
> > also have a different implementation).
> 
> This class is small enough that leaving some remote functionality in the base
class seems fine. If the functionality does start to get quite a bit, I'll
refactor it then.

I have two particular problems in mind:

1. GetUUID and GetValue (and probably GetPermissions) will have different
implementations. How are the two implementations going to live in the same file?
2. ReadRemoteDescriptor and WriteRemoteDescriptor should probably call the error
callback on local characteristics. How are you going to do this?

Anyway nothing to do here but those two issues point to there being two
different implementations.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:59: std::map<dbus::ObjectPath,
BluetoothGattCharacteristicBlueZ*>;
On 2016/04/14 at 19:27:27, Rahul Chaturvedi wrote:
> On 2016/04/14 17:42:35, ortuno wrote:
> > optional: Consider using unique_ptrs. If not then document that classes need
to
> > manually free the objects.
> 
> Please see my other comments about this being a refactor.

I still think you need to document that classes that add to the map need to free
the objects manually, but up to you.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:40: static
std::vector<uint8_t> temp;
On 2016/04/14 at 19:27:27, Rahul Chaturvedi wrote:
> On 2016/04/14 17:42:35, ortuno wrote:
> > I thought we tried to avoid static because of the destruction overhead?
> 
> This class is not being constructed, let alone destructed.

I'm thinking of this part of the style guide:
http://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

I might be reading it wrong, but I've been told in the past to avoid "static"
for variables.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:70: // Doesn't
apply for a local characteristic.
On 2016/04/14 19:27:27, Rahul Chaturvedi wrote:
> On 2016/04/14 17:42:35, ortuno wrote:
> > Please choose a pattern a follow it throughout:
> > 
> > 1. Call NOTIMLPEMENTED with a comment on top and not run the callbacks.
(This
> > what you do here.)
> > or 2. Log that the function is not supported on local objects and run
> callbacks.
> > See Register RemoteService.
> 
> I didn't change the code in the remote service Register method since that is
> what was already there. Please see my comments about this being a refactor.

nit: I understand the argument about the refactoring but this seems like new
code, no? All other code e.g. bluetooth_remote_gatt_characteristic_bluez.cc and
bluetooth_remote_gatt_service_bluez.cc call the error callbacks for methods
meant only for local objects. If you are planning on changing those methods then
OK to do in a follow up patch but if this is the code that will live in
BluetoothLocalGattCharacteristicBluez forever then I think it should conform to
existing patterns.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:36: bool
BluetoothLocalGattServiceBlueZ::IsPrimary() const {
On 2016/04/14 at 19:27:28, Rahul Chaturvedi wrote:
> On 2016/04/14 17:42:36, ortuno wrote:
> > This should be true right? There is no way to construct a secondary service.
> 
> This method doesn't apply to local characteristics.

But this is not a characteristic :P Anyway I realize that it's just not
implemented.

[rkc, 2016-04-14 21:59:38]

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
File device/bluetooth/bluetooth.gyp (right):

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth.gyp:143: 'bluetooth_gatt_characteristic_bluez.cc',
On 2016/04/14 21:17:37, ortuno wrote:
> On 2016/04/14 at 19:27:26, Rahul Chaturvedi wrote:
> > On 2016/04/14 17:42:34, ortuno wrote:
> > > The indentation seems wrong? Or is it just the review tool?
> > 
> > Seems fine here.
> 
> You used tabs instead of spaces. Seems fixed in latests PS though.

Acknowledged.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_characteristic_bluez.h:62:
std::map<dbus::ObjectPath, BluetoothGattDescriptorBlueZ*>;
On 2016/04/14 21:17:38, ortuno wrote:
> On 2016/04/14 at 19:27:26, Rahul Chaturvedi wrote:
> > On 2016/04/14 17:42:34, ortuno wrote:
> > > optional: Any reason why you leave the deletion of descriptors up to the
> local
> > > and remote implementations? If not please consider using unique_ptr here.
> > > 
> > > If end up not using unique_ptrs please document that any classes that add
> > > objects to this map
> > > are in charge of manually freeing them.
> > 
> > This is a refactor. The purpose is to try to move code around and only
change
> what really needs to be.
> 
> OK. I still think a comment that points out that if classes add to this map
they
> need to free them manually is needed, but up to you.

Done.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_descriptor_bluez.h:44: void
WriteRemoteDescriptor(const std::vector<uint8_t>& new_value,
On 2016/04/14 21:17:38, ortuno wrote:
> On 2016/04/14 at 19:27:27, Rahul Chaturvedi wrote:
> > On 2016/04/14 17:42:35, ortuno wrote:
> > > I think you need a separate class for local and remote. I count 4 methods
> that
> > > will have different implementations (5 if you include GetPermissions which
> will
> > > also have a different implementation).
> > 
> > This class is small enough that leaving some remote functionality in the
base
> class seems fine. If the functionality does start to get quite a bit, I'll
> refactor it then.
> 
> I have two particular problems in mind:
> 
> 1. GetUUID and GetValue (and probably GetPermissions) will have different
> implementations. How are the two implementations going to live in the same
file?
> 2. ReadRemoteDescriptor and WriteRemoteDescriptor should probably call the
error
> callback on local characteristics. How are you going to do this?
> 
> Anyway nothing to do here but those two issues point to there being two
> different implementations.

We can have two implementations by branching based on the value of the is_local_
variable.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_gatt_service_bluez.h:59: std::map<dbus::ObjectPath,
BluetoothGattCharacteristicBlueZ*>;
On 2016/04/14 21:17:38, ortuno wrote:
> On 2016/04/14 at 19:27:27, Rahul Chaturvedi wrote:
> > On 2016/04/14 17:42:35, ortuno wrote:
> > > optional: Consider using unique_ptrs. If not then document that classes
need
> to
> > > manually free the objects.
> > 
> > Please see my other comments about this being a refactor.
> 
> I still think you need to document that classes that add to the map need to
free
> the objects manually, but up to you.

Done.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:40: static
std::vector<uint8_t> temp;
On 2016/04/14 21:17:38, ortuno wrote:
> On 2016/04/14 at 19:27:27, Rahul Chaturvedi wrote:
> > On 2016/04/14 17:42:35, ortuno wrote:
> > > I thought we tried to avoid static because of the destruction overhead?
> > 
> > This class is not being constructed, let alone destructed.
> 
> I'm thinking of this part of the style guide:
> http://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
> 
> I might be reading it wrong, but I've been told in the past to avoid "static"
> for variables.

This doesn't really violate the spirit of the rule in the style guide, since
this object will *never* get constructed.
Since you seem to feel quite strongly about this, I've changed this to be a
one-time initialized pointer which will never get freed, as the style guide
recommends.

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_characteristic_bluez.cc:70: // Doesn't
apply for a local characteristic.
On 2016/04/14 21:17:38, ortuno wrote:
> On 2016/04/14 19:27:27, Rahul Chaturvedi wrote:
> > On 2016/04/14 17:42:35, ortuno wrote:
> > > Please choose a pattern a follow it throughout:
> > > 
> > > 1. Call NOTIMLPEMENTED with a comment on top and not run the callbacks.
> (This
> > > what you do here.)
> > > or 2. Log that the function is not supported on local objects and run
> > callbacks.
> > > See Register RemoteService.
> > 
> > I didn't change the code in the remote service Register method since that is
> > what was already there. Please see my comments about this being a refactor.
> 
> nit: I understand the argument about the refactoring but this seems like new
> code, no? All other code e.g. bluetooth_remote_gatt_characteristic_bluez.cc
and
> bluetooth_remote_gatt_service_bluez.cc call the error callbacks for methods
> meant only for local objects. If you are planning on changing those methods
then
> OK to do in a follow up patch but if this is the code that will live in
> BluetoothLocalGattCharacteristicBluez forever then I think it should conform
to
> existing patterns.

Once I start implementing this class, I will have a better idea of what I want
to go with. I'll decide and fix this then.

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

https://codereview.chromium.org/1872943002/diff/80001/device/bluetooth/blueto...
device/bluetooth/bluetooth_local_gatt_service_bluez.cc:36: bool
BluetoothLocalGattServiceBlueZ::IsPrimary() const {
On 2016/04/14 21:17:38, ortuno wrote:
> On 2016/04/14 at 19:27:28, Rahul Chaturvedi wrote:
> > On 2016/04/14 17:42:36, ortuno wrote:
> > > This should be true right? There is no way to construct a secondary
service.
> > 
> > This method doesn't apply to local characteristics.
> 
> But this is not a characteristic :P Anyway I realize that it's just not
> implemented.

Acknowledged.

[rkc, 2016-04-14 22:00:14]

The CQ bit was checked by rkc@chromium.org

[rkc, 2016-04-14 22:00:14]

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

[commit-bot: I haz the power, 2016-04-14 22:00:49]

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

[commit-bot: I haz the power, 2016-04-14 22:13:54]

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

[commit-bot: I haz the power, 2016-04-14 22:13:55]

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

[rkc, 2016-04-14 22:59:56]

The CQ bit was checked by rkc@chromium.org

[commit-bot: I haz the power, 2016-04-14 23:00:15]

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

[commit-bot: I haz the power, 2016-04-15 01:53:26]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-15 01:54:27]

Description was changed from

==========
Add support for local services/characteristics/descriptors.

We currently only support remote services, characteristics and descriptors.
This prevents us from implementing GATT server support. This CL refactors
the current code to allow us to have remote and local versions of these
attributes.

BluetoothGattDescriptors wasn't refactored into multiple classes since the
only difference between local and remote was one flag. In the future, we
might even remove this flag (at the moment it doesn't seem to have any
purpose).

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

to

==========
Add support for local services/characteristics/descriptors.

We currently only support remote services, characteristics and descriptors.
This prevents us from implementing GATT server support. This CL refactors
the current code to allow us to have remote and local versions of these
attributes.

BluetoothGattDescriptors wasn't refactored into multiple classes since the
only difference between local and remote was one flag. In the future, we
might even remove this flag (at the moment it doesn't seem to have any
purpose).

R=scheib@chromium.org
BUG=601935

Committed: https://crrev.com/4e63c0523e9e45a715f00371bbee0e93b95516c0
Cr-Commit-Position: refs/heads/master@{#387516}
==========

[commit-bot: I haz the power, 2016-04-15 01:54:28]

Patchset 7 (id:??) landed as
https://crrev.com/4e63c0523e9e45a715f00371bbee0e93b95516c0
Cr-Commit-Position: refs/heads/master@{#387516}

[Lei Zhang, 2016-04-15 06:35:38]

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

[Lei Zhang, 2016-04-15 06:35:39]

This CL added static initializers and made
https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/18235 go red.
I think it's because you included iostream.

[Lei Zhang, 2016-04-15 06:49:07]

On 2016/04/15 06:35:39, Lei Zhang wrote:
> This CL added static initializers and made
> https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/18235 go
red.
> I think it's because you included iostream.

https://codereview.chromium.org/1893553002