Bug 168888 - Implement font-stretch for installed fonts
Summary: Implement font-stretch for installed fonts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
: 169017 (view as bug list)
Depends on: 169017
Blocks: 162815
  Show dependency treegraph
 
Reported: 2017-02-26 21:09 PST by Myles C. Maxfield
Modified: 2017-03-01 22:53 PST (History)
6 users (show)

See Also:


Attachments
Needs @font-face and font shorthand (712.68 KB, patch)
2017-02-28 22:44 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (735.59 KB, patch)
2017-03-01 11:55 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (774.62 KB, patch)
2017-03-01 12:51 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (775.17 KB, patch)
2017-03-01 13:07 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (778.07 KB, patch)
2017-03-01 13:44 PST, Myles C. Maxfield
hyatt: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.16 MB, application/zip)
2017-03-01 14:44 PST, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (859.52 KB, application/zip)
2017-03-01 14:57 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (989.19 KB, application/zip)
2017-03-01 14:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (1.69 MB, application/zip)
2017-03-01 15:09 PST, Build Bot
no flags Details
Patch for committing (780.60 KB, patch)
2017-03-01 15:39 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (780.63 KB, patch)
2017-03-01 15:40 PST, Myles C. Maxfield
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.98 MB, application/zip)
2017-03-01 16:57 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-elcapitan (1.09 MB, application/zip)
2017-03-01 17:01 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (1.01 MB, application/zip)
2017-03-01 17:03 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.48 MB, application/zip)
2017-03-01 17:05 PST, Build Bot
no flags Details
Patch for committing (781.47 KB, patch)
2017-03-01 17:37 PST, Myles C. Maxfield
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (964.30 KB, application/zip)
2017-03-01 18:50 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.68 MB, application/zip)
2017-03-01 18:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (947.36 KB, application/zip)
2017-03-01 19:02 PST, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-elcapitan (912.34 KB, application/zip)
2017-03-01 19:13 PST, Build Bot
no flags Details
Patch for committing (781.16 KB, patch)
2017-03-01 19:46 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for committing (781.16 KB, patch)
2017-03-01 21:14 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2017-02-26 21:09:06 PST
Implementing the 'wdth' variation is important, and requires implementing font-stretch.
Comment 1 Myles C. Maxfield 2017-02-28 22:42:55 PST
*** Bug 169017 has been marked as a duplicate of this bug. ***
Comment 2 Myles C. Maxfield 2017-02-28 22:44:24 PST
Created attachment 303042 [details]
Needs @font-face and font shorthand
Comment 3 Jon Lee 2017-03-01 02:36:28 PST
rdar://problem/28984856
Comment 4 Myles C. Maxfield 2017-03-01 11:55:29 PST
Created attachment 303094 [details]
Patch
Comment 5 Jon Lee 2017-03-01 12:49:22 PST
rdar://problem/30332082
Comment 6 Myles C. Maxfield 2017-03-01 12:51:01 PST
Created attachment 303100 [details]
Patch
Comment 7 Myles C. Maxfield 2017-03-01 13:07:51 PST
Created attachment 303103 [details]
Patch
Comment 8 Myles C. Maxfield 2017-03-01 13:44:33 PST
Created attachment 303110 [details]
Patch
Comment 9 Build Bot 2017-03-01 14:44:37 PST
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
Comment 10 Build Bot 2017-03-01 14:44:41 PST
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
Comment 11 Build Bot 2017-03-01 14:57:24 PST
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
Comment 12 Build Bot 2017-03-01 14:57:28 PST
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
Comment 13 Darin Adler 2017-03-01 14:59:51 PST
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.
Comment 14 Build Bot 2017-03-01 14:59:55 PST
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
Comment 15 Dave Hyatt 2017-03-01 15:04:46 PST
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;
Comment 16 Build Bot 2017-03-01 15:09:39 PST
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
Comment 17 Build Bot 2017-03-01 15:09:42 PST
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
Comment 18 Myles C. Maxfield 2017-03-01 15:37:42 PST
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?
Comment 19 Myles C. Maxfield 2017-03-01 15:39:43 PST
Created attachment 303136 [details]
Patch for committing
Comment 20 Myles C. Maxfield 2017-03-01 15:40:42 PST
Created attachment 303137 [details]
Patch for committing
Comment 21 Build Bot 2017-03-01 16:57:39 PST
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
Comment 22 Build Bot 2017-03-01 16:57:43 PST
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
Comment 23 Build Bot 2017-03-01 17:01:08 PST
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
Comment 24 Build Bot 2017-03-01 17:01:12 PST
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
Comment 25 Build Bot 2017-03-01 17:03:45 PST
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
Comment 26 Build Bot 2017-03-01 17:03:48 PST
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
Comment 27 Build Bot 2017-03-01 17:05:35 PST
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
Comment 28 Build Bot 2017-03-01 17:05:38 PST
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
Comment 29 Myles C. Maxfield 2017-03-01 17:37:04 PST
Created attachment 303149 [details]
Patch for committing
Comment 30 Build Bot 2017-03-01 18:50:45 PST
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
Comment 31 Build Bot 2017-03-01 18:50:50 PST
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
Comment 32 Build Bot 2017-03-01 18:59:50 PST
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
Comment 33 Build Bot 2017-03-01 18:59:55 PST
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
Comment 34 Build Bot 2017-03-01 19:02:51 PST
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
Comment 35 Build Bot 2017-03-01 19:02:55 PST
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
Comment 36 Build Bot 2017-03-01 19:13:02 PST
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
Comment 37 Build Bot 2017-03-01 19:13:06 PST
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
Comment 38 Myles C. Maxfield 2017-03-01 19:46:53 PST
Created attachment 303162 [details]
Patch for committing
Comment 39 Myles C. Maxfield 2017-03-01 21:14:21 PST
Created attachment 303173 [details]
Patch for committing
Comment 40 WebKit Commit Bot 2017-03-01 22:44:29 PST
Comment on attachment 303173 [details]
Patch for committing

Clearing flags on attachment: 303173

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