Bug 158339

Summary: Show fallback text during font loads
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Severity: Normal CC: dino, herr.ernst, jonlee, koivisto, simon.fraser, thorton
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Needs tests
buildbot: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2 none

Description Myles C. Maxfield 2016-06-02 21:42:44 PDT
Show fallback text during font loads
Comment 1 Myles C. Maxfield 2016-06-02 21:42:59 PDT
Created attachment 280424 [details]
Comment 2 Myles C. Maxfield 2016-06-03 00:48:03 PDT
Created attachment 280432 [details]
Needs tests
Comment 3 Myles C. Maxfield 2016-06-03 11:35:50 PDT
Created attachment 280449 [details]
Comment 4 Build Bot 2016-06-03 12:35:17 PDT
Comment on attachment 280449 [details]

Attachment 280449 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1429928

New failing tests:
Comment 5 Build Bot 2016-06-03 12:35:19 PDT
Created attachment 280453 [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.11.4
Comment 6 Myles C. Maxfield 2016-06-03 18:12:49 PDT

*** This bug has been marked as a duplicate of bug 25207 ***
Comment 7 Darin Adler 2016-06-04 13:47:54 PDT
Comment on attachment 280449 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=280449&action=review

> Source/WebCore/css/CSSFontFace.cpp:354
> +    ASSERT(m_fontSelector);
> +    m_fontSelector->fontLoaded();
> +
> +    iterateClients(m_clients, [&](Client& client) {
> +        client.fontLoaded(*this);
> +    });

Seems confusing that fontLoaded will be called twice. Are all the callers prepared for this?

> Source/WebCore/css/CSSFontFace.cpp:442
> +        double timeout = 3; // seconds

Doesn’t seem good for this 3 second value to be here in the setStatus function. Can we put it at the top of the file with a comment explaining the reason we chose 3 seconds?

> Source/WebCore/css/CSSFontFace.h:160
> +    Timer m_timeoutTimer;

Timers are pretty big objects. Does each CSSFontFace really need to have its own timer just for this? Maybe use a std::unique_ptr<Timer> instead? Or maybe CSSFontFace is really huge already and this isn’t a significant additional stuff.

Is there some set of other data members that are only relevant for web fonts? Seems unfortunate the things about font loading are all mixed in with the things about fonts “at rest”.