bluetooth: bluez: Fixed issue with missing notifications after reconnect.

bluetooth: bluez: Fixed issue with missing notifications after reconnect.

BUG=680099
Review-Url: https://codereview.chromium.org/2625013003
Cr-Commit-Position: refs/heads/master@{#443451}
Committed: https://chromium.googlesource.com/chromium/src/+/db61493317991225f05f657bc5c78db7f96d55a7

[perja, 2017-01-11 13:29:44]

The CQ bit was checked by perja@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 13:29:59]

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-01-11 15:00:57]

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

[commit-bot: I haz the power, 2017-01-11 15:00:58]

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

[perja, 2017-01-11 15:51:44]

perja@opera.com changed reviewers:
+ scheib@chromium.org

[perja, 2017-01-11 15:51:45]

@scheib: PTAL. This change makes notifications work on Linux after a reconnect.

[scheib, 2017-01-11 18:44:02]

Thanks, I didn't understand the bluez difference in notification, though, would
you explain that in comments?

https://codereview.chromium.org/2625013003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/2625013003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.cc:931: DidDisconnectGatt(false /*
notifyDeviceChanged */);
Why false here?

[perja, 2017-01-12 09:26:28]

https://codereview.chromium.org/2625013003/diff/1/device/bluetooth/bluez/blue...
File device/bluetooth/bluez/bluetooth_device_bluez.cc (right):

https://codereview.chromium.org/2625013003/diff/1/device/bluetooth/bluez/blue...
device/bluetooth/bluez/bluetooth_device_bluez.cc:931: DidDisconnectGatt(false /*
notifyDeviceChanged */);
On 2017/01/11 18:44:02, scheib wrote:
> Why false here?

The bluez/dbus layer uses a property set (dbus::PropertySet) to keep track of
all properties for the connection. There is a separate OnPropertyChanged
callback fired when some of the properties change value. The notification for
changed device is handled through that mechanism. I will clarify with a comment
in the code.

[scheib, 2017-01-12 22:49:10]

LGTM

[scheib, 2017-01-12 22:49:13]

The CQ bit was checked by scheib@chromium.org

[commit-bot: I haz the power, 2017-01-12 22:50:25]

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-01-13 01:50:14]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484261353478640,
"parent_rev": "63db4c771190c186415af04477809368b0c53e0b", "commit_rev":
"db61493317991225f05f657bc5c78db7f96d55a7"}

[commit-bot: I haz the power, 2017-01-13 01:50:43]

Description was changed from

==========
bluetooth: bluez: Fixed issue with missing notifications after reconnect.

BUG=680099
==========

to

==========
bluetooth: bluez: Fixed issue with missing notifications after reconnect.

BUG=680099

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

[commit-bot: I haz the power, 2017-01-13 01:50:44]

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

[rkc1, 2017-05-15 23:56:59]

rkc@google.com changed reviewers:
+ rkc@google.com

[rkc1, 2017-05-15 23:56:59]

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:476:
SetGattServicesDiscoveryComplete(false);
What is the purpose of doing this? This doesn't actually notify BlueZ that
service discovery needs to be done again.

How does BlueZ know that Chrome has cleared its services cache and needs to be
notified of all services on the device again?

[ortuno, 2017-05-16 00:04:26]

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

[ortuno, 2017-05-16 00:04:27]

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:476:
SetGattServicesDiscoveryComplete(false);
On 2017/05/15 at 23:56:59, rkc1 wrote:
> What is the purpose of doing this? This doesn't actually notify BlueZ that
service discovery needs to be done again.
> 
> How does BlueZ know that Chrome has cleared its services cache and needs to be
notified of all services on the device again?

This line has no effect on BlueZ since the variable that it affects is not used
for IsGattServicesDiscoveryComplete().

