175384 – Implement font-display loading behaviors

RESOLVED FIXED 175384

Implement font-display loading behaviors

https://bugs.webkit.org/show_bug.cgi?id=175384

Summary Implement font-display loading behaviors

Myles C. Maxfield

Reported 2017-08-09 08:26:30 PDT

Implement font-display loading behaviors

Attachments
WIP (4.46 KB, patch) 2017-09-13 21:19 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
WIP (9.89 KB, patch) 2017-09-13 22:27 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.65 MB, application/zip) 2017-09-13 23:34 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.75 MB, application/zip) 2017-09-13 23:39 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews114 for mac-elcapitan (2.61 MB, application/zip) 2017-09-13 23:50 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.10 MB, application/zip) 2017-09-14 00:04 PDT, Build Bot	no flags	Details
WIP (40.55 KB, patch) 2017-09-26 18:28 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
WIP (42.08 KB, patch) 2017-09-28 11:51 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch (43.88 KB, patch) 2017-09-29 10:49 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch (52.60 KB, patch) 2017-09-29 11:42 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.08 MB, application/zip) 2017-09-29 13:22 PDT, Build Bot	no flags	Details
Patch (52.75 KB, patch) 2017-09-29 15:48 PDT, Myles C. Maxfield	darin: review+	Details Formatted Diff Diff
Rebased (50.59 KB, patch) 2017-10-03 10:15 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
WIP (57.13 KB, patch) 2017-10-04 23:43 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
WIP (57.20 KB, patch) 2017-10-04 23:51 PDT, Myles C. Maxfield	buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.33 MB, application/zip) 2017-10-05 00:41 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.64 MB, application/zip) 2017-10-05 00:49 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (1.55 MB, application/zip) 2017-10-05 01:16 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews115 for mac-elcapitan (2.17 MB, application/zip) 2017-10-05 01:25 PDT, Build Bot	no flags	Details
Patch (57.37 KB, patch) 2017-10-05 10:20 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch (57.41 KB, patch) 2017-10-05 10:53 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Patch (57.34 KB, patch) 2017-10-05 10:56 PDT, Myles C. Maxfield	no flags	Details Formatted Diff Diff
Show Obsolete (20) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2017-08-09 15:34:45 PDT

<rdar://problem/33813243>

Myles C. Maxfield

Comment 2 2017-09-13 21:19:19 PDT

Created attachment 320726 [details] WIP

Build Bot

Comment 3 2017-09-13 21:20:07 PDT

Attachment 320726 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.

Myles C. Maxfield

Comment 4 2017-09-13 22:27:46 PDT

Created attachment 320727 [details] WIP

Build Bot

Comment 5 2017-09-13 22:28:34 PDT

Attachment 320727 [details] did not pass style-queue: ERROR: Source/WebCore/css/CSSFontFaceSource.h:84: LITHERUM_timerFired is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/css/CSSFontFaceSource.h:85: LITHERUM_timer is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/WebCore/css/CSSFontFaceSource.cpp:107: CSSFontFaceSource::LITHERUM_timerFired is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 4 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.

Myles C. Maxfield

Comment 6 2017-09-13 22:31:37 PDT

<!DOCTYPE html> <html> <head> <style> @font-face { font-family: "WebFont"; src: url("Ahem.ttf") format("truetype"); font-display: auto; } @font-face { font-family: "WebFont 2"; src: url("Andale Mono.ttf") format("truetype"); } </style> </head> <body> <div style="font: 100px 'WebFont', 'WebFont 2', 'Helvetica';">Hello</div> </body> </html> When hacking up WebKit to make Ahem take 4 seconds to load and everything else take 0 seconds to load... In this content, "Ahem" is the "primary font," "Helvetica" is the "fallback font," and "Andale Mono" is the "Failure Font". Auto | 3s Invisible | 1s Fallback | Primary Block | 3s Invisible | 1s Fallback | Primary Swap | 0s Invisible | 4s Fallback | Primary Fallback | 0s Invisible | 3s Fallback | Failure Optional | 0s Invisible | 0s Fallback | Failure

Build Bot

Comment 7 2017-09-13 23:34:37 PDT

Comment on attachment 320727 [details] WIP Attachment 320727 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4542165 New failing tests: imported/mozilla/svg/text/clipPath-content.svg fast/css/cascade/background-clip-and-webkit-background-clip-cascade-order.html imported/mozilla/svg/text-scale-02.svg fast/css/variables/test-suite/096.html

