bluetooth: macOS: Adding log in BluetoothAdapterMac::DisconnectGatt()

Adding log in BluetoothAdapterMac::CreateGattConnection() and BluetoothAdapterMac::DisconnectGatt()

And removing from BluetoothLowEnergyDeviceMac::CreateGattConnectionImpl() and BluetoothLowEnergyDeviceMac::DisconnectGatt()

This is to make sure no log are missed (when calling directly BluetoothAdapterMac).

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

[jlebel, 2017-03-30 17:49:52]

Patchset #1 (id:1) has been deleted

[jlebel, 2017-03-30 17:49:58]

Patchset #1 (id:20001) has been deleted

[jlebel, 2017-03-30 17:53:28]

Description was changed from

==========
Adding log in BluetoothAdapterMac::DisconnectGatt()

BUG=
==========

to

==========
Adding log in BluetoothAdapterMac::CreateGattConnection() and
BluetoothAdapterMac::DisconnectGatt()

And removing from BluetoothLowEnergyDeviceMac::CreateGattConnectionImpl() and
BluetoothLowEnergyDeviceMac::DisconnectGatt()

This is to make sure no log are missed (when calling directly
BluetoothAdapterMac).

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

[jlebel, 2017-03-30 18:46:54]

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

[jlebel, 2017-03-30 18:46:54]

Hello Vincent,

Can you review this small patch about improving logs for bluetooth low energy
for macOS?

Thanks,

[jlebel, 2017-03-30 18:47:18]

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

[commit-bot: I haz the power, 2017-03-30 18:48:08]

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-30 19:10:16]

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

[commit-bot: I haz the power, 2017-03-30 19:10:16]

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

[jlebel, 2017-03-30 19:17:48]

Patchset #1 (id:40001) has been deleted

[jlebel, 2017-03-30 19:17:59]

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

[commit-bot: I haz the power, 2017-03-30 19:18:55]

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-30 20:40:50]

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

[commit-bot: I haz the power, 2017-03-30 20:40:51]

Dry run: This issue passed the CQ dry run.

[scheib, 2017-03-30 22:48:41]

lgtm, with suggested small fix:

https://codereview.chromium.org/2783283002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2783283002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:644: DCHECK(device_mac);
This DCHECK is redundant with the "*device_mac" isn't it? If device_mac is null
then a crash will occur and the stack should be produced blaming that line, it
should be easy to understand. -- If I'm wrong, by all means use a DCHECK to make
it clear.

[jlebel, 2017-03-30 23:16:08]

Let me know if you prefer me to remove the DCHECK.

https://codereview.chromium.org/2783283002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2783283002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:644: DCHECK(device_mac);
On 2017/03/30 22:48:41, scheib wrote:
> This DCHECK is redundant with the "*device_mac" isn't it? If device_mac is
null
> then a crash will occur and the stack should be produced blaming that line, it
> should be easy to understand. -- If I'm wrong, by all means use a DCHECK to
make
> it clear.

It will crash, but somewhere in operator<< related to
BluetoothLowEnergyDeviceMac. My idea was to make it very clear. There is also
one difference, null will be caught by the DCHECK, a deallocated device will
crash in the operator<<.

Do you think I should remove the DCHECK?

[scheib, 2017-03-31 01:26:48]

On 2017/03/30 23:16:08, jlebel wrote:
> Let me know if you prefer me to remove the DCHECK.
> 
>
https://codereview.chromium.org/2783283002/diff/60001/device/bluetooth/blueto...
> File device/bluetooth/bluetooth_adapter_mac.mm (right):
> 
>
https://codereview.chromium.org/2783283002/diff/60001/device/bluetooth/blueto...
> device/bluetooth/bluetooth_adapter_mac.mm:644: DCHECK(device_mac);
> On 2017/03/30 22:48:41, scheib wrote:
> > This DCHECK is redundant with the "*device_mac" isn't it? If device_mac is
> null
> > then a crash will occur and the stack should be produced blaming that line,
it
> > should be easy to understand. -- If I'm wrong, by all means use a DCHECK to
> make
> > it clear.
> 
> It will crash, but somewhere in operator<< related to
> BluetoothLowEnergyDeviceMac. My idea was to make it very clear. There is also
> one difference, null will be caught by the DCHECK, a deallocated device will
> crash in the operator<<.
> 
> Do you think I should remove the DCHECK?

I'd remove it myself. IMHO that is straight forward enough to understand why it
is crashing. That's consistent with general chromium culture to try to minimize
these. It's OK if you think this will save you time later to leave it, though,
it's a judgement call. If you added them because you ran into a confusing crash
and don't want to waste time again, then you are justified in keeping them.

[jlebel, 2017-03-31 07:39:12]

https://codereview.chromium.org/2783283002/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_adapter_mac.mm (right):

https://codereview.chromium.org/2783283002/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_adapter_mac.mm:644: DCHECK(device_mac);
On 2017/03/30 23:16:08, jlebel wrote:
> On 2017/03/30 22:48:41, scheib wrote:
> > This DCHECK is redundant with the "*device_mac" isn't it? If device_mac is
> null
> > then a crash will occur and the stack should be produced blaming that line,
it
> > should be easy to understand. -- If I'm wrong, by all means use a DCHECK to
> make
> > it clear.
> 
> It will crash, but somewhere in operator<< related to
> BluetoothLowEnergyDeviceMac. My idea was to make it very clear. There is also
> one difference, null will be caught by the DCHECK, a deallocated device will
> crash in the operator<<.
> 
> Do you think I should remove the DCHECK?

Done.

[jlebel, 2017-03-31 07:39:16]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-03-31 07:39:17]

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/2783283002/#ps80001
(title: "Removing DCHECK")

[commit-bot: I haz the power, 2017-03-31 07:39:30]

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-31 08:37:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490945956999540,
"parent_rev": "01409c3315646cb0acca04308f67421410791800", "commit_rev":
"bf7b01a5ecefb707c69f90baa0850d5d7570215a"}

[commit-bot: I haz the power, 2017-03-31 08:38:30]

Description was changed from

==========
Adding log in BluetoothAdapterMac::CreateGattConnection() and
BluetoothAdapterMac::DisconnectGatt()

And removing from BluetoothLowEnergyDeviceMac::CreateGattConnectionImpl() and
BluetoothLowEnergyDeviceMac::DisconnectGatt()

This is to make sure no log are missed (when calling directly
BluetoothAdapterMac).

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

to

==========
Adding log in BluetoothAdapterMac::CreateGattConnection() and
BluetoothAdapterMac::DisconnectGatt()

And removing from BluetoothLowEnergyDeviceMac::CreateGattConnectionImpl() and
BluetoothLowEnergyDeviceMac::DisconnectGatt()

This is to make sure no log are missed (when calling directly
BluetoothAdapterMac).

BUG=685972

Review-Url: https://codereview.chromium.org/2783283002
Cr-Commit-Position: refs/heads/master@{#461079}
Committed:
https://chromium.googlesource.com/chromium/src/+/bf7b01a5ecefb707c69f90baa085...
==========

[commit-bot: I haz the power, 2017-03-31 08:38:31]

Committed patchset #2 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/bf7b01a5ecefb707c69f90baa085...