Bug 147722

Summary: Implement font-feature-settings
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, dan.eden, dbates, dino, esprehn+autocc, glenn, jonlee, kondapallykalyan, rniwa, sam, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar, WebExposed
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 147719, 147751, 147765, 147775    
Bug Blocks:    
Attachments:
Description Flags
No caching yet
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch simon.fraser: review+

Description Myles C. Maxfield 2015-08-05 22:05:23 PDT
Implement font-feature-settings
Comment 1 Myles C. Maxfield 2015-08-05 22:06:11 PDT
Created attachment 258349 [details]
No caching yet
Comment 2 Myles C. Maxfield 2015-08-05 22:08:02 PDT
<div style="font: 100px 'Hiragino Sans GB'; -webkit-font-feature-settings: 'trad';">&#x4f53;</div> has a different on OS X
Comment 3 Myles C. Maxfield 2015-08-06 17:14:23 PDT
Created attachment 258425 [details]
Patch
Comment 4 Myles C. Maxfield 2015-08-06 17:15:04 PDT
Still needs tests. Reftests will be difficult, so I think (for now) I'll make dump-render-tree tests.
Comment 5 WebKit Commit Bot 2015-08-06 17:15:48 PDT
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.
Comment 6 Myles C. Maxfield 2015-08-06 18:00:35 PDT
Created attachment 258427 [details]
Patch
Comment 7 WebKit Commit Bot 2015-08-06 18:02:37 PDT
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.
Comment 8 Myles C. Maxfield 2015-08-06 18:22:36 PDT
Created attachment 258428 [details]
Patch
Comment 9 WebKit Commit Bot 2015-08-06 18:24:16 PDT
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.
Comment 10 Myles C. Maxfield 2015-08-06 18:44:34 PDT
Created attachment 258431 [details]
Patch
Comment 11 WebKit Commit Bot 2015-08-06 18:46:34 PDT
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.
Comment 12 Myles C. Maxfield 2015-08-06 19:01:07 PDT
Created attachment 258432 [details]
Patch
Comment 13 WebKit Commit Bot 2015-08-06 19:03:53 PDT
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.
Comment 14 Myles C. Maxfield 2015-08-06 19:13:36 PDT
Created attachment 258433 [details]
Patch
Comment 15 WebKit Commit Bot 2015-08-06 19:15:06 PDT
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.
Comment 16 Build Bot 2015-08-06 20:10:58 PDT
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
Comment 17 Build Bot 2015-08-06 20:11:00 PDT
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
Comment 18 Myles C. Maxfield 2015-08-06 21:24:27 PDT
Created attachment 258444 [details]
Patch
Comment 19 WebKit Commit Bot 2015-08-06 21:27:50 PDT
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.
Comment 20 Myles C. Maxfield 2015-08-06 22:34:54 PDT
Created attachment 258456 [details]
Patch
Comment 21 Myles C. Maxfield 2015-08-07 02:26:06 PDT
Created attachment 258472 [details]
Patch
Comment 22 Myles C. Maxfield 2015-08-07 03:10:43 PDT
Created attachment 258477 [details]
Patch
Comment 23 Myles C. Maxfield 2015-08-07 03:11:54 PDT
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)
Comment 24 Myles C. Maxfield 2015-08-07 10:10:41 PDT
Created attachment 258507 [details]
Patch
Comment 25 Myles C. Maxfield 2015-08-07 10:54:54 PDT
Created attachment 258511 [details]
Patch
Comment 26 Myles C. Maxfield 2015-08-07 10:57:52 PDT
Created attachment 258512 [details]
Patch
Comment 27 Myles C. Maxfield 2015-08-07 12:10:17 PDT
Committed r188146: <http://trac.webkit.org/changeset/188146>
Comment 28 Daniel Bates 2015-08-07 13:17:46 PDT
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?
Comment 29 Myles C. Maxfield 2015-08-07 14:05:23 PDT
(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
Comment 30 Myles C. Maxfield 2015-08-18 23:37:33 PDT
*** Bug 134332 has been marked as a duplicate of this bug. ***
Comment 31 Jon Lee 2015-12-04 15:48:05 PST
rdar://problem/18201710