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

Issue 2808013002: Resolve promises synchronously sometimes (Closed)

Created:
3 years, 8 months ago by suzyh_UTC10 (ex-contributor)
Modified:
3 years, 8 months ago
CC:
darktears, blink-reviews, blink-reviews-animation_chromium.org, blink-reviews-w3ctests_chromium.org, chromium-reviews, Eric Willigers, rjwright, shans
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Resolve promises synchronously sometimes https://codereview.chromium.org/2615253002 changed ~PlayStateUpdateScope() to resolve promises asynchronously in order to avoid script execution inside a forbidden scope. However, this causes the promises to behave differently from expected in some cases, demonstrated in part by a crash in the interfaces/Animation/cancel.html test due to failing a DCHECK. This patch modifies the promise resolution code to first check whether we are in a forbidden scope, and only resolve the promise asynchronously if so. BUG=708887 Review-Url: https://codereview.chromium.org/2808013002 Cr-Commit-Position: refs/heads/master@{#463907} Committed: https://chromium.googlesource.com/chromium/src/+/220578cfcb90370db3b268c6e59c1c4417a60708

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -14 lines) Patch
M third_party/WebKit/LayoutTests/TestExpectations View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animation/finish-expected.txt View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animation/finished-expected.txt View 1 chunk +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/animation/Animation.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/animation/Animation.cpp View 4 chunks +13 lines, -8 lines 0 comments Download

Messages

Total messages: 15 (9 generated)
suzyh_UTC10 (ex-contributor)
3 years, 8 months ago (2017-04-10 07:12:10 UTC) #4
adithyas
lgtm lgtm
3 years, 8 months ago (2017-04-10 14:42:54 UTC) #7
suzyh_UTC10 (ex-contributor)
-dstockwell (OOO) +alancutter
3 years, 8 months ago (2017-04-10 23:16:30 UTC) #9
alancutter (OOO until 2018)
lgtm
3 years, 8 months ago (2017-04-11 06:52:18 UTC) #10
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/2808013002/1
3 years, 8 months ago (2017-04-12 00:47:55 UTC) #12
commit-bot: I haz the power
3 years, 8 months ago (2017-04-12 02:42:05 UTC) #15
Message was sent while issue was closed.
Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/220578cfcb90370db3b268c6e59c...

Powered by Google App Engine
This is Rietveld 408576698