Build Bot

Comment 8 2017-09-13 23:34:38 PDT

Created attachment 320733 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 9 2017-09-13 23:39:24 PDT

Comment on attachment 320727 [details] WIP Attachment 320727 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4542169 New failing tests: imported/mozilla/svg/text/clipPath-content.svg fast/css/cascade/background-clip-and-webkit-background-clip-cascade-order.html imported/mozilla/svg/text-scale-02.svg fast/css/variables/test-suite/096.html

Build Bot

Comment 10 2017-09-13 23:39:26 PDT

Created attachment 320734 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 11 2017-09-13 23:50:39 PDT

Comment on attachment 320727 [details] WIP Attachment 320727 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4542177 New failing tests: fast/canvas/canvas-context-save-limit.html imported/mozilla/svg/text-scale-02.svg fast/css/cascade/background-clip-and-webkit-background-clip-cascade-order.html imported/mozilla/svg/text/clipPath-content.svg fast/text-autosizing/text-size-adjust-inline-style.html fast/text/font-face-set-javascript.html

Build Bot

Comment 12 2017-09-13 23:50:40 PDT

Created attachment 320735 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 13 2017-09-14 00:04:17 PDT

Comment on attachment 320727 [details] WIP Attachment 320727 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4542382 New failing tests: imported/mozilla/svg/text/clipPath-content.svg fast/css/cascade/background-clip-and-webkit-background-clip-cascade-order.html imported/mozilla/svg/text-scale-02.svg fast/text-autosizing/ios/ipad/text-size-adjust-inline-style.html fast/css/variables/test-suite/096.html

Build Bot

Comment 14 2017-09-14 00:04:18 PDT

Created attachment 320738 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Myles C. Maxfield

Comment 15 2017-09-26 18:28:30 PDT

Created attachment 321919 [details] WIP

Myles C. Maxfield

Comment 16 2017-09-28 11:51:21 PDT

Created attachment 322104 [details] WIP

Myles C. Maxfield

Comment 17 2017-09-29 10:49:28 PDT

Created attachment 322210 [details] Patch

Myles C. Maxfield

Comment 18 2017-09-29 11:42:34 PDT

Created attachment 322216 [details] Patch

Build Bot

Comment 19 2017-09-29 13:22:28 PDT

Comment on attachment 322216 [details] Patch Attachment 322216 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4700326 New failing tests: fast/text/loading-swap-nofinish.html

Build Bot

Comment 20 2017-09-29 13:22:30 PDT

Created attachment 322225 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6

Myles C. Maxfield

Comment 21 2017-09-29 15:42:51 PDT

The test fails because FontCache::lastResortFallbackFont() is different between macOS and iOS.

Myles C. Maxfield

Comment 22 2017-09-29 15:48:49 PDT

Created attachment 322247 [details] Patch

Myles C. Maxfield

Comment 23 2017-09-29 15:52:44 PDT

*** Bug 175989 has been marked as a duplicate of this bug. ***

WebKit Commit Bot

Comment 24 2017-09-29 17:19:02 PDT

Comment on attachment 322247 [details] Patch Rejecting attachment 322247 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 322247, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/4703488

Darin Adler

Comment 25 2017-09-30 16:47:26 PDT

