Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1749403002:
Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests (Closed)
DescriptionImplement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests.
This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests.
It also fixes implicit cast between semantically different integer types issue.
In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_,
and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc
to after simulate operation since the operation is asynchronous down to the OS on Windows.
BUG=579202, 592843, 427616, 597888
Committed: https://crrev.com/08539daf0b416e5cebf3109119f4c9e6beb09d54
Cr-Commit-Position: refs/heads/master@{#386150}
Patch Set 1 : #Patch Set 2 : #Patch Set 3 : adjust comments #
Total comments: 53
Patch Set 4 : check read/write attempts later #Patch Set 5 : #Patch Set 6 : address comments #
Total comments: 8
Patch Set 7 : add more unit tests #
Total comments: 13
Patch Set 8 : Move characteristic value changed notification to BluetoothTaskManager to solve multithread problem #Patch Set 9 : Split out BluetoothGattNotifySessionWin::Stop #Patch Set 10 : change new_value->at(i) to (*new_value)[i] #
Total comments: 16
Patch Set 11 : address comments #Patch Set 12 : #Patch Set 13 : rebase #
Total comments: 37
Patch Set 14 : #Patch Set 15 : #Patch Set 16 : add one more note #Patch Set 17 : Enable RememberCharacteristicForSubsequentAction and related tests #
Total comments: 8
Patch Set 18 : address comments #Patch Set 19 : set CCCD #Patch Set 20 : rebase #
Total comments: 24
Patch Set 21 : address comments #
Total comments: 2
Patch Set 22 : use static_cast #
Total comments: 20
Patch Set 23 : address comments #
Total comments: 2
Patch Set 24 : move comments #Messages
Total messages: 158 (81 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/100001
Patchset #1 (id:60001) has been deleted
Description was changed from ========== add notifications BUG= ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. BUG=579202 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/120001
gogerald@chromium.org changed reviewers: + scheib@chromium.org
Hi Vincent, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/140001
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ortuno@chromium.org changed reviewers: + ortuno@chromium.org
I did a first pass. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:933: #endif // defined(OS_ANDROID) || defined(OS_WIN) These test only cover starting notifications. Since your patch adds StopNotifications you should add tests for that as well. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_notify_session_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.cc:30: return characteristic_.get()->StopNotifySession(); Split this: characteristic_.get()->StopNotifySession(); return; https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.cc:31: callback.Run(); Stop should be async. Post a task. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.cc:847: base::File file(service_path, base::File::FLAG_OPEN | base::File::FLAG_READ); I'm curious. If BluetoothLowEnergyWrapper is supposed to be a very light wrapper around windows APIs why do you open the file here and not in the task manager? https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:179: // Register |event_type| of GATT events of the service with service device Register for GATT events of |event_type|... https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:21: (device::BluetoothRemoteGattCharacteristicWin*)context; Space between parentheses and context. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:169: callback.Run(std::move(notify_session)); StartNotifySession should be async. Post a task here. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:187: &OnGetGattEventWin, (PVOID) this); Wouldn't you get a null pointer dereference if windows calls OnGetGattEventWin after the characteristic is destroyed? https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:188: gatt_event_registeration_in_progress_ = true; Are you sure the registering for the event is enough? Don't you need to write to the CCC descriptor as well? https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:242: void BluetoothRemoteGattCharacteristicWin::OnGetRemoteGattEvent( Does this get called after reading or writing to a characteristic. Android and Chrome OS do it so I wouldn't be surprised if windows does it as well. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:247: (BLUETOOTH_GATT_VALUE_CHANGED_EVENT*)event_parameter; Space between parentheses and evet_parameter https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:250: scoped_ptr<uint8_t> new_value(new uint8_t[new_value_win->DataSize]); Why do you use a pointer to a uint8? I think scoped_ptr<std::vector<uint8_t>> would be better. scoped_ptr<std::vector<uint8_t>> new_value(new std::vector<uint8_t>(new_value_win->DataSize)); That way you can do the following in OnGattCharacteristicValueChanged: characteristic_value_.assign(new_value->first, new_value->last); https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:259: new_value_win->DataSize)); I think you should return here and add a NOTREACHED(); after. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:292: start_notify_session_callbacks_.clear(); When would you have pending callbacks? https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:433: characteristic_value_.push_back(new_value.get()[i]); As mentioned before I think you should pass in a vector to avoid manually copying the new value. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:447: for (auto callback : callbacks) { You should use a reference of the callback not a copy: for (auto callback& : callbacks) https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:448: scoped_ptr<BluetoothGattNotifySessionWin> notify_session( You don't need to create a scoped pointer first you can just do it inline: callback.first.Run(make_scoped_ptr(new BluetoothGattNotifySessionWin(...))); https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:450: callback.first.Run(std::move(notify_session)); I think you should just post a task here. Otherwise don't change gatt_event_registration_in_progress_ until you've run all the callbacks. A callback could try to register a characteristic again and at this point both IsNotifying() and gatt_event_registration_in_progress are false so we would try to register for notifications again. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:455: for (auto callback : callbacks) Same here use a reference not a copy.
Thanks for doing a fist pass. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:933: #endif // defined(OS_ANDROID) || defined(OS_WIN) On 2016/03/07 18:48:44, ortuno wrote: > These test only cover starting notifications. Since your patch adds > StopNotifications you should add tests for that as well. Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_notify_session_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.cc:30: return characteristic_.get()->StopNotifySession(); On 2016/03/07 18:48:44, ortuno wrote: > Split this: > > characteristic_.get()->StopNotifySession(); > return; Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.cc:31: callback.Run(); On 2016/03/07 18:48:44, ortuno wrote: > Stop should be async. Post a task. Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.cc:847: base::File file(service_path, base::File::FLAG_OPEN | base::File::FLAG_READ); On 2016/03/07 18:48:45, ortuno wrote: > I'm curious. If BluetoothLowEnergyWrapper is supposed to be a very light wrapper > around windows APIs why do you open the file here and not in the task manager? Here open file opens a device in OS. It makes this interface self included and convenient to implement corresponding fake interface. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:179: // Register |event_type| of GATT events of the service with service device On 2016/03/07 18:48:45, ortuno wrote: > Register for GATT events of |event_type|... Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:21: (device::BluetoothRemoteGattCharacteristicWin*)context; On 2016/03/07 18:48:45, ortuno wrote: > Space between parentheses and context. Is this google code style? It looks making more sense without space and 'git cl format' will remove space between them. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:169: callback.Run(std::move(notify_session)); On 2016/03/07 18:48:45, ortuno wrote: > StartNotifySession should be async. Post a task here. Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:187: &OnGetGattEventWin, (PVOID) this); On 2016/03/07 18:48:45, ortuno wrote: > Wouldn't you get a null pointer dereference if windows calls OnGetGattEventWin > after the characteristic is destroyed? Event will be unregistered in ~BluetoothRemoteGattCharacteristicWin() https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:188: gatt_event_registeration_in_progress_ = true; On 2016/03/07 18:48:45, ortuno wrote: > Are you sure the registering for the event is enough? Don't you need to write to > the CCC descriptor as well? No needed, https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:242: void BluetoothRemoteGattCharacteristicWin::OnGetRemoteGattEvent( On 2016/03/07 18:48:45, ortuno wrote: > Does this get called after reading or writing to a characteristic. Android and > Chrome OS do it so I wouldn't be surprised if windows does it as well. No the case on Windows. I don't see why it should work in that way (read doesn't change the value, write indicates you know the value changed). Saw you create a bug for it. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:247: (BLUETOOTH_GATT_VALUE_CHANGED_EVENT*)event_parameter; On 2016/03/07 18:48:45, ortuno wrote: > Space between parentheses and evet_parameter Acknowledged. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:250: scoped_ptr<uint8_t> new_value(new uint8_t[new_value_win->DataSize]); On 2016/03/07 18:48:45, ortuno wrote: > Why do you use a pointer to a uint8? I think scoped_ptr<std::vector<uint8_t>> > would be better. > > scoped_ptr<std::vector<uint8_t>> new_value(new > std::vector<uint8_t>(new_value_win->DataSize)); > > That way you can do the following in OnGattCharacteristicValueChanged: > > characteristic_value_.assign(new_value->first, new_value->last); Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:259: new_value_win->DataSize)); On 2016/03/07 18:48:45, ortuno wrote: > I think you should return here and add a NOTREACHED(); after. Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:292: start_notify_session_callbacks_.clear(); On 2016/03/07 18:48:45, ortuno wrote: > When would you have pending callbacks? Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:433: characteristic_value_.push_back(new_value.get()[i]); On 2016/03/07 18:48:45, ortuno wrote: > As mentioned before I think you should pass in a vector to avoid manually > copying the new value. Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:447: for (auto callback : callbacks) { On 2016/03/07 18:48:45, ortuno wrote: > You should use a reference of the callback not a copy: > > for (auto callback& : callbacks) Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:448: scoped_ptr<BluetoothGattNotifySessionWin> notify_session( On 2016/03/07 18:48:45, ortuno wrote: > You don't need to create a scoped pointer first you can just do it inline: > > callback.first.Run(make_scoped_ptr(new BluetoothGattNotifySessionWin(...))); Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:450: callback.first.Run(std::move(notify_session)); On 2016/03/07 18:48:45, ortuno wrote: > I think you should just post a task here. Otherwise don't change > gatt_event_registration_in_progress_ until you've run all the callbacks. A > callback could try to register a characteristic again and at this point both > IsNotifying() and gatt_event_registration_in_progress are false so we would try > to register for notifications again. Done. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:455: for (auto callback : callbacks) On 2016/03/07 18:48:45, ortuno wrote: > Same here use a reference not a copy. Done.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/180001
https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.cc:847: base::File file(service_path, base::File::FLAG_OPEN | base::File::FLAG_READ); On 2016/03/07 at 22:52:49, gogerald1 wrote: > On 2016/03/07 18:48:45, ortuno wrote: > > I'm curious. If BluetoothLowEnergyWrapper is supposed to be a very light wrapper > > around windows APIs why do you open the file here and not in the task manager? > > Here open file opens a device in OS. It makes this interface self included and convenient to implement corresponding fake interface. Ah that makes sense. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:21: (device::BluetoothRemoteGattCharacteristicWin*)context; On 2016/03/07 at 22:52:49, gogerald1 wrote: > On 2016/03/07 18:48:45, ortuno wrote: > > Space between parentheses and context. > > Is this google code style? It looks making more sense without space and 'git cl format' will remove space between them. Interesting. Well if git cl format says no space then no space it is. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:187: &OnGetGattEventWin, (PVOID) this); On 2016/03/07 at 22:52:49, gogerald1 wrote: > On 2016/03/07 18:48:45, ortuno wrote: > > Wouldn't you get a null pointer dereference if windows calls OnGetGattEventWin > > after the characteristic is destroyed? > > Event will be unregistered in ~BluetoothRemoteGattCharacteristicWin() Could the following happen? PostTask(~BluetoothRemoteGattCharacteristicWin()) PostTask(OnGetGattEventWin()) This would mean that there is a OnGetGattEventWin waiting to be executed, is ~BluetoothRemoteGattCharacteristicWin going to cancel the pending OnGetGattEventWin? https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:188: gatt_event_registeration_in_progress_ = true; On 2016/03/07 at 22:52:49, gogerald1 wrote: > On 2016/03/07 18:48:45, ortuno wrote: > > Are you sure the registering for the event is enough? Don't you need to write to > > the CCC descriptor as well? > > No needed, This sort of statements are not useful when trying to understand your process. Could you explain how you made sure that writing to the descriptor is not needed and document your findings in the code? https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:242: void BluetoothRemoteGattCharacteristicWin::OnGetRemoteGattEvent( On 2016/03/07 at 22:52:49, gogerald1 wrote: > On 2016/03/07 18:48:45, ortuno wrote: > > Does this get called after reading or writing to a characteristic. Android and > > Chrome OS do it so I wouldn't be surprised if windows does it as well. > > No the case on Windows. Can you confirm that you registered for an event and then read the value and the OnGetRemoteGattEvent didn't get called? > I don't see why it should work in that way (read doesn't change the value, write indicates you know the value changed). Saw you create a bug for it. That's not really up to the implementation. The implementation has to match the interface. If you don't agree with the interface you need to submit a change changing the description of the callback. https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:942: #endif This needs to be tested more thoroughly. A couple of cases I can think of: - Stop while there are pending callbacks - Stop twice I'm sure there is more. https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:255: new_value->at(i) = new_value_win->Data[i]; nit: You can just use the [] operator: new_value.get()[i]
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/200001
https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:188: gatt_event_registeration_in_progress_ = true; On 2016/03/08 00:05:36, ortuno wrote: > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > On 2016/03/07 18:48:45, ortuno wrote: > > > Are you sure the registering for the event is enough? Don't you need to > write to > > > the CCC descriptor as well? > > > > No needed, > > This sort of statements are not useful when trying to understand your process. > Could you explain how you made sure that writing to the descriptor is not needed > and document your findings in the code? If a question ever comes up from a reviewer it is best to explain the answer in code or comments. The same question is likely to come up again for future readers - and they will not read comments on the code review. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:242: void BluetoothRemoteGattCharacteristicWin::OnGetRemoteGattEvent( On 2016/03/08 00:05:36, ortuno wrote: > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > On 2016/03/07 18:48:45, ortuno wrote: > > > Does this get called after reading or writing to a characteristic. Android > and > > > Chrome OS do it so I wouldn't be surprised if windows does it as well. > > > > No the case on Windows. > > Can you confirm that you registered for an event and then read the value and the > OnGetRemoteGattEvent didn't get called? > > > I don't see why it should work in that way (read doesn't change the value, > write indicates you know the value changed). Saw you create a bug for it. > > That's not really up to the implementation. The implementation has to match the > interface. If you don't agree with the interface you need to submit a change > changing the description of the callback. > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... If the interface isn't documented precisely enough, please fix it as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:21: (device::BluetoothRemoteGattCharacteristicWin*)context; On 2016/03/08 00:05:36, ortuno wrote: > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > On 2016/03/07 18:48:45, ortuno wrote: > > > Space between parentheses and context. > > > > Is this google code style? It looks making more sense without space and 'git > cl format' will remove space between them. > > Interesting. Well if git cl format says no space then no space it is. Acknowledged. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:187: &OnGetGattEventWin, (PVOID) this); On 2016/03/08 00:05:36, ortuno wrote: > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > On 2016/03/07 18:48:45, ortuno wrote: > > > Wouldn't you get a null pointer dereference if windows calls > OnGetGattEventWin > > > after the characteristic is destroyed? > > > > Event will be unregistered in ~BluetoothRemoteGattCharacteristicWin() > > Could the following happen? > > PostTask(~BluetoothRemoteGattCharacteristicWin()) > PostTask(OnGetGattEventWin()) > > > This would mean that there is a OnGetGattEventWin waiting to be executed, is > ~BluetoothRemoteGattCharacteristicWin going to cancel the pending > OnGetGattEventWin? This could not happen since UnregisterGattCharacteristicValueChangedEvent is a synchronous operation. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:188: gatt_event_registeration_in_progress_ = true; On 2016/03/08 18:30:07, scheib wrote: > On 2016/03/08 00:05:36, ortuno wrote: > > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > > On 2016/03/07 18:48:45, ortuno wrote: > > > > Are you sure the registering for the event is enough? Don't you need to > > write to > > > > the CCC descriptor as well? > > > > > > No needed, > > > > This sort of statements are not useful when trying to understand your process. > > Could you explain how you made sure that writing to the descriptor is not > needed > > and document your findings in the code? > > If a question ever comes up from a reviewer it is best to explain the answer in > code or comments. The same question is likely to come up again for future > readers - and they will not read comments on the code review. My judgement is based on this API document (https://msdn.microsoft.com/en-us/library/windows/hardware/hh450806(v=vs.85).aspx) from Microsoft and experiment, what do you think is the best way to explain it in code or comments? https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:242: void BluetoothRemoteGattCharacteristicWin::OnGetRemoteGattEvent( On 2016/03/08 18:30:07, scheib wrote: > On 2016/03/08 00:05:36, ortuno wrote: > > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > > On 2016/03/07 18:48:45, ortuno wrote: > > > > Does this get called after reading or writing to a characteristic. Android > > and > > > > Chrome OS do it so I wouldn't be surprised if windows does it as well. > > > > > > No the case on Windows. > > > > Can you confirm that you registered for an event and then read the value and > the > > OnGetRemoteGattEvent didn't get called? > > > > > I don't see why it should work in that way (read doesn't change the value, > > write indicates you know the value changed). Saw you create a bug for it. > > > > That's not really up to the implementation. The implementation has to match > the > > interface. If you don't agree with the interface you need to submit a change > > changing the description of the callback. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > If the interface isn't documented precisely enough, please fix it as well. I have confirmed it by experiment. I think it's clear enough. Please suggest specific comments if you think could make it better. https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:942: #endif On 2016/03/08 00:05:36, ortuno wrote: > This needs to be tested more thoroughly. A couple of cases I can think of: > > - Stop while there are pending callbacks > - Stop twice > > I'm sure there is more. Add three separate unit tests which I think make sense. I could also set Stop function to be not implemented for now to give time to design cross platform unit tests. https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:255: new_value->at(i) = new_value_win->Data[i]; On 2016/03/08 00:05:36, ortuno wrote: > nit: You can just use the [] operator: new_value.get()[i] Acknowledged.
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. BUG=579202 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. BUG=579202,592843 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. BUG=579202,592843 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. BUG=579202,592843,427616 ==========
https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:188: gatt_event_registeration_in_progress_ = true; On 2016/03/08 at 19:22:23, gogerald1 wrote: > On 2016/03/08 18:30:07, scheib wrote: > > On 2016/03/08 00:05:36, ortuno wrote: > > > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > > > On 2016/03/07 18:48:45, ortuno wrote: > > > > > Are you sure the registering for the event is enough? Don't you need to > > > write to > > > > > the CCC descriptor as well? > > > > > > > > No needed, > > > > > > This sort of statements are not useful when trying to understand your process. > > > Could you explain how you made sure that writing to the descriptor is not > > needed > > > and document your findings in the code? > > > > If a question ever comes up from a reviewer it is best to explain the answer in > > code or comments. The same question is likely to come up again for future > > readers - and they will not read comments on the code review. > > My judgement is based on this API document (https://msdn.microsoft.com/en-us/library/windows/hardware/hh450806(v=vs.85).aspx) from Microsoft and experiment, what do you think is the best way to explain it in code or comments? Nothing in the link you sent mentions anything about notifications/indications. Could you please explain in detail how you know that you don't need to manually write to the descriptor? Do you have an app that tells you when you write to the descriptors? Or do you have a device like a heart rate monitor that starts notifying after you wrote to the descriptors? Sorry for being too pushy here but we've run into this problem before and I want to make sure we get it right this time. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:242: void BluetoothRemoteGattCharacteristicWin::OnGetRemoteGattEvent( On 2016/03/08 at 19:22:23, gogerald1 wrote: > On 2016/03/08 18:30:07, scheib wrote: > > On 2016/03/08 00:05:36, ortuno wrote: > > > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > > > On 2016/03/07 18:48:45, ortuno wrote: > > > > > Does this get called after reading or writing to a characteristic. Android > > > and > > > > > Chrome OS do it so I wouldn't be surprised if windows does it as well. > > > > > > > > No the case on Windows. > > > > > > Can you confirm that you registered for an event and then read the value and > > the > > > OnGetRemoteGattEvent didn't get called? > > > > > > > I don't see why it should work in that way (read doesn't change the value, > > > write indicates you know the value changed). Saw you create a bug for it. > > > > > > That's not really up to the implementation. The implementation has to match > > the > > > interface. If you don't agree with the interface you need to submit a change > > > changing the description of the callback. > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > > > If the interface isn't documented precisely enough, please fix it as well. > > I have confirmed it by experiment. I think it's clear enough. Please suggest specific comments if you think could make it better. Just to be sure, you registered for the event and then tried reading the characteristic and saw no event? https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:255: new_value->at(i) = new_value_win->Data[i]; On 2016/03/08 at 19:22:23, gogerald1 wrote: > On 2016/03/08 00:05:36, ortuno wrote: > > nit: You can just use the [] operator: new_value.get()[i] > > Acknowledged. Rather than just saying "Ackowledged", you should mention the reasons as to why you didn't follow the comment. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:282: EXPECT_EQ(1, gatt_read_characteristic_attempts_); This seems unrelated to notifications. Why are you changing it? https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:761: EXPECT_EQ(1, error_callback_count_); I don't understand how this test is passing if you never check the characteristics properties. We should check last_gatt_error_code_ to make sure we are running the callback with the right error. This applies to the other tests as well. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:876: // Tests Characteristic Value changing after a Notify Session and objects being Please add a comment as to why this doesn't apply to windows. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:936: #if defined(OS_WIN) The other tests have a comment about the test. We should be consistent and add comments to these new tests. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:964: EXPECT_EQ(1, callback_count_); I think you need a EXPECT_EQ(0, stop_notifications_attempt_) or similar. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:972: EXPECT_EQ(2, callback_count_); Same here. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:980: EXPECT_EQ(3, callback_count_); And here you would have EXPECT_EQ(1, stop_notifications_attempts_) or similar. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:987: EXPECT_EQ(4, callback_count_); Check stop_notifications_attempts here as well. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:1077: #endif // defined(OS_WIN) Nice tests. Another test I think we should do: 1. StartNotifySession whose callback stops notifications. 2. StartNotifySession. 3. Expect device to be notifying and the last session to still be active. Another test to cover a previous bug: 1. StartNotifySession whose callback starts another notify session. 2. Expect only one start_notifications_attempt_ and both sessions to be active. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:181: You should check that the characteristic supports notifications or indications.
https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:181: You should check that the characteristic supports notifications or indications.
Patchset #8 (id:220001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #8 (id:240001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/260001
https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:188: gatt_event_registeration_in_progress_ = true; On 2016/03/09 16:53:59, ortuno wrote: > On 2016/03/08 at 19:22:23, gogerald1 wrote: > > On 2016/03/08 18:30:07, scheib wrote: > > > On 2016/03/08 00:05:36, ortuno wrote: > > > > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > > > > On 2016/03/07 18:48:45, ortuno wrote: > > > > > > Are you sure the registering for the event is enough? Don't you need > to > > > > write to > > > > > > the CCC descriptor as well? > > > > > > > > > > No needed, > > > > > > > > This sort of statements are not useful when trying to understand your > process. > > > > Could you explain how you made sure that writing to the descriptor is not > > > needed > > > > and document your findings in the code? > > > > > > If a question ever comes up from a reviewer it is best to explain the answer > in > > > code or comments. The same question is likely to come up again for future > > > readers - and they will not read comments on the code review. > > > > My judgement is based on this API document > (https://msdn.microsoft.com/en-us/library/windows/hardware/hh450806(v=vs.85).aspx) > from Microsoft and experiment, what do you think is the best way to explain it > in code or comments? > > Nothing in the link you sent mentions anything about notifications/indications. > Could you please explain in detail how you know that you don't need to manually > write to the descriptor? Do you have an app that tells you when you write to the > descriptors? Or do you have a device like a heart rate monitor that starts > notifying after you wrote to the descriptors? > > Sorry for being too pushy here but we've run into this problem before and I want > to make sure we get it right this time. I definitely have told you that I have experimented these functionalities by using Smartsetup extensions and Simulator app with Nexus 6. My Nexus 6 and chrome can receive correct information. https://codereview.chromium.org/1749403002/diff/120001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:242: void BluetoothRemoteGattCharacteristicWin::OnGetRemoteGattEvent( On 2016/03/09 16:53:59, ortuno wrote: > On 2016/03/08 at 19:22:23, gogerald1 wrote: > > On 2016/03/08 18:30:07, scheib wrote: > > > On 2016/03/08 00:05:36, ortuno wrote: > > > > On 2016/03/07 at 22:52:49, gogerald1 wrote: > > > > > On 2016/03/07 18:48:45, ortuno wrote: > > > > > > Does this get called after reading or writing to a characteristic. > Android > > > > and > > > > > > Chrome OS do it so I wouldn't be surprised if windows does it as well. > > > > > > > > > > No the case on Windows. > > > > > > > > Can you confirm that you registered for an event and then read the value > and > > > the > > > > OnGetRemoteGattEvent didn't get called? > > > > > > > > > I don't see why it should work in that way (read doesn't change the > value, > > > > write indicates you know the value changed). Saw you create a bug for it. > > > > > > > > That's not really up to the implementation. The implementation has to > match > > > the > > > > interface. If you don't agree with the interface you need to submit a > change > > > > changing the description of the callback. > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > > > > > If the interface isn't documented precisely enough, please fix it as well. > > > > I have confirmed it by experiment. I think it's clear enough. Please suggest > specific comments if you think could make it better. > > Just to be sure, you registered for the event and then tried reading the > characteristic and saw no event? yes https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:282: EXPECT_EQ(1, gatt_read_characteristic_attempts_); On 2016/03/09 16:53:59, ortuno wrote: > This seems unrelated to notifications. Why are you changing it? The previous place of this check assumes ReadRemoteCharacteristic is a synchronous operation down to OS, however this is not the case on Windows. Made this change after discussing with Vincent. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:761: EXPECT_EQ(1, error_callback_count_); On 2016/03/09 16:53:59, ortuno wrote: > I don't understand how this test is passing if you never check the > characteristics properties. We should check last_gatt_error_code_ to make sure > we are running the callback with the right error. This applies to the other > tests as well. No idea how this unit test has been designed, may be a separate CL could be made to fix the tests you indicate. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:876: // Tests Characteristic Value changing after a Notify Session and objects being On 2016/03/09 16:53:59, ortuno wrote: > Please add a comment as to why this doesn't apply to windows. Done. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:936: #if defined(OS_WIN) On 2016/03/09 16:53:59, ortuno wrote: > The other tests have a comment about the test. We should be consistent and add > comments to these new tests. Done. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:964: EXPECT_EQ(1, callback_count_); On 2016/03/09 16:53:59, ortuno wrote: > I think you need a EXPECT_EQ(0, stop_notifications_attempt_) or similar. I don't see how much benefit with it on Windows. I'd like to leave it for the future work since I don't see symmetric start_notifications_attempt_ or similar. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:972: EXPECT_EQ(2, callback_count_); On 2016/03/09 16:53:59, ortuno wrote: > Same here. ditto https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:980: EXPECT_EQ(3, callback_count_); On 2016/03/09 16:54:00, ortuno wrote: > And here you would have EXPECT_EQ(1, stop_notifications_attempts_) or similar. ditto https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:987: EXPECT_EQ(4, callback_count_); On 2016/03/09 16:53:59, ortuno wrote: > Check stop_notifications_attempts here as well. ditto https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:1077: #endif // defined(OS_WIN) On 2016/03/09 16:54:00, ortuno wrote: > Nice tests. Another test I think we should do: > > 1. StartNotifySession whose callback stops notifications. > 2. StartNotifySession. > 3. Expect device to be notifying and the last session to still be active. > > Another test to cover a previous bug: > > 1. StartNotifySession whose callback starts another notify session. > 2. Expect only one start_notifications_attempt_ and both sessions to be active. Do not understand the first suggested test. For the second one, why "Expect only one start_notifications_attempt_"?. In addition, above unit tests has already made sure StartNotifySession is an asynchronous call. So I think above three tests is good enough for Windows for now. We could make a perfect cross platform unit test cover in the future given that I am not dedicated working on test fixture. Or I can set BluetoothGattNotifySessionWin::Stop to be "NOTIMPLEMENTED" as other platforms to give more time to design tests. https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:181: On 2016/03/09 16:54:00, ortuno wrote: > You should check that the characteristic supports notifications or indications. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #9 (id:280001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/290001
I split out BluetoothGattNotifySessionWin::Stop for now to give more time to design unit tests properly.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/310001
https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:255: new_value->at(i) = new_value_win->Data[i]; On 2016/03/09 16:53:59, ortuno wrote: > On 2016/03/08 at 19:22:23, gogerald1 wrote: > > On 2016/03/08 00:05:36, ortuno wrote: > > > nit: You can just use the [] operator: new_value.get()[i] > > > > Acknowledged. > > Rather than just saying "Ackowledged", you should mention the reasons as to why > you didn't follow the comment. I suppose nit means optional, ->at(i) looks neat and easy understand, anyway, I've changed it to (*new_value)[i].
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
@scheib: I left a question for you. PTAL. @gogerald: You should open a different issue for each new feature you are implementing. https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:255: new_value->at(i) = new_value_win->Data[i]; On 2016/03/10 at 16:10:54, gogerald1 wrote: > On 2016/03/09 16:53:59, ortuno wrote: > > On 2016/03/08 at 19:22:23, gogerald1 wrote: > > > On 2016/03/08 00:05:36, ortuno wrote: > > > > nit: You can just use the [] operator: new_value.get()[i] > > > > > > Acknowledged. > > > > Rather than just saying "Ackowledged", you should mention the reasons as to why > > you didn't follow the comment. > > I suppose nit means optional, ->at(i) looks neat and easy understand, anyway, I've changed it to (*new_value)[i]. Yes nit means optional. But it's important you communicate why you don't do it, that way we can both learn something :) https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:1077: #endif // defined(OS_WIN) On 2016/03/09 at 23:59:12, gogerald1 wrote: > On 2016/03/09 16:54:00, ortuno wrote: > > Nice tests. Another test I think we should do: > > > > 1. StartNotifySession whose callback stops notifications. > > 2. StartNotifySession. > > 3. Expect device to be notifying and the last session to still be active. > > > > Another test to cover a previous bug: > > > > 1. StartNotifySession whose callback starts another notify session. > > 2. Expect only one start_notifications_attempt_ and both sessions to be active. > > Do not understand the first suggested test. You no longer need this test since you are not implementing stop notify session yet. > For the second one, why "Expect only one start_notifications_attempt_"?. Sorry, I meant "gatt_notify_characteristic_attempts_". gatt_notify_characteristic_attempts_ should track the number of calls to BluetoothGattRegisterEvent. You had a bug earlier in which the following code would result in BluetoothGattRegisterEvent being called twice: characteristic1_.StartNotifySession([](scoped_ptr<BluetoothGattNotifySession> session) { notify_sessions.push_back(std::move(session)); characteristic1_->StartNotifySession( GetNotifyCallback(Call::EXPECTED), GetGattErrorCallback(Call::NOT_EXPECTED)); }); > In addition, above unit tests has already made sure StartNotifySession is an asynchronous call. So I think above three tests is good enough for Windows for now. We could make a perfect cross platform unit test cover in the future given that I am not dedicated working on test fixture. Or I can set BluetoothGattNotifySessionWin::Stop to be "NOTIMPLEMENTED" as other platforms to give more time to design tests. This has little to do with the test fixture. You are implementing a feature and new features require good test coverage. The tests I proposed are increasing the test coverage for your implementation. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:103: EXPECT_EQ(1, gatt_notify_characteristic_attempts_); Since it's not related to notifications I think this should have been done in a separate CL. Either do it in a separate CL or mention the change, why it is being made in the description and what are the consequences. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:747: #if defined(OS_ANDROID) || defined(OS_WIN) I think you misunderstood the purpose of these tests. (I don't blame you, I find these tests very confusing as well.) Let me explain the purpose of each test you enabled. @scheib please correct if I got something wrong. StartNotifySession_NoNotifyOrIndicate: The purpose of this test is to make sure implementations check that the characteristic has "Notify" or "Indicate" in it's properties. If the characteristic does not then the implementation should call the error callback with GATT_NOT_SUPPORTED. The code that checks for "Notify" or "Indicate" should be in BluetoothRemoteGattCharacteristicWin::StartNotifySession not in the fake RegisterEvent. StartNotifySession_NoConfigDescriptor: The purpose of this test is to make sure implementations check that the CCC descriptor exists before trying to enable notifications. If the descriptor doesn't exist then the implementation should return GATT_NOT_SUPPORTED. Similarly here, it should be the actual implementation i.e. BluetoothRemoteGattCharacteristicWin::StartNotifySession that performs this check, not the fake RegisterEvent. These last two tests don't check that the correct error is actually returned. This is because the android implementation does not yet return the right errors. Please add some code to these two tests to make sure the right error is being thrown. The code should be surrounded by: #if !defined(OS_ANDROID) // Android doesn't return the correct errors. http://crbug.com/584369 To understand the following two tests you need to know more about the Android implementation. In Android we perform two actions to enable notifications: 1. Call setCharacteristicNotification[1] 2. Write to the CCC descriptor. StartNotifySession_FailToSetCharacteristicNotification: The purpose of this test is to make sure the android implementation handles errors from step 1, call setCharacteristicNotification. I think this test is Android specific and you don't need to enable it on windows. StartNotifySession_WriteDescriptorSynchronousError The purpose of this tests is to make sure the Android implementation handles errors from step 2, write to the descriptor. I think this test is Android specific and you don't need to enable it on windows. StartNotifySession The purpose of this test is to make sure that when the characteristic supports "Notify" the implementation: 1. Tried to register for notifications. This is done through gatt_notify_characteristic_attempts_. In the case of Android the value of the variable is increased when setCharacteristicNotification is called. In windows, the value of this variable should be increased when you call RegisterEvent. 2. Called the success callback with a notify session. 3. Created only one notify session that points to the correct characteristic and is active. 6. Wrote 0x10 to the CCC descriptor. Now I don't think this one applies to Windows. @scheib: Do you think it's OK to surround the part of the code that checks for this with a #if !defined(OS_WINDOWS)?https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc&q=fakecharacteristicboilerplate&sq=package:chromium&type=cs&l=112 StartNotifySession_OnIndicate The purpose of this test is very similar to the previous one, except that 0x20 should be written to the descriptor because the characteristic supports "Indicate". The previous #if !defined(OS_WIN) should handle this as well. StartNotifySession_OnNotifyAndIndicate Similar to the two last tests except that the characteristic supports Notify and Indicate and the test expects 0x10 to be written to the descriptor. As you can see many of these tests are android specific and I'm sure there are errors that are Windows specific. Don't be afraid to add functions to BluetoothTest that tests windows only behavior. I'm sure RegisterEvent returns errors not yet covered by these tests. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:879: // deleting the device. [...] after deleting the device because we synchronously unregister from notifications when the BluetoothRemoteGattCharacteristic object is destroyed. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win_fake.cc (right): https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:244: HRESULT BluetoothLowEnergyWrapperFake::RegisterGattEvents( I hope that from the description of the tests it becomes clear that this fake is much more complicated than it should. This function should either: 1. Increase the value of gatt_notify_characteristic_attempts_, create the observer and succeed or 2. Fail with some windows specific error. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:468: for (auto observer : parent_service->observers) { Here you are making copies of the "BLUETOOTH_GATT_EVENT_HANDLE" in the vector. Is this what want? Consider using const auto& if not. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:469: GattServiceObserverTable::iterator it = nit: You can use auto for iterators. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:477: characteristic->value->DataSize + (ULONG)sizeof(ULONG); Why do you add the (ULONG)sizeof(ULONG)?
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check after simulate operation since the operation is no necessary synchronous down to the OS. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check after simulate operation since the operation is no necessary synchronous down to the OS. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check after simulate operation since the operation is no necessary synchronous down to the OS. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check after simulate operation since the operation is no necessary synchronous down to the OS. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check immediately after a operation to after simulate operation since the operation is no necessary synchronous down to the OS. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check immediately after a operation to after simulate operation since the operation is no necessary synchronous down to the OS. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check immediately after a operation to after simulate operation since the operation is no necessary synchronous down to the OS. BUG=579202,592843,427616 ==========
https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/180001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:255: new_value->at(i) = new_value_win->Data[i]; On 2016/03/14 01:37:37, ortuno wrote: > On 2016/03/10 at 16:10:54, gogerald1 wrote: > > On 2016/03/09 16:53:59, ortuno wrote: > > > On 2016/03/08 at 19:22:23, gogerald1 wrote: > > > > On 2016/03/08 00:05:36, ortuno wrote: > > > > > nit: You can just use the [] operator: new_value.get()[i] > > > > > > > > Acknowledged. > > > > > > Rather than just saying "Ackowledged", you should mention the reasons as to > why > > > you didn't follow the comment. > > > > I suppose nit means optional, ->at(i) looks neat and easy understand, anyway, > I've changed it to (*new_value)[i]. > > Yes nit means optional. But it's important you communicate why you don't do it, > that way we can both learn something :) nit == nitpick == a small, possibly trivial, problem (http://www.merriam-webster.com/dictionary/nit). It should not be used to imply optional though. Reviewers should explicitly state anything that is optional, and authors should reply to all comments. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:747: #if defined(OS_ANDROID) || defined(OS_WIN) On 2016/03/14 01:37:37, ortuno wrote: > I think you misunderstood the purpose of these tests. (I don't blame you, I find > these tests very confusing as well.) Let me explain the purpose of each test you > enabled. @scheib please correct if I got something wrong. > > StartNotifySession_NoNotifyOrIndicate: > The purpose of this test is to make sure implementations check that the > characteristic has "Notify" or "Indicate" in it's properties. If the > characteristic does not then the implementation should call the error callback > with GATT_NOT_SUPPORTED. The code that checks for "Notify" or "Indicate" should > be in BluetoothRemoteGattCharacteristicWin::StartNotifySession not in the fake > RegisterEvent. > > StartNotifySession_NoConfigDescriptor: > The purpose of this test is to make sure implementations check that the CCC > descriptor exists before trying to enable notifications. If the descriptor > doesn't exist then the implementation should return GATT_NOT_SUPPORTED. > Similarly here, it should be the actual implementation i.e. > BluetoothRemoteGattCharacteristicWin::StartNotifySession that performs this > check, not the fake RegisterEvent. > > These last two tests don't check that the correct error is actually returned. > This is because the android implementation does not yet return the right errors. > Please add some code to these two tests to make sure the right error is being > thrown. The code should be surrounded by: > > #if !defined(OS_ANDROID) // Android doesn't return the correct errors. > http://crbug.com/584369 > > To understand the following two tests you need to know more about the Android > implementation. In Android we perform two actions to enable notifications: > 1. Call setCharacteristicNotification[1] > 2. Write to the CCC descriptor. I agree with everything above. gogerald@ please add these descriptions to the test comments. > > StartNotifySession_FailToSetCharacteristicNotification: > The purpose of this test is to make sure the android implementation handles > errors from step 1, call setCharacteristicNotification. I think this test is > Android specific and you don't need to enable it on windows. I agree mostly, but not that this is necessarily Android specific. I do agree that this isn't relevant for Windows because when the Windows implementation performs this step it is asynchronous. gogerald@ please make a comment explaining this exception and use #if !defined(OS_WINDOWS) guards with comments (or referencing a single explanation). > > StartNotifySession_WriteDescriptorSynchronousError > The purpose of this tests is to make sure the Android implementation handles > errors from step 2, > write to the descriptor. I think this test is Android specific and you don't > need to enable it on windows. This too is not relevant for Windows, but should be excluded for Windows only similar to above. > StartNotifySession > The purpose of this test is to make sure that when the characteristic supports > "Notify" the implementation: > 1. Tried to register for notifications. This is done through > gatt_notify_characteristic_attempts_. In the case of Android the value of the > variable is increased when setCharacteristicNotification is called. In windows, > the value of this variable should be increased when you call RegisterEvent. > 2. Called the success callback with a notify session. > 3. Created only one notify session that points to the correct characteristic and > is active. > 6. Wrote 0x10 to the CCC descriptor. Now I don't think this one applies to > Windows. @scheib: Do you think it's OK to surround the part of the code that > checks for this with a #if > !defined(OS_WINDOWS)?https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc&q=fakecharacteristicboilerplate&sq=package:chromium&type=cs&l=112 Yes. > > StartNotifySession_OnIndicate > The purpose of this test is very similar to the previous one, except that 0x20 > should be written to the descriptor because the characteristic supports > "Indicate". The previous #if !defined(OS_WIN) should handle this as well. > > StartNotifySession_OnNotifyAndIndicate > Similar to the two last tests except that the characteristic supports Notify and > Indicate and the test expects 0x10 to be written to the descriptor. > > > > As you can see many of these tests are android specific and I'm sure there are > errors that are Windows specific. Don't be afraid to add functions to > BluetoothTest that tests windows only behavior. I'm sure RegisterEvent returns > errors not yet covered by these tests. Yes to all above here as well.
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check immediately after a operation to after simulate operation since the operation is no necessary synchronous down to the OS. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check immediately after an operation to after simulate operation since the operation is no necessary synchronous down to the OS on different platforms. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check immediately after an operation to after simulate operation since the operation is no necessary synchronous down to the OS on different platforms. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check immediately after an operation to after simulate operation since the operation is no necessary synchronous down to the OS on different platforms. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, this CL moves attempts check immediately after an operation to after simulate operation since the operation is no necessary synchronous down to the OS on different platforms. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, it moves attempts check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is not synchronous down to the OS on Windows. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, it moves attempts check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is not synchronous down to the OS on Windows. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, it moves attempts check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is not synchronous down to the OS on Windows. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, it moves attempts check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is not synchronous down to the OS on Windows. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, it moves *_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is not synchronous down to the OS on Windows. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, it moves *_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is not synchronous down to the OS on Windows. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, it moves *_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between sematically different integer types issue. In addition, it moves *_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves *_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves *_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616 ==========
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616 ==========
Patchset #11 (id:330001) has been deleted
Patchset #11 (id:350001) has been deleted
Patchset #11 (id:370001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/390001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/390001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/430001
PTAL https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/200001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:1077: #endif // defined(OS_WIN) On 2016/03/14 01:37:37, ortuno wrote: > On 2016/03/09 at 23:59:12, gogerald1 wrote: > > On 2016/03/09 16:54:00, ortuno wrote: > > > Nice tests. Another test I think we should do: > > > > > > 1. StartNotifySession whose callback stops notifications. > > > 2. StartNotifySession. > > > 3. Expect device to be notifying and the last session to still be active. > > > > > > Another test to cover a previous bug: > > > > > > 1. StartNotifySession whose callback starts another notify session. > > > 2. Expect only one start_notifications_attempt_ and both sessions to be > active. > > > > Do not understand the first suggested test. > > You no longer need this test since you are not implementing stop notify session > yet. > > > For the second one, why "Expect only one start_notifications_attempt_"?. > > Sorry, I meant "gatt_notify_characteristic_attempts_". > gatt_notify_characteristic_attempts_ should track the number of calls to > BluetoothGattRegisterEvent. You had a bug earlier in which the following code > would result in BluetoothGattRegisterEvent being called twice: > > characteristic1_.StartNotifySession([](scoped_ptr<BluetoothGattNotifySession> > session) { > notify_sessions.push_back(std::move(session)); > characteristic1_->StartNotifySession( > GetNotifyCallback(Call::EXPECTED), > GetGattErrorCallback(Call::NOT_EXPECTED)); > }); > > > In addition, above unit tests has already made sure StartNotifySession is an > asynchronous call. So I think above three tests is good enough for Windows for > now. We could make a perfect cross platform unit test cover in the future given > that I am not dedicated working on test fixture. Or I can set > BluetoothGattNotifySessionWin::Stop to be "NOTIMPLEMENTED" as other platforms to > give more time to design tests. > > This has little to do with the test fixture. You are implementing a feature and > new features require good test coverage. The tests I proposed are increasing the > test coverage for your implementation. Done. No longer need these tests in this CL https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:103: EXPECT_EQ(1, gatt_notify_characteristic_attempts_); On 2016/03/14 01:37:37, ortuno wrote: > Since it's not related to notifications I think this should have been done in a > separate CL. Either do it in a separate CL or mention the change, why it is > being made in the description and what are the consequences. Done. Mentioned in CL description. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:747: #if defined(OS_ANDROID) || defined(OS_WIN) On 2016/03/14 01:37:37, ortuno wrote: > I think you misunderstood the purpose of these tests. (I don't blame you, I find > these tests very confusing as well.) Let me explain the purpose of each test you > enabled. @scheib please correct if I got something wrong. > > StartNotifySession_NoNotifyOrIndicate: > The purpose of this test is to make sure implementations check that the > characteristic has "Notify" or "Indicate" in it's properties. If the > characteristic does not then the implementation should call the error callback > with GATT_NOT_SUPPORTED. The code that checks for "Notify" or "Indicate" should > be in BluetoothRemoteGattCharacteristicWin::StartNotifySession not in the fake > RegisterEvent. > > StartNotifySession_NoConfigDescriptor: > The purpose of this test is to make sure implementations check that the CCC > descriptor exists before trying to enable notifications. If the descriptor > doesn't exist then the implementation should return GATT_NOT_SUPPORTED. > Similarly here, it should be the actual implementation i.e. > BluetoothRemoteGattCharacteristicWin::StartNotifySession that performs this > check, not the fake RegisterEvent. > > These last two tests don't check that the correct error is actually returned. > This is because the android implementation does not yet return the right errors. > Please add some code to these two tests to make sure the right error is being > thrown. The code should be surrounded by: > > #if !defined(OS_ANDROID) // Android doesn't return the correct errors. > http://crbug.com/584369 > > To understand the following two tests you need to know more about the Android > implementation. In Android we perform two actions to enable notifications: > 1. Call setCharacteristicNotification[1] > 2. Write to the CCC descriptor. > > StartNotifySession_FailToSetCharacteristicNotification: > The purpose of this test is to make sure the android implementation handles > errors from step 1, call setCharacteristicNotification. I think this test is > Android specific and you don't need to enable it on windows. > > StartNotifySession_WriteDescriptorSynchronousError > The purpose of this tests is to make sure the Android implementation handles > errors from step 2, > write to the descriptor. I think this test is Android specific and you don't > need to enable it on windows. > > StartNotifySession > The purpose of this test is to make sure that when the characteristic supports > "Notify" the implementation: > 1. Tried to register for notifications. This is done through > gatt_notify_characteristic_attempts_. In the case of Android the value of the > variable is increased when setCharacteristicNotification is called. In windows, > the value of this variable should be increased when you call RegisterEvent. > 2. Called the success callback with a notify session. > 3. Created only one notify session that points to the correct characteristic and > is active. > 6. Wrote 0x10 to the CCC descriptor. Now I don't think this one applies to > Windows. @scheib: Do you think it's OK to surround the part of the code that > checks for this with a #if > !defined(OS_WINDOWS)?https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc&q=fakecharacteristicboilerplate&sq=package:chromium&type=cs&l=112 > > StartNotifySession_OnIndicate > The purpose of this test is very similar to the previous one, except that 0x20 > should be written to the descriptor because the characteristic supports > "Indicate". The previous #if !defined(OS_WIN) should handle this as well. > > StartNotifySession_OnNotifyAndIndicate > Similar to the two last tests except that the characteristic supports Notify and > Indicate and the test expects 0x10 to be written to the descriptor. > > > > As you can see many of these tests are android specific and I'm sure there are > errors that are Windows specific. Don't be afraid to add functions to > BluetoothTest that tests windows only behavior. I'm sure RegisterEvent returns > errors not yet covered by these tests. Done. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:747: #if defined(OS_ANDROID) || defined(OS_WIN) On 2016/03/14 17:54:46, scheib wrote: > On 2016/03/14 01:37:37, ortuno wrote: > > I think you misunderstood the purpose of these tests. (I don't blame you, I > find > > these tests very confusing as well.) Let me explain the purpose of each test > you > > enabled. @scheib please correct if I got something wrong. > > > > StartNotifySession_NoNotifyOrIndicate: > > The purpose of this test is to make sure implementations check that the > > characteristic has "Notify" or "Indicate" in it's properties. If the > > characteristic does not then the implementation should call the error callback > > with GATT_NOT_SUPPORTED. The code that checks for "Notify" or "Indicate" > should > > be in BluetoothRemoteGattCharacteristicWin::StartNotifySession not in the fake > > RegisterEvent. > > > > StartNotifySession_NoConfigDescriptor: > > The purpose of this test is to make sure implementations check that the CCC > > descriptor exists before trying to enable notifications. If the descriptor > > doesn't exist then the implementation should return GATT_NOT_SUPPORTED. > > Similarly here, it should be the actual implementation i.e. > > BluetoothRemoteGattCharacteristicWin::StartNotifySession that performs this > > check, not the fake RegisterEvent. > > > > These last two tests don't check that the correct error is actually returned. > > This is because the android implementation does not yet return the right > errors. > > Please add some code to these two tests to make sure the right error is being > > thrown. The code should be surrounded by: > > > > #if !defined(OS_ANDROID) // Android doesn't return the correct errors. > > http://crbug.com/584369 > > > > To understand the following two tests you need to know more about the Android > > implementation. In Android we perform two actions to enable notifications: > > 1. Call setCharacteristicNotification[1] > > 2. Write to the CCC descriptor. > > I agree with everything above. > gogerald@ please add these descriptions to the test comments. > > > > > StartNotifySession_FailToSetCharacteristicNotification: > > The purpose of this test is to make sure the android implementation handles > > errors from step 1, call setCharacteristicNotification. I think this test is > > Android specific and you don't need to enable it on windows. > > I agree mostly, but not that this is necessarily Android specific. I do > agree that this isn't relevant for Windows because when the Windows > implementation performs this step it is asynchronous. > > gogerald@ please make a comment explaining this exception and use > #if !defined(OS_WINDOWS) > guards with comments (or referencing a single explanation). > > > > > > StartNotifySession_WriteDescriptorSynchronousError > > The purpose of this tests is to make sure the Android implementation handles > > errors from step 2, > > write to the descriptor. I think this test is Android specific and you don't > > need to enable it on windows. > > This too is not relevant for Windows, but should be excluded for Windows > only similar to above. > > > StartNotifySession > > The purpose of this test is to make sure that when the characteristic supports > > "Notify" the implementation: > > 1. Tried to register for notifications. This is done through > > gatt_notify_characteristic_attempts_. In the case of Android the value of the > > variable is increased when setCharacteristicNotification is called. In > windows, > > the value of this variable should be increased when you call RegisterEvent. > > 2. Called the success callback with a notify session. > > 3. Created only one notify session that points to the correct characteristic > and > > is active. > > 6. Wrote 0x10 to the CCC descriptor. Now I don't think this one applies to > > Windows. @scheib: Do you think it's OK to surround the part of the code that > > checks for this with a #if > > > !defined(OS_WINDOWS)?https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/bluetooth_gatt_characteristic_unittest.cc&q=fakecharacteristicboilerplate&sq=package:chromium&type=cs&l=112 > > Yes. > > > > > StartNotifySession_OnIndicate > > The purpose of this test is very similar to the previous one, except that 0x20 > > should be written to the descriptor because the characteristic supports > > "Indicate". The previous #if !defined(OS_WIN) should handle this as well. > > > > StartNotifySession_OnNotifyAndIndicate > > Similar to the two last tests except that the characteristic supports Notify > and > > Indicate and the test expects 0x10 to be written to the descriptor. > > > > > > > > As you can see many of these tests are android specific and I'm sure there are > > errors that are Windows specific. Don't be afraid to add functions to > > BluetoothTest that tests windows only behavior. I'm sure RegisterEvent returns > > errors not yet covered by these tests. > > Yes to all above here as well. Done. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:879: // deleting the device. On 2016/03/14 01:37:37, ortuno wrote: > [...] after deleting the device because we synchronously unregister from > notifications when the BluetoothRemoteGattCharacteristic object is destroyed. Done. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win_fake.cc (right): https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:244: HRESULT BluetoothLowEnergyWrapperFake::RegisterGattEvents( On 2016/03/14 01:37:37, ortuno wrote: > I hope that from the description of the tests it becomes clear that this fake is > much more complicated than it should. This function should either: > > 1. Increase the value of gatt_notify_characteristic_attempts_, create the > observer and succeed > or 2. Fail with some windows specific error. Done. until now I don't see specific error needs to be tested. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:468: for (auto observer : parent_service->observers) { On 2016/03/14 01:37:37, ortuno wrote: > Here you are making copies of the "BLUETOOTH_GATT_EVENT_HANDLE" in the vector. > Is this what want? Consider using const auto& if not. Done. https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:469: GattServiceObserverTable::iterator it = On 2016/03/14 01:37:37, ortuno wrote: > nit: You can use auto for iterators. Acknowledged. Here use GattServiceObserverTable::iterator can explicitly indicate 'it' is a iterator https://codereview.chromium.org/1749403002/diff/310001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:477: characteristic->value->DataSize + (ULONG)sizeof(ULONG); On 2016/03/14 01:37:37, ortuno wrote: > Why do you add the (ULONG)sizeof(ULONG)? Done. Mistake
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng 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_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/450001
One test and a couple of minor things. Almost there! https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:766: #ifndef OS_ANDROID I think the style guide prefers #if defined(OS_ANDROID) https://www.chromium.org/developers/coding-style#TOC-Platform-specific-code https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:769: #endif #endif // defined(OS_ANDROID) https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:927: // being destroyed. What happens when you enable this test on windows? https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:943: You still need to write a reentrancy test: // Test // ... characteristic1_.StartNotifySession(GetReentrantStartNotifySessionCallback( Call::EXPECTED, characteristic1_)); EXPECT_EQ(0, callback_count_); SimulateGattNotifySessionStarted(characteristic1_); EXPECT_EQ(1, gatt_notify_characteristic_attempts_); EXPECT_EQ(2, callback_count_); EXPECT_EQ(0, error_callback_count_); ASSERT_EQ(2u, notify_sessions_.size()); ASSERT_TRUE(notify_sessions_[1]); EXPECT_EQ(characteristic1_->GetIdentifier(), notify_sessions_[0]->GetCharacteristicIdentifier()); EXPECT_TRUE(notify_sessions_[0]->IsActive()); EXPECT_EQ(characteristic1_->GetIdentifier(), notify_sessions_[1]->GetCharacteristicIdentifier()); EXPECT_TRUE(notify_sessions_[1]->IsActive()); // ... // bluetooth_test.cc BluetoothGattCharacteristic::NotifySessionCallback BluetoothTestBase::GetReentrantStartNotifySessionCallback( Call expected, BluetoothGattCharacteristic* characteristic) { if (expected == Call::EXPECTED) ++expected_success_callback_calls_; return base::Bind(&BluetoothTestBase::ReentrantStartNotifySessionCallback, weak_factory_.GetWeakPtr(), expected, characteristic); } void BluetoothTestBase::ReentrantStartNotifySessionCallback( Call expected, BluetoothGattCharacteristic* characteristic, scoped_ptr<BluetoothGattNotifySession> notify_session) { ++callback_count_; notify_sessions_.push_back(notify_session.release()); if (expected == Call::EXPECTED) ++actual_success_callback_calls_; else unexpected_success_callback_ = true; characteristic->StartNotifySesssion(GetNotifyCallback(Call::EXPECTED)); } https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_notify_session_win.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.cc:15: if (characteristic_.get() != nullptr) You don't need the "!= nullptr", characteristic_.get() should be enough. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_notify_session_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.h:22: std::string GetCharacteristicIdentifier() const override; // Override BluetoothGattNotifySession interfaces. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:183: // unique handle in OS for this registration. Could you add here that this function will write to the CCC descriptor? https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win_fake.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:254: base::string16 device_address = Why do you go through all the trouble of retrieving the device and the service? https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:449: characteristic->value->DataSize + sizeof(ULONG); Why isn't event.CharacteristicValueDataSize == characteristic->value->DataSize? https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:395: if (!IsNotifying()) I don't think you need this. The only time gatt_event_handle_ is false (and IsNotifying() is false) is after destruction. Since you passed a weak_ptr to the callback then we would never hit that case. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:160: base::AutoLock auto_lock(characteristic_value_changed_registrations_lock); Shouldn't you try to acquire this lock as soon as possible? https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.h:177: // asynchronously through |callback|. Post a task to register to receive value changed notifications from |characteristic| of |service_path|. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.h:184: // Post unregister characteristic value change notification. |event_handle| Post a task to unregister from value change notifications.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:766: #ifndef OS_ANDROID On 2016/03/15 02:55:31, ortuno wrote: > I think the style guide prefers #if defined(OS_ANDROID) > > https://www.chromium.org/developers/coding-style#TOC-Platform-specific-code Done. Do not see it explicitly, especially this looks only for Android, anyway, change it to reviewer happy. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:769: #endif On 2016/03/15 02:55:31, ortuno wrote: > #endif // defined(OS_ANDROID) Done. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:927: // being destroyed. On 2016/03/15 02:55:31, ortuno wrote: > What happens when you enable this test on windows? SimulateGattCharacteristicChanged has no target after the device has been deleted from the OS. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:943: On 2016/03/15 02:55:31, ortuno wrote: > You still need to write a reentrancy test: > > // Test > // ... > characteristic1_.StartNotifySession(GetReentrantStartNotifySessionCallback( > Call::EXPECTED, characteristic1_)); > EXPECT_EQ(0, callback_count_); > SimulateGattNotifySessionStarted(characteristic1_); > EXPECT_EQ(1, gatt_notify_characteristic_attempts_); > EXPECT_EQ(2, callback_count_); > EXPECT_EQ(0, error_callback_count_); > ASSERT_EQ(2u, notify_sessions_.size()); > ASSERT_TRUE(notify_sessions_[1]); > EXPECT_EQ(characteristic1_->GetIdentifier(), > notify_sessions_[0]->GetCharacteristicIdentifier()); > EXPECT_TRUE(notify_sessions_[0]->IsActive()); > EXPECT_EQ(characteristic1_->GetIdentifier(), > notify_sessions_[1]->GetCharacteristicIdentifier()); > EXPECT_TRUE(notify_sessions_[1]->IsActive()); > // ... > > // bluetooth_test.cc > BluetoothGattCharacteristic::NotifySessionCallback > BluetoothTestBase::GetReentrantStartNotifySessionCallback( > Call expected, > BluetoothGattCharacteristic* characteristic) { > if (expected == Call::EXPECTED) > ++expected_success_callback_calls_; > return base::Bind(&BluetoothTestBase::ReentrantStartNotifySessionCallback, > weak_factory_.GetWeakPtr(), expected, characteristic); > } > > void BluetoothTestBase::ReentrantStartNotifySessionCallback( > Call expected, > BluetoothGattCharacteristic* characteristic, > scoped_ptr<BluetoothGattNotifySession> notify_session) { > ++callback_count_; > notify_sessions_.push_back(notify_session.release()); > > if (expected == Call::EXPECTED) > ++actual_success_callback_calls_; > else > unexpected_success_callback_ = true; > > characteristic->StartNotifySesssion(GetNotifyCallback(Call::EXPECTED)); > } Do it in the next CL https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_notify_session_win.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.cc:15: if (characteristic_.get() != nullptr) On 2016/03/15 02:55:31, ortuno wrote: > You don't need the "!= nullptr", characteristic_.get() should be enough. Done. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_notify_session_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.h:22: std::string GetCharacteristicIdentifier() const override; On 2016/03/15 02:55:31, ortuno wrote: > // Override BluetoothGattNotifySession interfaces. Done. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:183: // unique handle in OS for this registration. On 2016/03/15 02:55:31, ortuno wrote: > Could you add here that this function will write to the CCC descriptor? May not good since I do not see Windows implementation code. OS may implement it in another way, like polling. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win_fake.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:254: base::string16 device_address = On 2016/03/15 02:55:31, ortuno wrote: > Why do you go through all the trouble of retrieving the device and the service? Done. Residual of removing CCCD https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:449: characteristic->value->DataSize + sizeof(ULONG); On 2016/03/15 02:55:31, ortuno wrote: > Why isn't event.CharacteristicValueDataSize == characteristic->value->DataSize? BTH_LE_GATT_CHARACTERISTIC_VALUE is a structure, include Data[] and DataSize which is ULONG. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:395: if (!IsNotifying()) On 2016/03/15 02:55:31, ortuno wrote: > I don't think you need this. The only time gatt_event_handle_ is false (and > IsNotifying() is false) is after destruction. Since you passed a weak_ptr to the > callback then we would never hit that case. Done. Residual of removing stop notify session. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:160: base::AutoLock auto_lock(characteristic_value_changed_registrations_lock); On 2016/03/15 02:55:31, ortuno wrote: > Shouldn't you try to acquire this lock as soon as possible? Acquire it as later as possible to give more multi-thread capability. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.h:177: // asynchronously through |callback|. On 2016/03/15 02:55:31, ortuno wrote: > Post a task to register to receive value changed notifications from > |characteristic| of |service_path|. Done. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.h:184: // Post unregister characteristic value change notification. |event_handle| On 2016/03/15 02:55:31, ortuno wrote: > Post a task to unregister from value change notifications. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/470001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/490001
https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:183: // unique handle in OS for this registration. On 2016/03/15 18:28:22, gogerald1 wrote: > On 2016/03/15 02:55:31, ortuno wrote: > > Could you add here that this function will write to the CCC descriptor? > > May not good since I do not see Windows implementation code. OS may implement it > in another way, like polling. Add similar comments in bluetooth_remote_gatt_characteristic_win.cc
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping reviewers,
https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:927: // being destroyed. On 2016/03/15 18:28:22, gogerald1 wrote: > On 2016/03/15 02:55:31, ortuno wrote: > > What happens when you enable this test on windows? > > SimulateGattCharacteristicChanged has no target after the device has been > deleted from the OS. ortuno mentioned in person wanting to understand this more. RememberCharacteristicForSubsequentAction impl in BluetoothTestWin stores a pointer, which is invalid and then can't be used by GetSimulatedCharacteristic. That means that for Windows all tests using RememberCharacteristicForSubsequentAction won't work. We 3 had discussed in chat that the Windows impl expects the characteristic object to always outlive the OS referencing that object. However, reading over the code I see that there is a fair amount of complexity, and it's not trivial to assert this even now, but certainly not in the future with refactoring being done. Please modify this code so that OnGetGattEventWin is called with cached data from RememberCharacteristicForSubsequentAction. To test that code works correctly it should be possible see SimulateGattCharacteristicChanged(/* use remembered characteristic */ nullptr ... succeed if a DeleteDevice(device_) was not called. (that is, verify that RememberCharacteristicForSubsequentAction works.) Then, after DeleteDevice is called, SimulateGattCharacteristicChanged should not crash.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/510001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/510001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:927: // being destroyed. On 2016/03/19 04:57:54, scheib OOO wrote: > On 2016/03/15 18:28:22, gogerald1 wrote: > > On 2016/03/15 02:55:31, ortuno wrote: > > > What happens when you enable this test on windows? > > > > SimulateGattCharacteristicChanged has no target after the device has been > > deleted from the OS. > > ortuno mentioned in person wanting to understand this more. > RememberCharacteristicForSubsequentAction impl in BluetoothTestWin stores a > pointer, which is invalid and then can't be used by GetSimulatedCharacteristic. > That means that for Windows all tests using > RememberCharacteristicForSubsequentAction won't work. > > We 3 had discussed in chat that the Windows impl expects the characteristic > object to always outlive the OS referencing that object. However, reading over > the code I see that there is a fair amount of complexity, and it's not trivial > to assert this even now, but certainly not in the future with refactoring being > done. > > Please modify this code so that OnGetGattEventWin is called with cached data > from RememberCharacteristicForSubsequentAction. > > To test that code works correctly it should be possible see > SimulateGattCharacteristicChanged(/* use remembered characteristic */ nullptr > ... > succeed if a DeleteDevice(device_) was not called. > > (that is, verify that RememberCharacteristicForSubsequentAction works.) > > Then, after DeleteDevice is called, SimulateGattCharacteristicChanged should not > crash. Done.
https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:927: // being destroyed. On 2016/03/21 at 14:22:22, gogerald1 wrote: > On 2016/03/19 04:57:54, scheib OOO wrote: > > On 2016/03/15 18:28:22, gogerald1 wrote: > > > On 2016/03/15 02:55:31, ortuno wrote: > > > > What happens when you enable this test on windows? > > > > > > SimulateGattCharacteristicChanged has no target after the device has been > > > deleted from the OS. > > > > ortuno mentioned in person wanting to understand this more. > > RememberCharacteristicForSubsequentAction impl in BluetoothTestWin stores a > > pointer, which is invalid and then can't be used by GetSimulatedCharacteristic. > > That means that for Windows all tests using > > RememberCharacteristicForSubsequentAction won't work. > > > > We 3 had discussed in chat that the Windows impl expects the characteristic > > object to always outlive the OS referencing that object. However, reading over > > the code I see that there is a fair amount of complexity, and it's not trivial > > to assert this even now, but certainly not in the future with refactoring being > > done. > > > > Please modify this code so that OnGetGattEventWin is called with cached data > > from RememberCharacteristicForSubsequentAction. > > > > To test that code works correctly it should be possible see > > SimulateGattCharacteristicChanged(/* use remembered characteristic */ nullptr > > ... > > succeed if a DeleteDevice(device_) was not called. > > > > (that is, verify that RememberCharacteristicForSubsequentAction works.) > > > > Then, after DeleteDevice is called, SimulateGattCharacteristicChanged should not > > crash. > > Done. Have you tried this locally? I just tried it and it's crashing. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:183: // unique handle in OS for this registration. On 2016/03/17 at 14:07:56, gogerald1 wrote: > On 2016/03/15 18:28:22, gogerald1 wrote: > > On 2016/03/15 02:55:31, ortuno wrote: > > > Could you add here that this function will write to the CCC descriptor? > > > > May not good since I do not see Windows implementation code. OS may implement it > > in another way, like polling. > > Add similar comments in bluetooth_remote_gatt_characteristic_win.cc I thought you said you tested it and it does write to the descriptor? https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_notify_session_win.cc (right): https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.cc:16: return characteristic_.get()->GetIdentifier(); fwiw, you don't need the get() here. https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win_fake.cc (right): https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:193: if (remembered_characteristic_ && I don't think you need all this logic and the error: the only reason to pass a nullptr is to use the remembered characteristic. If you want to make sure there is a remembered characteristic and that the attribute handle is the same then DCHECK. https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:229: if (remembered_characteristic_ && Same as above. No need for the logic and the error. https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:485: if (it != parent_service->included_characteristics.end()) DCHECK this. If someone passes in an non existing attribute handle then it's probably a mistake on their part and we should make it obvious.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:927: // being destroyed. On 2016/03/21 17:00:52, ortuno wrote: > On 2016/03/21 at 14:22:22, gogerald1 wrote: > > On 2016/03/19 04:57:54, scheib OOO wrote: > > > On 2016/03/15 18:28:22, gogerald1 wrote: > > > > On 2016/03/15 02:55:31, ortuno wrote: > > > > > What happens when you enable this test on windows? > > > > > > > > SimulateGattCharacteristicChanged has no target after the device has been > > > > deleted from the OS. > > > > > > ortuno mentioned in person wanting to understand this more. > > > RememberCharacteristicForSubsequentAction impl in BluetoothTestWin stores a > > > pointer, which is invalid and then can't be used by > GetSimulatedCharacteristic. > > > That means that for Windows all tests using > > > RememberCharacteristicForSubsequentAction won't work. > > > > > > We 3 had discussed in chat that the Windows impl expects the characteristic > > > object to always outlive the OS referencing that object. However, reading > over > > > the code I see that there is a fair amount of complexity, and it's not > trivial > > > to assert this even now, but certainly not in the future with refactoring > being > > > done. > > > > > > Please modify this code so that OnGetGattEventWin is called with cached data > > > from RememberCharacteristicForSubsequentAction. > > > > > > To test that code works correctly it should be possible see > > > SimulateGattCharacteristicChanged(/* use remembered characteristic */ > nullptr > > > ... > > > succeed if a DeleteDevice(device_) was not called. > > > > > > (that is, verify that RememberCharacteristicForSubsequentAction works.) > > > > > > Then, after DeleteDevice is called, SimulateGattCharacteristicChanged should > not > > > crash. > > > > Done. > > Have you tried this locally? I just tried it and it's crashing. Of course, and it passed trybots https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:183: // unique handle in OS for this registration. On 2016/03/21 17:00:52, ortuno wrote: > On 2016/03/17 at 14:07:56, gogerald1 wrote: > > On 2016/03/15 18:28:22, gogerald1 wrote: > > > On 2016/03/15 02:55:31, ortuno wrote: > > > > Could you add here that this function will write to the CCC descriptor? > > > > > > May not good since I do not see Windows implementation code. OS may > implement it > > > in another way, like polling. > > > > Add similar comments in bluetooth_remote_gatt_characteristic_win.cc > > I thought you said you tested it and it does write to the descriptor? No, I said I tested the functionality of these interfaces, for this interface it means I can receive characteristic value change notification from my mobile after calling this interface. https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_notify_session_win.cc (right): https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_notify_session_win.cc:16: return characteristic_.get()->GetIdentifier(); On 2016/03/21 17:00:53, ortuno wrote: > fwiw, you don't need the get() here. Done. https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win_fake.cc (right): https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:193: if (remembered_characteristic_ && On 2016/03/21 17:00:53, ortuno wrote: > I don't think you need all this logic and the error: the only reason to pass a > nullptr is to use the remembered characteristic. If you want to make sure there > is a remembered characteristic and that the attribute handle is the same then > DCHECK. Done. https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:229: if (remembered_characteristic_ && On 2016/03/21 17:00:53, ortuno wrote: > Same as above. No need for the logic and the error. Done. https://codereview.chromium.org/1749403002/diff/510001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:485: if (it != parent_service->included_characteristics.end()) On 2016/03/21 17:00:53, ortuno wrote: > DCHECK this. If someone passes in an non existing attribute handle then it's > probably a mistake on their part and we should make it obvious. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/530001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/530001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:927: // being destroyed. On 2016/03/21 at 22:09:16, gogerald1 wrote: > On 2016/03/21 17:00:52, ortuno wrote: > > On 2016/03/21 at 14:22:22, gogerald1 wrote: > > > On 2016/03/19 04:57:54, scheib OOO wrote: > > > > On 2016/03/15 18:28:22, gogerald1 wrote: > > > > > On 2016/03/15 02:55:31, ortuno wrote: > > > > > > What happens when you enable this test on windows? > > > > > > > > > > SimulateGattCharacteristicChanged has no target after the device has been > > > > > deleted from the OS. > > > > > > > > ortuno mentioned in person wanting to understand this more. > > > > RememberCharacteristicForSubsequentAction impl in BluetoothTestWin stores a > > > > pointer, which is invalid and then can't be used by > > GetSimulatedCharacteristic. > > > > That means that for Windows all tests using > > > > RememberCharacteristicForSubsequentAction won't work. > > > > > > > > We 3 had discussed in chat that the Windows impl expects the characteristic > > > > object to always outlive the OS referencing that object. However, reading > > over > > > > the code I see that there is a fair amount of complexity, and it's not > > trivial > > > > to assert this even now, but certainly not in the future with refactoring > > being > > > > done. > > > > > > > > Please modify this code so that OnGetGattEventWin is called with cached data > > > > from RememberCharacteristicForSubsequentAction. > > > > > > > > To test that code works correctly it should be possible see > > > > SimulateGattCharacteristicChanged(/* use remembered characteristic */ > > nullptr > > > > ... > > > > succeed if a DeleteDevice(device_) was not called. > > > > > > > > (that is, verify that RememberCharacteristicForSubsequentAction works.) > > > > > > > > Then, after DeleteDevice is called, SimulateGattCharacteristicChanged should > > not > > > > crash. > > > > > > Done. > > > > Have you tried this locally? I just tried it and it's crashing. > > Of course, and it passed trybots After looking into it a bit more I think we need to fix the test. The test is incorrect because no observers are added so the code that would trigger a crash is not being tested. If you add an observer then the test will be correct and you'll observer a crash. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:183: // unique handle in OS for this registration. On 2016/03/21 at 22:09:16, gogerald1 wrote: > On 2016/03/21 17:00:52, ortuno wrote: > > On 2016/03/17 at 14:07:56, gogerald1 wrote: > > > On 2016/03/15 18:28:22, gogerald1 wrote: > > > > On 2016/03/15 02:55:31, ortuno wrote: > > > > > Could you add here that this function will write to the CCC descriptor? > > > > > > > > May not good since I do not see Windows implementation code. OS may > > implement it > > > > in another way, like polling. > > > > > > Add similar comments in bluetooth_remote_gatt_characteristic_win.cc > > > > I thought you said you tested it and it does write to the descriptor? > > No, I said I tested the functionality of these interfaces, for this interface it means I can receive characteristic value change notification from my mobile after calling this interface. Maybe a little context will make it clear why we need to know if the descriptor was written. Devices that support notifications are not always sending out notifications. They only start sending notifications once the CCC descriptor has been written to. If Windows doesn't write to the descriptor then we are going to start getting reports of people not getting any notifications from their devices. So please find out whether or not BluetoothGATTRegisterEvent writes to the CCC descriptor or not.
Patchset #19 (id:550001) has been deleted
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616,597888 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/570001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/570001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #20 (id:590001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/610001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/610001
Noticed two new tests have been added for StartNotifySession, enable them in the upcoming CL since this CL gets big and complex. https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:927: // being destroyed. On 2016/03/22 15:02:24, ortuno wrote: > On 2016/03/21 at 22:09:16, gogerald1 wrote: > > On 2016/03/21 17:00:52, ortuno wrote: > > > On 2016/03/21 at 14:22:22, gogerald1 wrote: > > > > On 2016/03/19 04:57:54, scheib OOO wrote: > > > > > On 2016/03/15 18:28:22, gogerald1 wrote: > > > > > > On 2016/03/15 02:55:31, ortuno wrote: > > > > > > > What happens when you enable this test on windows? > > > > > > > > > > > > SimulateGattCharacteristicChanged has no target after the device has > been > > > > > > deleted from the OS. > > > > > > > > > > ortuno mentioned in person wanting to understand this more. > > > > > RememberCharacteristicForSubsequentAction impl in BluetoothTestWin > stores a > > > > > pointer, which is invalid and then can't be used by > > > GetSimulatedCharacteristic. > > > > > That means that for Windows all tests using > > > > > RememberCharacteristicForSubsequentAction won't work. > > > > > > > > > > We 3 had discussed in chat that the Windows impl expects the > characteristic > > > > > object to always outlive the OS referencing that object. However, > reading > > > over > > > > > the code I see that there is a fair amount of complexity, and it's not > > > trivial > > > > > to assert this even now, but certainly not in the future with > refactoring > > > being > > > > > done. > > > > > > > > > > Please modify this code so that OnGetGattEventWin is called with cached > data > > > > > from RememberCharacteristicForSubsequentAction. > > > > > > > > > > To test that code works correctly it should be possible see > > > > > SimulateGattCharacteristicChanged(/* use remembered characteristic */ > > > nullptr > > > > > ... > > > > > succeed if a DeleteDevice(device_) was not called. > > > > > > > > > > (that is, verify that RememberCharacteristicForSubsequentAction works.) > > > > > > > > > > Then, after DeleteDevice is called, SimulateGattCharacteristicChanged > should > > > not > > > > > crash. > > > > > > > > Done. > > > > > > Have you tried this locally? I just tried it and it's crashing. > > > > Of course, and it passed trybots > > After looking into it a bit more I think we need to fix the test. The test is > incorrect because no observers are added so the code that would trigger a crash > is not being tested. If you add an observer then the test will be correct and > you'll observer a crash. Done. Rebase to fixed test https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.h (right): https://codereview.chromium.org/1749403002/diff/430001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.h:183: // unique handle in OS for this registration. On 2016/03/22 15:02:24, ortuno wrote: > On 2016/03/21 at 22:09:16, gogerald1 wrote: > > On 2016/03/21 17:00:52, ortuno wrote: > > > On 2016/03/17 at 14:07:56, gogerald1 wrote: > > > > On 2016/03/15 18:28:22, gogerald1 wrote: > > > > > On 2016/03/15 02:55:31, ortuno wrote: > > > > > > Could you add here that this function will write to the CCC > descriptor? > > > > > > > > > > May not good since I do not see Windows implementation code. OS may > > > implement it > > > > > in another way, like polling. > > > > > > > > Add similar comments in bluetooth_remote_gatt_characteristic_win.cc > > > > > > I thought you said you tested it and it does write to the descriptor? > > > > No, I said I tested the functionality of these interfaces, for this interface > it means I can receive characteristic value change notification from my mobile > after calling this interface. > > Maybe a little context will make it clear why we need to know if the descriptor > was written. Devices that support notifications are not always sending out > notifications. They only start sending notifications once the CCC descriptor has > been written to. If Windows doesn't write to the descriptor then we are going to > start getting reports of people not getting any notifications from their > devices. So please find out whether or not BluetoothGATTRegisterEvent writes to > the CCC descriptor or not. Done. From HCI log, Windows does not write CCCD implicitly, modified by setting CCCD explicitly, verified by HCI log.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_win.cc (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_win.cc:46: // Explicitly take and erase GATT services one by one to ensure that calling This seems unrelated to this change. Why are you doing it now? Please keep unrelated changes in separate CLs. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_win.cc:47: // GetGattService on removed service in GattServiceRemoved return null. s/return/returns/ https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:777: #if !defined(OS_ANDROID) You no longer need this #if. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:804: #if defined(OS_ANDROID) See below. You can make this test pass if you use GetDescriptorsByUUID and check if the resulting vector equals 1. See how android does it: https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:828: // Windows: Characteristic notification is set to OS asynchronously. You need a test to make sure that an error from RegisterGattEvents is propagated correctly. OK to do in a follow up CL https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:896: #if defined(OS_ANDROID) || defined(OS_WIN) We also need a test for when BluetoothGattSetDescriptorValue fails. OK to do in follow up patch with the rest of the tests enabled. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:179: !included_descriptors_discovered_) { Maybe we don't need this logic. One should only access a characteristic if all descriptors have been discovered. Web Bluetooth for example will only allow you to access a characteristic once GattServicesDiscovered is true for a device, which happens only after all services, characteristics and descriptors have been discovered for the device. Similarly in Chrome Apps, the Service Added event is only called once all characteristics and descriptors are discovered for the service. Why do you think we need to check if the descriptors have been discovered? https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:186: void BluetoothRemoteGattCharacteristicWin::StartNotifySession() { Move this function after GattEventRegistrationCallback to match the header. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:189: if (ccc_descriptor_identifier_.empty()) { I don't think you need to save the descriptor. You can just use GetDescriptorsByUUID and follow the same pattern as android: https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... That code will also let you pass one of the new tests. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:312: included_descriptors_[id].reset(); Why reset and then delete? https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.h (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.h:86: void StartNotifySession(); There are now two functions called StartNotifySession which is very confusing. Please change this name to make it clearer what this function does. Also comment what the function does.
Patchset #21 (id:630001) has been deleted
https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_win.cc (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_win.cc:46: // Explicitly take and erase GATT services one by one to ensure that calling On 2016/03/30 22:34:23, ortuno wrote: > This seems unrelated to this change. Why are you doing it now? Please keep > unrelated changes in separate CLs. This is a related change, otherwise GetGattService in GattServiceRemoved in the unit test will return true, default release does not set the scoped_ptr to null first (take the scoped_ptr). https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_win.cc:47: // GetGattService on removed service in GattServiceRemoved return null. On 2016/03/30 22:34:23, ortuno wrote: > s/return/returns/ Done. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:777: #if !defined(OS_ANDROID) On 2016/03/30 22:34:23, ortuno wrote: > You no longer need this #if. Done. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:804: #if defined(OS_ANDROID) On 2016/03/30 22:34:23, ortuno wrote: > See below. You can make this test pass if you use GetDescriptorsByUUID and check > if the resulting vector equals 1. See how android does it: > > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... Done. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:828: // Windows: Characteristic notification is set to OS asynchronously. On 2016/03/30 22:34:23, ortuno wrote: > You need a test to make sure that an error from RegisterGattEvents is propagated > correctly. OK to do in a follow up CL Postpone to follow up CL. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:896: #if defined(OS_ANDROID) || defined(OS_WIN) On 2016/03/30 22:34:23, ortuno wrote: > We also need a test for when BluetoothGattSetDescriptorValue fails. OK to do in > follow up patch with the rest of the tests enabled. Postpone to follow up CL. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:179: !included_descriptors_discovered_) { On 2016/03/30 22:34:23, ortuno wrote: > Maybe we don't need this logic. One should only access a characteristic if all > descriptors have been discovered. Web Bluetooth for example will only allow you > to access a characteristic once GattServicesDiscovered is true for a device, > which happens only after all services, characteristics and descriptors have been > discovered for the device. Similarly in Chrome Apps, the Service Added event is > only called once all characteristics and descriptors are discovered for the > service. Why do you think we need to check if the descriptors have been > discovered? remove as we discussed on hangout https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:186: void BluetoothRemoteGattCharacteristicWin::StartNotifySession() { On 2016/03/30 22:34:23, ortuno wrote: > Move this function after GattEventRegistrationCallback to match the header. Done. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:189: if (ccc_descriptor_identifier_.empty()) { On 2016/03/30 22:34:23, ortuno wrote: > I don't think you need to save the descriptor. You can just use > GetDescriptorsByUUID and follow the same pattern as android: > https://code.google.com/p/chromium/codesearch#chromium/src/device/bluetooth/b... > > That code will also let you pass one of the new tests. Done. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:312: included_descriptors_[id].reset(); On 2016/03/30 22:34:23, ortuno wrote: > Why reset and then delete? In case of infinitely recurses, refer https://codereview.chromium.org/1763983002/ https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.h (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.h:86: void StartNotifySession(); On 2016/03/30 22:34:23, ortuno wrote: > There are now two functions called StartNotifySession which is very confusing. > Please change this name to make it clearer what this function does. > > Also comment what the function does. Done. no longer needed.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/650001
lgtm bar one small cast and the condition that you add the Failed to Write descriptor and Failed to register gatt event tests in a follow up patch. https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_win.cc (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_win.cc:46: // Explicitly take and erase GATT services one by one to ensure that calling On 2016/03/31 at 17:44:12, gogerald1 wrote: > On 2016/03/30 22:34:23, ortuno wrote: > > This seems unrelated to this change. Why are you doing it now? Please keep > > unrelated changes in separate CLs. > > This is a related change, otherwise GetGattService in GattServiceRemoved in the unit test will return true, default release does not set the scoped_ptr to null first (take the scoped_ptr). It still seems a bug not related to the feature you are implementing. OK to leave now, but in the feature if you find a bug while on another patch, the best thing to do is to submit a separate patch. Otherwise someone looking at git blame will have no idea why you added that piece of code in this seemingly unrelated feature. https://codereview.chromium.org/1749403002/diff/650001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/650001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:197: ((BluetoothRemoteGattDescriptorWin*)ccc_descriptors[0]) static_cast<BluetoothRemoteGattDescriptorWin*>(ccc_descriptors[0])
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/670001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/670001
https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... File device/bluetooth/bluetooth_device_win.cc (right): https://codereview.chromium.org/1749403002/diff/610001/device/bluetooth/bluet... device/bluetooth/bluetooth_device_win.cc:46: // Explicitly take and erase GATT services one by one to ensure that calling On 2016/03/31 18:09:19, ortuno wrote: > On 2016/03/31 at 17:44:12, gogerald1 wrote: > > On 2016/03/30 22:34:23, ortuno wrote: > > > This seems unrelated to this change. Why are you doing it now? Please keep > > > unrelated changes in separate CLs. > > > > This is a related change, otherwise GetGattService in GattServiceRemoved in > the unit test will return true, default release does not set the scoped_ptr to > null first (take the scoped_ptr). > > It still seems a bug not related to the feature you are implementing. OK to > leave now, but in the feature if you find a bug while on another patch, the best > thing to do is to submit a separate patch. Otherwise someone looking at git > blame will have no idea why you added that piece of code in this seemingly > unrelated feature. Acknowledged. https://codereview.chromium.org/1749403002/diff/650001/device/bluetooth/bluet... File device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc (right): https://codereview.chromium.org/1749403002/diff/650001/device/bluetooth/bluet... device/bluetooth/bluetooth_remote_gatt_characteristic_win.cc:197: ((BluetoothRemoteGattDescriptorWin*)ccc_descriptors[0]) On 2016/03/31 18:09:19, ortuno wrote: > static_cast<BluetoothRemoteGattDescriptorWin*>(ccc_descriptors[0]) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
In change description: > It also fixes implicit cast between semantically different integer types issue. Preferably split out all changes like this from larger patches. If in this patch, explicitly call out what files, or what types are being referenced. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:825: // Windows: Characteristic notification is set to OS asynchronously. Windows: Synchronous Test Not Applicable: OS calls are all made asynchronously from BluetoothTaskManagerWin. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.cc:472: if (device_interface_data->InterfaceClassGuid != Double check that you intend this change -- I don't yet see relevance to this patch and didn't find discussion in previous comments. If it's just a related bug fix consider explaining in change description or a stand alone patch. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win_fake.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:419: (PBTH_LE_GATT_CHARACTERISTIC_VALUE)(new UCHAR[sizeof(ULONG)]); sizeof(BTH_LE_GATT_CHARACTERISTIC_VALUE) https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:506: remembered_characteristic_ = std::move(it->second); Are you intending to move the characteristic out of the parent_service->included_characteristics? If so, use remembered_characteristic_.swap(it->second), and call out in comments what is happening. (it's not as obvious, because of the iterator reference) remembered_characteristic_.swap(parent_service->included_characteristics[attribute_handle]); would be even clearer. But, if we can get away without doing so it would be best. The reason to do so would be that the unittest code deleting the device must result in the service being deleted. but if it is possible to only delete the chrome implementation objects, and keep the fake objects alive, that would be better at reflecting the intent of the tests: The OS have objects alive and valid while our objects are destroyed. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:133: typedef std::unordered_map<PVOID, CharacteristicValueChangedRegistration> Why does the pointer need to be cast to PVOID? https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:137: characteristic_value_changed_registrations; Move these to private member variables, unless there's a reason for them to be globals. If there is, they should go in the anonymous namespace and use a g_ prefix. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:144: if (type != CharacteristicValueChangedEvent) { Add DCHECK to assert what threads this is or is not run on. From below I read that it may be a different thread then we make the windows API calls from. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:897: BLUETOOTH_GATT_EVENT_HANDLE win_event_handle = NULL; DCHECK(bluetooth_task_runner_->RunsTasksOnCurrentThread()); https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:928: characteristic_value_changed_registrations[user_event_handle] = This also needs to be guarded?
Patchset #23 (id:690001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/710001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/710001
Patchset #23 (id:710001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/730001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/730001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_gatt_characteristic_unittest.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_gatt_characteristic_unittest.cc:825: // Windows: Characteristic notification is set to OS asynchronously. On 2016/04/05 22:44:02, scheib OOO wrote: > Windows: Synchronous Test Not Applicable: OS calls are all made asynchronously > from BluetoothTaskManagerWin. Done. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win.cc:472: if (device_interface_data->InterfaceClassGuid != On 2016/04/05 22:44:02, scheib wrote: > Double check that you intend this change -- I don't yet see relevance to this > patch and didn't find discussion in previous comments. If it's just a related > bug fix consider explaining in change description or a stand alone patch. Done. Split out https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_low_energy_win_fake.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:419: (PBTH_LE_GATT_CHARACTERISTIC_VALUE)(new UCHAR[sizeof(ULONG)]); On 2016/04/05 22:44:02, scheib OOO wrote: > sizeof(BTH_LE_GATT_CHARACTERISTIC_VALUE) Done. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_low_energy_win_fake.cc:506: remembered_characteristic_ = std::move(it->second); On 2016/04/05 22:44:02, scheib OOO wrote: > Are you intending to move the characteristic out of the > parent_service->included_characteristics? > > If so, use remembered_characteristic_.swap(it->second), and call out in comments > what is happening. (it's not as obvious, because of the iterator reference) > > remembered_characteristic_.swap(parent_service->included_characteristics[attribute_handle]); > would be even clearer. > > But, if we can get away without doing so it would be best. The reason to do so > would be that the unittest code deleting the device must result in the service > being deleted. but if it is possible to only delete the chrome implementation > objects, and keep the fake objects alive, that would be better at reflecting the > intent of the tests: The OS have objects alive and valid while our objects are > destroyed. Done. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:133: typedef std::unordered_map<PVOID, CharacteristicValueChangedRegistration> On 2016/04/05 22:44:02, scheib OOO wrote: > Why does the pointer need to be cast to PVOID? It was passed into OS as context which is PVOID when registering event, and the OS will pass it back as context identifier. Add additional comments. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:137: characteristic_value_changed_registrations; On 2016/04/05 22:44:02, scheib OOO wrote: > Move these to private member variables, unless there's a reason for them to be > globals. If there is, they should go in the anonymous namespace and use a g_ > prefix. Done. These variables are used by BluetoothTaskManagerWin and OS, so I think it's clear to keep them in anonymous namespace. Here is in anonymous namespace. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:144: if (type != CharacteristicValueChangedEvent) { On 2016/04/05 22:44:02, scheib wrote: > Add DCHECK to assert what threads this is or is not run on. From below I read > that it may be a different thread then we make the windows API calls from. Post callback to ui_task_runner_ below and according to my test OS invokes this function in BluetoothApis.dll thread (comment below). I did not DCHECK(!ui_task_runner_) since test code simulates characteristic value change in ui_task_runner_. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:897: BLUETOOTH_GATT_EVENT_HANDLE win_event_handle = NULL; On 2016/04/05 22:44:02, scheib wrote: > DCHECK(bluetooth_task_runner_->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:928: characteristic_value_changed_registrations[user_event_handle] = On 2016/04/05 22:44:02, scheib wrote: > This also needs to be guarded? may have no problem since remove and add operation are running in the same thread (bluetooth_task_runner_), and add and read operation should have no conflict. Add operation should always add a different callback. But add the lock here to secure.
LGTM, though please move the threading comment: https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:928: characteristic_value_changed_registrations[user_event_handle] = On 2016/04/06 22:54:48, gogerald1 wrote: > On 2016/04/05 22:44:02, scheib wrote: > > This also needs to be guarded? > > may have no problem since remove and add operation are running in the same > thread (bluetooth_task_runner_), and add and read operation should have no > conflict. Add operation should always add a different callback. But add the lock > here to secure. The map was searched and added to from different threads, so there was a race. The lock here is required. https://codereview.chromium.org/1749403002/diff/730001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/1749403002/diff/730001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:154: void OnGetGattEventWin(BTH_LE_GATT_EVENT_TYPE type, Move thread comment here so that the thread of the method is known when started to read it.
https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/1749403002/diff/670001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:928: characteristic_value_changed_registrations[user_event_handle] = On 2016/04/06 23:56:39, scheib wrote: > On 2016/04/06 22:54:48, gogerald1 wrote: > > On 2016/04/05 22:44:02, scheib wrote: > > > This also needs to be guarded? > > > > may have no problem since remove and add operation are running in the same > > thread (bluetooth_task_runner_), and add and read operation should have no > > conflict. Add operation should always add a different callback. But add the > lock > > here to secure. > > The map was searched and added to from different threads, so there was a race. > The lock here is required. Acknowledged. https://codereview.chromium.org/1749403002/diff/730001/device/bluetooth/bluet... File device/bluetooth/bluetooth_task_manager_win.cc (right): https://codereview.chromium.org/1749403002/diff/730001/device/bluetooth/bluet... device/bluetooth/bluetooth_task_manager_win.cc:154: void OnGetGattEventWin(BTH_LE_GATT_EVENT_TYPE type, On 2016/04/06 23:56:39, scheib wrote: > Move thread comment here so that the thread of the method is known when started > to read it. Done.
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ortuno@chromium.org, scheib@chromium.org Link to the patchset: https://codereview.chromium.org/1749403002/#ps750001 (title: "move comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/750001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/750001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1749403002/750001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1749403002/750001
Message was sent while issue was closed.
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616,597888 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616,597888 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:750001)
Message was sent while issue was closed.
Description was changed from ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616,597888 ========== to ========== Implement BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. This CL implements BluetoothRemoteGattCharacteristicWin::StartNotifySession and related unit tests. It also fixes implicit cast between semantically different integer types issue. In addition, it moves gatt_notify_characteristic_attempts_, gatt_write_characteristic_attempts_, and gatt_read_characteristic_attempts_ check immediately after an operation in bluetooth_gatt_characteristic_unittest.cc to after simulate operation since the operation is asynchronous down to the OS on Windows. BUG=579202,592843,427616,597888 Committed: https://crrev.com/08539daf0b416e5cebf3109119f4c9e6beb09d54 Cr-Commit-Position: refs/heads/master@{#386150} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/08539daf0b416e5cebf3109119f4c9e6beb09d54 Cr-Commit-Position: refs/heads/master@{#386150} |
