Switch to HarfBuzz on Mac and remove CoreText shaper

Switch to HarfBuzz on Mac and remove CoreText shaper

Move to HarfBuzz for shaping on mac and remove the old mac-specific
CoreText shaper Test case added based on handmade ChromiumAATTest font
for verifying AAT shaping is functional.

TEST=fast/text/aat-morx.html
BUG=334269
NOTRY=true
R=eae@chromium.org, esprehn@chromium.org, behdad@chromium.org

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

[Dominik Röttsches, 2014-03-18 14:58:59]

(Not ready for review yet, but) Together with cl 203123003 this is now working
for AAT. 

The question is: How should we test this - create our own simple AAT ligature
font to test? Or use a simple test based on Zapfino for example?

[behdad, 2014-03-18 17:19:00]

lgtm

Neat!

[Dominik Röttsches, 2014-03-20 18:11:23]

On 2014/03/18 17:19:00, behdad wrote:
> lgtm
> 
> Neat!

Thanks!

Emil, do you wanna have a look as well? If there are no objections, I am
planning to land this one, once CLs 197533012 and 197533012 landed. (building hb
with coretext, and adding the test font)

[eae, 2014-03-20 18:15:40]

On 2014/03/20 18:11:23, Dominik Röttsches wrote:
> On 2014/03/18 17:19:00, behdad wrote:
> > lgtm
> > 
> > Neat!
> 
> Thanks!
> 
> Emil, do you wanna have a look as well? If there are no objections, I am
> planning to land this one, once CLs 197533012 and 197533012 landed. (building
hb
> with coretext, and adding the test font)

Didn't realize this was ready for review. Looking at it now.

[eae, 2014-03-20 18:20:30]

https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/m...
File Source/platform/fonts/mac/FontComplexTextMac.cpp (right):

https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontComplexTextMac.cpp:41: 
Do we still need this file? It doesn't look much different from
FontHarfBuzz.cpp.

https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/m...
Source/platform/fonts/mac/FontComplexTextMac.cpp:77: float
Font::floatWidthForComplexText(const TextRun& run, HashSet<const
SimpleFontData*>* fallbackFonts, GlyphOverflow* glyphOverflow) const
We really want to use the FontHarfBuzz.cpp implementation of
floatWidthForComplexText here or we'll loose glyphOverflow support (which we
just implemented for the harfbuzz code path yesterday).

[Dominik Röttsches, 2014-03-20 18:27:53]

On 2014/03/20 18:20:30, eae wrote:
>
https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/m...
> File Source/platform/fonts/mac/FontComplexTextMac.cpp (right):
> 
>
https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/m...
> Source/platform/fonts/mac/FontComplexTextMac.cpp:41: 
> Do we still need this file? It doesn't look much different from
> FontHarfBuzz.cpp.

Not for long, but I was thinking to do that merge separately. IMHO, it's easier
to follow the commits separately and there are some build issues with
HarfBuzzFaceSkia vs. HarfBuzzFaceCoreText which we need to sort out before we
can merge the two files.

>
https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/m...
> Source/platform/fonts/mac/FontComplexTextMac.cpp:77: float
> Font::floatWidthForComplexText(const TextRun& run, HashSet<const
> SimpleFontData*>* fallbackFonts, GlyphOverflow* glyphOverflow) const
> We really want to use the FontHarfBuzz.cpp implementation of
> floatWidthForComplexText here or we'll loose glyphOverflow support (which we
> just implemented for the harfbuzz code path yesterday).

So, your call - I could try to do all of the above in one patch, or duplicate
the new glyphOverflow code temporarily here in FontComplexTextMac, then do the
merge. What do you think?

[eae, 2014-03-20 18:28:44]

On 2014/03/20 18:27:53, Dominik Röttsches wrote:
> On 2014/03/20 18:20:30, eae wrote:
> >
>
https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/m...
> > File Source/platform/fonts/mac/FontComplexTextMac.cpp (right):
> > 
> >
>
https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/m...
> > Source/platform/fonts/mac/FontComplexTextMac.cpp:41: 
> > Do we still need this file? It doesn't look much different from
> > FontHarfBuzz.cpp.
> 
> Not for long, but I was thinking to do that merge separately. IMHO, it's
easier
> to follow the commits separately and there are some build issues with
> HarfBuzzFaceSkia vs. HarfBuzzFaceCoreText which we need to sort out before we
> can merge the two files.