Comment on attachment 322247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322247&action=review Normally for testing we don’t use magic numeric values. It’s more common to use strings in JavaScript to specify such things. I think that would be better rather than these special numbers. For one thing, the numbers will all all have to be renumbered if the CSS specification adds a new one. > Source/WebCore/css/CSSFontFace.cpp:501 > + // Don't cancel the font download, so that it might exist upon the next page the user navigates to. I think the wording “so that it might exist upon” is strange here. It would be good to write this out with more specific language. Are we saying that we should finish the font download so it gets into the cache? Or is there some more precise way of talking about what’s good about finishing the download? Exactly what is the benefit when the next page uses the same font? Also, will the download continue indefinitely? Will it stop when we leave this page? I understand that the page-specified time out needn’t cancel the download. But will the download ever get canceled if it takes a really long time? More precise language in this comment could help me understand what I think of the policy. And might help you clarify to others what your rationale is. Lets word it clearly enough that someone can disagree. I hope, though, it doesn’t turn into a paragraph. > Source/WebCore/css/CSSFontFace.cpp:515 > + if (status() == Status::Failure) > + return true; Confusing to add this here and not change the name of the function. Seems like this can now return true when it’s not true that all sources failed. > Source/WebCore/css/CSSFontFace.cpp:618 > + struct TimeoutValues { > + Seconds blockPeriod; > + Seconds swapPeriod; > + } timeoutValues[] = { > + { 3_s, Seconds(std::numeric_limits<double>::infinity()) }, > + { 3_s, Seconds(std::numeric_limits<double>::infinity()) }, > + { 0_s, Seconds(std::numeric_limits<double>::infinity()) }, > + { 0.1_s, 3_s }, > + { 0.1_s, 0_s }, > + // Debugging values below > + { Seconds(std::numeric_limits<double>::infinity()), 0_s }, > + { 0_s, Seconds(std::numeric_limits<double>::infinity()) }, > + { 0_s, 0_s }, > + }; > + auto index = fontTimeoutIndex().value_or(static_cast<unsigned>(m_loadingBehavior)); > + const TimeoutValues& values = timeoutValues[index]; The timeoutValues array should probably be declared const or constexpr. Should write Seconds::infinity() instead of that longer thing. I don’t see anything here that checks that the length of the array matches the number of values in the enumeration, and there’s also no check at runtime: I don’t like the idea that a bad value for the enum could result in a buffer overrun. Is there a clean way to tighten those two things up? I’d use auto& for the the of "values". I don’t think the name values is really great for the timeouts. Makes the code below a bit hard to read. > Source/WebCore/css/CSSFontFace.cpp:625 > + if (values.blockPeriod > 0_s && values.blockPeriod < Seconds(std::numeric_limits<double>::infinity())) Should write Seconds::infinity() instead of that longer thing. Could also consider using isfinite or !isinf instead. > Source/WebCore/css/CSSFontFace.cpp:626 > + m_timeoutTimer.startOneShot(values.blockPeriod); Do we need an else that stops the timer? What guarantees we don’t already have one going? > Source/WebCore/css/CSSFontFace.cpp:629 > + if (values.swapPeriod > 0_s && values.swapPeriod < Seconds(std::numeric_limits<double>::infinity())) Ditto. > Source/WebCore/css/CSSFontFace.cpp:630 > + m_timeoutTimer.startOneShot(values.swapPeriod); Ditto. > Source/WebCore/css/CSSFontFace.cpp:644 > + // The state transition across a 0-second timer should be synchronous. Comments needs to say "why". This comment should both say why this is desirable (suggested behavior in the specification? helpful for testing? important to user experience?) and maybe also why it is OK to do the work synchronously. In other contexts it has proven quite important to not do things synchronously so that side effects don’t cause infinite recursion. Why is that not an issue here? There is also a fragile dependency between code above, which checks this same condition and does not set a timer, and the code here. Would be cleaner if there were not two separate pieces of code that both need to implement the policy of doing things synchronously. I believe that we could move all the timer management down here so that we could either start the timer or make the synchronous call in the same place. I don’t see any reason the timer has to be started/stopped before calling out to the clients. Could also factor things so we did a better job sharing the logic about moving forward from one state to the next; the only difference between loading -> timed-out and timed-out -> failure here is which time constant is used and what status values are involved. > Source/WebCore/css/CSSFontFace.cpp:748 > + if (allSourcesFailed()) > + return nullptr; Why is this needed? The loop below will do nothing if the sources have already failed. Maybe because allSourcesFailed has been changed to check something different? > Source/WebCore/css/CSSFontFace.h:152 > + std::optional<unsigned> fontTimeoutIndex() const; I don’t think anything makes it clear that this is for testing only nor is the concept of a "font timeout index" really a clear one; seems very specific to using an enumeration as an index into a table inside one function, not a good abstraction or concept for what we are specifying. To cite an example, the name fontLoadingTimingOverrideForTesting would make it clear. Since we don’t want testing to race, it’s also pretty clear that there are only a few values that are useful for our testing. Zero, infinity, not sure what else. > Source/WebCore/css/CSSSegmentedFontFace.cpp:72 > + // We need to retain the intermediate results. Otherwise, we get use-after-free. I think this comment needs a rewrite. - I suggest writing the comment in terms of needing an owner for the result. We need to store a reference to the object since callers will not take ownership and it expects us to own it and expects the pointer to be valid as long as the font face object is alive. At least I think that’s what callers expect here. Unclear in fact. - I think it’s a mess to Ref all the intermediate results we have ever returned. For one thing, could easily be a waste of memory! Can we come up with a different design? Maybe we can change the font function to return RefPtr<Font> or Ref<Font> instead of extending the lifetime ourselves by keeping a vector of everything we have ever returned. - The word retain is part of Objective-C terminology for this, not our Ref/RefPtr terminology. - "otherwise, we get use-after-free" is not the clearest way to put it; we could say that about any object lifetime mistake and I think it’s a bit excessively dramatic

