Bug 168888 – Implement font-stretch for installed fonts

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

[Myles C. Maxfield]

Implementing the 'wdth' variation is important, and requires implementing font-stretch.

[Myles C. Maxfield]

*** Bug 169017 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

Created attachment 303042 [details]
Needs @font-face and font shorthand

[Jon Lee]

rdar://problem/28984856

[Myles C. Maxfield]

Created attachment 303094 [details]
Patch

[Jon Lee]

rdar://problem/30332082

[Myles C. Maxfield]

Created attachment 303100 [details]
Patch

[Myles C. Maxfield]

Created attachment 303103 [details]
Patch

[Myles C. Maxfield]

Created attachment 303110 [details]
Patch

[Build Bot]

Comment on attachment 303110 [details]
Patch

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

New failing tests:
fast/text/font-stretch-parse.html
fast/css/font-shorthand-mix-inherit.html

[Build Bot]

Created attachment 303123 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[Build Bot]

Comment on attachment 303110 [details]
Patch

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

New failing tests:
fast/text/font-stretch-parse.html
fast/css/font-shorthand-mix-inherit.html
fast/css/css2-system-fonts.html

[Build Bot]

Created attachment 303128 [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.6

[Darin Adler]

Comment on attachment 303110 [details]
Patch

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

I think we should try to figure out how to do more of what the  fast/text/font-weights test covers using some kind of reference test or special hook to help detect exactly what font was chosen. The expected PNG file for that test seems especially wasteful since it's covering only the small start of the tall document, so it both has poor coverage and is sensitive to font rendering details that we are not trying to test.

> Source/WebCore/css/StyleBuilderConverter.h:1157
> +inline float StyleBuilderConverter::convertFontStretch(StyleResolver&, const CSSValue& value)

Can this take a CSSPrimitiveValue and put the typecast at the call site, or is there some reason not to do that?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:885
> +static RefPtr<CSSPrimitiveValue> consumeFontStretch(CSSParserTokenRange& range, FontStretchValuesAccepted accept = FontStretchValuesAccepted::AllValues)

This should be two separate functions, not one function with a boolean/enum argument. Something like consumeFontStretchKeyword and consumeFontStretchValue (maybe you can come up with even better names); the value one would call the keyword one.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:888
> +    const CSSParserToken& token = range.peek();
> +    switch (token.id()) {

I don’t think the local variable is helpful here. I would just write range.peek().id() instead.

> Source/WebCore/platform/graphics/FontCache.h:112
> +        hasher.add(m_stretch);

It is best to hash just the integer part of the stretch, guaranteeing collisions any time the difference is only in the fractional part? Another possibility would be to hash the actual 32 bits of the float, using an appropriate typecast to pass those bits as an unsigned 32-bit integer to IntegerHasher. Or maybe IntegerHasher already has an overload for float that does that?

> Source/WebCore/platform/graphics/FontCache.h:114
>          for (unsigned flagItem : m_flags)
>              hasher.add(flagItem);

We typically would want to hash the length of m_flags too in some way, not just the content. By perhaps hashing a fixed trailing value, or by literally hashing the length.

> Source/WebCore/platform/graphics/FontCache.h:115
>          hasher.add(m_featureSettings.hash());

Hash of a hash is not as good as actually hashing all the data, although I suppose we are using that technique elsewhere too.

Ideally, we need to test our hashes with test cases that record collision rates in hash tables.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:1019
> +                return threshold - font.stretch.maximum;

This is indented one tab stop too many.

[Build Bot]

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

[Dave Hyatt]

Comment on attachment 303110 [details]
Patch

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

Some comments about parser-specific stuff:

> Source/WebCore/css/parser/CSSPropertyParser.cpp:888
> +    const CSSParserToken& token = range.peek();
> +    switch (token.id()) {

You can just write:

switch (range.peek().id())

> Source/WebCore/css/parser/CSSPropertyParser.cpp:898
> +    case CSSValueUltraCondensed:
> +    case CSSValueExtraCondensed:
> +    case CSSValueCondensed:
> +    case CSSValueSemiCondensed:
> +    case CSSValueNormal:
> +    case CSSValueSemiExpanded:
> +    case CSSValueExpanded:
> +    case CSSValueExtraExpanded:
> +    case CSSValueUltraExpanded:
> +        return consumeIdent(range);

consumeIdent does take a <> syntax where you can list the idents you want to accept. Will leave it up to you if you want to make that change, e.g.,

auto ident = consumeIdent<CSSValueUltraCondensed, CSSValueExtraCondensed, etc.>(range);
if (ident)
return ident;

[Build Bot]

Comment on attachment 303110 [details]
Patch

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

New failing tests:
fast/text/font-stretch-parse.html
fast/css/font-shorthand-mix-inherit.html

[Build Bot]

Created attachment 303132 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

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

[Myles C. Maxfield]

Comment on attachment 303110 [details]
Patch

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

>> Source/WebCore/css/StyleBuilderConverter.h:1157
>> +inline float StyleBuilderConverter::convertFontStretch(StyleResolver&, const CSSValue& value)
> 
> Can this take a CSSPrimitiveValue and put the typecast at the call site, or is there some reason not to do that?

Unfortunately, this is called from generated code :(

>> Source/WebCore/platform/graphics/FontCache.h:112
>> +        hasher.add(m_stretch);
> 
> It is best to hash just the integer part of the stretch, guaranteeing collisions any time the difference is only in the fractional part? Another possibility would be to hash the actual 32 bits of the float, using an appropriate typecast to pass those bits as an unsigned 32-bit integer to IntegerHasher. Or maybe IntegerHasher already has an overload for float that does that?

This is a good idea; I think it's worth migrating to fixed-point values. However, that would be a big enough change to warrant its own patch.

>> Source/WebCore/platform/graphics/FontCache.h:114
>>              hasher.add(flagItem);
> 
> We typically would want to hash the length of m_flags too in some way, not just the content. By perhaps hashing a fixed trailing value, or by literally hashing the length.

I don't understand. m_flags has a fixed size, so what value is there in hashing a constant "2" for all items in the cache?

[Myles C. Maxfield]

Created attachment 303136 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 303137 [details]
Patch for committing

[Build Bot]

Comment on attachment 303137 [details]
Patch for committing

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

New failing tests:
imported/blink/fast/css/font-face-inherit-initial.html
fast/css/font-shorthand-mix-inherit.html
fast/text/font-stretch-shorthand.html
fast/text/font-stretch-parse.html

[Build Bot]

Created attachment 303140 [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 303137 [details]
Patch for committing

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

New failing tests:
fast/text/font-stretch-shorthand.html
fast/css/font-shorthand-mix-inherit.html
fast/text/font-stretch-parse.html

[Build Bot]

Created attachment 303141 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

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

[Build Bot]

Comment on attachment 303137 [details]
Patch for committing

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

New failing tests:
fast/text/font-stretch-shorthand.html
fast/css/font-shorthand-mix-inherit.html
fast/text/font-stretch-parse.html

[Build Bot]

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

[Build Bot]

Comment on attachment 303137 [details]
Patch for committing

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

New failing tests:
fast/text/font-stretch-shorthand.html
fast/css/font-shorthand-mix-inherit.html
fast/text/font-stretch-parse.html

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 303149 [details]
Patch for committing

[Build Bot]

Comment on attachment 303149 [details]
Patch for committing

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

New failing tests:
fast/inspector-support/style.html
fast/css/font-shorthand.html
fast/text/font-stretch-parse.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 303149 [details]
Patch for committing

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

New failing tests:
fast/inspector-support/style.html
fast/css/font-shorthand.html
fast/text/font-stretch-parse.html

[Build Bot]

Created attachment 303157 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Build Bot]

Comment on attachment 303149 [details]
Patch for committing

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

New failing tests:
fast/inspector-support/style.html
fast/css/font-shorthand.html
fast/text/font-stretch-parse.html

[Build Bot]

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

[Build Bot]

Comment on attachment 303149 [details]
Patch for committing

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

New failing tests:
fast/inspector-support/style.html
fast/css/font-shorthand.html
fast/text/font-stretch-parse.html

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 303162 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 303173 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 303173 [details]
Patch for committing

Clearing flags on attachment: 303173

Committed r213267: <http://trac.webkit.org/changeset/213267>