Implement font-display loading behaviors
<rdar://problem/33813243>
Created attachment 320726 [details] WIP
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.
Created attachment 320727 [details] WIP
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.
<!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
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
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
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
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
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
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
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
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
Created attachment 321919 [details] WIP
Created attachment 322104 [details] WIP
Created attachment 322210 [details] Patch
Created attachment 322216 [details] Patch
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
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
The test fails because FontCache::lastResortFallbackFont() is different between macOS and iOS.
Created attachment 322247 [details] Patch
*** Bug 175989 has been marked as a duplicate of this bug. ***
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
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
Created attachment 322540 [details] Rebased
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.
Created attachment 322790 [details] WIP
Created attachment 322793 [details] WIP
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
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
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
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
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
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
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
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
Created attachment 322847 [details] Patch
Created attachment 322856 [details] Patch
Created attachment 322858 [details] Patch
Committed r222926: <http://trac.webkit.org/changeset/222926>
Committed r222969: <http://trac.webkit.org/changeset/222969>