Remove requestedStyle from SkTypefaceCache.

The typeface cache contains typefaces which can be compared against to
determine if an already known typeface will work instead of creating a
new typeface id. This is primarily for sharing scaler contexts. How that
typeface was requested is not relevant to this comparison, so don't cache
it. Request caching must be handled separately.

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1879423002

Review URL: https://codereview.chromium.org/1879423002

src/core/SkTypefaceCache.cpp

@@ -15,46 +15,29 @@
 
 SkTypefaceCache::SkTypefaceCache() {}
 
-SkTypefaceCache::~SkTypefaceCache() {
+void SkTypefaceCache::add(SkTypeface* face) {
-    const Rec* curr = fArray.begin();
+    if (fTypefaces.count() >= TYPEFACE_CACHE_LIMIT) {
-    const Rec* stop = fArray.end();
-    while (curr < stop) {
-        curr->fFace->unref();
-        curr += 1;
-    }
-}
-
-void SkTypefaceCache::add(SkTypeface* face, const SkFontStyle& requestedStyle) {
-    if (fArray.count() >= TYPEFACE_CACHE_LIMIT) {
         this->purge(TYPEFACE_CACHE_LIMIT >> 2);
     }
 
-    Rec* rec = fArray.append();
+    fTypefaces.emplace_back(SkRef(face));
-    rec->fFace = SkRef(face);
-    rec->fRequestedStyle = requestedStyle;
 }
 
 SkTypeface* SkTypefaceCache::findByProcAndRef(FindProc proc, void* ctx) const {
-    const Rec* curr = fArray.begin();
+    for (const sk_sp<SkTypeface>& typeface : fTypefaces) {
-    const Rec* stop = fArray.end();
+        if (proc(typeface.get(), ctx)) {
-    while (curr < stop) {
+            return SkRef(typeface.get());
-        SkTypeface* currFace = curr->fFace;
-        if (proc(currFace, curr->fRequestedStyle, ctx)) {
-            return SkRef(currFace);
         }
-        curr += 1;
     }
     return nullptr;
 }
 
 void SkTypefaceCache::purge(int numToPurge) {
-    int count = fArray.count();
+    int count = fTypefaces.count();
     int i = 0;
     while (i < count) {
-        SkTypeface* face = fArray[i].fFace;
+        if (fTypefaces[i]->unique()) {
-        if (face->unique()) {
+            fTypefaces.removeShuffle(i);
-            face->unref();
-            fArray.remove(i);
             --count;
             if (--numToPurge == 0) {
                 return;
@@ -66,7 +49,7 @@ void SkTypefaceCache::purge(int numToPurge) {
 }
 
 void SkTypefaceCache::purgeAll() {
-    this->purge(fArray.count());
+    this->purge(fTypefaces.count());
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -83,15 +66,14 @@ SkFontID SkTypefaceCache::NewFontID() {
 
 SK_DECLARE_STATIC_MUTEX(gMutex);
 
-void SkTypefaceCache::Add(SkTypeface* face, const SkFontStyle& requestedStyle) {
+void SkTypefaceCache::Add(SkTypeface* face) {
     SkAutoMutexAcquire ama(gMutex);
-    Get().add(face, requestedStyle);
+    Get().add(face);
 }
 
 SkTypeface* SkTypefaceCache::FindByProcAndRef(FindProc proc, void* ctx) {
     SkAutoMutexAcquire ama(gMutex);
-    SkTypeface* typeface = Get().findByProcAndRef(proc, ctx);
+    return Get().findByProcAndRef(proc, ctx);
-    return typeface;
 }
 
 void SkTypefaceCache::PurgeAll() {
@@ -102,16 +84,19 @@ void SkTypefaceCache::PurgeAll() {
 ///////////////////////////////////////////////////////////////////////////////
 
 #ifdef SK_DEBUG
-static bool DumpProc(SkTypeface* face, const SkFontStyle& s, void* ctx) {
+static bool DumpProc(SkTypeface* face, void* ctx) {
-    SkDebugf("SkTypefaceCache: face %p fontID %d weight %d width %d style %d refcnt %d\n",
+    SkString n;
-             face, face->uniqueID(), s.weight(), s.width(), s.slant(), face->getRefCnt());
+    face->getFamilyName(&n);
+    SkFontStyle s = face->fontStyle();
+    SkFontID id = face->uniqueID();
+    SkDebugf("SkTypefaceCache: face %p fontID %d weight %d width %d style %d refcnt %d name %s\n",
+             face, id, s.weight(), s.width(), s.slant(), face->getRefCnt(), n.c_str());
     return false;
 }
 #endif
 
 void SkTypefaceCache::Dump() {
 #ifdef SK_DEBUG
-    SkAutoMutexAcquire ama(gMutex);
     (void)Get().findByProcAndRef(DumpProc, nullptr);
 #endif
 }

src/core/SkTypefaceCache.h

@@ -10,27 +10,20 @@
 #ifndef SkTypefaceCache_DEFINED
 #define SkTypefaceCache_DEFINED
 
+#include "SkRefCnt.h"
 #include "SkTypeface.h"
-#include "SkTDArray.h"
+#include "SkTArray.h"
-
-/*  TODO
- *  Provide std way to cache name+requestedStyle aliases to the same typeface.
- *
- *  The current mechanism ends up create a diff typeface for each one, even if
- *  they map to the same internal obj (e.g. CTFontRef on the mac)
- */
 
 class SkTypefaceCache {
 public:
     SkTypefaceCache();
-    ~SkTypefaceCache();
 
     /**
      * Callback for FindByProc. Returns true if the given typeface is a match
      * for the given context. The passed typeface is owned by the cache and is
      * not additionally ref()ed. The typeface may be in the disposed state.
      */
-    typedef bool(*FindProc)(SkTypeface*, const SkFontStyle&, void* context);
+    typedef bool(*FindProc)(SkTypeface*, void* context);
 
     /**
      *  Add a typeface to the cache. This ref()s the typeface, so that the
@@ -38,7 +31,7 @@ public:
      *  whose refcnt is 1 (meaning only the cache is an owner) will be
      *  unref()ed.
      */
-    void add(SkTypeface*, const SkFontStyle& requested);
+    void add(SkTypeface*);
 
     /**
      *  Iterate through the cache, calling proc(typeface, ctx) with each
@@ -63,7 +56,7 @@ public:
 
     // These are static wrappers around a global instance of a cache.
 
-    static void Add(SkTypeface*, const SkFontStyle& requested);
+    static void Add(SkTypeface*);
     static SkTypeface* FindByProcAndRef(FindProc proc, void* ctx);
     static void PurgeAll();
 
@@ -77,11 +70,7 @@ private:
 
     void purge(int count);
 
-    struct Rec {
+    SkTArray<sk_sp<SkTypeface>> fTypefaces;
-        SkTypeface* fFace;
-        SkFontStyle fRequestedStyle;
-    };
-    SkTDArray<Rec> fArray;
 };
 
 #endif

src/ports/SkFontHost_fontconfig.cpp

@@ -58,7 +58,7 @@ SkFontConfigInterface* SkFontHost_fontconfig_ref_global() {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-static bool find_by_FontIdentity(SkTypeface* cachedTypeface, const SkFontStyle&, void* ctx) {
+static bool find_by_FontIdentity(SkTypeface* cachedTypeface, void* ctx) {
     typedef SkFontConfigInterface::FontIdentity FontIdentity;
     FontConfigTypeface* cachedFCTypeface = static_cast<FontConfigTypeface*>(cachedTypeface);
     FontIdentity* identity = static_cast<FontIdentity*>(ctx);
@@ -196,7 +196,7 @@ SkTypeface* FontConfigTypeface::LegacyCreateTypeface(const char requestedFamilyN
     if (!face) {
         face = FontConfigTypeface::Create(outStyle, identity, outFamilyName);
         // Add this FontIdentity to the FontIdentity cache.
-        SkTypefaceCache::Add(face, outStyle);
+        SkTypefaceCache::Add(face);
     }
     // Add this request to the request cache.
     SkFontHostRequestCache::Add(face, request.release());

src/ports/SkFontHost_mac.cpp

@@ -486,7 +486,7 @@ static SkTypeface* NewFromFontRef(CTFontRef fontRef, CFTypeRef resourceRef, bool
     return new SkTypeface_Mac(fontRef, resourceRef, style, isFixedPitch, isLocalStream);
 }
 
-static bool find_by_CTFontRef(SkTypeface* cached, const SkFontStyle&, void* context) {
+static bool find_by_CTFontRef(SkTypeface* cached, void* context) {
     CTFontRef self = (CTFontRef)context;
     CTFontRef other = ((SkTypeface_Mac*)cached)->fFontRef;
 
@@ -546,7 +546,7 @@ static SkTypeface* NewFromName(const char familyName[], const SkFontStyle& theSt
         return face;
     }
     face = NewFromFontRef(ctFont.release(), nullptr, false);
-    SkTypefaceCache::Add(face, face->fontStyle());
+    SkTypefaceCache::Add(face);
     return face;
 }
 
@@ -583,7 +583,7 @@ SkTypeface* SkCreateTypefaceFromCTFont(CTFontRef fontRef, CFTypeRef resourceRef)
         CFRetain(resourceRef);
     }
     face = NewFromFontRef(fontRef, resourceRef, false);
-    SkTypefaceCache::Add(face, face->fontStyle());
+    SkTypefaceCache::Add(face);
     return face;
 }
 
@@ -2166,7 +2166,7 @@ static SkTypeface* createFromDesc(CTFontDescriptorRef desc) {
     }
 
     face = NewFromFontRef(ctFont.release(), nullptr, false);
-    SkTypefaceCache::Add(face, face->fontStyle());
+    SkTypefaceCache::Add(face);
     return face;
 }
 

src/ports/SkFontHost_win.cpp

@@ -308,13 +308,11 @@ static const LOGFONT& get_default_font() {
     return gDefaultFont;
 }
 
-static bool FindByLogFont(SkTypeface* face, const SkFontStyle& requestedStyle, void* ctx) {
+static bool FindByLogFont(SkTypeface* face, void* ctx) {
     LogFontTypeface* lface = static_cast<LogFontTypeface*>(face);
     const LOGFONT* lf = reinterpret_cast<const LOGFONT*>(ctx);
 
-    return lface &&
+    return !memcmp(&lface->fLogFont, lf, sizeof(LOGFONT));
-           get_style(lface->fLogFont) == requestedStyle &&
-           !memcmp(&lface->fLogFont, lf, sizeof(LOGFONT));
 }
 
 /**
@@ -327,7 +325,7 @@ SkTypeface* SkCreateTypefaceFromLOGFONT(const LOGFONT& origLF) {
     SkTypeface* face = SkTypefaceCache::FindByProcAndRef(FindByLogFont, &lf);
     if (nullptr == face) {
         face = LogFontTypeface::Create(lf);
-        SkTypefaceCache::Add(face, get_style(lf));
+        SkTypefaceCache::Add(face);
     }
     return face;
 }

src/ports/SkFontMgr_fontconfig.cpp

@@ -605,7 +605,7 @@ class SkFontMgr_fontconfig : public SkFontMgr {
                                           sizes.begin(), names.count());
     }
 
-    static bool FindByFcPattern(SkTypeface* cached, const SkFontStyle&, void* ctx) {
+    static bool FindByFcPattern(SkTypeface* cached, void* ctx) {
         SkTypeface_fontconfig* cshFace = static_cast<SkTypeface_fontconfig*>(cached);
         FcPattern* ctxPattern = static_cast<FcPattern*>(ctx);
         return FcTrue == FcPatternEqual(cshFace->fPattern, ctxPattern);
@@ -624,7 +624,7 @@ class SkFontMgr_fontconfig : public SkFontMgr {
             FcPatternReference(pattern);
             face = SkTypeface_fontconfig::Create(pattern);
             if (face) {
-                fTFCache.add(face, SkFontStyle());
+                fTFCache.add(face);
             }
         }
         return face;

src/ports/SkFontMgr_win_dw.cpp

@@ -353,7 +353,7 @@ struct ProtoDWriteTypeface {
     IDWriteFontFamily* fDWriteFontFamily;
 };
 
-static bool FindByDWriteFont(SkTypeface* cached, const SkFontStyle&, void* ctx) {
+static bool FindByDWriteFont(SkTypeface* cached, void* ctx) {
     DWriteFontTypeface* cshFace = reinterpret_cast<DWriteFontTypeface*>(cached);
     ProtoDWriteTypeface* ctxFace = reinterpret_cast<ProtoDWriteTypeface*>(ctx);
     bool same;
@@ -457,7 +457,7 @@ SkTypeface* SkFontMgr_DirectWrite::createTypefaceFromDWriteFont(
     if (nullptr == face) {
         face = DWriteFontTypeface::Create(fFactory.get(), fontFace, font, fontFamily);
         if (face) {
-            fTFCache.add(face, get_style(font));
+            fTFCache.add(face);
         }
     }
     return face;

tests/TypefaceTest.cpp

@@ -5,7 +5,9 @@
  * found in the LICENSE file.
  */
 
+#include "SkRefCnt.h"
 #include "SkTypeface.h"
+#include "SkTypefaceCache.h"
 #include "Test.h"
 
 DEF_TEST(Typeface, reporter) {
@@ -24,3 +26,68 @@ DEF_TEST(Typeface, reporter) {
     REPORTER_ASSERT(reporter, nullptr == t3.get());
 #endif
 }
+
+class SkEmptyTypeface : public SkTypeface {
+public:
+    static sk_sp<SkTypeface> Create(SkFontID id) { return sk_sp<SkTypeface>(new SkEmptyTypeface(id)); }
+protected:
+    SkEmptyTypeface(SkFontID id) : SkTypeface(SkFontStyle(), id, true) { }
+
+    SkStreamAsset* onOpenStream(int* ttcIndex) const override { return nullptr; }
+    SkScalerContext* onCreateScalerContext(const SkScalerContextEffects&,
+                                           const SkDescriptor*) const override {
+        return nullptr;
+    }
+    void onFilterRec(SkScalerContextRec*) const override { }
+    virtual SkAdvancedTypefaceMetrics* onGetAdvancedTypefaceMetrics(
+                                PerGlyphInfo,
+                                const uint32_t*, uint32_t) const override { return nullptr; }
+    void onGetFontDescriptor(SkFontDescriptor*, bool*) const override { }
+    virtual int onCharsToGlyphs(const void* chars, Encoding encoding,
+                                uint16_t glyphs[], int glyphCount) const override {
+        SK_ABORT("unimplemented");
+        return 0;
+    }
+    int onCountGlyphs() const override { return 0; };
+    int onGetUPEM() const override { return 0; };
+    void onGetFamilyName(SkString* familyName) const override { familyName->reset(); }
+    SkTypeface::LocalizedStrings* onCreateFamilyNameIterator() const override {
+        SK_ABORT("unimplemented");
+        return nullptr;
+    };
+    int onGetTableTags(SkFontTableTag tags[]) const override { return 0; }
+    size_t onGetTableData(SkFontTableTag, size_t, size_t, void*) const override { return 0; }
+};
+
+static bool count_proc(SkTypeface* face, void* ctx) {
+    int* count = static_cast<int*>(ctx);
+    *count = *count + 1;
+    return false;
+}
+static int count(skiatest::Reporter* reporter, const SkTypefaceCache& cache) {
+    int count = 0;
+    SkTypeface* none = cache.findByProcAndRef(count_proc, &count);
+    REPORTER_ASSERT(reporter, none == nullptr);
+    return count;
+}
+
+DEF_TEST(TypefaceCache, reporter) {
+    sk_sp<SkTypeface> t1(SkEmptyTypeface::Create(1));
+    {
+        SkTypefaceCache cache;
+        REPORTER_ASSERT(reporter, count(reporter, cache) == 0);
+        {
+            sk_sp<SkTypeface> t0(SkEmptyTypeface::Create(0));
+            cache.add(t0.get());
+            REPORTER_ASSERT(reporter, count(reporter, cache) == 1);
+            cache.add(t1.get());
+            REPORTER_ASSERT(reporter, count(reporter, cache) == 2);
+            cache.purgeAll();
+            REPORTER_ASSERT(reporter, count(reporter, cache) == 2);
+        }
+        REPORTER_ASSERT(reporter, count(reporter, cache) == 2);
+        cache.purgeAll();
+        REPORTER_ASSERT(reporter, count(reporter, cache) == 1);
+    }
+    REPORTER_ASSERT(reporter, t1->unique());
+}