Bluetooth: macOS: Adding logs

Bluetooth: macOS: Adding logs

Adding logs for macOS adapter, devices, services, characteristics and descriptors.

BUG=685972
Review-Url: https://codereview.chromium.org/2745983003
Cr-Commit-Position: refs/heads/master@{#458691}
Committed: https://chromium.googlesource.com/chromium/src/+/234aec4f17e0b08204b0e8df547ab7426100b404

[jlebel, 2017-03-12 23:18:24]

Patchset #1 (id:1) has been deleted

[jlebel, 2017-03-12 23:29:49]

Patchset #1 (id:20001) has been deleted

[jlebel, 2017-03-12 23:42:57]

Patchset #1 (id:40001) has been deleted

[jlebel, 2017-03-13 00:26:10]

Description was changed from

==========
Adding logs

BUG=685972
==========

to

==========
bluetooth: macOS: Adding logs

BUG=685972
==========

[jlebel, 2017-03-13 00:26:16]

Patchset #1 (id:60001) has been deleted

[jlebel, 2017-03-13 00:33:02]

Description was changed from

==========
bluetooth: macOS: Adding logs

BUG=685972
==========

to

==========
bluetooth: macOS: Adding logs

Adding logs for macOS adapter, devices, services, characteristics and
descriptors.

BUG=685972
==========

[jlebel, 2017-03-13 00:35:36]

Description was changed from

==========
bluetooth: macOS: Adding logs

Adding logs for macOS adapter, devices, services, characteristics and
descriptors.

BUG=685972
==========

to

==========
Bluetooth: macOS: Adding logs

Adding logs for macOS adapter, devices, services, characteristics and
descriptors.

BUG=685972
==========

[jlebel, 2017-03-13 00:55:45]

Patchset #1 (id:80001) has been deleted

[jlebel, 2017-03-13 18:56:07]

Patchset #1 (id:100001) has been deleted

[jlebel, 2017-03-13 19:34:49]

Patchset #1 (id:120001) has been deleted

[jlebel, 2017-03-13 19:35:59]

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

[jlebel, 2017-03-13 19:35:59]

Hello Giovanni,

Can you review this patch about adding more logs to the macOS implementation?

Thanks,

[ortuno, 2017-03-14 00:34:34]

Last time we added logs we accidentally adding a nullptr dereference. Could you
run device_unittests with logging enabled to make sure we are not breaking
anything?

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:39: std::string
GetCentralManagerStateString(CBCentralManagerState state) {
I feel bad for adding these strings to the binary and increasing its size. It
doesn't seem to hard to just look up the error number. What do you think?

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:549: VLOG(1) << device_mac->ToString()
<< ": Device updated with "
Make this a VLOG(3). We are going to get tens of these per second.

Also remove the VLOG at
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_low_energy_di...

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:632: VLOG(1) <<
"RetrieveGattConnectedDevicesWithService " << uuid->value();
uuid could be null so let's do the logging inside the if/else:

if (!uuid) {
  VLOG(1) << "Retrieving all connected devices";
  // ...
} else {
  VLOG(1) << "Retrieving connected devices with UUID: " <<
uuid->canonical_value();
  // ...
}

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:654: VLOG(1) << "  " <<
device_mac->ToString();
Please follow the same pattern for all logs:

VLOG(1) << device_mac->ToString() << ": New Connected Device.";

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.h (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.h:129: // Returns a description
for the device, that should be used for logs.
I should have suggested this before but I wasn't sure if you were planning on
using ToString outside of logging.

Given that all uses of the method are for logging we can add an implementation
of the << operator:

DEVICE_BLUETOOTH_EXPORT std::ostream& operator<<(std::ostream& out,
                                          const BluetoothDevice& device);

Then you can just do:

VLOG(1) << *this;

See
https://cs.chromium.org/chromium/src/content/common/bluetooth/web_bluetooth_d...

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:232:
adapter_->NotifyGattServiceAdded(gatt_service);
I like this change but I would keep it out of this CL since it's pretty
unrelated.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:362: VLOG(1) << ToString()
<< ": DidDiscoverDescriptors, pending count "
This log seems incorrect. Why do we say DidDiscoverDescriptors?

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:417: VLOG(1) << ToString()
<< ": Did disconnect.";
We are double logging here, bluetooth adapter already logs this. We should
decide where we want to actually do the logging. Should it be at the
bluetooth_adapter layer or at the bluetooth_device layer. Then move all the logs
to the layer we decide. Which layer do you think we should have the logging on?

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:234: VLOG(1) <<
ToString() << ": Unsubscribe to characteristic.";
Unsubscribe from characteristic.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:257: VLOG(1) <<
ToString() << ": Update value.";
I would get rid of this one and log each case separately. See below

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:271: UpdateValue();
Read request arrived.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:274: UpdateValue();
Notification arrived.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:288: VLOG(1) <<
ToString() << ": Update value.";
This seems a bit unnecessary. We are already logging function that call this.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:303: VLOG(1) <<
ToString() << ": Write value.";
Move this all the way down and change it to: "Write value succeeded."

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:448:
GetIdentifier() + ", service " + gatt_service_->GetIdentifier() + ">";
A UUID is more useful than the identifier.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_descriptor_mac.mm:79: ", characteristic "
+ gatt_characteristic_->GetIdentifier() + ">";
Same here. The UUID would be more useful.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_service_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_service_mac.mm:223: ", device " +
bluetooth_device_mac_->GetIdentifier() + ">";
Same here: UUID might be more useful.

[ortuno, 2017-03-14 02:12:01]

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:434: const char* name_cstr =
name ? name->c_str() : "";
You can use GetNameForDisplay() to always get a name back.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:435: return
std::string("<BluetoothLowEnergyDeviceMac ") + GetIdentifier() +
Could you use a StringPiece here?

[jlebel, 2017-03-14 09:49:59]

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

[commit-bot: I haz the power, 2017-03-14 09:50:19]

Dry run: 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, 2017-03-14 10:52:40]

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

[commit-bot: I haz the power, 2017-03-14 10:52:41]

Dry run: This issue passed the CQ dry run.

[jlebel, 2017-03-15 01:08:41]

Hello Giovanni,

I've just tested the unit tests for this patch using "--vmodule=*bluetooth*=9",
on my computer.

Jérôme,

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:39: std::string
GetCentralManagerStateString(CBCentralManagerState state) {
On 2017/03/14 00:34:33, ortuno wrote:
> I feel bad for adding these strings to the binary and increasing its size. It
> doesn't seem to hard to just look up the error number. What do you think?

This should not be a big source of problems, so we should not need read those
values often.
I guess I can remove that function.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:549: VLOG(1) << device_mac->ToString()
<< ": Device updated with "
On 2017/03/14 00:34:33, ortuno wrote:
> Make this a VLOG(3). We are going to get tens of these per second.
> 
> Also remove the VLOG at
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_low_energy_di...

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:632: VLOG(1) <<
"RetrieveGattConnectedDevicesWithService " << uuid->value();
On 2017/03/14 00:34:33, ortuno wrote:
> uuid could be null so let's do the logging inside the if/else:
> 
> if (!uuid) {
>   VLOG(1) << "Retrieving all connected devices";
>   // ...
> } else {
>   VLOG(1) << "Retrieving connected devices with UUID: " <<
> uuid->canonical_value();
>   // ...
> }

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_adapter_mac.mm:654: VLOG(1) << "  " <<
device_mac->ToString();
On 2017/03/14 00:34:33, ortuno wrote:
> Please follow the same pattern for all logs:
> 
> VLOG(1) << device_mac->ToString() << ": New Connected Device.";

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.h (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.h:129: // Returns a description
for the device, that should be used for logs.
On 2017/03/14 00:34:33, ortuno wrote:
> I should have suggested this before but I wasn't sure if you were planning on
> using ToString outside of logging.
> 
> Given that all uses of the method are for logging we can add an implementation
> of the << operator:
> 
> DEVICE_BLUETOOTH_EXPORT std::ostream& operator<<(std::ostream& out,
>                                           const BluetoothDevice& device);
> 
> Then you can just do:
> 
> VLOG(1) << *this;
> 
> See
>
https://cs.chromium.org/chromium/src/content/common/bluetooth/web_bluetooth_d...

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:232:
adapter_->NotifyGattServiceAdded(gatt_service);
On 2017/03/14 00:34:33, ortuno wrote:
> I like this change but I would keep it out of this CL since it's pretty
> unrelated.

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:362: VLOG(1) << ToString()
<< ": DidDiscoverDescriptors, pending count "
On 2017/03/14 00:34:33, ortuno wrote:
> This log seems incorrect. Why do we say DidDiscoverDescriptors?

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:417: VLOG(1) << ToString()
<< ": Did disconnect.";
On 2017/03/14 00:34:33, ortuno wrote:
> We are double logging here, bluetooth adapter already logs this. We should
> decide where we want to actually do the logging. Should it be at the
> bluetooth_adapter layer or at the bluetooth_device layer. Then move all the
logs
> to the layer we decide. Which layer do you think we should have the logging
on?

I think this should be done on the device layer. That's what I've done for the
rest:
I've added logs when the action is really done, not when passing the event from
one layer to the other.
I also had the issue with BluetoothDevice::DidFailToConnectGatt()

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:434: const char* name_cstr =
name ? name->c_str() : "";
On 2017/03/14 02:12:00, ortuno wrote:
> You can use GetNameForDisplay() to always get a name back.

I can't run the unit tests with GetNameForDisplay(). DCHECK at
resource_bundle.cc:246 fails.
https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?q=re...

Called by:
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.cc?q=b...

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:435: return
std::string("<BluetoothLowEnergyDeviceMac ") + GetIdentifier() +
On 2017/03/14 02:12:00, ortuno wrote:
> Could you use a StringPiece here?

I'm not sure to understand how to use StringPiece and what is the benefit of it.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:234: VLOG(1) <<
ToString() << ": Unsubscribe to characteristic.";
On 2017/03/14 00:34:33, ortuno wrote:
> Unsubscribe from characteristic.

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:257: VLOG(1) <<
ToString() << ": Update value.";
On 2017/03/14 00:34:33, ortuno wrote:
> I would get rid of this one and log each case separately. See below

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:271: UpdateValue();
On 2017/03/14 00:34:33, ortuno wrote:
> Read request arrived.

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:274: UpdateValue();
On 2017/03/14 00:34:33, ortuno wrote:
> Notification arrived.

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:288: VLOG(1) <<
ToString() << ": Update value.";
On 2017/03/14 00:34:33, ortuno wrote:
> This seems a bit unnecessary. We are already logging function that call this.

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:303: VLOG(1) <<
ToString() << ": Write value.";
On 2017/03/14 00:34:33, ortuno wrote:
> Move this all the way down and change it to: "Write value succeeded."

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:448:
GetIdentifier() + ", service " + gatt_service_->GetIdentifier() + ">";
On 2017/03/14 00:34:33, ortuno wrote:
> A UUID is more useful than the identifier.

The identifier contains the uuid and the memory address. I chose that to make
sure we don't have confusion between 2 characteristics.
But I can change it to UUID if you prefer.

[scheib, 2017-03-21 20:18:32]

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

[commit-bot: I haz the power, 2017-03-21 20:19:11]

Dry run: 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, 2017-03-21 21:22:17]

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

[commit-bot: I haz the power, 2017-03-21 21:22:19]

Dry run: This issue passed the CQ dry run.

[scheib, 2017-03-21 21:43:31]

lgtm, with at least the TODO comment below and perhaps best to fix the
::ToString() methods to explicitly use UUID and pointer.

Other notes optional.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:417: VLOG(1) << ToString()
<< ": Did disconnect.";
On 2017/03/15 01:08:40, jlebel wrote:
> On 2017/03/14 00:34:33, ortuno wrote:
> > We are double logging here, bluetooth adapter already logs this. We should
> > decide where we want to actually do the logging. Should it be at the
> > bluetooth_adapter layer or at the bluetooth_device layer. Then move all the
> logs
> > to the layer we decide. Which layer do you think we should have the logging
> on?
> 
> I think this should be done on the device layer. That's what I've done for the
> rest:
> I've added logs when the action is really done, not when passing the event
from
> one layer to the other.
> I also had the issue with BluetoothDevice::DidFailToConnectGatt()

Unsure if ortuno will agree here or not. I think we can follow up later if
needed. I'm also OK erring on the side of double logging.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:434: const char* name_cstr =
name ? name->c_str() : "";
On 2017/03/15 01:08:40, jlebel wrote:
> On 2017/03/14 02:12:00, ortuno wrote:
> > You can use GetNameForDisplay() to always get a name back.
> 
> I can't run the unit tests with GetNameForDisplay(). DCHECK at
> resource_bundle.cc:246 fails.
>
https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?q=re...
> 
> Called by:
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.cc?q=b...

:( yeah, we need to fix that some day. Leave a TODO here with that issue number
please so we can improve this later.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:435: return
std::string("<BluetoothLowEnergyDeviceMac ") + GetIdentifier() +
On 2017/03/15 01:08:40, jlebel wrote:
> On 2017/03/14 02:12:00, ortuno wrote:
> > Could you use a StringPiece here?
> 
> I'm not sure to understand how to use StringPiece and what is the benefit of
it.

See string_piece.h top of header file comment. A better description in google3
where it came from originally. It can save conversions between string types.
It's not a priority here I think since VLOGs will be enabled rarely.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:448:
GetIdentifier() + ", service " + gatt_service_->GetIdentifier() + ">";
On 2017/03/15 01:08:40, jlebel wrote:
> On 2017/03/14 00:34:33, ortuno wrote:
> > A UUID is more useful than the identifier.
> 
> The identifier contains the uuid and the memory address. I chose that to make
> sure we don't have confusion between 2 characteristics.
> But I can change it to UUID if you prefer.

I think GetIdentifier is OK as jlebel states - no matter what the format is
internally it will be uniquely identifiable. It's nice that it includes the UUID
for now. Best solution would be to explicitly print UUID and the pointer here to
make the code very clear in it's intent.

[jlebel, 2017-03-21 23:22:45]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-03-21 23:22:45]

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/2745983003/#ps260001
(title: "Changing GetIdentifier() by GetUUID() + pointer")

[commit-bot: I haz the power, 2017-03-21 23:23:05]

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, 2017-03-21 23:36:43]

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

[commit-bot: I haz the power, 2017-03-21 23:36:44]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jlebel, 2017-03-21 23:57:59]

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_low_energy_device_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_low_energy_device_mac.mm:434: const char* name_cstr =
name ? name->c_str() : "";
On 2017/03/21 21:43:30, scheib wrote:
> On 2017/03/15 01:08:40, jlebel wrote:
> > On 2017/03/14 02:12:00, ortuno wrote:
> > > You can use GetNameForDisplay() to always get a name back.
> > 
> > I can't run the unit tests with GetNameForDisplay(). DCHECK at
> > resource_bundle.cc:246 fails.
> >
>
https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?q=re...
> > 
> > Called by:
> >
>
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_device.cc?q=b...
> 
> :( yeah, we need to fix that some day. Leave a TODO here with that issue
number
> please so we can improve this later.

Done.

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (right):

https://codereview.chromium.org/2745983003/diff/200001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:448:
GetIdentifier() + ", service " + gatt_service_->GetIdentifier() + ">";
On 2017/03/21 21:43:31, scheib wrote:
> On 2017/03/15 01:08:40, jlebel wrote:
> > On 2017/03/14 00:34:33, ortuno wrote:
> > > A UUID is more useful than the identifier.
> > 
> > The identifier contains the uuid and the memory address. I chose that to
make
> > sure we don't have confusion between 2 characteristics.
> > But I can change it to UUID if you prefer.
> 
> I think GetIdentifier is OK as jlebel states - no matter what the format is
> internally it will be uniquely identifiable. It's nice that it includes the
UUID
> for now. Best solution would be to explicitly print UUID and the pointer here
to
> make the code very clear in it's intent.

Done.

[jlebel, 2017-03-22 08:50:00]

The CQ bit was checked by jlebel@chromium.org

[commit-bot: I haz the power, 2017-03-22 08:50: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, 2017-03-22 08:59:40]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490172600644550,
"parent_rev": "cb6fe98c9e8e2fa4c68a7bc1268565d7cdeab4f4", "commit_rev":
"234aec4f17e0b08204b0e8df547ab7426100b404"}

[commit-bot: I haz the power, 2017-03-22 09:00:30]

Description was changed from

==========
Bluetooth: macOS: Adding logs

Adding logs for macOS adapter, devices, services, characteristics and
descriptors.

BUG=685972
==========

to

==========
Bluetooth: macOS: Adding logs

Adding logs for macOS adapter, devices, services, characteristics and
descriptors.

BUG=685972

Review-Url: https://codereview.chromium.org/2745983003
Cr-Commit-Position: refs/heads/master@{#458691}
Committed:
https://chromium.googlesource.com/chromium/src/+/234aec4f17e0b08204b0e8df547a...
==========

[commit-bot: I haz the power, 2017-03-22 09:00:32]

Committed patchset #7 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/234aec4f17e0b08204b0e8df547a...