Only SkFontMgr_New_DirectWrite should call GetSystemFontCollection.

src/ports/SkFontHost_win_dw.cpp

 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkTypes.h"
 #undef GetGlyphIndices
 
@@ skipping 111 matching lines @@
     SkTypeface* FindByProcAndRef(SkTypefaceCache::FindProc proc, void* ctx) {
         SkAutoMutexAcquire ama(fTFCacheMutex);
         SkTypeface* typeface = fTFCache.findByProcAndRef(proc, ctx);
         return typeface;
     }
 
     friend class SkFontStyleSet_DirectWrite;
     SkTScopedComPtr<IDWriteFontCollection> fFontCollection;
     SkSMallocWCHAR fLocaleName;
     SkTypefaceCache fTFCache;
+
+    HRESULT getByFamilyName(const char familyName[], IDWriteFontFamily** fontFamily);

[caryclark, 2013/11/20 19:09:14]

why return HRESULT if caller doesn't check it?

[bungeman-skia, 2013/11/20 21:38:39]

Re-worked, it's now checked (at least in one place).

+    HRESULT getDefaultFont(IDWriteFont** font);

[caryclark, 2013/11/20 19:09:14]

looks like the existing pattern is private helper functions, then private data
members

[bungeman-skia, 2013/11/20 21:38:39]

Done.

 };
 
 class SkFontStyleSet_DirectWrite : public SkFontStyleSet {
 public:
     SkFontStyleSet_DirectWrite(SkFontMgr_DirectWrite* fontMgr, IDWriteFontFamily* fontFamily)
         : fFontMgr(SkRef(fontMgr))
         , fFontFamily(SkRefComPtr(fontFamily))
     { }
 
     virtual int count() SK_OVERRIDE;
@@ skipping 449 matching lines @@
     virtual void generateFontMetrics(SkPaint::FontMetrics* mX,
                                      SkPaint::FontMetrics* mY) SK_OVERRIDE;
 
 private:
     DWriteOffscreen fOffscreen;
     DWRITE_MATRIX fXform;
     SkAutoTUnref<DWriteFontTypeface> fTypeface;
     int fGlyphCount;
 };
 
