Summary: | Implement font-feature-settings | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Myles C. Maxfield
2015-08-05 22:05:23 PDT
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. *** |