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.
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
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
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
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
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
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
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
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
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
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
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.
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.
2017-05-10 14:09 PDT, Myles C. Maxfield
2017-05-11 15:41 PDT, Myles C. Maxfield
2017-05-11 16:00 PDT, Myles C. Maxfield
2017-05-11 16:15 PDT, Myles C. Maxfield
2017-05-11 18:20 PDT, Build Bot
2017-05-12 02:10 PDT, Myles C. Maxfield
2017-05-12 11:10 PDT, Myles C. Maxfield
2017-05-12 13:56 PDT, Myles C. Maxfield
2017-05-12 15:00 PDT, Myles C. Maxfield
2017-05-12 17:49 PDT, Myles C. Maxfield
2017-05-12 18:05 PDT, Myles C. Maxfield
2017-05-12 18:16 PDT, Myles C. Maxfield
2017-05-12 18:21 PDT, Myles C. Maxfield
2017-05-12 18:31 PDT, Myles C. Maxfield
2017-05-12 19:39 PDT, Build Bot
2017-05-12 20:06 PDT, Build Bot
2017-05-12 20:14 PDT, Build Bot
2017-05-12 23:20 PDT, Myles C. Maxfield
2017-05-13 01:06 PDT, Build Bot
2017-05-13 11:12 PDT, Myles C. Maxfield
2017-05-13 12:51 PDT, Build Bot
2017-05-14 00:06 PDT, Myles C. Maxfield
2017-05-14 01:14 PDT, Build Bot
2017-05-14 01:18 PDT, Build Bot
2017-05-14 01:22 PDT, Build Bot
2017-05-14 01:44 PDT, Build Bot
2017-05-14 11:22 PDT, Myles C. Maxfield
2017-05-16 11:55 PDT, Myles C. Maxfield