Allow custom font collection for DirectWrite font manager.

src/ports/SkFontMgr_win_dw.cpp

Index: src/ports/SkFontMgr_win_dw.cpp
diff --git a/src/ports/SkFontMgr_win_dw.cpp b/src/ports/SkFontMgr_win_dw.cpp
index 585d809406a765454dbc85f3850b303c8ba05561..3ab36a282aef9e5751676bc9323b088dd2099360 100644
--- a/src/ports/SkFontMgr_win_dw.cpp
+++ b/src/ports/SkFontMgr_win_dw.cpp
@@ -1067,7 +1067,8 @@ SkTypeface* SkFontStyleSet_DirectWrite::matchStyle(const SkFontStyle& pattern) {
 ////////////////////////////////////////////////////////////////////////////////
 #include "SkTypeface_win.h"
 
-SK_API SkFontMgr* SkFontMgr_New_DirectWrite(IDWriteFactory* factory) {
+SK_API SkFontMgr* SkFontMgr_New_DirectWrite(IDWriteFactory* factory,
+                                            IDWriteFontCollection* collection) {
     if (nullptr == factory) {
         factory = sk_get_dwrite_factory();
         if (nullptr == factory) {
@@ -1075,9 +1076,14 @@ SK_API SkFontMgr* SkFontMgr_New_DirectWrite(IDWriteFactory* factory) {
         }
     }
 
-    SkTScopedComPtr<IDWriteFontCollection> sysFontCollection;
-    HRNM(factory->GetSystemFontCollection(&sysFontCollection, FALSE),
-         "Could not get system font collection.");
+    // SkTScopedComPtr will not AddRef the object when constructed, so we have
+    // to do that ourselves. SkSafeRefComPtr will AddRef only if the pointer is
+    // non-null, so it is safe to use even if |collection| might be null.

[bungeman-skia, 2016/01/29 22:57:24]

I don't think any of this commentary is really required here. The 'Safe' in
SkSafeRefComPtr already indicates that it checks for nullptr. SkTScopedComPtr
doesn't AddRef because the intent is to bind a reference to the stack frame.
Also, calling AddRef in the constructor would be strange for a COM smart
pointer, as they are normally initialized this way be a function which returns a
COM pointer which must already be ref'ed. Note that this is COM where reference
counts logically start at 1 (because the stack owns that reference) and not 0
(as Chrome likes to do).

As a more practical example, if SkTScopedComPtr did AddRef in the constructor,
then everywhere it is used like 'SkTScopedComPtr<X> x(new X)' would need to
immediately call Unref on the raw object, which is just ugly.

The real confusion here is that we do not logically need to add a reference to
'collection', we could just pass it through to the constructor below (which will
retain a reference for itself it it needs to). It's just the case where we
create our own IDWriteFontCollection where it becomes necessary for a reference
to be automatically disposed of. In other words, perhaps it is clearer to write
this as:


IDWriteFontCollection* fontCollection = collection;
SkTScopedComPtr<IDWriteFontCollection> systemFontCollection;
if (nullptr == fontCollection.get()) {
    HRNM(factory->GetSystemFontCollection(&systemFontCollection, FALSE),
         "Could not get system font collection.");
    fontCollection = systemFontCollection.get();
}

and then drop the '.get()' from 'fontCollection.get()' when used below. This
avoids the issue with needing to AddRef anything, as well as avoids unwanted
reference count churn.

[Ilya Kulshin, 2016/02/02 22:50:23]

Switched code to avoid the extra addref.

+    SkTScopedComPtr<IDWriteFontCollection> fontCollection(SkSafeRefComPtr(collection));
+    if (nullptr == fontCollection.get()) {
+      HRNM(factory->GetSystemFontCollection(&fontCollection, FALSE),

[bungeman-skia, 2016/01/29 22:57:24]

Skia uses four space indents.

[Ilya Kulshin, 2016/02/02 22:50:22]

Done.

+        "Could not get system font collection.");

[bungeman-skia, 2016/01/29 22:57:24]

The start of this line should be indented to go right under 'factory' in the
line above.

[Ilya Kulshin, 2016/02/02 22:50:23]

Done.

+    }
 
     WCHAR localeNameStorage[LOCALE_NAME_MAX_LENGTH];
     WCHAR* localeName = nullptr;
@@ -1095,7 +1101,7 @@ SK_API SkFontMgr* SkFontMgr_New_DirectWrite(IDWriteFactory* factory) {
         };
     }
 
-    return new SkFontMgr_DirectWrite(factory, sysFontCollection.get(), localeName, localeNameLen);
+    return new SkFontMgr_DirectWrite(factory, fontCollection.get(), localeName, localeNameLen);
 }
 
 #include "SkFontMgr_indirect.h"