[rkc1, 2017-05-16 00:10:09]

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:476:
SetGattServicesDiscoveryComplete(false);
On 2017/05/16 00:04:27, ortuno wrote:
> On 2017/05/15 at 23:56:59, rkc1 wrote:
> > What is the purpose of doing this? This doesn't actually notify BlueZ that
> service discovery needs to be done again.
> > 
> > How does BlueZ know that Chrome has cleared its services cache and needs to
be
> notified of all services on the device again?
> 
> This line has no effect on BlueZ since the variable that it affects is not
used
> for IsGattServicesDiscoveryComplete().

Right, and since this function is being called from the BlueZ code and these
lines are added for a "BlueZ" fix, it seems a bit odd that we do this. What is
the purpose of setting that variable?

I am very confused by this fix. Not having a CL description doesn't make things
any easier.

Perja, could you please provide some context on what you are trying to do
exactly?

[ortuno, 2017-05-16 00:40:41]

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:476:
SetGattServicesDiscoveryComplete(false);
On 2017/05/16 at 00:10:09, rkc1 wrote:
> On 2017/05/16 00:04:27, ortuno wrote:
> > On 2017/05/15 at 23:56:59, rkc1 wrote:
> > > What is the purpose of doing this? This doesn't actually notify BlueZ that
> > service discovery needs to be done again.
> > > 
> > > How does BlueZ know that Chrome has cleared its services cache and needs
to be
> > notified of all services on the device again?
> > 
> > This line has no effect on BlueZ since the variable that it affects is not
used
> > for IsGattServicesDiscoveryComplete().
> 
> Right, and since this function is being called from the BlueZ code and these
lines are added for a "BlueZ" fix, it seems a bit odd that we do this. What is
the purpose of setting that variable?
> 
> I am very confused by this fix. Not having a CL description doesn't make
things any easier.
> 
> Perja, could you please provide some context on what you are trying to do
exactly?

I agree. It's unclear what the issue was and how this fixes it :(

There are other issue with this patch e.g. it causes mac to clear it services
twice[1]. Maybe we should revert to surface the bug again and fix it?

[1]
https://cs.chromium.org/chromium/src/device/bluetooth/bluetooth_low_energy_de...

[chromium-reviews, 2017-05-16 01:48:55]

+1 for revert

/rkc

On Mon, May 15, 2017 at 5:40 PM, <ortuno@chromium.org> wrote:

>
> https://codereview.chromium.org/2625013003/diff/20001/
> device/bluetooth/bluetooth_device.cc
> File device/bluetooth/bluetooth_device.cc (right):
>
> https://codereview.chromium.org/2625013003/diff/20001/
> device/bluetooth/bluetooth_device.cc#newcode476
> device/bluetooth/bluetooth_device.cc:476:
> SetGattServicesDiscoveryComplete(false);
> On 2017/05/16 at 00:10:09, rkc1 wrote:
> > On 2017/05/16 00:04:27, ortuno wrote:
> > > On 2017/05/15 at 23:56:59, rkc1 wrote:
> > > > What is the purpose of doing this? This doesn't actually notify
> BlueZ that
> > > service discovery needs to be done again.
> > > >
> > > > How does BlueZ know that Chrome has cleared its services cache and
> needs to be
> > > notified of all services on the device again?
> > >
> > > This line has no effect on BlueZ since the variable that it affects
> is not used
> > > for IsGattServicesDiscoveryComplete().
> >
> > Right, and since this function is being called from the BlueZ code and
> these lines are added for a "BlueZ" fix, it seems a bit odd that we do
> this. What is the purpose of setting that variable?
> >
> > I am very confused by this fix. Not having a CL description doesn't
> make things any easier.
> >
> > Perja, could you please provide some context on what you are trying to
> do exactly?
>
> I agree. It's unclear what the issue was and how this fixes it :(
>
> There are other issue with this patch e.g. it causes mac to clear it
> services twice[1]. Maybe we should revert to surface the bug again and
> fix it?
>
> [1]
> https://cs.chromium.org/chromium/src/device/bluetooth/
> bluetooth_low_energy_device_mac.mm?type=cs&q=device_uuids_
> .ClearServiceUUIDs&l=483
>
> https://codereview.chromium.org/2625013003/
>

