Initialize font fallback when creating DWrite font manager

Initialize the font fallback when the direct write font
manager is created.

This ensures that the fallback is initialized prior to
sandbox startup in Chromium.

This enables the use of font fallback when combined with 
Chromium change https://codereview.chromium.org/1740593002/

Committed: https://skia.googlesource.com/skia/+/042f859c19f71ca9feacddd1cb058ff59eed8963

[Ilya Kulshin, 2016-02-25 03:19:55]

Description was changed from

==========
Add an option to override dwrite font fallback.

BUG=skia:
==========

to

==========
Add an option to override dwrite font fallback.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Ilya Kulshin, 2016-02-29 22:40:09]

Description was changed from

==========
Add an option to override dwrite font fallback.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add an option for callers to specify their own DirectWrite
FontFallback when creating the font manager.

This is expected to be used by Chromium change
https://codereview.chromium.org/1740593002/

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Ilya Kulshin, 2016-02-29 22:46:00]

kulshin@chromium.org changed reviewers:
+ bsalomon@chromium.org, reed@chromium.org

[Ilya Kulshin, 2016-02-29 22:46:01]

ptal

[reed1, 2016-03-01 14:05:42]

reed@google.com changed reviewers:
+ bungeman@google.com, reed@google.com

[reed1, 2016-03-01 14:05:43]

over to ben...

https://codereview.chromium.org/1740533003/diff/1/include/ports/SkTypeface_win.h
File include/ports/SkTypeface_win.h (right):

https://codereview.chromium.org/1740533003/diff/1/include/ports/SkTypeface_wi...
include/ports/SkTypeface_win.h:47: SK_API SkFontMgr*
SkFontMgr_New_DirectWrite(IDWriteFactory* factory = NULL,
Nit: this function is getting too many default args. Lets create a version that
takes all its parameters explicitly.

[bungeman-skia, 2016-03-01 16:06:58]

Description was changed from

==========
Add an option for callers to specify their own DirectWrite
FontFallback when creating the font manager.

This is expected to be used by Chromium change
https://codereview.chromium.org/1740593002/

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add an option for callers to specify their own DirectWrite
FontFallback when creating the font manager.

This is expected to be used by Chromium change
https://codereview.chromium.org/1740593002/

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bungeman-skia, 2016-03-01 16:09:24]

https://codereview.chromium.org/1740533003/diff/1/src/ports/SkFontMgr_win_dw.cpp
File src/ports/SkFontMgr_win_dw.cpp (right):

