Only SkFontMgr_New_DirectWrite should call GetSystemFontCollection.

src/ports/SkFontHost_win_dw.cpp

Index: src/ports/SkFontHost_win_dw.cpp
===================================================================
--- src/ports/SkFontHost_win_dw.cpp	(revision 12313)
+++ src/ports/SkFontHost_win_dw.cpp	(working copy)
@@ -129,6 +129,9 @@
     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 {
@@ -598,73 +601,6 @@
     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))) {
@@ -778,14 +714,6 @@
            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)
@@ -1431,27 +1359,6 @@
     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)
@@ -1830,17 +1737,54 @@
     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.");
     }