-- 
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.

[perja, 2017-05-16 08:23:24]

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_device.cc (right):

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:476:
SetGattServicesDiscoveryComplete(false);
On 2017/05/16 00:10:09, rkc1 wrote:
> On 2017/05/16 00:04:27, ortuno wrote:
> > On 2017/05/15 at 23:56:59, rkc1 wrote:
> > > What is the purpose of doing this? This doesn't actually notify BlueZ that
> > service discovery needs to be done again.
> > > 
> > > How does BlueZ know that Chrome has cleared its services cache and needs
to
> be
> > notified of all services on the device again?
> > 
> > This line has no effect on BlueZ since the variable that it affects is not
> used
> > for IsGattServicesDiscoveryComplete().
> 
> Right, and since this function is being called from the BlueZ code and these
> lines are added for a "BlueZ" fix, it seems a bit odd that we do this. What is
> the purpose of setting that variable?
> 
 
> Perja, could you please provide some context on what you are trying to do
> exactly?

It's been a while since I looked into this, but when a disconnect happened the
state in BlueZ was correct and Chrome had the old cache of services. So my fix
was to clear the caches in Chrome. But this was already done for Android so I
moved the three lines from bluetooth_device_android.cc to bluetooth_device.c

https://codereview.chromium.org/2625013003/diff/20001/device/bluetooth/blueto...
device/bluetooth/bluetooth_device.cc:476:
SetGattServicesDiscoveryComplete(false);
On 2017/05/16 00:40:41, ortuno wrote:
> On 2017/05/16 at 00:10:09, rkc1 wrote:
> > On 2017/05/16 00:04:27, ortuno wrote:
> > > On 2017/05/15 at 23:56:59, rkc1 wrote:
> > > > What is the purpose of doing this? This doesn't actually notify BlueZ
that
> > > service discovery needs to be done again.
> > > > 
> > > > How does BlueZ know that Chrome has cleared its services cache and needs
> to be
> > > notified of all services on the device again?
> > > 
> > > This line has no effect on BlueZ since the variable that it affects is not
> used
> > > for IsGattServicesDiscoveryComplete().
> > 
> > Right, and since this function is being called from the BlueZ code and these
> lines are added for a "BlueZ" fix, it seems a bit odd that we do this. What is
> the purpose of setting that variable?
> > 
> > I am very confused by this fix. Not having a CL description doesn't make
> things any easier.
> > 
> > Perja, could you please provide some context on what you are trying to do
> exactly?
> 
> I agree. It's unclear what the issue was and how this fixes it :(
> 
> There are other issue with this patch e.g. it causes mac to clear it services
> twice[1]. Maybe we should revert to surface the bug again and fix it?

Sure, that sounds like a good approach now.

[rkc, 2017-05-16 18:45:11]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2891493002/ by rkc@chromium.org.

The reason for reverting is: Doesn't fix the issue correctly, introduces new
issues.
.

[scheib, 2017-05-16 20:08:14]

On 2017/05/16 18:45:11, rkc wrote:
> A revert of this CL (patchset #2 id:20001) has been created in
> https://codereview.chromium.org/2891493002/ by mailto:rkc@chromium.org.
> 
> The reason for reverting is: Doesn't fix the issue correctly, introduces new
> issues.
> .

I will revert manually, as auto-revert had patch conflict.

[scheib, 2017-05-16 20:11:48]

On 2017/05/16 20:08:14, scheib wrote:
> On 2017/05/16 18:45:11, rkc wrote:
> > A revert of this CL (patchset #2 id:20001) has been created in
> > https://codereview.chromium.org/2891493002/ by mailto:rkc@chromium.org.
> > 
> > The reason for reverting is: Doesn't fix the issue correctly, introduces new
> > issues.
> > .
> 
> I will revert manually, as auto-revert had patch conflict.

Already being done in https://codereview.chromium.org/2888663002/