Bug 168487 – REGRESSION(r212513): LastResort is platform-dependent, so its semantics should not be required to perform font loading correctly.

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Myles C. Maxfield]

LastResort is platform-dependent, so its semantics should not be required to perform font loading correctly.

[Myles C. Maxfield]

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

[Michael Catanzaro]

In other words: it'd be really great if we didn't display text using system fonts and then later switch it over to using web fonts. The visual transition there is jarring. It works properly on macOS because of the invisible LastResort font available there.

Or something like that.

[Myles C. Maxfield]

<rdar://problem/31180111>

[Myles C. Maxfield]

Created attachment 309631 [details]
WIP

[Myles C. Maxfield]

Created attachment 309802 [details]
WIP

[Myles C. Maxfield]

Created attachment 309805 [details]
WIP

[Myles C. Maxfield]

Created attachment 309810 [details]
Patch

[Michael Catanzaro]

I can't test this right now, but thank you!

[Build Bot]

Comment on attachment 309810 [details]
Patch

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

New failing tests:
http/tests/webfont/fallback-font-while-loading.html

[Build Bot]

Created attachment 309840 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Myles C. Maxfield]

Created attachment 309884 [details]
WIP

[Myles C. Maxfield]

Created attachment 309919 [details]
WIP

[Myles C. Maxfield]

Created attachment 309939 [details]
WIP

[Myles C. Maxfield]

Created attachment 309950 [details]
WIP

[Myles C. Maxfield]

Created attachment 309978 [details]
Patch

[Myles C. Maxfield]

Created attachment 309984 [details]
Patch

[Myles C. Maxfield]

Created attachment 309985 [details]
Patch

[Myles C. Maxfield]

Created attachment 309988 [details]
Patch

[Myles C. Maxfield]

Created attachment 309991 [details]
Patch

[Build Bot]

Comment on attachment 309991 [details]
Patch

Attachment 309991 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3729904

New failing tests:
fast/text/web-font-load-fallback-during-loading.html

[Build Bot]

Created attachment 309998 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309991 [details]
Patch

Attachment 309991 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3729961

New failing tests:
fast/text/web-font-load-fallback-during-loading.html

[Build Bot]

