Fix getting notified twice after subscribe to notifications and call readValue

Fix getting notified twice after subscribe to notifications and call readValue

The CL:
https://codereview.chromium.org/2718583002
adds code that sends an event on the readValue callback. So if subscribes
to notifications and then calls readValue, will get notified twice. This
CL fixes this issue.

BUG=697702
Review-Url: https://codereview.chromium.org/2728623004
Cr-Commit-Position: refs/heads/master@{#455652}
Committed: https://chromium.googlesource.com/chromium/src/+/50cead395565600cca22d02bf404ea1fc37e3897

[juncai, 2017-03-02 21:40:29]

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

[commit-bot: I haz the power, 2017-03-02 21:41:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2017-03-02 21:53:13]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[juncai, 2017-03-02 21:55:20]

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

[juncai, 2017-03-02 21:55:21]

Please take a look.

[commit-bot: I haz the power, 2017-03-02 22:28:13]

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

[commit-bot: I haz the power, 2017-03-02 22:28:14]

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

[juncai, 2017-03-02 23:21:20]

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

[commit-bot: I haz the power, 2017-03-02 23:22:04]

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-03 02:57:12]

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

[commit-bot: I haz the power, 2017-03-03 02:57:13]

Dry run: This issue passed the CQ dry run.

[ortuno, 2017-03-03 05:28:37]

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (left):

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:260:
UpdateValueAndNotify();
Ah good old mac. There are actually no bots on the CQ that run our tests so we
need to run them manually for mac.  I believe this change will break a couple of
tests. More specifically the UpdateValueAndNotify() function does two things:

1. Updates value_
2. Send the event

Since we are no longer calling this function value_ is never updated.

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (left):

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:995: EXPECT_EQ(0,
observer.gatt_characteristic_value_changed_count());
Please update them to reflect the correct value rather than deleting them.

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_remote_gatt_characteristic_bluez.cc
(right):

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_remote_gatt_characteristic_bluez.cc:188:
characteristic_value_read_in_progress_ = true;
If the read fails then this never goes back to false. Could you write or update
current tests to make sure we are doing the right thing?

[ortuno, 2017-03-03 05:31:04]

also please run CQ before sending patches for review. It's hard to keep track
which patch I'm currently reviewing.

[juncai, 2017-03-04 00:11:54]

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

[commit-bot: I haz the power, 2017-03-04 00:12: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-04 01:32:01]

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

[commit-bot: I haz the power, 2017-03-04 01:32:02]

Dry run: This issue passed the CQ dry run.

[juncai, 2017-03-04 01:33:59]

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm (left):

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_mac.mm:260:
UpdateValueAndNotify();
On 2017/03/03 05:28:37, ortuno wrote:
> Ah good old mac. There are actually no bots on the CQ that run our tests so we
> need to run them manually for mac.  I believe this change will break a couple
of
> tests. More specifically the UpdateValueAndNotify() function does two things:
> 
> 1. Updates value_
> 2. Send the event
> 
> Since we are no longer calling this function value_ is never updated.

Yes, it does break some device unit tests on Mac. Fixed it.

Done.

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (left):

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:995: EXPECT_EQ(0,
observer.gatt_characteristic_value_changed_count());
On 2017/03/03 05:28:37, ortuno wrote:
> Please update them to reflect the correct value rather than deleting them.

Done.

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_remote_gatt_characteristic_bluez.cc
(right):

https://codereview.chromium.org/2728623004/diff/40001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_remote_gatt_characteristic_bluez.cc:188:
characteristic_value_read_in_progress_ = true;
On 2017/03/03 05:28:37, ortuno wrote:
> If the read fails then this never goes back to false. Could you write or
update
> current tests to make sure we are doing the right thing?

Done.

[ortuno, 2017-03-06 09:15:37]

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (left):

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:499: //
Callback that make sure GattCharacteristicValueChanged has been called
Could you change an existing test to assert that the event is not getting fired?

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:1104: // Issue a read
request. A successful read results in a
Fix comment.

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:1160: EXPECT_EQ(1,
observer.gatt_characteristic_value_changed_count());
Please spend some time trying to understand tests before modifying them
otherwise we might break behavior in unintended ways or our implementation might
be buggy.

In this part of the test there are only Read requests so there shouldn't be a
characteristic value changed event. I think the following is happening:

1. We make first Read request which sets pending to true.
2. We make second Read request which sets pending to true.
3. Second read request fails with In Progress errors and sets pending to false.
4. First read request succeeds and notifies of value change since pending was
set to false we dispatch the even to all observers.

[juncai, 2017-03-06 19:55:00]

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

[commit-bot: I haz the power, 2017-03-06 19:55:53]

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-06 21:47:27]

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

[commit-bot: I haz the power, 2017-03-06 21:47:28]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[juncai, 2017-03-08 21:04:41]

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

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

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-08 23:17:26]

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

[commit-bot: I haz the power, 2017-03-08 23:17:27]

Dry run: This issue passed the CQ dry run.

[juncai, 2017-03-08 23:20:39]

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/blueto...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (left):

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/blueto...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:499: //
Callback that make sure GattCharacteristicValueChanged has been called
On 2017/03/06 09:15:37, ortuno wrote:
> Could you change an existing test to assert that the event is not getting
fired?