Ok, makes sense.

LGTM

[Dominik Röttsches, 2014-03-21 08:45:02]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-03-21 08:45:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/175253002/...

[commit-bot: I haz the power, 2014-03-21 09:21:17]

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

[commit-bot: I haz the power, 2014-03-21 09:21:17]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_rel

[Dominik Röttsches, 2014-03-21 10:44:09]

On 2014/03/21 09:21:17, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.blink on mac_blink_rel

Behdad, can you take a look at the test results?
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/77...

To me, most of them seem minuscule pixel difference - but I will check why it
reports a few crashes.

[behdad, 2014-03-21 17:12:05]

On 2014/03/21 10:44:09, Dominik Röttsches wrote:
> On 2014/03/21 09:21:17, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   tryserver.blink on mac_blink_rel
> 
> Behdad, can you take a look at the test results?
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/77...
> 
> To me, most of them seem minuscule pixel difference - but I will check why it
> reports a few crashes.

Looks like we're having a vertical line height difference that is causing most
of the failures.  We should look into that.

[behdad, 2014-03-21 17:13:32]

On 2014/03/21 17:12:05, behdad wrote:
> On 2014/03/21 10:44:09, Dominik Röttsches wrote:
> > On 2014/03/21 09:21:17, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   tryserver.blink on mac_blink_rel
> > 
> > Behdad, can you take a look at the test results?
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/77...
> > 
> > To me, most of them seem minuscule pixel difference - but I will check why
it
> > reports a few crashes.
> 
> Looks like we're having a vertical line height difference that is causing most
> of the failures.  We should look into that.

Some, like svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg, are actually
improvements.  HarfBuzz does mark stacking if the font doesn't have GPOS, so
instead of marks colliding (CoreText), we get stacking now.

[behdad, 2014-03-21 17:17:26]

On 2014/03/21 17:13:32, behdad wrote:
> On 2014/03/21 17:12:05, behdad wrote:
> > On 2014/03/21 10:44:09, Dominik Röttsches wrote:
> > > On 2014/03/21 09:21:17, I haz the power (commit-bot) wrote:
> > > > Try jobs failed on following builders:
> > > >   tryserver.blink on mac_blink_rel
> > > 
> > > Behdad, can you take a look at the test results?
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/77...
> > > 
> > > To me, most of them seem minuscule pixel difference - but I will check why
> it
> > > reports a few crashes.
> > 
> > Looks like we're having a vertical line height difference that is causing
most
> > of the failures.  We should look into that.
> 
> Some, like svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg, are actually
> improvements.  HarfBuzz does mark stacking if the font doesn't have GPOS, so
> instead of marks colliding (CoreText), we get stacking now.

This one is surprising and cause for concern:

  fast/text/international/vertical-positioning-with-combining-marks

Emil and I will be meeting again on Tuesday, so we can look into whatever's left
by then.

[eae, 2014-04-03 16:59:46]

On 2014/03/21 17:17:26, behdad wrote:
> On 2014/03/21 17:13:32, behdad wrote:
> > On 2014/03/21 17:12:05, behdad wrote:
> > > On 2014/03/21 10:44:09, Dominik Röttsches wrote:
> > > > On 2014/03/21 09:21:17, I haz the power (commit-bot) wrote:
> > > > > Try jobs failed on following builders:
> > > > >   tryserver.blink on mac_blink_rel
> > > > 
> > > > Behdad, can you take a look at the test results?
> > > >
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/77...
> > > > 
> > > > To me, most of them seem minuscule pixel difference - but I will check
why
> > it
> > > > reports a few crashes.
> > > 
> > > Looks like we're having a vertical line height difference that is causing
> most
> > > of the failures.  We should look into that.
> > 
> > Some, like svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg, are actually
> > improvements.  HarfBuzz does mark stacking if the font doesn't have GPOS, so
> > instead of marks colliding (CoreText), we get stacking now.
> 
> This one is surprising and cause for concern:
> 
>   fast/text/international/vertical-positioning-with-combining-marks
> 
> Emil and I will be meeting again on Tuesday, so we can look into whatever's
left
> by then.

