Do not use SkCreateTypefaceFromCTFont for custom fonts

Do not use SkCreateTypefaceFromCTFont for custom fonts

SkCreateTypefaceFromCTFont implies fonts are non-local
(i.e. serialized as font name only, not the glyphs). This
does not work with the dynamically loaded fonts, so just
keep the SkTypeface we used to create in FontCustomPlatformDataMac
rather than create one from CTFont.

BUG=426088
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184884

[caseq, 2014-10-30 17:05:10]

caseq@chromium.org changed reviewers:
+ dominik.rottsches@intel.com, schenney@chromium.org

[caseq, 2014-10-30 17:05:10]

Skia keeps a flag that tells it whether a typeface needs to be serialized with
all the glyph data (isLocalStream) or it's sufficient to serialize just the font
name/family (!isLocalStream). Current implementation of
SkCreateTypefaceFromCTFont always implies !isLocalStream, so dynamically loaded
fonts created via SkCreateTypefaceFromCTFont won't be properly serialized. Since
we already call SkTypeface::CreateFromStream() that properly marks typeface as
local, just pass whatever it returns down to FontPlatformData instead of
creating another instance of typeface via SkCreateTypefaceFromCTFont.
See https://codereview.chromium.org/692553002/ for more discussion on the
subject.

[reed1, 2014-10-30 17:25:32]

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

[caseq, 2014-10-31 13:10:11]

Oh, ok, I get it -- this bypasses the logic in FontPlatformData::ctFont()
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
that replaces ctFont with one adjusted for width variation.

Surprisingly, the failing tests pass locally for me (and on another mac with
retina that we tried). I guess I should resort to the originally proposed
solution of passing a flag to SkCreateTypefaceFromCTFont. Please see
https://codereview.chromium.org/697473005 (chromium part) and
https://codereview.chromium.org/692553002/ (skia).

[bungeman-chromium, 2014-10-31 19:23:37]

On 2014/10/31 13:10:11, caseq wrote:
> Oh, ok, I get it -- this bypasses the logic in FontPlatformData::ctFont()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> that replaces ctFont with one adjusted for width variation.
> 
> Surprisingly, the failing tests pass locally for me (and on another mac with
> retina that we tried). I guess I should resort to the originally proposed
> solution of passing a flag to SkCreateTypefaceFromCTFont. Please see
> https://codereview.chromium.org/697473005 (chromium part) and
> https://codereview.chromium.org/692553002/ (skia).

Not to derail the need for a fix soon, but I'm trying to figure out why the
width variation is being specified at CTFont creation time like this. This is
just a font feature, and so I would expect this to be handled by the shaper (the
point of the shaper is to interpret font features and the like, basically doing
glyph substitutions). Is there something special about this feature that it is
being set here and not just going down the complex shaping path? On Linux it
appears that this test is shaped correctly (though when I run it locally I still
get some FAILED lines).

[bungeman-chromium, 2014-11-04 20:06:30]

On 2014/10/31 19:23:37, bungeman2 wrote:
> On 2014/10/31 13:10:11, caseq wrote:
> > Oh, ok, I get it -- this bypasses the logic in FontPlatformData::ctFont()
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > that replaces ctFont with one adjusted for width variation.
> > 
> > Surprisingly, the failing tests pass locally for me (and on another mac with
> > retina that we tried). I guess I should resort to the originally proposed
> > solution of passing a flag to SkCreateTypefaceFromCTFont. Please see
> > https://codereview.chromium.org/697473005 (chromium part) and
> > https://codereview.chromium.org/692553002/ (skia).
> 
> Not to derail the need for a fix soon, but I'm trying to figure out why the
> width variation is being specified at CTFont creation time like this. This is
> just a font feature, and so I would expect this to be handled by the shaper
(the
> point of the shaper is to interpret font features and the like, basically
doing
> glyph substitutions). Is there something special about this feature that it is
> being set here and not just going down the complex shaping path? On Linux it
> appears that this test is shaped correctly (though when I run it locally I
still
> get some FAILED lines).