Myles C. Maxfield

Comment 26 2017-10-03 10:15:02 PDT

Created attachment 322540 [details] Rebased

Myles C. Maxfield

Comment 27 2017-10-04 22:55:12 PDT

Comment on attachment 322247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322247&action=review >> Source/WebCore/css/CSSFontFace.cpp:626 >> + m_timeoutTimer.startOneShot(values.blockPeriod); > > Do we need an else that stops the timer? What guarantees we don’t already have one going? This is the Success and Failure case below. >> Source/WebCore/css/CSSFontFace.cpp:748 >> + return nullptr; > > Why is this needed? The loop below will do nothing if the sources have already failed. Maybe because allSourcesFailed has been changed to check something different? Yep. pump() might have put us in the failure state, and if that occurs, we shouldn't run the code below.

Myles C. Maxfield

Comment 28 2017-10-04 23:43:05 PDT

Created attachment 322790 [details] WIP

Myles C. Maxfield

Comment 29 2017-10-04 23:51:12 PDT

Created attachment 322793 [details] WIP

Build Bot

Comment 30 2017-10-05 00:41:56 PDT

Comment on attachment 322793 [details] WIP Attachment 322793 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4764364 New failing tests: fast/text/loading-failure-finish.html fast/text/loading-swap-nofinish.html fast/text/loading-block-nofinish.html fast/text/loading-failure-nofinish.html

Build Bot

Comment 31 2017-10-05 00:41:57 PDT

Created attachment 322798 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Build Bot

Comment 32 2017-10-05 00:49:51 PDT

Comment on attachment 322793 [details] WIP Attachment 322793 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4764462 New failing tests: fast/text/loading-failure-finish.html fast/text/loading-swap-nofinish.html fast/text/loading-block-nofinish.html fast/text/loading-failure-nofinish.html

Build Bot

Comment 33 2017-10-05 00:49:52 PDT

Created attachment 322799 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Build Bot

Comment 34 2017-10-05 01:16:13 PDT

Comment on attachment 322793 [details] WIP Attachment 322793 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4764548 New failing tests: fast/text/loading-failure-finish.html fast/text/loading-swap-nofinish.html fast/text/loading-block-nofinish.html fast/text/loading-failure-nofinish.html

Build Bot

Comment 35 2017-10-05 01:16:15 PDT

Created attachment 322803 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6

Build Bot

Comment 36 2017-10-05 01:25:12 PDT

Comment on attachment 322793 [details] WIP Attachment 322793 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4764600 New failing tests: fast/text/loading-failure-finish.html fast/text/loading-swap-nofinish.html fast/text/loading-block-nofinish.html fast/text/loading-failure-nofinish.html

Build Bot

Comment 37 2017-10-05 01:25:14 PDT

Created attachment 322804 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Myles C. Maxfield

Comment 38 2017-10-05 10:20:30 PDT

Created attachment 322847 [details] Patch

Myles C. Maxfield

Comment 39 2017-10-05 10:53:25 PDT

Created attachment 322856 [details] Patch

Myles C. Maxfield

Comment 40 2017-10-05 10:56:56 PDT

Created attachment 322858 [details] Patch

Myles C. Maxfield

Comment 41 2017-10-05 12:02:13 PDT

Committed r222926: <http://trac.webkit.org/changeset/222926>

Carlos Alberto Lopez Perez

Comment 42 2017-10-06 01:52:47 PDT

Committed r222969: <http://trac.webkit.org/changeset/222969>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version Other

Hardware Unspecified

OS Unspecified

Product WebKit

Component Text

Assignee

Myles C. Maxfield

Reported

2017-08-09 08:26 PDT

Modified

2017-10-06 01:52 PDT History

CC List

7 users Show

URL

Keywords InRadar

Duplicates (1)

175989 View as bug list

Depends on

175382

Blocks

167332 175383

Dependencies

tree graph