Supports the cap height for FreeType even when TT OS2 version is 1.

Supports the cap height for FreeType even when TT OS2 version is 1.

BUG=http://crbug.com/318645
R=bungeman@google.com, reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12689

[Yuki, 2013-12-05 06:05:50]

Hi Ben and Mike,

Could you guys review this CL?  I'd like Issue 318645 (and 315402, 324771,
324521) to be fixed soon.  So let me send this CL.

This is my first time to send a CL to Skia project.  So I'm not sure if I'm
doing this in the right way.  If something is wrong, please let me know.

Thanks,
Yuki Shiino

[reed1, 2013-12-05 14:01:59]

lgtm -- deferring to bungeman

[Yuki, 2013-12-09 02:31:20]

Hi bungeman@,

Could you take a look at this CL?

[bungeman-skia, 2013-12-09 15:23:59]

It's probably a good idea, but let's wait for
https://codereview.chromium.org/23684041/ to land.

[Yuki, 2013-12-11 06:30:25]

On 2013/12/09 15:23:59, bungeman1 wrote:
> It's probably a good idea, but let's wait for
> https://codereview.chromium.org/23684041/ to land.

It seems http://crrev.com/23684041 was committed.

Could you take a look?

[bungeman-skia, 2013-12-11 15:29:21]

So I think this is probably ok, but I am very much questioning the reasons for
this. It appears that you want this in order to get the cap height in Chromium.
However, please don't do this. getAdvancedTypefaceMetrics is super heavy and an
internal hack for PDF.

It looks like what you really want is a metric for the baseline offset which
causes the text to be "centered". For horizontal text it seems that this should
be (in some order) the ideographic baseline offset, the strikethrough position,
the x-height, then maybe half the cap height.

There are two things which can be done here. The first of which is for Skia to
add a fCapHeight to SkPaint::FontMetrics and have all of the
SkScalerContext::generateMetrics ports fill it out. The second is to let
chromium just use getTableData and parse this out itself. (Note that there is no
loss of generality here from existing code which only sees the cap height from
TT fonts anyway.)

[Yuki, 2013-12-12 09:43:48]

On 2013/12/11 15:29:21, bungeman1 wrote:
> So I think this is probably ok, but I am very much questioning the reasons for
> this. It appears that you want this in order to get the cap height in
Chromium.
> However, please don't do this. getAdvancedTypefaceMetrics is super heavy and
an
> internal hack for PDF.
> 
> It looks like what you really want is a metric for the baseline offset which
> causes the text to be "centered". For horizontal text it seems that this
should
> be (in some order) the ideographic baseline offset, the strikethrough
position,
> the x-height, then maybe half the cap height.
> 
> There are two things which can be done here. The first of which is for Skia to
> add a fCapHeight to SkPaint::FontMetrics and have all of the
> SkScalerContext::generateMetrics ports fill it out. The second is to let
> chromium just use getTableData and parse this out itself. (Note that there is
no
> loss of generality here from existing code which only sees the cap height from
> TT fonts anyway.)

Thanks for the advice.  I've changed the approach.
Could you take another look?

[bungeman-skia, 2013-12-12 17:12:34]

This looks much better.

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

https://codereview.chromium.org/101333004/diff/40001/include/core/SkPaint.h#n...
include/core/SkPaint.h:744: SkScalar    fCapHeight;  //!< The cap height (will
be => 0)
add or '0' if cannot be determined here.

While you're currently updating the FreeType backend to basically never return
0, the others still return 0 and we should document the 'cannot be determined
value' similar to x-height.

https://codereview.chromium.org/101333004/diff/40001/src/ports/SkFontHost_Fre...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/101333004/diff/40001/src/ports/SkFontHost_Fre...
src/ports/SkFontHost_FreeType.cpp:588: (TT_OS2*)FT_Get_Sfnt_Table(face,
ft_sfnt_os2)) != NULL) &&
Note that Skia now has an 80 soft / 100 hard column limit. I'd say put this
assignment all on one line now.