Taking a closer look at the 'failing' tests in the trybot run, it appears that
the 'failure' is that with this change *more* subtests pass. The subtests which
fail still fail the same way they used to. This CL should probably just land
with the new expectations.

[bungeman-chromium, 2014-11-04 21:31:41]

On 2014/11/04 20:06:30, bungeman2 wrote:
> On 2014/10/31 19:23:37, bungeman2 wrote:
> > On 2014/10/31 13:10:11, caseq wrote:
> > > Oh, ok, I get it -- this bypasses the logic in FontPlatformData::ctFont()
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > > that replaces ctFont with one adjusted for width variation.
> > > 
> > > Surprisingly, the failing tests pass locally for me (and on another mac
with
> > > retina that we tried). I guess I should resort to the originally proposed
> > > solution of passing a flag to SkCreateTypefaceFromCTFont. Please see
> > > https://codereview.chromium.org/697473005 (chromium part) and
> > > https://codereview.chromium.org/692553002/ (skia).
> > 
> > Not to derail the need for a fix soon, but I'm trying to figure out why the
> > width variation is being specified at CTFont creation time like this. This
is
> > just a font feature, and so I would expect this to be handled by the shaper
> (the
> > point of the shaper is to interpret font features and the like, basically
> doing
> > glyph substitutions). Is there something special about this feature that it
is
> > being set here and not just going down the complex shaping path? On Linux it
> > appears that this test is shaped correctly (though when I run it locally I
> still
> > get some FAILED lines).
> 
> Taking a closer look at the 'failing' tests in the trybot run, it appears that
> the 'failure' is that with this change *more* subtests pass. The subtests
which
> fail still fail the same way they used to. This CL should probably just land
> with the new expectations.

Taking a closer look, it appears the reason why the bots are 'failing' with the
correct result is due to the bot using the fallback

mac-snowleopard -> mac-lion -> mac-mountainlion -> mac -> generic

(apparently these bots are running 10.6?) . mac-lion has these bad expectations,
apparently the CoreText didn't support these font features at the time. As a
result, you will only see the 'failures' on 10.7 and earlier. These 'failures'
appear to be fixed with the move to HarfBuzz, which does support these features.
So the explicit setting of these features may just be leftover cruft from using
the CoreText shaper. So I think this change (PS1) is good, with the additional
change that

./platform/mac-lion/fast/text/font-variant-width-expected.txt
./platform/mac-lion/virtual/antialiasedtext/fast/text/font-variant-width-expected.txt

are deleted, since these now match the 'generic' expectations.

[caseq, 2014-11-05 01:49:55]

> Taking a closer look, it appears the reason why the bots are 'failing' with
the
> correct result is due to the bot using the fallback

> mac-snowleopard -> mac-lion -> mac-mountainlion -> mac -> generic

Hmm, indeed, thanks a lot for spotting that! Trying with removed lion
expectations now.

[bungeman-chromium, 2014-11-05 15:29:33]

The CQ bit was checked by bungeman@chromium.org

[bungeman-chromium, 2014-11-05 15:29:33]

lgtm

[commit-bot: I haz the power, 2014-11-05 15:29:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/691733004/20001

[commit-bot: I haz the power, 2014-11-05 15:33:36]

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

[commit-bot: I haz the power, 2014-11-05 15:33:38]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[bungeman-chromium, 2014-11-05 16:06:26]

bungeman@chromium.org changed reviewers:
+ eae@chromium.org

[bungeman-chromium, 2014-11-05 16:06:27]

I like this, but we need an OWNER.

[f(malita), 2014-11-05 22:42:46]

fmalita@chromium.org changed reviewers:
+ fmalita@chromium.org

[f(malita), 2014-11-05 22:42:46]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2014-11-05 22:42:46]

lgtm

[commit-bot: I haz the power, 2014-11-05 22:43:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/691733004/20001

[commit-bot: I haz the power, 2014-11-05 22:45:46]

Committed patchset #2 (id:20001) as 184884