https://codereview.chromium.org/1740533003/diff/1/src/ports/SkFontMgr_win_dw....
src/ports/SkFontMgr_win_dw.cpp:769: if (fFactory2.get() || fFontFallback.get())
{
As this code stands, this test actually needs to be

if (fFactory2.get()) {

we're still using fFactory2 below to create the number substitution, so it must
be non-null. Whether or not fFontFallback is nullptr at this point does not make
a difference.

https://codereview.chromium.org/1740533003/diff/1/src/ports/SkFontMgr_win_dw....
src/ports/SkFontMgr_win_dw.cpp:775: }
This code (resolving an IDWriteFontFallback) should be done either in the in the
SkFontMgr_DirectWrite constructor or the SkFontMgr_New_DirectWrite factory.

Note that this means the above test (line 769, just after #if SK_HAS_DWRITE_2_H)
would become

if (fFactory2.get() && fFontFallback.get()) {

[Ilya Kulshin, 2016-03-07 22:16:40]

Patchset #2 (id:20001) has been deleted

[Ilya Kulshin, 2016-03-07 23:42:04]

ptal. I refactored the change so that Skia will now initialize the font fallback
when the direct write font manager is created, rather than having the caller
pass it in.

This ensures that the fallback is initialized prior to sandbox startup in
Chromium.

https://codereview.chromium.org/1740533003/diff/1/include/ports/SkTypeface_win.h
File include/ports/SkTypeface_win.h (right):

https://codereview.chromium.org/1740533003/diff/1/include/ports/SkTypeface_wi...
include/ports/SkTypeface_win.h:47: SK_API SkFontMgr*
SkFontMgr_New_DirectWrite(IDWriteFactory* factory = NULL,
On 2016/03/01 14:05:42, reed1 wrote:
> Nit: this function is getting too many default args. Lets create a version
that
> takes all its parameters explicitly.

Reworked this patch so that the extra arg is no longer needed (that wasn't the
reason for reworking the patch).

https://codereview.chromium.org/1740533003/diff/1/src/ports/SkFontMgr_win_dw.cpp
File src/ports/SkFontMgr_win_dw.cpp (right):

https://codereview.chromium.org/1740533003/diff/1/src/ports/SkFontMgr_win_dw....
src/ports/SkFontMgr_win_dw.cpp:769: if (fFactory2.get() || fFontFallback.get())
{
On 2016/03/01 16:09:24, bungeman1 wrote:
> As this code stands, this test actually needs to be
> 
> if (fFactory2.get()) {
> 
> we're still using fFactory2 below to create the number substitution, so it
must
> be non-null. Whether or not fFontFallback is nullptr at this point does not
make
> a difference.

Acknowledged.

https://codereview.chromium.org/1740533003/diff/1/src/ports/SkFontMgr_win_dw....
src/ports/SkFontMgr_win_dw.cpp:775: }
On 2016/03/01 16:09:24, bungeman1 wrote:
> This code (resolving an IDWriteFontFallback) should be done either in the in
the
> SkFontMgr_DirectWrite constructor or the SkFontMgr_New_DirectWrite factory.
> 
> Note that this means the above test (line 769, just after #if
SK_HAS_DWRITE_2_H)
> would become
> 
> if (fFactory2.get() && fFontFallback.get()) {

Moved it to SkfontMgr_New_DirectWrite, since that function already has other
code that does similar things.

[Ilya Kulshin, 2016-03-07 23:43:17]

Description was changed from

==========
Add an option for callers to specify their own DirectWrite
FontFallback when creating the font manager.

This is expected to be used by Chromium change
https://codereview.chromium.org/1740593002/

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Initialize the font fallback when the direct write font
manager is created, rather than having the caller pass it
in.

This ensures that the fallback is initialized prior to
sandbox startup in Chromium.

This enables the use of font fallback when combined with 
Chromium change https://codereview.chromium.org/1740593002/

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[Ilya Kulshin, 2016-03-07 23:43:37]

Description was changed from

==========
Initialize the font fallback when the direct write font
manager is created, rather than having the caller pass it
in.

This ensures that the fallback is initialized prior to
sandbox startup in Chromium.

This enables the use of font fallback when combined with 
Chromium change https://codereview.chromium.org/1740593002/

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Initialize the font fallback when the direct write font
manager is created.

This ensures that the fallback is initialized prior to
sandbox startup in Chromium.

This enables the use of font fallback when combined with 
Chromium change https://codereview.chromium.org/1740593002/

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[bungeman-skia, 2016-03-08 01:54:26]

The CQ bit was checked by bungeman@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 01:54:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1740533003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1740533003/60001

[commit-bot: I haz the power, 2016-03-08 02:05:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-08 02:05:34]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-03-08 19:02:06]

Description was changed from

==========
Initialize the font fallback when the direct write font
manager is created.

This ensures that the fallback is initialized prior to
sandbox startup in Chromium.

This enables the use of font fallback when combined with 
Chromium change https://codereview.chromium.org/1740593002/

GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Initialize the font fallback when the direct write font
manager is created.

This ensures that the fallback is initialized prior to
sandbox startup in Chromium.

This enables the use of font fallback when combined with 
Chromium change https://codereview.chromium.org/1740593002/
==========

[bungeman-skia, 2016-03-08 19:02:45]

lgtm

https://codereview.chromium.org/1740533003/diff/60001/src/ports/SkFontMgr_win...
File src/ports/SkFontMgr_win_dw.cpp (right):

https://codereview.chromium.org/1740533003/diff/60001/src/ports/SkFontMgr_win...
src/ports/SkFontMgr_win_dw.cpp:1099: #endif
This is now somewhat squirrelly, in that we're allowing the user to specify a
custom collection but not a fallback. As a result, if the collection isn't
really the system collection, the fallback won't actually be the 'right' one.
(This seems to be more of an issue with the DirectWrite API, a collection or
fontset should be able to provide its own fallback.)

That being said, I'm ok with this for now, since that's what we were doing
before anyway, but I'll need to make it possible to send in a fallback as well
at some point.

[Ilya Kulshin, 2016-03-09 01:28:59]

The CQ bit was checked by kulshin@chromium.org

[commit-bot: I haz the power, 2016-03-09 01:29:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1740533003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1740533003/60001

[commit-bot: I haz the power, 2016-03-09 01:30:10]

Description was changed from

==========
Initialize the font fallback when the direct write font
manager is created.

This ensures that the fallback is initialized prior to
sandbox startup in Chromium.

This enables the use of font fallback when combined with 
Chromium change https://codereview.chromium.org/1740593002/
==========

to

==========
Initialize the font fallback when the direct write font
manager is created.

This ensures that the fallback is initialized prior to
sandbox startup in Chromium.

This enables the use of font fallback when combined with 
Chromium change https://codereview.chromium.org/1740593002/

Committed:
https://skia.googlesource.com/skia/+/042f859c19f71ca9feacddd1cb058ff59eed8963
==========

[commit-bot: I haz the power, 2016-03-09 01:30:12]

Committed patchset #3 (id:60001) as
https://skia.googlesource.com/skia/+/042f859c19f71ca9feacddd1cb058ff59eed8963