Finally got a mac build going, looking into this now.

[eae, 2014-04-03 17:30:21]

Changing Font::drawComplexText to call adjustStartPoint fixes a few of the tests

    FloatPoint adjustedPoint = shaper.adjustStartPoint(point);
    drawGlyphBuffer(context, runInfo, glyphBuffer, adjustedPoint);

fast/text/international/vertical-positioning-with-combining-marks is still
failing though so we must be missing some other adjustment.

[Dominik Röttsches, 2014-04-04 11:57:32]

On 2014/04/03 17:30:21, eae wrote:
> Changing Font::drawComplexText to call adjustStartPoint fixes a few of the
tests

Updated the patch with your suggestion.

[Dominik Röttsches, 2014-08-29 16:42:35]

Okay, I am going all in and removing FontMac and FontComplexTextMac as well, I
have a local WiP patch. Needed to get vertical text support.

[eae, 2014-08-29 19:07:54]

On 2014/08/29 16:42:35, Dominik Röttsches wrote:
> Okay, I am going all in and removing FontMac and FontComplexTextMac as well, I
> have a local WiP patch. Needed to get vertical text support.

Awesome! Is there anything I can do to help?

[jungshik at Google, 2014-08-29 22:03:56]

On 2014/08/29 16:42:35, Dominik Röttsches wrote:
> Okay, I am going all in and removing FontMac and FontComplexTextMac as well, I
> have a local WiP patch. Needed to get vertical text support.

Great !! Thanks !

[Dominik Röttsches, 2014-09-01 14:24:32]

On 2014/08/29 19:07:54, eae wrote:
> Awesome! Is there anything I can do to help?

If you look at the bug, there are a couple of remaining spacing issues that we
need to look at. I summarized those at the end. It'd be great if you want to
have a look at those. We can also chat this evening (EEST) and sync a bit.

Other than that, in terms of vertical text, I think I cannot fix this without
fixing our HarfBuzzShaper direction handling for TTB and BTT cases which is not
implemented onany platforms really. I am falling back to aiming for reducing the
amount of brokenness here at least.

Another case of: Let's get rid of the simple path and the OPENTYPE_VERTICAL
implementation which tries to do what the shaper is supposed to do as part of
blink and GlyphPageTreeNode.

[eae, 2014-09-02 18:17:06]

On 2014/09/01 14:24:32, Dominik Röttsches wrote:
> On 2014/08/29 19:07:54, eae wrote:
> > Awesome! Is there anything I can do to help?
> 
> If you look at the bug, there are a couple of remaining spacing issues that we
> need to look at. I summarized those at the end. It'd be great if you want to
> have a look at those. We can also chat this evening (EEST) and sync a bit.

I'll take a look at those later tonight and see what I can find.

> 
> Other than that, in terms of vertical text, I think I cannot fix this without
> fixing our HarfBuzzShaper direction handling for TTB and BTT cases which is
not
> implemented onany platforms really. I am falling back to aiming for reducing
the
> amount of brokenness here at least.
> 
> Another case of: Let's get rid of the simple path and the OPENTYPE_VERTICAL
> implementation which tries to do what the shaper is supposed to do as part of
> blink and GlyphPageTreeNode.

Sounds like a good plan.

[Dominik Röttsches, 2014-09-18 13:37:13]

Patchset #12 (id:240001) has been deleted

[Dominik Röttsches, 2014-09-18 13:45:58]

Patchset #12 (id:260001) has been deleted

[Dominik Röttsches, 2014-09-24 07:54:03]

Patchset #13 (id:300001) has been deleted

[eae, 2014-09-26 14:32:08]

LGTM! Awesome job Dominik!

https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/c...
File Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm (right):

https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/c...
Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm:52: shouldAntialias =
shouldAntialias && (!LayoutTestSupport::isRunningLayoutTest() ||
LayoutTestSupport::isFontAntialiasingEnabledForTest());
Nit: you might want to wrap this line.

https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm (right):

