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

Issue 2584473002: Merge setTimeout calls with same timeout for webfont tests. (Closed)

Created:
4 years ago by rune
Modified:
4 years ago
CC:
blink-reviews, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Merge setTimeout calls with same timeout for webfont tests. Landing async stylesheet update caused a regression in font-display tests. The values "fallback" and "optional" block display for 100ms according to the spec. The tests had a setTimeout call to trigger font loading and a setTimeout call to trigger notifyDone() to render before 100ms has passed with the same timeout value. However, the timer for allowing fallback display triggers before the notifyDone triggers in Debug builds on Mac. Calling notifyDone from the same setTimeout callback as triggering font loading. The intention of the test is to trigger the screen dump when 0s has passed, so this should be OK. I have not identified what exactly changed with the async stylesheet patch and why the timeout methods are interleaved with the timeout for enabling fallback rendering. Removing one of the other tests or one of the font-display values from the test array also makes the "fallback" and "optional" start passing without this change, so there is clearly a timing issue here. BUG=567021 Committed: https://crrev.com/ad541b1bdc92608984093593d9eb864ad51ed1e6 Cr-Commit-Position: refs/heads/master@{#439026}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -8 lines) Patch
M third_party/WebKit/LayoutTests/http/tests/webfont/font-display.html View 1 chunk +9 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/webfont/font-display-intervention.html View 1 chunk +9 lines, -4 lines 0 comments Download

Messages

Total messages: 15 (9 generated)
rune
ptal
4 years ago (2016-12-15 09:58:42 UTC) #4
Kunihiko Sakamoto
lgtm interesting...
4 years ago (2016-12-16 02:36:01 UTC) #7
esprehn
Interesting indeed! I wonder if there's an actual race in the system here we should ...
4 years ago (2016-12-16 03:38:22 UTC) #8
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/2584473002/1
4 years ago (2016-12-16 03:49:12 UTC) #10
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years ago (2016-12-16 04:53:07 UTC) #13
commit-bot: I haz the power
4 years ago (2016-12-16 04:55:37 UTC) #15
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/ad541b1bdc92608984093593d9eb864ad51ed1e6
Cr-Commit-Position: refs/heads/master@{#439026}

Powered by Google App Engine
This is Rietveld 408576698