refactor emboldenGlyph

This patch makes SkScalerContext_FreeType to be the only one which embolden the glyphs.

Add bench cases for different font styles.

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=12792

[zheng.xu, 2013-12-13 08:31:29]

I create a new cl, for the rebasing cause the original one
https://codereview.chromium.org/106133002/ difficult to review.

[bungeman-skia, 2013-12-13 15:27:21]

See https://code.google.com/p/skia/issues/detail?id=1896 . This is the opposite
behavior of what we want. We don't care if the font style claims to be 'bold' or
whatever, we always want to fake bold when this flag is set.

[zheng.xu, 2013-12-16 02:55:01]

On 2013/12/13 15:27:21, bungeman1 wrote:
> See https://code.google.com/p/skia/issues/detail?id=1896 . This is the
opposite
> behavior of what we want. We don't care if the font style claims to be 'bold'
or
> whatever, we always want to fake bold when this flag is set.

fixed

[bungeman-skia, 2013-12-16 16:33:36]

To summarize the following comments:

1. emboldenIfNeeded should just go on SkScalerContext_FreeType (the base doesn't
use it, and there's no need for it there).

2. It doesn't appear that we need any of the bench changes for two reasons.
First, this isn't calling setFakeBoldText so adding other styles should not
affect anything. Second, no one actually defines SK_USE_FREETYPE_EMBOLDEN so
most of this code is currently dead and isn't being called anyway. This may
explain why we haven't been seeing issues with it.

https://codereview.chromium.org/101423004/diff/10001/bench/FontScalerBench.cpp
File bench/FontScalerBench.cpp (right):

https://codereview.chromium.org/101423004/diff/10001/bench/FontScalerBench.cp...
bench/FontScalerBench.cpp:1: /*
I'm unsure of what the changes to this file are for. Nowhere is
paint.setFakeBoldText(true) called, and this is a bench (not a GM) so changes to
the output canvas are not tested.

https://codereview.chromium.org/101423004/diff/10001/bench/FontScalerBench.cp...
bench/FontScalerBench.cpp:61: SkTypeface* face =
SkTypeface::CreateFromName(NULL, fStyle);
SkAutoTUnref<SkTypeface> face(SkTypeface::CreateFromName(NULL, fStyle));

and remove the SkSafeUnref below.

https://codereview.chromium.org/101423004/diff/10001/src/ports/SkFontHost_Fre...
File src/ports/SkFontHost_FreeType_common.cpp (left):

https://codereview.chromium.org/101423004/diff/10001/src/ports/SkFontHost_Fre...
src/ports/SkFontHost_FreeType_common.cpp:550: if (fRec.fFlags &
SkScalerContext::kEmbolden_Flag) {
I am of two minds about removing this from here and moving it up to
SkFontHost_FreeType. On the one hand this changes the behavior of this method
such that it no longer respects the kEmbolden flag. On the other hand it seems
wrong for this method to respect such flags as the patch (or image, above) must
have the same emboldening as the metrics and the metrics are not calculated
here.

On the other hand it seems like this would change behavior. Fortunately, that
isn't an issue, because the kEmbolden flag is only ever set if
SK_USE_FREETYPE_EMBOLDEN is defined at build time, and no one does that (as far
as I'm aware).

https://codereview.chromium.org/101423004/diff/10001/src/ports/SkFontHost_Fre...
File src/ports/SkFontHost_FreeType_common.cpp (right):

https://codereview.chromium.org/101423004/diff/10001/src/ports/SkFontHost_Fre...
src/ports/SkFontHost_FreeType_common.cpp:572: void
SkScalerContext_FreeType_Base::emboldenIfNeeded(FT_Face face, FT_GlyphSlot
glyph)
Having this here seems a bit silly if it's only called from SkFontHost_FreeType.
Since all of the path/image/metrics must use the same embolden (otherwise
something will end up incorrect) and since the metrics aren't handled here, I'd
say just move this over to SkFontHost_FreeType.cpp where it can also be inlined.

[bungeman-skia, 2013-12-16 17:12:53]

Correction, it turns out Chromium is defining SK_USE_FREETYPE_EMBOLDEN, though
they have a TODO not to do so...

[zheng.xu, 2013-12-17 10:02:51]

https://codereview.chromium.org/101423004/diff/10001/bench/FontScalerBench.cpp
File bench/FontScalerBench.cpp (right):

https://codereview.chromium.org/101423004/diff/10001/bench/FontScalerBench.cp...
bench/FontScalerBench.cpp:1: /*
There are GM cases covers different font styles.
I will revert the changes on this file, since it tries to cover the code path
that won't be used later.

https://codereview.chromium.org/101423004/diff/10001/bench/FontScalerBench.cp...
bench/FontScalerBench.cpp:61: SkTypeface* face =
SkTypeface::CreateFromName(NULL, fStyle);
Thanks for the hint. This is quite convenient.

https://codereview.chromium.org/101423004/diff/10001/src/ports/SkFontHost_Fre...
File src/ports/SkFontHost_FreeType_common.cpp (left):

https://codereview.chromium.org/101423004/diff/10001/src/ports/SkFontHost_Fre...
src/ports/SkFontHost_FreeType_common.cpp:550: if (fRec.fFlags &
SkScalerContext::kEmbolden_Flag) {
Moving this to SkFontHost_FreeType changes the behavior of the function
SkScalerContext_FreeType_Base::generateGlyphPath. But this protected function is
not the interface function of SkScalerContext. 

Only the inherited class need to take care of this change. The related changes
has been already applied by this patch sets. The behavior of the interface
function SkScalerContext_FreeType::generatePath still keeps unchanged.

https://codereview.chromium.org/101423004/diff/10001/src/ports/SkFontHost_Fre...
File src/ports/SkFontHost_FreeType_common.cpp (right):

https://codereview.chromium.org/101423004/diff/10001/src/ports/SkFontHost_Fre...
src/ports/SkFontHost_FreeType_common.cpp:572: void
SkScalerContext_FreeType_Base::emboldenIfNeeded(FT_Face face, FT_GlyphSlot
glyph)
On 2013/12/16 16:33:36, bungeman1 wrote:
> Having this here seems a bit silly if it's only called from
SkFontHost_FreeType.
> Since all of the path/image/metrics must use the same embolden (otherwise
> something will end up incorrect) and since the metrics aren't handled here,
I'd
> say just move this over to SkFontHost_FreeType.cpp where it can also be
inlined.

Done.

[bungeman-skia, 2013-12-17 15:07:22]

lgtm at Patch Set 3.

[zheng.xu, 2013-12-19 12:00:42]

@bungeman1, Can this be merged? So that I can try to modify
https://codereview.chromium.org/103923002/ which depends on this.

[bungeman-skia, 2013-12-19 14:39:46]

On 2013/12/19 12:00:42, zheng.xu wrote:
> @bungeman1, Can this be merged? So that I can try to modify
> https://codereview.chromium.org/103923002/ which depends on this.

lgtm, you should be able to hit the commit button to send this through the
commit queue. If that doesn't work, let me know.

[commit-bot: I haz the power, 2013-12-20 02:02:43]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zheng.xu@arm.com/101423004/50001

[commit-bot: I haz the power, 2013-12-20 04:39:03]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/zheng.xu@arm.com/101423004/50001

[commit-bot: I haz the power, 2013-12-20 04:39:14]

Change committed as 12792

[benchen, 2013-12-20 13:38:09]

A revert of this CL has been created in
https://codereview.chromium.org/119943002/ by bensong@google.com.

The reason for reverting is: broke some win7 bots.

[bungeman-skia, 2013-12-20 14:59:09]

On 2013/12/20 13:38:09, benchen wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/119943002/ by mailto:bensong@google.com.
> 
> The reason for reverting is: broke some win7 bots.

Ugh, this didn't break the win7 bots, the failure seen was actually
https://code.google.com/p/skia/issues/detail?id=1906 which is a pre-existing
known race. There isn't anything in the change which could affect the Windows
tests. I'll see about re-landing this.

[bungeman-skia, 2013-12-20 14:59:11]

On 2013/12/20 13:38:09, benchen wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/119943002/ by mailto:bensong@google.com.
> 
> The reason for reverting is: broke some win7 bots.

Ugh, this didn't break the win7 bots, the failure seen was actually
https://code.google.com/p/skia/issues/detail?id=1906 which is a pre-existing
known race. There isn't anything in the change which could affect the Windows
tests. I'll see about re-landing this.

[bungeman-skia, 2013-12-20 14:59:12]

On 2013/12/20 13:38:09, benchen wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/119943002/ by mailto:bensong@google.com.
> 
> The reason for reverting is: broke some win7 bots.

Ugh, this didn't break the win7 bots, the failure seen was actually
https://code.google.com/p/skia/issues/detail?id=1906 which is a pre-existing
known race. There isn't anything in the change which could affect the Windows
tests. I'll see about re-landing this.

[robertphillips, 2014-01-06 14:55:51]

This was relanded as r12822 (https://codereview.chromium.org/117963003).