Customize SkFontMgr::Factory to control DirectWrite vs. GDI selection

Customize SkFontMgr::Factory to control DirectWrite vs. GDI selection

Move SkFontMgr::Factory to skia/ext so that we can use the warmed-up
sandbox version when using DirectWrite that's created by content/. This
avoids the previous default behaviour of creating a GDI SkFontMgr which
is always wrong when DirectWrite is being used (and causes crashes
when --enable_win32k_renderer_lockdown is enabled).

R=jamesr@chromium.org,bungeman@google.com
BUG=385926, 356950, 385717
TEST=http://www.effectgames.com/demos/canvascycle/ http://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_canvas_filltext and enabling FPS in about:flags with both --enable_win32k_renderer_lockdown and --disable-direct-write

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278222

[scottmg, 2014-06-18 19:08:31]

https://codereview.chromium.org/344653002/diff/1/content/renderer/renderer_ma...
File content/renderer/renderer_main_platform_delegate_win.cc (right):

https://codereview.chromium.org/344653002/diff/1/content/renderer/renderer_ma...
content/renderer/renderer_main_platform_delegate_win.cc:64:
SetDefaultSkiaFactory(GetPreSandboxWarmupFontMgr());
this has to be set from here rather than having the other side pull it when
necessary to avoid skia have a link dependency -> content.

[bungeman-skia, 2014-06-18 19:21:36]

https://codereview.chromium.org/344653002/diff/20001/skia/ext/fontmgr_default...
File skia/ext/fontmgr_default_win.cc (right):

https://codereview.chromium.org/344653002/diff/20001/skia/ext/fontmgr_default...
skia/ext/fontmgr_default_win.cc:8: #include
"content/public/renderer/render_font_warmup_win.h"
Isn't part of the point of this file not to have these two 'content' includes?
They don't appear to be used.

[scottmg, 2014-06-18 19:30:55]

https://codereview.chromium.org/344653002/diff/20001/skia/ext/fontmgr_default...
File skia/ext/fontmgr_default_win.cc (right):

https://codereview.chromium.org/344653002/diff/20001/skia/ext/fontmgr_default...
skia/ext/fontmgr_default_win.cc:8: #include
"content/public/renderer/render_font_warmup_win.h"
On 2014/06/18 19:21:35, bungeman1 wrote:
> Isn't part of the point of this file not to have these two 'content' includes?
> They don't appear to be used.

Oops, yes, you're right. Thanks, done.

[jamesr, 2014-06-18 19:36:46]

lgtm

[bungeman-skia, 2014-06-18 19:49:36]

https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
File skia/ext/fontmgr_default_win.cc (right):

https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
skia/ext/fontmgr_default_win.cc:25: return g_default_fontmgr;
Note that SkFontMgr is SkRefCnt, so it might make sense to ref this. It may not
make any difference at the moment, but I think that's more correct. The idea was
that this will always create an SkFontMgr, so usually this is creating and then
returning the creation ref. In this case it looks like the global should hold a
ref, and then return a ref each time this is called.

See also skia/ext/refptr.h for Chromium's wrapper around Skia reference
counting, if you're not already familiar with that.

[scottmg, 2014-06-18 20:02:49]

https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
File skia/ext/fontmgr_default_win.cc (right):

https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
skia/ext/fontmgr_default_win.cc:25: return g_default_fontmgr;
On 2014/06/18 19:49:36, bungeman1 wrote:
> Note that SkFontMgr is SkRefCnt, so it might make sense to ref this. It may
not
> make any difference at the moment, but I think that's more correct. The idea
was
> that this will always create an SkFontMgr, so usually this is creating and
then
> returning the creation ref. In this case it looks like the global should hold
a
> ref, and then return a ref each time this is called.

Done.

Just to confirm, the interpretation now is that the raw pointers in content and
now here in g_default_fontmgr are aliases for the initial creation ref, and any
further requests will be additional refs. So, it will never be freed, but that's
sort of what we want in this case. Is that correct?


> 
> See also skia/ext/refptr.h for Chromium's wrapper around Skia reference
> counting, if you're not already familiar with that.

[bungeman-skia, 2014-06-18 20:04:41]

On 2014/06/18 20:02:49, scottmg wrote:
>
https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
> File skia/ext/fontmgr_default_win.cc (right):
> 
>
https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
> skia/ext/fontmgr_default_win.cc:25: return g_default_fontmgr;
> On 2014/06/18 19:49:36, bungeman1 wrote:
> > Note that SkFontMgr is SkRefCnt, so it might make sense to ref this. It may
> not
> > make any difference at the moment, but I think that's more correct. The idea
> was
> > that this will always create an SkFontMgr, so usually this is creating and
> then
> > returning the creation ref. In this case it looks like the global should
hold
> a
> > ref, and then return a ref each time this is called.
> 
> Done.
> 
> Just to confirm, the interpretation now is that the raw pointers in content
and
> now here in g_default_fontmgr are aliases for the initial creation ref, and
any
> further requests will be additional refs. So, it will never be freed, but
that's
> sort of what we want in this case. Is that correct?
> 
> 

sgtm

> > 
> > See also skia/ext/refptr.h for Chromium's wrapper around Skia reference
> > counting, if you're not already familiar with that.

lgtm

[jamesr, 2014-06-18 20:06:52]

On 2014/06/18 19:49:36, bungeman1 wrote:
>
https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
> File skia/ext/fontmgr_default_win.cc (right):
> 
>
https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
> skia/ext/fontmgr_default_win.cc:25: return g_default_fontmgr;
> Note that SkFontMgr is SkRefCnt, so it might make sense to ref this. It may
not
> make any difference at the moment, but I think that's more correct. The idea
was
> that this will always create an SkFontMgr, so usually this is creating and
then
> returning the creation ref. In this case it looks like the global should hold
a
> ref, and then return a ref each time this is called.

Is SkFontMgr managed by refcount?  Not everything in skia that inherits from
SkRefCnt is managed by ref counting, some objects are both SkRefCnt and stack
allocated or managed by raw new/deletes.
> 
> See also skia/ext/refptr.h for Chromium's wrapper around Skia reference
> counting, if you're not already familiar with that.

[scottmg, 2014-06-18 20:12:06]

On 2014/06/18 20:06:52, jamesr wrote:
> On 2014/06/18 19:49:36, bungeman1 wrote:
> >
>
https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
> > File skia/ext/fontmgr_default_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/344653002/diff/40001/skia/ext/fontmgr_default...
> > skia/ext/fontmgr_default_win.cc:25: return g_default_fontmgr;
> > Note that SkFontMgr is SkRefCnt, so it might make sense to ref this. It may
> not
> > make any difference at the moment, but I think that's more correct. The idea
> was
> > that this will always create an SkFontMgr, so usually this is creating and
> then
> > returning the creation ref. In this case it looks like the global should
hold
> a
> > ref, and then return a ref each time this is called.
> 
> Is SkFontMgr managed by refcount?  Not everything in skia that inherits from
> SkRefCnt is managed by ref counting, some objects are both SkRefCnt and stack
> allocated or managed by raw new/deletes.

It's ref'd here
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...
saved statically, and not otherwise managed as far as I can tell. So in
practice, it doesn't matter too much, afaict.

> > 
> > See also skia/ext/refptr.h for Chromium's wrapper around Skia reference
> > counting, if you're not already familiar with that.

[scottmg, 2014-06-18 20:12:38]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-06-18 20:13:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/scottmg@chromium.org/344653002/60001

[commit-bot: I haz the power, 2014-06-19 01:20:15]

Change committed as 278222