Bug 147722 – Implement font-feature-settings

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Implement font-feature-settings

[Myles C. Maxfield]

Created attachment 258349 [details]
No caching yet

[Myles C. Maxfield]

<div style="font: 100px 'Hiragino Sans GB'; -webkit-font-feature-settings: 'trad';">&#x4f53;</div> has a different on OS X

[Myles C. Maxfield]

Created attachment 258425 [details]
Patch

[Myles C. Maxfield]

Still needs tests. Reftests will be difficult, so I think (for now) I'll make dump-render-tree tests.

[WebKit Commit Bot]

Attachment 258425 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:19:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 258427 [details]
Patch

[WebKit Commit Bot]

Attachment 258427 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:19:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 258428 [details]
Patch

[WebKit Commit Bot]

Attachment 258428 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:19:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 258431 [details]
Patch

[WebKit Commit Bot]

Attachment 258431 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:19:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 258432 [details]
Patch

[WebKit Commit Bot]

Attachment 258432 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:19:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 258433 [details]
Patch

[WebKit Commit Bot]

Attachment 258433 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:19:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 258433 [details]
Patch

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

New failing tests:
fonts/unicode-character-font-crash.html

[Build Bot]

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

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

[Myles C. Maxfield]

Created attachment 258444 [details]
Patch

[WebKit Commit Bot]

Attachment 258444 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:19:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

Created attachment 258456 [details]
Patch

[Myles C. Maxfield]

Created attachment 258472 [details]
Patch

[Myles C. Maxfield]

Created attachment 258477 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 258477 [details]
Patch

Marking as r? because this is ready to review (even though EWS is going to show all red because this patch depends on https://bugs.webkit.org/show_bug.cgi?id=147775)

[Myles C. Maxfield]

Created attachment 258507 [details]
Patch

[Myles C. Maxfield]

Created attachment 258511 [details]
Patch

[Myles C. Maxfield]

Created attachment 258512 [details]
Patch

[Myles C. Maxfield]

Committed r188146: <http://trac.webkit.org/changeset/188146>

[Daniel Bates]

Comment on attachment 258512 [details]
Patch

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

I know that this patch was already reviewed and landed. I had some comments and questions.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:33
> +    // FIXME: We should map OpenType feature strings to the TrueType feature type identifiers listed in <CoreText/SFNTLayoutTypes.h>

Nit: Missing a period at the end of this sentence.

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:35
> +    UNUSED_PARAM(features);
> +    UNUSED_PARAM(feature);

Do we get a compiler warning for an empty function? If not then I suggest we remove the name of each argument (e.g. features) in the prototype for this function such that is reads:

static inline void appendTrueTypeFeature(CFMutableArrayRef, const FontFeature&)

Then we do not need to use UNUSED_PARAM().

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:45
> +    CFStringRef featureDictionaryKeys[] = {kCTFontOpenTypeFeatureTag, kCTFontOpenTypeFeatureValue};
> +    CFTypeRef featureDictionaryValues[] = {featureKey.get(), featureValue.get()};

Nit: It is an unwritten convention that when using aggregate initialization syntax that we put a space character after the opening brace and a space character before the closing brace such that these line would read:

> Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:46
> +    RetainPtr<CFDictionaryRef> featureDictionary = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, (const void**)featureDictionaryKeys, featureDictionaryValues, 2, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));

We should use WTF_ARRAY_LENGTH(featureDictionaryValues) to compute the number of elements in the array featureDictionaryValues instead of hardcoding its size (2).

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:88
> +    if (substituteFont && description.featureSettings() && description.featureSettings()->size())

The first conjunct is unnecessary because substituteFont is always non-null when we reach this line (since we return early if substituteFont is null on line 86).

> Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:698
> +    if (ctFont && fontDescription.featureSettings() && fontDescription.featureSettings()->size())

Similarly, the first conjunct is unnecessary because ctFont will always be non-null when we reach this line.

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:517
> +    if (result && description.featureSettings() && description.featureSettings()->size())
> +        result = applyFontFeatureSettings(result.get(), *description.featureSettings());

We check the same conditions before each call to applyFontFeatureSettings(). Unless these call site show up in a profile, I suggest moving the checks into applyFontFeatureSettings().

> Source/WebCore/platform/graphics/mac/FontCacheMac.mm:519
>      if (!result)
>          return nullptr;

I take it that it is less likely for result to be non-null in this function and hence the choice to have this if-block come after the if-block that starts on line 516?

[Myles C. Maxfield]

(In reply to comment #28)
> Comment on attachment 258512 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258512&action=review
> 
> I know that this patch was already reviewed and landed. I had some comments
> and questions.
> 
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:33
> > +    // FIXME: We should map OpenType feature strings to the TrueType feature type identifiers listed in <CoreText/SFNTLayoutTypes.h>
> 
> Nit: Missing a period at the end of this sentence.
> 
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:35
> > +    UNUSED_PARAM(features);
> > +    UNUSED_PARAM(feature);
> 
> Do we get a compiler warning for an empty function? If not then I suggest we
> remove the name of each argument (e.g. features) in the prototype for this
> function such that is reads:
> 
> static inline void appendTrueTypeFeature(CFMutableArrayRef, const
> FontFeature&)
> 
> Then we do not need to use UNUSED_PARAM().
> 
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:45
> > +    CFStringRef featureDictionaryKeys[] = {kCTFontOpenTypeFeatureTag, kCTFontOpenTypeFeatureValue};
> > +    CFTypeRef featureDictionaryValues[] = {featureKey.get(), featureValue.get()};
> 
> Nit: It is an unwritten convention that when using aggregate initialization
> syntax that we put a space character after the opening brace and a space
> character before the closing brace such that these line would read:
> 
> > Source/WebCore/platform/graphics/cocoa/FontCacheCoreText.cpp:46
> > +    RetainPtr<CFDictionaryRef> featureDictionary = adoptCF(CFDictionaryCreate(kCFAllocatorDefault, (const void**)featureDictionaryKeys, featureDictionaryValues, 2, &kCFTypeDictionaryKeyCallBacks, &kCFTypeDictionaryValueCallBacks));
> 
> We should use WTF_ARRAY_LENGTH(featureDictionaryValues) to compute the
> number of elements in the array featureDictionaryValues instead of
> hardcoding its size (2).
> 
> > Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:88
> > +    if (substituteFont && description.featureSettings() && description.featureSettings()->size())
> 
> The first conjunct is unnecessary because substituteFont is always non-null
> when we reach this line (since we return early if substituteFont is null on
> line 86).
> 
> > Source/WebCore/platform/graphics/ios/FontCacheIOS.mm:698
> > +    if (ctFont && fontDescription.featureSettings() && fontDescription.featureSettings()->size())
> 
> Similarly, the first conjunct is unnecessary because ctFont will always be
> non-null when we reach this line.
> 
> > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:517
> > +    if (result && description.featureSettings() && description.featureSettings()->size())
> > +        result = applyFontFeatureSettings(result.get(), *description.featureSettings());
> 
> We check the same conditions before each call to applyFontFeatureSettings().
> Unless these call site show up in a profile, I suggest moving the checks
> into applyFontFeatureSettings().
> 
> > Source/WebCore/platform/graphics/mac/FontCacheMac.mm:519
> >      if (!result)
> >          return nullptr;
> 
> I take it that it is less likely for result to be non-null in this function
> and hence the choice to have this if-block come after the if-block that
> starts on line 516?

Performed in https://bugs.webkit.org/show_bug.cgi?id=147793

[Myles C. Maxfield]

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

[Jon Lee]

rdar://problem/18201710