Bug 175384 - Implement font-display loading behaviors
Summary: Implement font-display loading behaviors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 175989 (view as bug list)
Depends on: 175382
Blocks: 167332 175383
  Show dependency treegraph
 
Reported: 2017-08-09 08:26 PDT by Myles C. Maxfield
Modified: 2017-10-06 01:52 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2017-08-09 08:26:30 PDT
Implement font-display loading behaviors
Comment 1 Radar WebKit Bug Importer 2017-08-09 15:34:45 PDT
<rdar://problem/33813243>
Comment 2 Myles C. Maxfield 2017-09-13 21:19:19 PDT
Created attachment 320726 [details]
WIP
Comment 3 Build Bot 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.
Comment 4 Myles C. Maxfield 2017-09-13 22:27:46 PDT
Created attachment 320727 [details]
WIP
Comment 5 Build Bot 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.
Comment 6 Myles C. Maxfield 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Build Bot 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
Comment 13 Build Bot 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
Comment 14 Build Bot 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
Comment 15 Myles C. Maxfield 2017-09-26 18:28:30 PDT
Created attachment 321919 [details]
WIP
Comment 16 Myles C. Maxfield 2017-09-28 11:51:21 PDT
Created attachment 322104 [details]
WIP
Comment 17 Myles C. Maxfield 2017-09-29 10:49:28 PDT
Created attachment 322210 [details]
Patch
Comment 18 Myles C. Maxfield 2017-09-29 11:42:34 PDT
Created attachment 322216 [details]
Patch
Comment 19 Build Bot 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
Comment 20 Build Bot 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
Comment 21 Myles C. Maxfield 2017-09-29 15:42:51 PDT
The test fails because FontCache::lastResortFallbackFont() is different between macOS and iOS.
Comment 22 Myles C. Maxfield 2017-09-29 15:48:49 PDT
Created attachment 322247 [details]
Patch
Comment 23 Myles C. Maxfield 2017-09-29 15:52:44 PDT
*** Bug 175989 has been marked as a duplicate of this bug. ***
Comment 24 WebKit Commit Bot 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
Comment 25 Darin Adler 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
Comment 26 Myles C. Maxfield 2017-10-03 10:15:02 PDT
Created attachment 322540 [details]
Rebased
Comment 27 Myles C. Maxfield 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.
Comment 28 Myles C. Maxfield 2017-10-04 23:43:05 PDT
Created attachment 322790 [details]
WIP
Comment 29 Myles C. Maxfield 2017-10-04 23:51:12 PDT
Created attachment 322793 [details]
WIP
Comment 30 Build Bot 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
Comment 31 Build Bot 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
Comment 32 Build Bot 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
Comment 33 Build Bot 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
Comment 34 Build Bot 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
Comment 35 Build Bot 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
Comment 36 Build Bot 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
Comment 37 Build Bot 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
Comment 38 Myles C. Maxfield 2017-10-05 10:20:30 PDT
Created attachment 322847 [details]
Patch
Comment 39 Myles C. Maxfield 2017-10-05 10:53:25 PDT
Created attachment 322856 [details]
Patch
Comment 40 Myles C. Maxfield 2017-10-05 10:56:56 PDT
Created attachment 322858 [details]
Patch
Comment 41 Myles C. Maxfield 2017-10-05 12:02:13 PDT
Committed r222926: <http://trac.webkit.org/changeset/222926>
Comment 42 Carlos Alberto Lopez Perez 2017-10-06 01:52:47 PDT
Committed r222969: <http://trac.webkit.org/changeset/222969>