Created attachment 310003 [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 on attachment 309991 [details]
Patch

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

New failing tests:
fast/text/web-font-load-fallback-during-loading.html
fast/text/web-font-load-invisible-during-loading.html

[Build Bot]

Created attachment 310004 [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.11.6

[Myles C. Maxfield]

Created attachment 310014 [details]
WIP

[Build Bot]

Comment on attachment 310014 [details]
WIP

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

New failing tests:
fast/text/web-font-load-fallback-during-loading.html
fast/text/web-font-load-invisible-during-loading.html

[Build Bot]

Created attachment 310015 [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.11.6

[Myles C. Maxfield]

Created attachment 310020 [details]
Patch

[Build Bot]

Comment on attachment 310020 [details]
Patch

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

New failing tests:
fast/text/web-font-load-fallback-during-loading.html
fast/text/web-font-load-invisible-during-loading.html

[Build Bot]

Created attachment 310026 [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.11.6

[Myles C. Maxfield]

Created attachment 310073 [details]
Patch

[Build Bot]

Comment on attachment 310073 [details]
Patch

Attachment 310073 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3737260

New failing tests:
fast/text/web-font-load-fallback-during-loading.html

[Build Bot]

Created attachment 310076 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310073 [details]
Patch

Attachment 310073 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3737291

New failing tests:
fast/text/web-font-load-fallback-during-loading.html

[Build Bot]

Created attachment 310077 [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 on attachment 310073 [details]
Patch

Attachment 310073 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3737302

New failing tests:
fast/text/web-font-load-fallback-during-loading.html

[Build Bot]

Created attachment 310078 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310073 [details]
Patch

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

New failing tests:
fast/text/web-font-load-fallback-during-loading.html

[Build Bot]

Created attachment 310080 [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.11.6

[Myles C. Maxfield]

Created attachment 310094 [details]
Patch

[Antti Koivisto]

Comment on attachment 310094 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:532
> +            m_timeoutTimer.startOneShot(3_s);

Please use an informatively named constants for numeric values like this. It can be inline, just above the using line.

> Source/WebCore/platform/graphics/Font.h:90
> +    enum class ShouldDisplay {
> +        Visible,
> +        Invisible
> +    };

"Display" and "Visible" seems like two different words for the same thing. How about calling this enum "Visibility"?

> Source/WebCore/platform/graphics/Font.h:94
> +    enum class OrientationFallback {
> +        Fallback,
> +        NoFallback
> +    };

"Fallback" seems redundant in the value. Just Yes/No maybe like Interstitial has?

> Source/WebCore/platform/graphics/Font.h:189
> +    Interstitial isInterstitial() const { return m_interstitial ? Interstitial::Yes : Interstitial::No; }

is* functions should return bools. Either rename this to interstitial() or make it return a bool. Currently the call sites read awkwardly.

> Source/WebCore/platform/graphics/Font.h:190
> +    ShouldDisplay shouldDisplay() const { return m_shouldDisplay ? ShouldDisplay::Visible : ShouldDisplay::Invisible; }

As should should* functions. "Visibility visibility() const" would work.

> Source/WebCore/platform/graphics/Font.h:318
>      unsigned m_treatAsFixedPitch : 1;
> -    unsigned m_isCustomFont : 1; // Whether or not we are custom font loaded via @font-face
> -    unsigned m_isLoading : 1; // Whether or not this custom font is still in the act of loading.
> +    unsigned m_origin : 1; // Whether or not we are custom font loaded via @font-face
> +    unsigned m_interstitial : 1; // Whether or not this custom font is the last resort placeholder for a loading font
> +    unsigned m_shouldDisplay : 1; // @font-face's internal timer can cause us to show fonts even when a font is being downloaded.
>  
> -    unsigned m_isTextOrientationFallback : 1;
> +    unsigned m_orientationFallback : 1;
>      unsigned m_isBrokenIdeographFallback : 1;
>      unsigned m_hasVerticalGlyphs : 1;

There are not enough Fonts for it to be worth to optimize this with bitfields (and size of associated actual font data is many orders of magnitude greater). It would be better to just use properly typed enum values.

> Source/WebCore/platform/graphics/FontCascade.cpp:1402
> +    // Don't draw anything while we are using custom fonts that are in the process of loading,
> +    // except if the 'force' argument is set to true (in which case it will use a fallback
> +    // font).
> +    return font.isInterstitial() == Font::Interstitial::No || font.shouldDisplay() == Font::ShouldDisplay::Visible || customFontNotReadyAction == FontCascade::CustomFontNotReadyAction::UseFallbackIfFontNotReady;

I'm not sure I understand how the comment and the code matches. The comment mentions 'force' argument and loading. The code has nothing called 'force' and nothing obviously about loading. It also tests three conditions while the text implies two.

> Source/WebCore/platform/graphics/FontRanges.h:40
> +enum class ForbidDownloadingExternalResource {
> +    Forbid,
> +    Allow
> +};

ExternalResourceDownloadPolicy? You even already name the variable 'policy' in many place.

[Myles C. Maxfield]

Comment on attachment 310094 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.h:318
>>      unsigned m_hasVerticalGlyphs : 1;
> 
> There are not enough Fonts for it to be worth to optimize this with bitfields (and size of associated actual font data is many orders of magnitude greater). It would be better to just use properly typed enum values.

I'll keep the Yes/No types as bitfields since their accessors are isXXXX() and will return bools. But I'll make Origin and Visibility use their real types internally.

[Myles C. Maxfield]

Created attachment 310289 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 310289 [details]
Patch for committing

Rejecting attachment 310289 [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-03', 'validate-changelog', '--check-oops', '--non-interactive', 310289, '--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/3752297

[Myles C. Maxfield]

Committed r216944: <http://trac.webkit.org/changeset/216944>