Bug 168114 – font-weight in @font-face can cause a font to be downloaded even when it's not used

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

font-weight in @font-face can cause a font to be downloaded even when it's not used

[Myles C. Maxfield]

Created attachment 301147 [details]
Patch

[Myles C. Maxfield]

Created attachment 301148 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/30301317>

[Myles C. Maxfield]

This patch saves 7MB from http://www.apple.com/cn/iphone-7/

[Build Bot]

Comment on attachment 301148 [details]
Patch

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

New failing tests:
svg/custom/svg-fonts-in-html-linebreaks.html
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg
http/tests/webfont/fallback-font-while-loading.html

[Build Bot]

Created attachment 301150 [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 301148 [details]
Patch

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

New failing tests:
svg/custom/svg-fonts-in-html-linebreaks.html
http/tests/webfont/fallback-font-while-loading.html
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg

[Build Bot]

Created attachment 301151 [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

[Build Bot]

Comment on attachment 301148 [details]
Patch

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

New failing tests:
svg/custom/svg-fonts-in-html-linebreaks.html
svg/W3C-SVG-1.1-SE/struct-dom-11-f.svg
http/tests/webfont/fallback-font-while-loading.html

[Build Bot]

Created attachment 301152 [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

[Myles C. Maxfield]

Created attachment 301203 [details]
Patch

[Myles C. Maxfield]

Created attachment 301206 [details]
Patch

[Myles C. Maxfield]

Created attachment 301212 [details]
Patch

[Jon Lee]

Comment on attachment 301212 [details]
Patch

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

> LayoutTests/fast/text/font-weight-download-2.html:22
> +    font-weight: 900;

Is it useful to extend the tests to make sure font-style also doesn't cause excessive downloads?

[Myles C. Maxfield]

Created attachment 301264 [details]
Now with fewer words

[Michael Catanzaro]

Comment on attachment 301264 [details]
Now with fewer words

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

> Source/WebCore/ChangeLog:13
> +        When we are in the middle of a download, we will use a special interstitial font,
> +        and this special font has a flag set which will cause it to be invisible when it is
> +        drawn. However, when we start using this font during the load, we give it a

Hm, in WebKitGTK+, when loading a page that uses web fonts, it's can be pretty obvious that the text is first drawn using a system font, and then later replaced with the web font. Does that not happen on macOS? Is it because of your use of this LastResort font with invisible characters?

Is that really a preinstalled system font?

> Source/WebCore/ChangeLog:22
> +        The second problem with the font loading code is that this interstital font is just
> +        Times. Times doesn't support every character, which means that if we are trying
> +        to render some exotic character, we fall back to other weights. The solution here
> +        is to use LastResort as the interstitial font, because it supports all characters.
> +        Because its metrics are reasonable and we don't ever actually paint this
> +        interstitial font, this choice is no worse than Times.

So the font returned by FontCache::lastResortFallbackFontForEveryCharacter() only actually supports all characters on macOS. On Windows and Linux it's just going to be some generic font. It's a bit of a shame that this solution is platform-specific.

Since the fallback font has such specific requirements that other ports cannot meet, perhaps it should be built into WebKit...?

> LayoutTests/fast/text/font-style-download-expected.txt:2
> +Ahem.ttf - willSendRequest <NSURLRequest URL Ahem.ttf, main document URL font-style-download.html, http method GET> redirectResponse (null)

Since the test expectations contain NSURLRequest, they should be platform-specific expectations. We'll add our own in a follow-up.

[Darin Adler]

Comment on attachment 301264 [details]
Now with fewer words

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

Please consider Michael’s comments before landing.

> Source/WebCore/platform/graphics/Font.h:343
> +    // The loading font (LastResort) only includes a few glyphs, which means many characters map to the same glyphs.
> +    // Therefore, this check is overly broad in this circumstance.

It took me a really long time to understand this comment. Part of the problem is that "this check" refers to the first half of the if statement, and the old code. But future readers will think that "this check" means the entire thing inside the if. Here’s the comment I would write:

    // The optimization of returning 0 for the zero-width-space glyph is incorrect for the LastResort font,
    // used in place of the actual font when isLoading() is true on both macOS and iOS.
    // The zero-width-space glyph in that font does not have a width of zero and, further, that glyph is used
    // for many other characters and must not be zero width when used for them.

Of course, once the comment is written like this, other questions arise about whether this code is fully correct. Examples: Is a special case for the zero-width-space glyph truly only an optimization or is it needed for correctness? Is it legitimate to allow the zero-width-space glyph to have a non-zero width in the LastResort font when it’s used for the zero-width-space character? Does it make sense to name the glyph used for the zero-width-space character the zero-width-space glyph, when it might be something more like a “missing character” glyph? Should our code rely on the presence of a zero-width glyph in the font for the zero-width-space character (since the LastResort font, at least, does not have that), or should it instead enforce a rule that renders nothing and measures as zero width at the WebKit level ignoring the glyph that the font specifies? Can it even do that for the complex text code path?

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:39
> +#if !PLATFORM(IOS)

I prefer to write this as #if PLATFORM(MAC).

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:53
> +#if !PLATFORM(IOS)

Ditto.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:395
> +#endif // !PLATFORM(IOS)

Ditto. Also, please leave a blank line before so that the spacing here matches the spacing after the #if.

>> LayoutTests/fast/text/font-style-download-expected.txt:2
>> +Ahem.ttf - willSendRequest <NSURLRequest URL Ahem.ttf, main document URL font-style-download.html, http method GET> redirectResponse (null)
> 
> Since the test expectations contain NSURLRequest, they should be platform-specific expectations. We'll add our own in a follow-up.

But do they need to be for these tests? Can we come back here and change the logging so it's not platform specific?

[Myles C. Maxfield]

Comment on attachment 301264 [details]
Now with fewer words

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

>> Source/WebCore/ChangeLog:13
>> +        drawn. However, when we start using this font during the load, we give it a
> 
> Hm, in WebKitGTK+, when loading a page that uses web fonts, it's can be pretty obvious that the text is first drawn using a system font, and then later replaced with the web font. Does that not happen on macOS? Is it because of your use of this LastResort font with invisible characters?
> 
> Is that really a preinstalled system font?

Prior to this patch, some scenarios had the behavior you describe but some didn't. If there was another font in the CSSSegmentedFontFace, we would use that during the load (ignoring the fact that using it might cause a second load). However, if not, the interstitial font should be used which gets drawn invisibly. This patch modifies the first scenario to not kick off an additional load.

>> Source/WebCore/ChangeLog:22
>> +        interstitial font, this choice is no worse than Times.
> 
> So the font returned by FontCache::lastResortFallbackFontForEveryCharacter() only actually supports all characters on macOS. On Windows and Linux it's just going to be some generic font. It's a bit of a shame that this solution is platform-specific.
> 
> Since the fallback font has such specific requirements that other ports cannot meet, perhaps it should be built into WebKit...?

Yeah, you're right. A different solution would be to find every place where we check if a font supports a particular character, and guard that to make the interstitial font short-circuit that machinery. However, from talking to Jon, we've decided to take this simpler approach in the short-term, but then also work on https://bugs.webkit.org/show_bug.cgi?id=168487 after this is committed.

>> Source/WebCore/platform/graphics/Font.h:343
>> +    // Therefore, this check is overly broad in this circumstance.
> 
> It took me a really long time to understand this comment. Part of the problem is that "this check" refers to the first half of the if statement, and the old code. But future readers will think that "this check" means the entire thing inside the if. Here’s the comment I would write:
> 
>     // The optimization of returning 0 for the zero-width-space glyph is incorrect for the LastResort font,
>     // used in place of the actual font when isLoading() is true on both macOS and iOS.
>     // The zero-width-space glyph in that font does not have a width of zero and, further, that glyph is used
>     // for many other characters and must not be zero width when used for them.
> 
> Of course, once the comment is written like this, other questions arise about whether this code is fully correct. Examples: Is a special case for the zero-width-space glyph truly only an optimization or is it needed for correctness? Is it legitimate to allow the zero-width-space glyph to have a non-zero width in the LastResort font when it’s used for the zero-width-space character? Does it make sense to name the glyph used for the zero-width-space character the zero-width-space glyph, when it might be something more like a “missing character” glyph? Should our code rely on the presence of a zero-width glyph in the font for the zero-width-space character (since the LastResort font, at least, does not have that), or should it instead enforce a rule that renders nothing and measures as zero width at the WebKit level ignoring the glyph that the font specifies? Can it even do that for the complex text code path?

Because this font isn't actually seen by users, the exact metrics aren't particularly important (since the entire content reflows anyway when the real font finishes downloading). In LastResort, the space character is mapped to the same character as ZWS, which means that this function was returning different results than were returned in other places in WebCore where we measure widths. We want measuring text in different ways to agree with each other, which usually happens naturally, but needs extra care here because LastResort has uncommon glyph mappings. I've updated the comment.

>>> LayoutTests/fast/text/font-style-download-expected.txt:2
>>> +Ahem.ttf - willSendRequest <NSURLRequest URL Ahem.ttf, main document URL font-style-download.html, http method GET> redirectResponse (null)
>> 
>> Since the test expectations contain NSURLRequest, they should be platform-specific expectations. We'll add our own in a follow-up.
> 
> But do they need to be for these tests? Can we come back here and change the logging so it's not platform specific?

Probably not, but I can tackle this in a follow-up patch

[Myles C. Maxfield]

Committed r212513: <http://trac.webkit.org/changeset/212513>