Done.

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/bluez/...
File device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc (right):

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:1104: // Issue a read
request. A successful read results in a
On 2017/03/06 09:15:37, ortuno wrote:
> Fix comment.

Done.

https://codereview.chromium.org/2728623004/diff/60001/device/bluetooth/bluez/...
device/bluetooth/bluez/bluetooth_gatt_bluez_unittest.cc:1160: EXPECT_EQ(1,
observer.gatt_characteristic_value_changed_count());
On 2017/03/06 09:15:37, ortuno wrote:
> Please spend some time trying to understand tests before modifying them
> otherwise we might break behavior in unintended ways or our implementation
might
> be buggy.
> 
> In this part of the test there are only Read requests so there shouldn't be a
> characteristic value changed event. I think the following is happening:
> 
> 1. We make first Read request which sets pending to true.
> 2. We make second Read request which sets pending to true.
> 3. Second read request fails with In Progress errors and sets pending to
false.
> 4. First read request succeeds and notifies of value change since pending was
> set to false we dispatch the even to all observers.

Done.

[ortuno, 2017-03-09 00:01:52]

lgtm!

https://codereview.chromium.org/2728623004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2728623004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:556: //
TODO(juncai): remove this #if once the bug on Windows is fixed.
optional: Since we share so many issues and someone else usually ends up
addressing TODOs, we usually just include the number in the TODO e.g.
TODO(crbug.com/699694): Remove this #if ...

https://codereview.chromium.org/2728623004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:558: #if
!defined(OS_WIN)
optional:

TODO...
#if defined(OS_WIN)
  EXPECT_FALSE(observer.last_gatt_characteristic_id().empty());
  EXPECT_TRUE(observer.last_gatt_characteristic_uuid().IsValid());
#else
  EXPECT_TRUE(observer.last_gatt_characteristic_id().empty());
  EXPECT_FALSE(observer.last_gatt_characteristic_uuid().IsValid());
#endif

That way if someone improves the windows testing framework and ends up fixing
this issue they'll notice.

[juncai, 2017-03-09 00:59:57]

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

[commit-bot: I haz the power, 2017-03-09 01:00:43]

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-09 02:35:11]

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

[commit-bot: I haz the power, 2017-03-09 02:35:12]

Dry run: This issue passed the CQ dry run.

[juncai, 2017-03-09 03:03:07]

https://codereview.chromium.org/2728623004/diff/100001/device/bluetooth/bluet...
File device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc (right):

https://codereview.chromium.org/2728623004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:556: //
TODO(juncai): remove this #if once the bug on Windows is fixed.
On 2017/03/09 00:01:52, ortuno wrote:
> optional: Since we share so many issues and someone else usually ends up
> addressing TODOs, we usually just include the number in the TODO e.g.
> TODO(crbug.com/699694): Remove this #if ...

Done.

https://codereview.chromium.org/2728623004/diff/100001/device/bluetooth/bluet...
device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc:558: #if
!defined(OS_WIN)
On 2017/03/09 00:01:52, ortuno wrote:
> optional:
> 
> TODO...
> #if defined(OS_WIN)
>   EXPECT_FALSE(observer.last_gatt_characteristic_id().empty());
>   EXPECT_TRUE(observer.last_gatt_characteristic_uuid().IsValid());
> #else
>   EXPECT_TRUE(observer.last_gatt_characteristic_id().empty());
>   EXPECT_FALSE(observer.last_gatt_characteristic_uuid().IsValid());
> #endif
> 
> That way if someone improves the windows testing framework and ends up fixing
> this issue they'll notice.

Done.

[juncai, 2017-03-09 03:03:27]

The CQ bit was checked by juncai@chromium.org

[juncai, 2017-03-09 03:03:28]

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

[commit-bot: I haz the power, 2017-03-09 03:03:53]

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-09 03:09:54]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489028607724830,
"parent_rev": "746ad2a88e424ff48b4153b7db219eea98a0e79d", "commit_rev":
"50cead395565600cca22d02bf404ea1fc37e3897"}

[commit-bot: I haz the power, 2017-03-09 03:10:36]

Description was changed from

==========
Fix getting notified twice after subscribe to notifications and call readValue

The CL:
https://codereview.chromium.org/2718583002
adds code that sends an event on the readValue callback. So if subscribes
to notifications and then calls readValue, will get notified twice. This
CL fixes this issue.

BUG=697702
==========

to

==========
Fix getting notified twice after subscribe to notifications and call readValue

The CL:
https://codereview.chromium.org/2718583002
adds code that sends an event on the readValue callback. So if subscribes
to notifications and then calls readValue, will get notified twice. This
CL fixes this issue.

BUG=697702

Review-Url: https://codereview.chromium.org/2728623004
Cr-Commit-Position: refs/heads/master@{#455652}
Committed:
https://chromium.googlesource.com/chromium/src/+/50cead395565600cca22d02bf404...
==========

[commit-bot: I haz the power, 2017-03-09 03:10:37]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/50cead395565600cca22d02bf404...