https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm:75: WTF_LOG_ERROR("Unable
to retrieve glyph advance for %@ %f", [font displayName], pointSize);
Is this something we want to add an ASSERT for?

https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm:103: // CTFontRef ctFont
= reinterpret_cast<FontPlatformData*>(fontData)->ctFont();
Remove commented out code.

[d.roettsches, 2014-09-26 14:42:43]

On 2014/09/26 14:32:08, eae wrote:
> LGTM! Awesome job Dominik!

Thanks!

>
https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/c...
> File Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm (right):
> 
>
https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/c...
> Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm:52: shouldAntialias =
> shouldAntialias && (!LayoutTestSupport::isRunningLayoutTest() ||
> LayoutTestSupport::isFontAntialiasingEnabledForTest());
> Nit: you might want to wrap this line.

Done.

>
https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/h...
> File Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm (right):
> 
>
https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm:75:
WTF_LOG_ERROR("Unable
> to retrieve glyph advance for %@ %f", [font displayName], pointSize);
> Is this something we want to add an ASSERT for?

As discussed, let's leave it like this for now - taken from the old code.

>
https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm:103: // CTFontRef
ctFont
> = reinterpret_cast<FontPlatformData*>(fontData)->ctFont();
> Remove commented out code.

Removed.

[Dominik Röttsches, 2014-09-26 14:45:51]

dominik.rottsches@intel.com changed reviewers:
+ abarth@chromium.org, dglazkov@chromium.org

[Dominik Röttsches, 2014-09-26 14:45:52]

dglazkov@, abarth@: Could you RS the changes for web/tests? Thanks.

[Dominik Röttsches, 2014-09-29 11:37:44]

dominik.rottsches@intel.com changed reviewers:
+ esprehn@chromium.org, tkent@chromium.org

[Dominik Röttsches, 2014-09-29 11:38:19]

Adding esprehn@, tkent@ - could you help RS'ing the changes for web/tests?
Thanks.

[esprehn, 2014-09-29 20:27:32]

lgtm

https://codereview.chromium.org/175253002/diff/540001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm (right):

https://codereview.chromium.org/175253002/diff/540001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm:70: void
advanceForGlyph(Glyph glyph, const FontPlatformData& platformData, CGSize*
advance) {
static

[Dominik Röttsches, 2014-09-30 08:30:32]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-09-30 08:31:07]

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

[commit-bot: I haz the power, 2014-09-30 08:39:50]

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

[commit-bot: I haz the power, 2014-09-30 08:39:52]

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

[Dominik Röttsches, 2014-09-30 08:42:02]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-09-30 08:42:15]

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

[Dominik Röttsches, 2014-09-30 11:10:17]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-09-30 11:10:22]

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

[commit-bot: I haz the power, 2014-09-30 12:47:36]

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

[commit-bot: I haz the power, 2014-09-30 12:47:39]

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

[Dominik Röttsches, 2014-09-30 13:12:13]

Patchset #29 (id:640001) has been deleted

[Dominik Röttsches, 2014-09-30 13:14:26]

The CQ bit was checked by dominik.rottsches@intel.com

[Dominik Röttsches, 2014-09-30 13:14:30]

The CQ bit was unchecked by dominik.rottsches@intel.com

[Dominik Röttsches, 2014-09-30 13:14:45]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2014-09-30 13:15:41]

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

[commit-bot: I haz the power, 2014-09-30 13:17:16]

Committed patchset #29 (id:660001) as 182920

[Dominik Röttsches, 2014-09-30 13:19:59]

NOTRY=true after discussion on #chromium with sergeyv - previous CQ try run was
all green except a TestExpectations lint failure right at the end.

[Zhenyao Mo, 2014-09-30 17:39:39]

A revert of this CL (patchset #29 id:660001) has been created in
https://codereview.chromium.org/618723003/ by zmo@chromium.org.

The reason for reverting is: Caused a bunch layout tests to timeout on mac

editing/pasteboard/copy-crash.html
fast/events/event-view-toString.html
http/tests/navigation/scrollstate-after-http-equiv-refresh.html
http/tests/navigation/scrollstate-after-location-reload.html.

[Xianzhu, 2015-06-19 16:49:39]

Dominik, could you finish the NeedsManualRebaselines left after this CL and
cleanup/restore the disabled expectations? Thanks.