Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(711)

Issue 2825393004: bluetooth: Check the order of event and promise resolution for readValue (Closed)

Created:
3 years, 8 months ago by ortuno
Modified:
3 years, 8 months ago
Reviewers:
scheib
CC:
blink-reviews, chromium-reviews, ortuno+watch_chromium.org, scheib+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

bluetooth: Check the order of event and promise resolution for readValue Introduces assert_promise_event_order_ that checks the order of promise resolution and events firing is correct. Makes assert_event_fires_after_promise and assert_event_fires_before_promise use this function. Also removes some obsolete TODOs. BUG=697698 Review-Url: https://codereview.chromium.org/2825393004 Cr-Commit-Position: refs/heads/master@{#467261} Committed: https://chromium.googlesource.com/chromium/src/+/04c5a1998f2c7079efca383a22e77e968251db4c

Patch Set 1 #

Patch Set 2 : docs cleanup #

Total comments: 6

Patch Set 3 : Address feedback #

Patch Set 4 : Address moar feedback #

Messages

Total messages: 21 (16 generated)
ortuno
scheib: PTAL
3 years, 8 months ago (2017-04-21 04:21:09 UTC) #5
scheib
lgtm with nits https://codereview.chromium.org/2825393004/diff/20001/third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/add-multiple-event-listeners.html File third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/add-multiple-event-listeners.html (right): https://codereview.chromium.org/2825393004/diff/20001/third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/add-multiple-event-listeners.html#newcode17 third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/add-multiple-event-listeners.html:17: return assert_event_fires_after_promise(characteristic, legacy issue, but still: ...
3 years, 8 months ago (2017-04-21 06:32:12 UTC) #8
ortuno
Thanks! https://codereview.chromium.org/2825393004/diff/20001/third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/add-multiple-event-listeners.html File third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/add-multiple-event-listeners.html (right): https://codereview.chromium.org/2825393004/diff/20001/third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/add-multiple-event-listeners.html#newcode17 third_party/WebKit/LayoutTests/bluetooth/characteristic/notifications/add-multiple-event-listeners.html:17: return assert_event_fires_after_promise(characteristic, On 2017/04/21 at 06:32:12, scheib wrote: ...
3 years, 8 months ago (2017-04-26 05:56:19 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2825393004/60001
3 years, 8 months ago (2017-04-26 07:20:20 UTC) #18
commit-bot: I haz the power
3 years, 8 months ago (2017-04-26 07:25:19 UTC) #21
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/04c5a1998f2c7079efca383a22e7...

Powered by Google App Engine
This is Rietveld 408576698