SkTypeface::MakeFromName to take SkFontStyle.

SkTypeface::MakeFromName to take SkFontStyle.

SkTypeface::MakeFromName currently takes SkTypeface::Style,
which is quite limited. This starts the transition to this
function taking SkFontStyle instead.

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

TBR=reed
He said it sounded like a good idea.

Committed: https://skia.googlesource.com/skia/+/ee6a9919a362e16c1d84a870ce867d1ad7b8a141

[Mikus, 2016-03-21 14:57:49]

Description was changed from

==========
Add the method 'CreateFromNameAndStyle' for creating SkTypefaces using a font
style.

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

to

==========
Add the method 'CreateFromNameAndStyle' for creating SkTypefaces using a font
style.

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

[Mikus, 2016-03-21 14:59:36]

Description was changed from

==========
Add the method 'CreateFromNameAndStyle' for creating SkTypefaces using a font
style.

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

to

==========
Add the method 'CreateFromNameAndStyle' for creating SkTypefaces using a font
style.

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

[Mikus, 2016-03-21 14:59:37]

mboc@opera.com changed reviewers:
+ bungeman@google.com

[bungeman-skia, 2016-03-21 16:12:59]

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

[commit-bot: I haz the power, 2016-03-21 16:13:03]

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

[bungeman-skia, 2016-03-21 16:13:18]

Description was changed from

==========
Add the method 'CreateFromNameAndStyle' for creating SkTypefaces using a font
style.

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

to

==========
Add the method 'CreateFromNameAndStyle' for creating SkTypefaces using a font
style.

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

[bungeman-skia, 2016-03-21 16:14:25]

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

[bungeman-skia, 2016-03-21 16:14:26]

See https://codereview.chromium.org/1819753003/ for reasons.

https://codereview.chromium.org/1818043002/diff/1/include/core/SkTypeface.h
File include/core/SkTypeface.h (right):

