Implement font-feature-settings
Created attachment 258349 [details] No caching yet
<div style="font: 100px 'Hiragino Sans GB'; -webkit-font-feature-settings: 'trad';">体</div> has a different on OS X
Created attachment 258425 [details] Patch
Still needs tests. Reftests will be difficult, so I think (for now) I'll make dump-render-tree tests.
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.
Created attachment 258427 [details] Patch
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.
Created attachment 258428 [details] Patch
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.
Created attachment 258431 [details] Patch
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.
Created attachment 258432 [details] Patch
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.
Created attachment 258433 [details] Patch
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 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
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
Created attachment 258444 [details] Patch
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.
Created attachment 258456 [details] Patch
Created attachment 258472 [details] Patch
Created attachment 258477 [details] Patch
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)
Created attachment 258507 [details] Patch
Created attachment 258511 [details] Patch
Created attachment 258512 [details] Patch
Committed r188146: <http://trac.webkit.org/changeset/188146>
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?
(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
*** Bug 134332 has been marked as a duplicate of this bug. ***
rdar://problem/18201710