Switch non-gdi windows font path to use new SkFontMgr

Switch non-gdi windows font path to use new SkFontMgr

Switch the Skia font rendering codepath (which is not on by default)
to use the new SkFontMgr class for typeface construction.

BUG=249099
R=reed@google.com

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157244

[eae, 2013-08-31 01:52:25]

Would either of you mind reviewing this?

[bungeman-chromium, 2013-08-31 02:57:24]

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/FontCache.h (right):

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/FontCache.h:144: SkFontMgr* m_fontManager;
SkFontMgr is SkRefCnt which is compatible with RefPtr/PassRefPtr, so you may
wish to use RefPtr<SkFontMgr> here. I think this also means that yout don't need
to declare/implement the destructor as the unref/deref is implicit. (Was the
destructor really declared but never defined?)

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/skia/FontCacheSkia.cpp:113: void
FontCache::getFontNameIncludingFallback(const FontDescription& fontDescription,
const AtomicString& family, CString& name)
I really liked the createTypeface abstraction, it seems like a much cleaner
separation of concerns (and less duplicated code). It seems the only reason for
this change of name/behavior is that the FontConfig port isn't implementing the
FontMgr interface well. If that's all that's motivating this change, let's fix
it, I'd hate to see this get too messy to support just because we haven't
implemented something which really needs to be implemented anyway.

[reed1, 2013-09-03 17:38:46]

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/FontCache.h (right):

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/FontCache.h:144: SkFontMgr* m_fontManager;
Do we want to have 2 fontmgrs now (one for GDI, one for DW), or do we think
we'll just have one?

[eae, 2013-09-03 20:57:51]

On 2013/09/03 17:38:46, reed1 wrote:
>
https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
> File Source/core/platform/graphics/FontCache.h (right):
> 
>
https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
> Source/core/platform/graphics/FontCache.h:144: SkFontMgr* m_fontManager;
> Do we want to have 2 fontmgrs now (one for GDI, one for DW), or do we think
> we'll just have one?

Ideally just one.

[eae, 2013-09-03 21:04:14]

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/FontCache.h (right):

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/FontCache.h:144: SkFontMgr* m_fontManager;
On 2013/08/31 02:57:24, bungeman2 wrote:
> SkFontMgr is SkRefCnt which is compatible with RefPtr/PassRefPtr, so you may
> wish to use RefPtr<SkFontMgr> here. I think this also means that yout don't
need
> to declare/implement the destructor as the unref/deref is implicit. (Was the
> destructor really declared but never defined?)

Great, I'll do that.

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
File Source/core/platform/graphics/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/23480016/diff/1/Source/core/platform/graphics...
Source/core/platform/graphics/skia/FontCacheSkia.cpp:113: void
FontCache::getFontNameIncludingFallback(const FontDescription& fontDescription,
const AtomicString& family, CString& name)
On 2013/08/31 02:57:24, bungeman2 wrote:
> I really liked the createTypeface abstraction, it seems like a much cleaner
> separation of concerns (and less duplicated code). It seems the only reason
for
> this change of name/behavior is that the FontConfig port isn't implementing
the
> FontMgr interface well. If that's all that's motivating this change, let's fix
> it, I'd hate to see this get too messy to support just because we haven't
> implemented something which really needs to be implemented anyway.

I agree, I'll add the windows-specific code in here under an ifdef until we have
FontMgr for other platforms. How does that sounds?

[eae, 2013-09-04 16:44:43]

PTAL

[reed1, 2013-09-04 17:19:25]

lgtm

[commit-bot: I haz the power, 2013-09-04 19:44:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eae@chromium.org/23480016/24001

[commit-bot: I haz the power, 2013-09-04 21:04:59]

Change committed as 157244