From d5eebba99ee2a20b9f812094614b401440b2132f Mon Sep 17 00:00:00 2001
From: Ben Wagner <bungeman@google.com>
Date: Thu, 16 Jan 2020 13:59:28 -0500
Subject: [PATCH] Fix 'trak' and system variable fonts on macOS.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The 'trak' suppression was only applied if the 'opsz' was specified.
Change the 'trak' suppression to always apply.

Incidentally, this fixes issues with system variable fonts only working
at named instances on macOS 10.12 through 10.14. Versions 10.11 and
earlier still have issues with system variable fonts only working at
named instances. In general on macOS different code is run depending on
whether the attributes parameter to CTFontCreateWithGraphicsFont is
nullptr or not. The code which runs when the parameter is nullptr has
the issue until 10.15, but the code run when the parameter is
non-nullptr does not appear to have the issue in 10.12 through 10.14.

Bug: skia:9747,skia:9544
Change-Id: Icbd3a909f544437fb9811fff9bf6a630f1e2b6dc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/264694
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Commit-Queue: Ben Wagner <bungeman@google.com>
---
 src/ports/SkFontHost_mac.cpp | 40 +++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/src/ports/SkFontHost_mac.cpp b/src/ports/SkFontHost_mac.cpp
index 6893bfeec2..e1af1a30f6 100644
--- a/src/ports/SkFontHost_mac.cpp
+++ b/src/ports/SkFontHost_mac.cpp
@@ -1005,24 +1005,21 @@ private:
 // 4. the opsz variation in kCTFontVariationAttribute in CTFontDescriptor (crashes 10.10)
 // 5. the size requested (can fudge in SkTypeface but not SkScalerContext)
 // The first one which is found will be used to set the opsz variation (after clamping).
-static SkUniqueCFRef<CTFontDescriptorRef> create_opsz_descriptor(double opsz) {
-    SkUniqueCFRef<CFMutableDictionaryRef> attr(
-            CFDictionaryCreateMutable(kCFAllocatorDefault, 1,
-                                      &kCFTypeDictionaryKeyCallBacks,
-                                      &kCFTypeDictionaryValueCallBacks));
+static void add_opsz_attr(CFMutableDictionaryRef attr, double opsz) {
     SkUniqueCFRef<CFNumberRef> opszValueNumber(
         CFNumberCreate(kCFAllocatorDefault, kCFNumberDoubleType, &opsz));
     // Avoid using kCTFontOpticalSizeAttribute directly
     CFStringRef SkCTFontOpticalSizeAttribute = CFSTR("NSCTFontOpticalSizeAttribute");
-    CFDictionarySetValue(attr.get(), SkCTFontOpticalSizeAttribute, opszValueNumber.get());
+    CFDictionarySetValue(attr, SkCTFontOpticalSizeAttribute, opszValueNumber.get());
+}
 
+// This turns off application of the 'trak' table to advances, but also all other tracking.
+static void add_notrak_attr(CFMutableDictionaryRef attr) {
     int zero = 0;
     SkUniqueCFRef<CFNumberRef> unscaledTrackingNumber(
         CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &zero));
     CFStringRef SkCTFontUnscaledTrackingAttribute = CFSTR("NSCTFontUnscaledTrackingAttribute");
-    CFDictionarySetValue(attr.get(),SkCTFontUnscaledTrackingAttribute,unscaledTrackingNumber.get());
-
-    return SkUniqueCFRef<CTFontDescriptorRef>(CTFontDescriptorCreateWithAttributes(attr.get()));
+    CFDictionarySetValue(attr, SkCTFontUnscaledTrackingAttribute, unscaledTrackingNumber.get());
 }
 
 // CTFontCreateCopyWithAttributes or CTFontCreateCopyWithSymbolicTraits cannot be used on 10.10
@@ -1035,19 +1032,30 @@ static SkUniqueCFRef<CTFontRef> ctfont_create_exact_copy(CTFontRef baseFont, CGF
 {
     SkUniqueCFRef<CGFontRef> baseCGFont(CTFontCopyGraphicsFont(baseFont, nullptr));
 
-    // The last parameter (CTFontDescriptorRef attributes) *must* not set variations.
-    // If it sets variations then with fonts with variation axes the copy will fail in
-    // CGFontVariationFromDictCallback when it assumes kCGFontVariationAxisName is CFNumberRef
-    // which it quite obviously is not.
-
     // Because we cannot setup the CTFont descriptor to match, the same restriction applies here
     // as other uses of CTFontCreateWithGraphicsFont which is that such CTFonts should not escape
     // the scaler context, since they aren't 'normal'.
 
-    SkUniqueCFRef<CTFontDescriptorRef> desc;
+    SkUniqueCFRef<CFMutableDictionaryRef> attr(
+    CFDictionaryCreateMutable(kCFAllocatorDefault, 1,
+                              &kCFTypeDictionaryKeyCallBacks,
+                              &kCFTypeDictionaryValueCallBacks));
+
     if (opsz.isSet) {
-        desc = create_opsz_descriptor(opsz.value);
+        add_opsz_attr(attr.get(), opsz.value);
     }
+    add_notrak_attr(attr.get());
+
+    SkUniqueCFRef<CTFontDescriptorRef> desc(CTFontDescriptorCreateWithAttributes(attr.get()));
+
+    // The attributes parameter to CTFontCreateWithGraphicsFont *must* not set variations.
+    // If it sets variations then with fonts with variation axes the copy will fail in
+    // CGFontVariationFromDictCallback when it assumes kCGFontVariationAxisName is CFNumberRef
+    // which it quite obviously is not (in 10.11, fixed by 10.14).
+
+    // However, the attributes parameter to CTFontCreateWithGraphicsFont *must* not be nullptr.
+    // If it is then variable system fonts will only work on named instances on 10.14 and earlier.
+
     return SkUniqueCFRef<CTFontRef>(
             CTFontCreateWithGraphicsFont(baseCGFont.get(), textSize, nullptr, desc.get()));
 }