https://codereview.chromium.org/1818043002/diff/1/include/core/SkTypeface.h#n...
include/core/SkTypeface.h:107: static SkTypeface* CreateFromNameAndStyle(const
char familyName[],
All of the static 'Create' methods on Typeface are more or less deprecated (or
at least 'toy'). I'd hate to add another one. The idea is that users should hold
onto a SkFontMgr instance and use that instead of relying on an ugly library
static default. On the other hand, this can be used to just replace
CreateFromName, so we could let this in, convert everything over, then remove
CreateFromName. Probably something we should do anyway.

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

https://codereview.chromium.org/1818043002/diff/1/src/ports/SkFontHost_win.cp...
src/ports/SkFontHost_win.cpp:123: lf->lfWidth = 0;
This seems wrong. Why is it here?

[commit-bot: I haz the power, 2016-03-21 16:24:54]

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

[commit-bot: I haz the power, 2016-03-21 16:24:54]

Dry run: This issue passed the CQ dry run.

[Mikus, 2016-03-22 09:56:43]

On 2016/03/21 16:14:26, bungeman1 wrote:
> See https://codereview.chromium.org/1819753003/ for reasons.
> 
> https://codereview.chromium.org/1818043002/diff/1/include/core/SkTypeface.h
> File include/core/SkTypeface.h (right):
> 
>
https://codereview.chromium.org/1818043002/diff/1/include/core/SkTypeface.h#n...
> include/core/SkTypeface.h:107: static SkTypeface* CreateFromNameAndStyle(const
> char familyName[],
> All of the static 'Create' methods on Typeface are more or less deprecated (or
> at least 'toy'). I'd hate to add another one. The idea is that users should
hold
> onto a SkFontMgr instance and use that instead of relying on an ugly library
> static default. On the other hand, this can be used to just replace
> CreateFromName, so we could let this in, convert everything over, then remove
> CreateFromName. Probably something we should do anyway.

Done.

> https://codereview.chromium.org/1818043002/diff/1/src/ports/SkFontHost_win.cpp
> File src/ports/SkFontHost_win.cpp (right):
> 
>
https://codereview.chromium.org/1818043002/diff/1/src/ports/SkFontHost_win.cp...
> src/ports/SkFontHost_win.cpp:123: lf->lfWidth = 0;
> This seems wrong. Why is it here?

SkFontStyleSetGDI creates a set of fonts for the given family name. It sets the
default size of the font, which results in the font created by 

SkCreateTypefaceFromLOGFONT
SkFontStyleSetGDI::createTypeface(int)

to be of the default size, not the requested size. This broke the unittests:

    ComboboxTest.NotifyOnClickWithMouse 
    LabelButtonTest.FontList 
    LabelTest.GetTooltipHandlerForPoint 
    MenuControllerTest.SelectChildButtonView 
    TextfieldTest.DoubleAndTripleClickTest 
    TextfieldTest.DragToSelect 
    TextfieldTest.GetCompositionCharacterBoundsTest 
    TouchSelectionControllerImplTest.HiddenSelectionHandleRetainsCursorPosition 
    TouchSelectionControllerImplTest.SelectRectCallbackTest 
    TouchSelectionMenuRunnerViewsTest.QuickMenuAdjustsAnchorRect

Maybe there's a better way of handling this, investigating.

[Mikus, 2016-03-22 11:00:18]

On 2016/03/22 09:56:43, Mikus wrote:
> On 2016/03/21 16:14:26, bungeman1 wrote:
> > See https://codereview.chromium.org/1819753003/ for reasons.
> > 
> > https://codereview.chromium.org/1818043002/diff/1/include/core/SkTypeface.h
> > File include/core/SkTypeface.h (right):
> > 
> >
>
https://codereview.chromium.org/1818043002/diff/1/include/core/SkTypeface.h#n...
> > include/core/SkTypeface.h:107: static SkTypeface*
CreateFromNameAndStyle(const
> > char familyName[],
> > All of the static 'Create' methods on Typeface are more or less deprecated
(or
> > at least 'toy'). I'd hate to add another one. The idea is that users should
> hold
> > onto a SkFontMgr instance and use that instead of relying on an ugly library
> > static default. On the other hand, this can be used to just replace
> > CreateFromName, so we could let this in, convert everything over, then
remove
> > CreateFromName. Probably something we should do anyway.
> 
> Done.
> 
> >
https://codereview.chromium.org/1818043002/diff/1/src/ports/SkFontHost_win.cpp
> > File src/ports/SkFontHost_win.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1818043002/diff/1/src/ports/SkFontHost_win.cp...
> > src/ports/SkFontHost_win.cpp:123: lf->lfWidth = 0;
> > This seems wrong. Why is it here?
> 
> SkFontStyleSetGDI creates a set of fonts for the given family name. It sets
the
> default size of the font, which results in the font created by 
> 
> SkCreateTypefaceFromLOGFONT
> SkFontStyleSetGDI::createTypeface(int)
> 
> to be of the default size, not the requested size. This broke the unittests:
> 
>     ComboboxTest.NotifyOnClickWithMouse 
>     LabelButtonTest.FontList 
>     LabelTest.GetTooltipHandlerForPoint 
>     MenuControllerTest.SelectChildButtonView 
>     TextfieldTest.DoubleAndTripleClickTest 
>     TextfieldTest.DragToSelect 
>     TextfieldTest.GetCompositionCharacterBoundsTest 
>    
TouchSelectionControllerImplTest.HiddenSelectionHandleRetainsCursorPosition 
>     TouchSelectionControllerImplTest.SelectRectCallbackTest 
>     TouchSelectionMenuRunnerViewsTest.QuickMenuAdjustsAnchorRect
> 
> Maybe there's a better way of handling this, investigating.


According to MSDN
https://msdn.microsoft.com/en-us/library/windows/desktop/dd145037(v=vs.85).aspx

lfWidth
The average width, in logical units, of characters in the font. If lfWidth is
zero, the aspect ratio of the device is matched against the digitization aspect
ratio of the available fonts to find the closest match, determined by the
absolute value of the difference.

So we are trying to create a font of default size, where we should create a face
without any average width constraint.

I wonder whether the lfWidth = 0 should be called in make_canonical or in
SkCreateTypefaceFromLOGFONT, probably the latter to decrease the change scope.

[bungeman-skia, 2016-03-22 20:16:43]

https://codereview.chromium.org/1818043002/diff/40001/include/core/SkTypeface.h
File include/core/SkTypeface.h (right):

https://codereview.chromium.org/1818043002/diff/40001/include/core/SkTypeface...
include/core/SkTypeface.h:111: static SkTypeface* CreateFromName(const char
familyName[],
You're not going to be able to change this signature like this without staging
it. There's no implicit conversion from the old 'unsigned' to SkFontStyle. In
other words, if this change is made to Skia then Skia won't be able to roll into
Chromium (without Chromium being simultaneously updated, as part of the roll).

https://codereview.chromium.org/1818043002/diff/40001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/40001/src/core/SkTypeface.cpp...
src/core/SkTypeface.cpp:123: return fm->matchFamilyStyle(name, fontStyle);
I'm not sure we can make this change directly either without some extra thought.
'legacyCreateTypeface' never fails unless something really bad happens, it does
all sorts of generally unwanted fallback automatically to make sure it doesn't
return nullptr. 'matchFamilyStyle' however, is much more conservative, and will
only return 'good' matches. If the caller isn't used to handling fallback
itself, making this change might be surprising.

https://codereview.chromium.org/1818043002/diff/40001/src/ports/SkFontHost_wi...
File src/ports/SkFontHost_win.cpp (right):

https://codereview.chromium.org/1818043002/diff/40001/src/ports/SkFontHost_wi...
src/ports/SkFontHost_win.cpp:327: lf.lfWidth = 0;
Now that I look at what is actually happening and how this really works, I now
think you were right the first time, it just 'looked wrong' because 'width' is
overloaded. On the other hand, the issue were seeing should be fixed. Since
fixing this existing issue isn't directly related to this CL, I've created
https://codereview.chromium.org/1819973003/ to correct this.

[Mikus, 2016-03-23 10:47:17]

https://codereview.chromium.org/1818043002/diff/40001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/40001/src/core/SkTypeface.cpp...
src/core/SkTypeface.cpp:123: return fm->matchFamilyStyle(name, fontStyle);
On 2016/03/22 20:16:43, bungeman1 wrote:
> I'm not sure we can make this change directly either without some extra
thought.
> 'legacyCreateTypeface' never fails unless something really bad happens, it
does
> all sorts of generally unwanted fallback automatically to make sure it doesn't
> return nullptr. 'matchFamilyStyle' however, is much more conservative, and
will
> only return 'good' matches. If the caller isn't used to handling fallback
> itself, making this change might be surprising.


What do you suggest, then? Should I restore the CreateFromNameAndStyle or
overload CreateFromName?

[Mikus, 2016-03-23 10:48:02]

Also, how do I simultaneously roll Skia with Chromium?

[Mikus, 2016-03-24 12:08:26]

https://codereview.chromium.org/1818043002/diff/40001/include/core/SkTypeface.h
File include/core/SkTypeface.h (right):

https://codereview.chromium.org/1818043002/diff/40001/include/core/SkTypeface...
include/core/SkTypeface.h:111: static SkTypeface* CreateFromName(const char
familyName[],
On 2016/03/22 20:16:43, bungeman1 wrote:
> You're not going to be able to change this signature like this without staging
> it. There's no implicit conversion from the old 'unsigned' to SkFontStyle. In
> other words, if this change is made to Skia then Skia won't be able to roll
into
> Chromium (without Chromium being simultaneously updated, as part of the roll).

Done. Added and ifdef for the legacy code.

https://codereview.chromium.org/1818043002/diff/40001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/40001/src/core/SkTypeface.cpp...
src/core/SkTypeface.cpp:123: return fm->matchFamilyStyle(name, fontStyle);
On 2016/03/23 10:47:17, Mikus wrote:
> On 2016/03/22 20:16:43, bungeman1 wrote:
> > I'm not sure we can make this change directly either without some extra
> thought.
> > 'legacyCreateTypeface' never fails unless something really bad happens, it
> does
> > all sorts of generally unwanted fallback automatically to make sure it
doesn't
> > return nullptr. 'matchFamilyStyle' however, is much more conservative, and
> will
> > only return 'good' matches. If the caller isn't used to handling fallback
> > itself, making this change might be surprising.
> 
> 
> What do you suggest, then? Should I restore the CreateFromNameAndStyle or
> overload CreateFromName?

I suggest that we roll this and check the test results.

https://codereview.chromium.org/1818043002/diff/40001/src/ports/SkFontHost_wi...
File src/ports/SkFontHost_win.cpp (right):

https://codereview.chromium.org/1818043002/diff/40001/src/ports/SkFontHost_wi...
src/ports/SkFontHost_win.cpp:327: lf.lfWidth = 0;
On 2016/03/22 20:16:43, bungeman1 wrote:
> Now that I look at what is actually happening and how this really works, I now
> think you were right the first time, it just 'looked wrong' because 'width' is
> overloaded. On the other hand, the issue were seeing should be fixed. Since
> fixing this existing issue isn't directly related to this CL, I've created
> https://codereview.chromium.org/1819973003/ to correct this.

Acknowledged.

[bungeman-skia, 2016-04-08 18:59:21]

https://codereview.chromium.org/1818043002/diff/80001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/80001/src/core/SkTypeface.cpp...
src/core/SkTypeface.cpp:142: return fm->matchFamilyStyle(name, fontStyle);
I believe Chromium on Linux is currently using an default SkFontMgr for which
this will not work. They should be using src/ports/SkFontMgr_fontconfig but are
instead using src/fonts/SkFontMgr_fontconfig.cpp because that's set as the
hobbled default for Blink for IPC.

[Mikus, 2016-05-19 09:34:02]

https://codereview.chromium.org/1818043002/diff/80001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/80001/src/core/SkTypeface.cpp...
src/core/SkTypeface.cpp:142: return fm->matchFamilyStyle(name, fontStyle);
On 2016/04/08 18:59:21, bungeman-skia wrote:
> I believe Chromium on Linux is currently using an default SkFontMgr for which
> this will not work. They should be using src/ports/SkFontMgr_fontconfig but
are
> instead using src/fonts/SkFontMgr_fontconfig.cpp because that's set as the
> hobbled default for Blink for IPC.

Maybe I'll disable this for Linux and let the Linux people fix this later?

BTW sorry for the month-long delay, I was on a vacation.

[Mikus, 2016-05-20 06:54:07]

On 2016/05/19 09:34:02, Mikus wrote:
> https://codereview.chromium.org/1818043002/diff/80001/src/core/SkTypeface.cpp
> File src/core/SkTypeface.cpp (right):
> 
>
https://codereview.chromium.org/1818043002/diff/80001/src/core/SkTypeface.cpp...
> src/core/SkTypeface.cpp:142: return fm->matchFamilyStyle(name, fontStyle);
> On 2016/04/08 18:59:21, bungeman-skia wrote:
> > I believe Chromium on Linux is currently using an default SkFontMgr for
which
> > this will not work. They should be using src/ports/SkFontMgr_fontconfig but
> are
> > instead using src/fonts/SkFontMgr_fontconfig.cpp because that's set as the
> > hobbled default for Blink for IPC.
> 
> Maybe I'll disable this for Linux and let the Linux people fix this later?
> 
> BTW sorry for the month-long delay, I was on a vacation.

Actually, with your recent changes to MakeFromName I was able to implement this
correctly. Please review.

[bungeman-skia, 2016-05-24 14:42:33]

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

[commit-bot: I haz the power, 2016-05-24 14:42:38]

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

[commit-bot: I haz the power, 2016-05-24 14:45:28]

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

[commit-bot: I haz the power, 2016-05-24 14:45:29]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[bungeman-skia, 2016-05-24 14:49:26]

https://codereview.chromium.org/1818043002/diff/100001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/100001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:33: sk_sp<SkTypeface>
(*gNewCreateTypefaceDelegate)(const char[], SkFontStyle) = nullptr;
Only Skia uses this gCreateTypefaceDelegate, and only for testing. It would be
better to make sure that Skia is updated and just update the existing type. If
Skia is no longer using SkTypeface::MakeFromName(const char[], Style), then the
'if (gCreateTypefaceDelegate) {' block in that call can just be removed. This
avoids the awkward 'New' in the name which will, of course, soon not be new. I
think the only user that would need to be updated is dm/DM.cpp. You'll need to
do that anyway to get dm to compile.

https://codereview.chromium.org/1818043002/diff/100001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:146: if (nullptr == name) {
MakeDefault is just for caching common requests, and calling this way simply
gives incorrect results. At the very least the test here would need to be

if (nullptr == name &&
     (fontStyle.slant() == SkFontStyle::kItalic_Slant ||
      fontStyle.slant() == SkFontStyle::kNormal_Slant) &&
     (fontStyle.weight() == skFontStyle::kBold_Weight ||
      fontStyle.weight() == skFontStyle::kNormal_Weight)
{
    ...
}

otherwise calling this with nullptr as 'name' may give the wrong result.

https://codereview.chromium.org/1818043002/diff/100001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:147: return
MakeDefault(static_cast<Style>((fontStyle.slant() == SkFontStyle::kItalic_Slant
? kItalic : kNormal) |
nit (here and a few other places): Skia allows 100 character long lines, and
these are a bit long.

[Mikus, 2016-05-24 15:22:26]

https://codereview.chromium.org/1818043002/diff/100001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/100001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:33: sk_sp<SkTypeface>
(*gNewCreateTypefaceDelegate)(const char[], SkFontStyle) = nullptr;
On 2016/05/24 14:49:26, bungeman-skia wrote:
> Only Skia uses this gCreateTypefaceDelegate, and only for testing. It would be
> better to make sure that Skia is updated and just update the existing type. If
> Skia is no longer using SkTypeface::MakeFromName(const char[], Style), then
the
> 'if (gCreateTypefaceDelegate) {' block in that call can just be removed. This
> avoids the awkward 'New' in the name which will, of course, soon not be new. I
> think the only user that would need to be updated is dm/DM.cpp. You'll need to
> do that anyway to get dm to compile.

Done. Seems to me that the 'new' version is not needed at all.

https://codereview.chromium.org/1818043002/diff/100001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:146: if (nullptr == name) {
On 2016/05/24 14:49:26, bungeman-skia wrote:
> MakeDefault is just for caching common requests, and calling this way simply
> gives incorrect results. At the very least the test here would need to be
> 
> if (nullptr == name &&
>      (fontStyle.slant() == SkFontStyle::kItalic_Slant ||
>       fontStyle.slant() == SkFontStyle::kNormal_Slant) &&
>      (fontStyle.weight() == skFontStyle::kBold_Weight ||
>       fontStyle.weight() == skFontStyle::kNormal_Weight)
> {
>     ...
> }
> 
> otherwise calling this with nullptr as 'name' may give the wrong result.

Done.

https://codereview.chromium.org/1818043002/diff/100001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:147: return
MakeDefault(static_cast<Style>((fontStyle.slant() == SkFontStyle::kItalic_Slant
? kItalic : kNormal) |
On 2016/05/24 14:49:26, bungeman-skia wrote:
> nit (here and a few other places): Skia allows 100 character long lines, and
> these are a bit long.

Done.

[bungeman-skia, 2016-05-24 15:23:15]

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

[commit-bot: I haz the power, 2016-05-24 15:23:22]

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

[commit-bot: I haz the power, 2016-05-24 15:26:31]

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

[commit-bot: I haz the power, 2016-05-24 15:26:32]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[bungeman-skia, 2016-05-24 15:29:11]

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:39: bool UsesOnlyItalicOrBoldStyling(SkFontStyle style)
{
I would prefer not to give this a name and staying power, it's a hack that
should be used in one place. Indeed, I would probably prefer it not exist at all
because I don't think the MakeDefault call this is protecting is even needed.

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:150: if (gCreateTypefaceDelegate &&
UsesOnlyItalicOrBoldStyling(fontStyle)) {
Far from adding to this hack, we need to make it easier to remove in the future.
You should just move gCreateTypefaceDelegate to the SkFontStyle signature and
update the one single user in dm/DM.cpp.

[bungeman-skia, 2016-05-24 17:45:44]

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:132: #ifndef SK_DONT_USE_LEGACY_TYPEFACE_MAKE_FROM_NAME
Usually we would add a define to Chromium to keep the old signature around. I've
created https://codereview.chromium.org/2008003002/ to add
SK_SUPPORT_LEGACY_TYPEFACE_MAKE_FROM_NAME to do this instead so that the logic
has fewer negations in it and so that it's obvious for which users this still
exists.

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:134: if (gCreateTypefaceDelegate) {
Since the intent is to make this go away in the end, it would probably be better
to make the entire implementation of this method

SkTypeface::MakeFromName(name, SkFontStyle::FromOldStyle(style);

[Mikus, 2016-05-25 10:57:25]

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cpp
File src/core/SkTypeface.cpp (right):

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:39: bool UsesOnlyItalicOrBoldStyling(SkFontStyle style)
{
On 2016/05/24 15:29:11, bungeman-skia wrote:
> I would prefer not to give this a name and staying power, it's a hack that
> should be used in one place. Indeed, I would probably prefer it not exist at
all
> because I don't think the MakeDefault call this is protecting is even needed.

Done.

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:132: #ifndef SK_DONT_USE_LEGACY_TYPEFACE_MAKE_FROM_NAME
On 2016/05/24 17:45:44, bungeman-skia wrote:
> Usually we would add a define to Chromium to keep the old signature around.
I've
> created https://codereview.chromium.org/2008003002/ to add
> SK_SUPPORT_LEGACY_TYPEFACE_MAKE_FROM_NAME to do this instead so that the logic
> has fewer negations in it and so that it's obvious for which users this still
> exists.

Done.

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:134: if (gCreateTypefaceDelegate) {
On 2016/05/24 17:45:44, bungeman-skia wrote:
> Since the intent is to make this go away in the end, it would probably be
better
> to make the entire implementation of this method
> 
> SkTypeface::MakeFromName(name, SkFontStyle::FromOldStyle(style);

Done.

https://codereview.chromium.org/1818043002/diff/120001/src/core/SkTypeface.cp...
src/core/SkTypeface.cpp:150: if (gCreateTypefaceDelegate &&
UsesOnlyItalicOrBoldStyling(fontStyle)) {
On 2016/05/24 15:29:11, bungeman-skia wrote:
> Far from adding to this hack, we need to make it easier to remove in the
future.
> You should just move gCreateTypefaceDelegate to the SkFontStyle signature and
> update the one single user in dm/DM.cpp.

Done.

[bungeman-skia, 2016-05-25 12:00:02]

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

[commit-bot: I haz the power, 2016-05-25 12:00:06]

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

[commit-bot: I haz the power, 2016-05-25 12:03:38]

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

[commit-bot: I haz the power, 2016-05-25 12:03:40]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[bungeman-skia, 2016-05-25 12:16:08]

Aside from the not compiling part, this looks pretty good. Just wondering, how
are you compiling this locally? I'm guessing from the base url here that you're
making these changes inside Chromium's third_party/skia pulled by DEPS. You may
be interested in https://skia.org/dev/chrome/repo . The Skia bots build at least
the 'most' target (which I think is the default) using Skia's gyp build.

[Mikus, 2016-05-25 14:49:27]

On 2016/05/25 12:16:08, bungeman-skia wrote:
> Aside from the not compiling part, this looks pretty good. Just wondering, how
> are you compiling this locally? I'm guessing from the base url here that
you're
> making these changes inside Chromium's third_party/skia pulled by DEPS. You
may
> be interested in https://skia.org/dev/chrome/repo . The Skia bots build at
least
> the 'most' target (which I think is the default) using Skia's gyp build.

Thanks for the tip, yes, I've been compiling this as a submodule to Chromium,
running gyp-and-sync to generate the VS solution.

[Mikus, 2016-05-25 15:00:05]

On 2016/05/25 14:49:27, Mikus wrote:
> On 2016/05/25 12:16:08, bungeman-skia wrote:
> > Aside from the not compiling part, this looks pretty good. Just wondering,
how
> > are you compiling this locally? I'm guessing from the base url here that
> you're
> > making these changes inside Chromium's third_party/skia pulled by DEPS. You
> may
> > be interested in https://skia.org/dev/chrome/repo . The Skia bots build at
> least
> > the 'most' target (which I think is the default) using Skia's gyp build.
> 
> Thanks for the tip, yes, I've been compiling this as a submodule to Chromium,
> running gyp-and-sync to generate the VS solution.

BTW. The compilation problems have been solved.

[bungeman-skia, 2016-05-26 15:05:35]

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

[commit-bot: I haz the power, 2016-05-26 15:05:48]

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

[commit-bot: I haz the power, 2016-05-26 15:11:39]

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

[commit-bot: I haz the power, 2016-05-26 15:11:40]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[bungeman-skia, 2016-05-26 15:39:33]

https://codereview.chromium.org/1818043002/diff/200001/gm/fontcache.cpp
File gm/fontcache.cpp (right):

https://codereview.chromium.org/1818043002/diff/200001/gm/fontcache.cpp#newco...
gm/fontcache.cpp:38: SkFontStyle::FromOldStyle(SkTypeface::kItalic));
nit: this is a bit awkward looking since this line has different indentation
than the similar one above. I think we would prefer the four space indent here.

https://codereview.chromium.org/1818043002/diff/200001/tests/TypefaceTest.cpp
File tests/TypefaceTest.cpp (right):

https://codereview.chromium.org/1818043002/diff/200001/tests/TypefaceTest.cpp...
tests/TypefaceTest.cpp:25: sk_sp<SkTypeface>
t3(SkTypeface::MakeFromName("non-existent-font", SkTypeface::kNormal));
Need to update this one as well. (The wonders of multi-platform development and
users. Some Android apps appear to currently depend on this returning nullptr,
even though the interface states that it will return the 'default font' and
never return nullptr.)

I pulled this patch down and updated this use. Android built, so I think this is
the last one.

[bungeman-skia, 2016-05-26 20:13:14]

Description was changed from

==========
Add the method 'CreateFromNameAndStyle' for creating SkTypefaces using a font
style.

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

to

==========
SkTypeface::MakeFromName to take SkFontStyle.

SkTypeface::MakeFromName currently takes SkTypeface::Style,
which is quite limited. This starts the transition to this
function taking SkFontStyle instead.

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

[Mikus, 2016-05-30 14:18:39]

https://codereview.chromium.org/1818043002/diff/200001/gm/fontcache.cpp
File gm/fontcache.cpp (right):

https://codereview.chromium.org/1818043002/diff/200001/gm/fontcache.cpp#newco...
gm/fontcache.cpp:38: SkFontStyle::FromOldStyle(SkTypeface::kItalic));
On 2016/05/26 15:39:33, bungeman-skia wrote:
> nit: this is a bit awkward looking since this line has different indentation
> than the similar one above. I think we would prefer the four space indent
here.

Done.

https://codereview.chromium.org/1818043002/diff/200001/tests/TypefaceTest.cpp
File tests/TypefaceTest.cpp (right):

https://codereview.chromium.org/1818043002/diff/200001/tests/TypefaceTest.cpp...
tests/TypefaceTest.cpp:25: sk_sp<SkTypeface>
t3(SkTypeface::MakeFromName("non-existent-font", SkTypeface::kNormal));
On 2016/05/26 15:39:33, bungeman-skia wrote:
> Need to update this one as well. (The wonders of multi-platform development
and
> users. Some Android apps appear to currently depend on this returning nullptr,
> even though the interface states that it will return the 'default font' and
> never return nullptr.)
> 
> I pulled this patch down and updated this use. Android built, so I think this
is
> the last one.

Done.

[bungeman-skia, 2016-05-31 16:03:52]

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

[commit-bot: I haz the power, 2016-05-31 16:03:59]

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

[commit-bot: I haz the power, 2016-05-31 16:21:33]

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

[commit-bot: I haz the power, 2016-05-31 16:21:35]

Dry run: This issue passed the CQ dry run.

[bungeman-skia, 2016-05-31 18:39:40]

Description was changed from

==========
SkTypeface::MakeFromName to take SkFontStyle.

SkTypeface::MakeFromName currently takes SkTypeface::Style,
which is quite limited. This starts the transition to this
function taking SkFontStyle instead.

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

to

==========
SkTypeface::MakeFromName to take SkFontStyle.

SkTypeface::MakeFromName currently takes SkTypeface::Style,
which is quite limited. This starts the transition to this
function taking SkFontStyle instead.

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

TBR=reed
He said it sounded like a good idea.
==========

[bungeman-skia, 2016-05-31 18:39:55]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2016-05-31 18:40:05]

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

[commit-bot: I haz the power, 2016-05-31 18:40:07]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2016-06-01 00:40 UTC

[bungeman-skia, 2016-05-31 18:40:19]

lgtm

[commit-bot: I haz the power, 2016-05-31 18:42:40]

Description was changed from

==========
SkTypeface::MakeFromName to take SkFontStyle.

SkTypeface::MakeFromName currently takes SkTypeface::Style,
which is quite limited. This starts the transition to this
function taking SkFontStyle instead.

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

TBR=reed
He said it sounded like a good idea.
==========

to

==========
SkTypeface::MakeFromName to take SkFontStyle.

SkTypeface::MakeFromName currently takes SkTypeface::Style,
which is quite limited. This starts the transition to this
function taking SkFontStyle instead.

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

TBR=reed
He said it sounded like a good idea.

Committed:
https://skia.googlesource.com/skia/+/ee6a9919a362e16c1d84a870ce867d1ad7b8a141
==========

[commit-bot: I haz the power, 2016-05-31 18:42:42]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/ee6a9919a362e16c1d84a870ce867d1ad7b8a141