Test CTFonts for equality, not just name and style.

src/ports/SkFontHost_mac.cpp

Index: src/ports/SkFontHost_mac.cpp
diff --git a/src/ports/SkFontHost_mac.cpp b/src/ports/SkFontHost_mac.cpp
index 53d5610575b1f4867a7ae13188823a0b78bb4dbf..ca6c24d50df82fa9c4632dbde8f2cfa0787a140d 100644
--- a/src/ports/SkFontHost_mac.cpp
+++ b/src/ports/SkFontHost_mac.cpp
@@ -558,10 +558,11 @@ static SkTypeface* NewFromName(const char familyName[], const SkFontStyle& theSt
     }
 
     SkTypeface* face = SkTypefaceCache::FindByProcAndRef(find_by_CTFontRef, (void*)ctFont.get());
-    if (!face) {
-        face = NewFromFontRef(ctFont.release(), nullptr, nullptr, false);
-        SkTypefaceCache::Add(face, face->fontStyle());
+    if (face) {
+        return face;
     }
+    face = NewFromFontRef(ctFont.release(), nullptr, nullptr, false);
+    SkTypefaceCache::Add(face, face->fontStyle());
     return face;
 }
 
@@ -573,7 +574,6 @@ static SkTypeface* GetDefaultFace() {
 
     if (nullptr == gDefaultFace) {
         gDefaultFace = NewFromName(FONT_DEFAULT_NAME, SkFontStyle());
-        SkTypefaceCache::Add(gDefaultFace, SkFontStyle());
     }
     return gDefaultFace;
 }
@@ -591,14 +591,15 @@ CTFontRef SkTypeface_GetCTFontRef(const SkTypeface* face) {
  */
 SkTypeface* SkCreateTypefaceFromCTFont(CTFontRef fontRef, CFTypeRef resourceRef) {
     SkTypeface* face = SkTypefaceCache::FindByProcAndRef(find_by_CTFontRef, (void*)fontRef);
-    if (!face) {
-        CFRetain(fontRef);
-        if (resourceRef) {
-            CFRetain(resourceRef);
-        }
-        face = NewFromFontRef(fontRef, resourceRef, nullptr, false);
-        SkTypefaceCache::Add(face, face->fontStyle());
+    if (face) {
+        return face;
     }
+    CFRetain(fontRef);
+    if (resourceRef) {
+        CFRetain(resourceRef);
+    }
+    face = NewFromFontRef(fontRef, resourceRef, nullptr, false);
+    SkTypefaceCache::Add(face, face->fontStyle());
     return face;
 }
 
@@ -2180,27 +2181,20 @@ static int compute_metric(const SkFontStyle& a, const SkFontStyle& b) {
 }
 
 static SkTypeface* createFromDesc(CFStringRef cfFamilyName, CTFontDescriptorRef desc) {
-    NameStyle cacheRequest;
     SkString skFamilyName;
     CFStringToSkString(cfFamilyName, &skFamilyName);
-    cacheRequest.fName = skFamilyName.c_str();
-    cacheRequest.fStyle = fontstyle_from_descriptor(desc);
-
-    SkTypeface* face = SkTypefaceCache::FindByProcAndRef(find_by_NameStyle, &cacheRequest);
-    if (face) {
-        return face;
-    }
 
     AutoCFRelease<CTFontRef> ctFont(CTFontCreateWithFontDescriptor(desc, 0, nullptr));
     if (!ctFont) {
         return nullptr;
     }
 
-    bool isFixedPitch;
-    (void)computeStyleBits(ctFont, &isFixedPitch);
+    SkTypeface* face = SkTypefaceCache::FindByProcAndRef(find_by_CTFontRef, (void*)ctFont.get());
+    if (face) {
+        return face;
+    }
 
-    face = new SkTypeface_Mac(ctFont.release(), nullptr, cacheRequest.fStyle, isFixedPitch,
-                              skFamilyName.c_str(), false);
+    face = NewFromFontRef(ctFont.release(), nullptr, skFamilyName.c_str(), false);
     SkTypefaceCache::Add(face, face->fontStyle());
     return face;
 }
@@ -2612,17 +2606,16 @@ protected:
 
         NameStyle cacheRequest = { familyName, style };
         SkTypeface* face = SkTypefaceCache::FindByProcAndRef(find_by_NameStyle, &cacheRequest);

[bungeman-skia, 2016/04/05 22:03:26]

Leaving this find_by_NameStyle here is trouble, which shows up in some gms. Some
gms like typefacestyles_kerningMac end up with some of the lines looking
different. Now that we're doing the other lookups correctly, there are a lot
more typefaces in the cache after running the font iteration gms and tests, so
it is more likely that the wrong one is going to be a cache hit. This is
normally observed as the fist line of typefacestyles_kerningMac being the
'wrong' font, it has the right name and style value, but it is not the one which
would have been selected had we done the lookup.

+        if (face) {
+            return face;
+        }
 
-        if (nullptr == face) {
-            face = NewFromName(familyName, style);
-            if (face) {
-                SkTypefaceCache::Add(face, style);
-            } else {
-                face = GetDefaultFace();
-                face->ref();
-            }
+        face = NewFromName(familyName, style);
+        if (face) {
+            return face;
         }
-        return face;
+
+        return SkSafeRef(GetDefaultFace());
     }
 };