-#define SK_DWRITE_DEFAULT_FONT_NAMED 1
-#define SK_DWRITE_DEFAULT_FONT_MESSAGE 2
-#define SK_DWRITE_DEFAULT_FONT_THEME 3
-#define SK_DWRITE_DEFAULT_FONT_SHELLDLG 4
-#define SK_DWRITE_DEFAULT_FONT_GDI 5
-#define SK_DWRITE_DEFAULT_FONT_STRATEGY SK_DWRITE_DEFAULT_FONT_MESSAGE
-
-static HRESULT get_default_font(IDWriteFont** font) {
-    IDWriteFactory* factory;
-    HRM(get_dwrite_factory(&factory), "Could not get factory.");
-
-#if SK_DWRITE_DEFAULT_FONT_STRATEGY == SK_DWRITE_DEFAULT_FONT_NAMED
-    SkTScopedComPtr<IDWriteFontCollection> sysFonts;
-    HRM(factory->GetSystemFontCollection(&sysFonts, false),
-        "Could not get system font collection.");
-
-    UINT32 index;
-    BOOL exists;
-    //hr = sysFonts->FindFamilyName(L"Georgia", &index, &exists);
-    HRM(sysFonts->FindFamilyName(L"Microsoft Sans Serif", &index, &exists),
-        "Could not access family names.");
-
-    if (!exists) {
-        SkDEBUGF(("The hard coded font family does not exist."));
-        return E_UNEXPECTED;
-    }
-
-    SkTScopedComPtr<IDWriteFontFamily> fontFamily;
-    HRM(sysFonts->GetFontFamily(index, &fontFamily),
-        "Could not load the requested font family.");
-
-    HRM(fontFamily->GetFont(0, font), "Could not get first font from family.");
-
-#elif SK_DWRITE_DEFAULT_FONT_STRATEGY == SK_DWRITE_DEFAULT_FONT_MESSAGE

[bungeman-skia, 2013/11/20 18:45:27]

Previously, everything but this block was effectively ifdef-ed out. This change
moves this into getDefaultFont.

-    SkTScopedComPtr<IDWriteGdiInterop> gdi;
-    HRM(factory->GetGdiInterop(&gdi), "Could not get GDI interop.");
-
-    NONCLIENTMETRICSW metrics;
-    metrics.cbSize = sizeof(metrics);
-    if (0 == SystemParametersInfoW(SPI_GETNONCLIENTMETRICS,
-                                   sizeof(metrics),
-                                   &metrics,
-                                   0)) {
-        return E_UNEXPECTED;
-    }
-    HRM(gdi->CreateFontFromLOGFONT(&metrics.lfMessageFont, font),
-        "Could not create DWrite font from LOGFONT.");
-
-#elif SK_DWRITE_DEFAULT_FONT_STRATEGY == SK_DWRITE_DEFAULT_FONT_THEME
-    //Theme body font?
-
-#elif SK_DWRITE_DEFAULT_FONT_STRATEGY == SK_DWRITE_DEFAULT_FONT_SHELLDLG
-    //"MS Shell Dlg" or "MS Shell Dlg 2"?
-
-#elif SK_DWRITE_DEFAULT_FONT_STRATEGY == SK_DWRITE_DEFAULT_FONT_GDI
-    //Never works.
-    SkTScopedComPtr<IDWriteGdiInterop> gdi;
-    HRM(factory->GetGdiInterop(&gdi), "Could not get GDI interop.");
-
-    static LOGFONTW gDefaultFont = {};
-    gDefaultFont.lfFaceName
-    HRM(gdi->CreateFontFromLOGFONT(&gDefaultFont, font),
-        "Could not create DWrite font from LOGFONT.";
-#endif
-    return S_OK;
-}
-
 static bool are_same(IUnknown* a, IUnknown* b) {
     SkTScopedComPtr<IUnknown> iunkA;
     if (FAILED(a->QueryInterface(&iunkA))) {
         return false;
     }
 
     SkTScopedComPtr<IUnknown> iunkB;
     if (FAILED(b->QueryInterface(&iunkB))) {
         return false;
     }
@@ skipping 93 matching lines @@
 
     SkSMallocWCHAR dwFontFamilyNameChar(dwFontFamilyNameLength+1);
     SkSMallocWCHAR dwFontNameChar(dwFontNameLength+1);
     HRB(dwFontFamilyNames->GetString(0, dwFontFamilyNameChar.get(), dwFontFamilyNameLength+1));
     HRB(dwFontNames->GetString(0, dwFontNameChar.get(), dwFontNameLength+1));
 
     return wcscmp(dwFaceFontFamilyNameChar.get(), dwFontFamilyNameChar.get()) == 0 &&
            wcscmp(dwFaceFontNameChar.get(), dwFontNameChar.get()) == 0;
 }
 
-void SkDWriteFontFromTypeface(const SkTypeface* face, IDWriteFont** font) {

[bungeman-skia, 2013/11/20 18:45:27]

This was a caller to get_default_font. However, it is not used and has no
external declaration. It was put here when mirroring the gdi backend was a
priority, but this capability should no longer be needed with the newer
interfaces.

-    if (NULL == face) {
-        HRVM(get_default_font(font), "Could not get default font.");
-    } else {
-        *font = SkRefComPtr(static_cast<const DWriteFontTypeface*>(face)->fDWriteFont.get());
-    }
-}
-
 SkScalerContext_DW::SkScalerContext_DW(DWriteFontTypeface* typeface,
                                                  const SkDescriptor* desc)
         : SkScalerContext(typeface, desc)
         , fTypeface(SkRef(typeface))
         , fGlyphCount(-1) {
     SkAutoMutexAcquire ac(gFTMutex);
 
     fXform.m11 = SkScalarToFloat(fRec.fPost2x2[0][0]);
     fXform.m12 = SkScalarToFloat(fRec.fPost2x2[1][0]);
     fXform.m21 = SkScalarToFloat(fRec.fPost2x2[0][1]);
@@ skipping 625 matching lines @@
                                              &fontFileStream),
          "Could not create font file stream.");
 
     return SkNEW_ARGS(SkDWriteFontFileStream, (fontFileStream.get()));
 }
 
 SkScalerContext* DWriteFontTypeface::onCreateScalerContext(const SkDescriptor* desc) const {
     return SkNEW_ARGS(SkScalerContext_DW, (const_cast<DWriteFontTypeface*>(this), desc));
 }
 