https://codereview.chromium.org/101333004/diff/40001/src/ports/SkFontHost_Fre...
src/ports/SkFontHost_FreeType.cpp:1411: got_m = getCBoxForLetter('M', &m_bbox);
I think traditionally H is used if available. If the .notdef (glyph 0 by
convention and enforced by FreeType, see
http://www.microsoft.com/typography/otspec/recom.htm) exists and has a non-zero
extent it would probably also be a good candidate. 'M's especially often have
descending flourishes, so it seems like one would use the yMax and ignore the
yMin. I'm assuming that any fonts you tested on have yMin close to 0 for these
characters. Note that above we're just using the yMax for the x-height for
similar reasons.

Since these are the majuscules you're using, it may make more sense to name
these got_M and got_X.

[Yuki, 2013-12-13 09:32:14]

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

https://codereview.chromium.org/101333004/diff/40001/include/core/SkPaint.h#n...
include/core/SkPaint.h:744: SkScalar    fCapHeight;  //!< The cap height (will
be => 0)
On 2013/12/12 17:12:35, bungeman1 wrote:
> add or '0' if cannot be determined here.
> 
> While you're currently updating the FreeType backend to basically never return
> 0, the others still return 0 and we should document the 'cannot be determined
> value' similar to x-height.

Done.

https://codereview.chromium.org/101333004/diff/40001/src/ports/SkFontHost_Fre...
File src/ports/SkFontHost_FreeType.cpp (right):

https://codereview.chromium.org/101333004/diff/40001/src/ports/SkFontHost_Fre...
src/ports/SkFontHost_FreeType.cpp:588: (TT_OS2*)FT_Get_Sfnt_Table(face,
ft_sfnt_os2)) != NULL) &&
On 2013/12/12 17:12:35, bungeman1 wrote:
> Note that Skia now has an 80 soft / 100 hard column limit. I'd say put this
> assignment all on one line now.

Done.

https://codereview.chromium.org/101333004/diff/40001/src/ports/SkFontHost_Fre...
src/ports/SkFontHost_FreeType.cpp:1411: got_m = getCBoxForLetter('M', &m_bbox);
On 2013/12/12 17:12:35, bungeman1 wrote:
> I think traditionally H is used if available. If the .notdef (glyph 0 by
> convention and enforced by FreeType, see
> http://www.microsoft.com/typography/otspec/recom.htm) exists and has a
non-zero
> extent it would probably also be a good candidate. 'M's especially often have
> descending flourishes, so it seems like one would use the yMax and ignore the
> yMin. I'm assuming that any fonts you tested on have yMin close to 0 for these
> characters. Note that above we're just using the yMax for the x-height for
> similar reasons.
> 
> Since these are the majuscules you're using, it may make more sense to name
> these got_M and got_X.

Following your recommendation and tradition, I use yMax of 'H'.

[bungeman-skia, 2013-12-13 15:31:08]

lgtm

[Yuki, 2013-12-16 10:46:25]

bungeman,

Could you commit this change instead of me?  It seems I cannot do it because I'm
not a committer of Skia project.

[bungeman-skia, 2013-12-16 17:02:43]

Committed patchset #4 manually as r12689.

[bungeman-skia, 2013-12-16 17:11:27]

On 2013/12/16 10:46:25, Yuki wrote:
> bungeman,
> 
> Could you commit this change instead of me?  It seems I cannot do it because
I'm
> not a committer of Skia project.

I believe the commit checkbox should work after getting the green light.
However, it looks like the commit queue didn't work because of the base url. It
looks like you submitted this from the chromium mirror which isn't set up for
Skia's commit queue. I've landed manually.

[Yuki, 2013-12-17 03:52:06]

On 2013/12/16 17:11:27, bungeman1 wrote:
> On 2013/12/16 10:46:25, Yuki wrote:
> > bungeman,
> > 
> > Could you commit this change instead of me?  It seems I cannot do it because
> I'm
> > not a committer of Skia project.
> 
> I believe the commit checkbox should work after getting the green light.
> However, it looks like the commit queue didn't work because of the base url.
It
> looks like you submitted this from the chromium mirror which isn't set up for
> Skia's commit queue. I've landed manually.

Thanks a lot!
You're right.  I had to set up for Skia correctly.  Thanks.