-static HRESULT get_by_family_name(const char familyName[], IDWriteFontFamily** fontFamily) {

[bungeman-skia, 2013/11/20 18:45:27]

This is moved into getByFamilyName and changed to use fFontCollection instead of
the system font collection.

-    IDWriteFactory* factory;
-    HR(get_dwrite_factory(&factory));
-
-    SkTScopedComPtr<IDWriteFontCollection> sysFontCollection;
-    HR(factory->GetSystemFontCollection(&sysFontCollection, FALSE));
-
-    SkSMallocWCHAR wideFamilyName;
-    HR(cstring_to_wchar(familyName, &wideFamilyName));
-
-    UINT32 index;
-    BOOL exists;
-    HR(sysFontCollection->FindFamilyName(wideFamilyName.get(), &index, &exists));
-
-    if (exists) {
-        HR(sysFontCollection->GetFontFamily(index, fontFamily));
-        return S_OK;
-    }
-    return S_FALSE;
-}
-
 void DWriteFontTypeface::onFilterRec(SkScalerContext::Rec* rec) const {
     if (rec->fFlags & SkScalerContext::kLCD_BGROrder_Flag ||
         rec->fFlags & SkScalerContext::kLCD_Vertical_Flag)
     {
         rec->fMaskFormat = SkMask::kA8_Format;
     }
 
     unsigned flagsWeDontSupport = SkScalerContext::kDevKernText_Flag |
                                   SkScalerContext::kAutohinting_Flag |
                                   SkScalerContext::kEmbeddedBitmapText_Flag |
@@ skipping 358 matching lines @@
 SkTypeface* SkFontMgr_DirectWrite::onCreateFromData(SkData* data, int ttcIndex) {
     SkAutoTUnref<SkStream> stream(SkNEW_ARGS(SkMemoryStream, (data)));
     return this->createFromStream(stream, ttcIndex);
 }
 
 SkTypeface* SkFontMgr_DirectWrite::onCreateFromFile(const char path[], int ttcIndex) {
     SkAutoTUnref<SkStream> stream(SkStream::NewFromFile(path));
     return this->createFromStream(stream, ttcIndex);
 }
 
+HRESULT SkFontMgr_DirectWrite::getByFamilyName(const char familyName[],
+                                               IDWriteFontFamily** fontFamily) {
+    SkSMallocWCHAR wideFamilyName;
+    HR(cstring_to_wchar(familyName, &wideFamilyName));
+
+    UINT32 index;
+    BOOL exists;
+    HR(fFontCollection->FindFamilyName(wideFamilyName.get(), &index, &exists));
+
+    if (exists) {
+        HR(fFontCollection->GetFontFamily(index, fontFamily));
+        return S_OK;
+    }
+    return S_FALSE;
+}
+
+HRESULT SkFontMgr_DirectWrite::getDefaultFont(IDWriteFont** font) {
+    IDWriteFactory* factory;
+    HRM(get_dwrite_factory(&factory), "Could not get factory.");
+
+    SkTScopedComPtr<IDWriteGdiInterop> gdi;
+    HRM(factory->GetGdiInterop(&gdi), "Could not get GDI interop.");
+
+    NONCLIENTMETRICSW metrics;
+    metrics.cbSize = sizeof(metrics);
+    if (0 == SystemParametersInfoW(SPI_GETNONCLIENTMETRICS,
+                                   sizeof(metrics),
+                                   &metrics,
+                                   0)) {
+        return E_UNEXPECTED;
+    }
+    HRM(gdi->CreateFontFromLOGFONT(&metrics.lfMessageFont, font),
+        "Could not create DWrite font from LOGFONT.");
+
+    return S_OK;
+}

[caryclark, 2013/11/20 19:09:14]

getDefaultFont could be a local static function, right? Or did I miss where it
depends on SkFontMgr_DirectWrite?

[bungeman-skia, 2013/11/20 21:38:39]

That was going to be my next change, to get rid of the gdi interop for the
default font, since under the hood it interacts with the system font collection.
Since it simplifies the code a bit and addresses this comment, I've done that.

+
 SkTypeface* SkFontMgr_DirectWrite::onLegacyCreateTypeface(const char familyName[],
                                                           unsigned styleBits) {
     SkTScopedComPtr<IDWriteFontFamily> fontFamily;
     if (familyName) {
-        get_by_family_name(familyName, &fontFamily);
+        this->getByFamilyName(familyName, &fontFamily);
     }
 
     if (NULL == fontFamily.get()) {
         //No good family found, go with default.
         SkTScopedComPtr<IDWriteFont> defaultFont;
-        HRNM(get_default_font(&defaultFont), "Could not get default font.");
+        HRNM(this->getDefaultFont(&defaultFont), "Could not get default font.");
         HRNM(defaultFont->GetFontFamily(&fontFamily), "Could not get default font family.");
     }
 
     SkTScopedComPtr<IDWriteFont> font;
     DWRITE_FONT_WEIGHT weight = (styleBits & SkTypeface::kBold)
                               ? DWRITE_FONT_WEIGHT_BOLD
                               : DWRITE_FONT_WEIGHT_NORMAL;
     DWRITE_FONT_STRETCH stretch = DWRITE_FONT_STRETCH_UNDEFINED;
     DWRITE_FONT_STYLE italic = (styleBits & SkTypeface::kItalic)
                              ? DWRITE_FONT_STYLE_ITALIC
@@ skipping 115 matching lines @@
         SK_TRACEHR(hr, "Could not get GetUserDefaultLocaleName.");
     } else {
         localeNameLen = getUserDefaultLocaleNameProc(localeNameStorage, LOCALE_NAME_MAX_LENGTH);
         if (localeNameLen) {
             localeName = localeNameStorage;
         };
     }
 
     return SkNEW_ARGS(SkFontMgr_DirectWrite, (sysFontCollection.get(), localeName, localeNameLen));
 }