Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(325)

Issue 175253002: Switch to HarfBuzz on Mac and remove CoreText shaper (Closed)

Created:
6 years, 10 months ago by Dominik Röttsches
Modified:
5 years, 6 months ago
CC:
blink-reviews, Rik, danakj, krit, dsinclair, jamesr, jbroman, pdr., rwlbuis, Stephen Chennney
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Updated gypi, removed fontDataForCombiningCharacterSequence #

Patch Set 3 : Removing isAATFont check, still not painting #

Patch Set 4 : Connected to HarfBuzz CGFont constructor #

Patch Set 5 : ChromiumAATTest font based test added, header inclusion order fixed #

Total comments: 2

Patch Set 6 : ttf file removed, this is provided by content_shell now #

Patch Set 7 : Updated with Emil's fix suggestion #

Patch Set 8 : Round advances when subpixel positioning is not enabled #

Patch Set 9 : Fixing disappearing text in ellipsis #

Patch Set 10 : New version, removing FontMac and FontComplexTextMac, after merging FontPlatformData(HarfBuzz) #

Patch Set 11 : Antialiasing settings for LayoutTests #

Patch Set 12 : Moving to SimpleFontDataSkia, removing SimpleFontData(Mac|CoreText) (re-up) #

Patch Set 13 : Addressing test failures, fixing vertical #

Patch Set 14 : Adding test case #

Patch Set 15 : Rebased on master, TestExpectations tuned, ready to land? #

Total comments: 3

Patch Set 16 : Review comments addressed #

Patch Set 17 : Try fixing GN build #

Patch Set 18 : More TestExpectation tweaks, postponing looking into ASSERT that seems to happen only on bots #

Patch Set 19 : Rebased TestExpectations #

Patch Set 20 : Adjusting TestExpectations #

Patch Set 21 : Nudging TestExpectations #

Patch Set 22 : Fuse two additional rebaselines #

Patch Set 23 : Rebasing TestExpectations #

Patch Set 24 : Marking word-spacing-characters-complex-text.html as Skip, adding canvas-normalize-string.html reba… #

Total comments: 1

Patch Set 25 : advanceForGlyph -> static #

Patch Set 26 : Marking a few flakes #

Patch Set 27 : emphasis-combined-text.html is marked as failure in trunk #

Patch Set 28 : emphasis-combined-text.html ftw #

Patch Set 29 : Fighting trunk TestExpectation changes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7662 lines, -2428 lines) Patch
M LayoutTests/NeverFixTests View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -0 lines 0 comments Download
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 11 chunks +7513 lines, -40 lines 0 comments Download
A LayoutTests/fast/text/aat-morx.html View 1 2 3 4 13 1 chunk +10 lines, -0 lines 0 comments Download
A LayoutTests/platform/mac/virtual/antialiasedtext/fast/text/aat-morx-expected.png View 1 2 3 4 5 6 7 8 13 Binary file 0 comments Download
A LayoutTests/platform/mac/virtual/antialiasedtext/fast/text/aat-morx-expected.txt View 1 2 3 4 5 6 7 8 13 1 chunk +12 lines, -0 lines 0 comments Download
M Source/build/features.gypi View 1 2 3 4 5 6 7 8 9 2 chunks +1 line, -6 lines 0 comments Download
M Source/platform/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +0 lines, -4 lines 0 comments Download
M Source/platform/blink_platform.gyp View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +9 lines, -4 lines 0 comments Download
M Source/platform/blink_platform.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -8 lines 0 comments Download
M Source/platform/fonts/Font.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -3 lines 0 comments Download
M Source/platform/fonts/FontPlatformData.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -2 lines 0 comments Download
M Source/platform/fonts/FontPlatformData.cpp View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +32 lines, -29 lines 0 comments Download
M Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +27 lines, -15 lines 0 comments Download
D Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +0 lines, -146 lines 0 comments Download
A + Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 3 chunks +46 lines, -31 lines 0 comments Download
M Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +0 lines, -1 line 0 comments Download
D Source/platform/fonts/mac/ComplexTextController.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -180 lines 0 comments Download
D Source/platform/fonts/mac/ComplexTextController.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -614 lines 0 comments Download
D Source/platform/fonts/mac/ComplexTextControllerCoreText.mm View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -297 lines 0 comments Download
D Source/platform/fonts/mac/FontComplexTextMac.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +0 lines, -209 lines 0 comments Download
D Source/platform/fonts/mac/FontMac.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -182 lines 0 comments Download
D Source/platform/fonts/mac/SimpleFontDataCoreText.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -201 lines 0 comments Download
M Source/platform/fonts/mac/SimpleFontDataMac.mm View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +0 lines, -454 lines 0 comments Download
M Source/platform/fonts/skia/SimpleFontDataSkia.cpp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M Source/web/tests/PinchViewportTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -1 line 0 comments Download
M Source/web/tests/WebFrameTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 52 (14 generated)
Dominik Röttsches
(Not ready for review yet, but) Together with cl 203123003 this is now working for ...
6 years, 9 months ago (2014-03-18 14:58:59 UTC) #1
behdad
lgtm Neat!
6 years, 9 months ago (2014-03-18 17:19:00 UTC) #2
Dominik Röttsches
On 2014/03/18 17:19:00, behdad wrote: > lgtm > > Neat! Thanks! Emil, do you wanna ...
6 years, 9 months ago (2014-03-20 18:11:23 UTC) #3
eae
On 2014/03/20 18:11:23, Dominik Röttsches wrote: > On 2014/03/18 17:19:00, behdad wrote: > > lgtm ...
6 years, 9 months ago (2014-03-20 18:15:40 UTC) #4
eae
https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/mac/FontComplexTextMac.cpp File Source/platform/fonts/mac/FontComplexTextMac.cpp (right): https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/mac/FontComplexTextMac.cpp#newcode41 Source/platform/fonts/mac/FontComplexTextMac.cpp:41: Do we still need this file? It doesn't look ...
6 years, 9 months ago (2014-03-20 18:20:30 UTC) #5
Dominik Röttsches
On 2014/03/20 18:20:30, eae wrote: > https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/mac/FontComplexTextMac.cpp > File Source/platform/fonts/mac/FontComplexTextMac.cpp (right): > > https://codereview.chromium.org/175253002/diff/100001/Source/platform/fonts/mac/FontComplexTextMac.cpp#newcode41 > ...
6 years, 9 months ago (2014-03-20 18:27:53 UTC) #6
eae
On 2014/03/20 18:27:53, Dominik Röttsches wrote: > On 2014/03/20 18:20:30, eae wrote: > > > ...
6 years, 9 months ago (2014-03-20 18:28:44 UTC) #7
Dominik Röttsches
The CQ bit was checked by dominik.rottsches@intel.com
6 years, 9 months ago (2014-03-21 08:45:02 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dominik.rottsches@intel.com/175253002/120001
6 years, 9 months ago (2014-03-21 08:45:11 UTC) #9
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-21 09:21:17 UTC) #10
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.blink on mac_blink_rel
6 years, 9 months ago (2014-03-21 09:21:17 UTC) #11
Dominik Röttsches
On 2014/03/21 09:21:17, I haz the power (commit-bot) wrote: > Try jobs failed on following ...
6 years, 9 months ago (2014-03-21 10:44:09 UTC) #12
behdad
On 2014/03/21 10:44:09, Dominik Röttsches wrote: > On 2014/03/21 09:21:17, I haz the power (commit-bot) ...
6 years, 9 months ago (2014-03-21 17:12:05 UTC) #13
behdad
On 2014/03/21 17:12:05, behdad wrote: > On 2014/03/21 10:44:09, Dominik Röttsches wrote: > > On ...
6 years, 9 months ago (2014-03-21 17:13:32 UTC) #14
behdad
On 2014/03/21 17:13:32, behdad wrote: > On 2014/03/21 17:12:05, behdad wrote: > > On 2014/03/21 ...
6 years, 9 months ago (2014-03-21 17:17:26 UTC) #15
eae
On 2014/03/21 17:17:26, behdad wrote: > On 2014/03/21 17:13:32, behdad wrote: > > On 2014/03/21 ...
6 years, 8 months ago (2014-04-03 16:59:46 UTC) #16
eae
Changing Font::drawComplexText to call adjustStartPoint fixes a few of the tests FloatPoint adjustedPoint = shaper.adjustStartPoint(point); ...
6 years, 8 months ago (2014-04-03 17:30:21 UTC) #17
Dominik Röttsches
On 2014/04/03 17:30:21, eae wrote: > Changing Font::drawComplexText to call adjustStartPoint fixes a few of ...
6 years, 8 months ago (2014-04-04 11:57:32 UTC) #18
Dominik Röttsches
Okay, I am going all in and removing FontMac and FontComplexTextMac as well, I have ...
6 years, 3 months ago (2014-08-29 16:42:35 UTC) #19
eae
On 2014/08/29 16:42:35, Dominik Röttsches wrote: > Okay, I am going all in and removing ...
6 years, 3 months ago (2014-08-29 19:07:54 UTC) #20
jungshik at Google
On 2014/08/29 16:42:35, Dominik Röttsches wrote: > Okay, I am going all in and removing ...
6 years, 3 months ago (2014-08-29 22:03:56 UTC) #21
Dominik Röttsches
On 2014/08/29 19:07:54, eae wrote: > Awesome! Is there anything I can do to help? ...
6 years, 3 months ago (2014-09-01 14:24:32 UTC) #22
eae
On 2014/09/01 14:24:32, Dominik Röttsches wrote: > On 2014/08/29 19:07:54, eae wrote: > > Awesome! ...
6 years, 3 months ago (2014-09-02 18:17:06 UTC) #23
eae
LGTM! Awesome job Dominik! https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm File Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm (right): https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm#newcode52 Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm:52: shouldAntialias = shouldAntialias && (!LayoutTestSupport::isRunningLayoutTest() ...
6 years, 2 months ago (2014-09-26 14:32:08 UTC) #27
d.roettsches
On 2014/09/26 14:32:08, eae wrote: > LGTM! Awesome job Dominik! Thanks! > https://codereview.chromium.org/175253002/diff/360001/Source/platform/fonts/cocoa/FontPlatformDataCocoa.mm > File ...
6 years, 2 months ago (2014-09-26 14:42:43 UTC) #28
Dominik Röttsches
dglazkov@, abarth@: Could you RS the changes for web/tests? Thanks.
6 years, 2 months ago (2014-09-26 14:45:52 UTC) #30
Dominik Röttsches
Adding esprehn@, tkent@ - could you help RS'ing the changes for web/tests? Thanks.
6 years, 2 months ago (2014-09-29 11:38:19 UTC) #32
esprehn
lgtm https://codereview.chromium.org/175253002/diff/540001/Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm File Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm (right): https://codereview.chromium.org/175253002/diff/540001/Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm#newcode70 Source/platform/fonts/harfbuzz/HarfBuzzFaceCoreText.mm:70: void advanceForGlyph(Glyph glyph, const FontPlatformData& platformData, CGSize* advance) ...
6 years, 2 months ago (2014-09-29 20:27:32 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/175253002/580001
6 years, 2 months ago (2014-09-30 08:31:07 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/16335)
6 years, 2 months ago (2014-09-30 08:39:52 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/175253002/600001
6 years, 2 months ago (2014-09-30 08:42:15 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/175253002/620001
6 years, 2 months ago (2014-09-30 11:10:22 UTC) #41
commit-bot: I haz the power
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)
6 years, 2 months ago (2014-09-30 12:47:39 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/175253002/660001
6 years, 2 months ago (2014-09-30 13:15:41 UTC) #48
commit-bot: I haz the power
Committed patchset #29 (id:660001) as 182920
6 years, 2 months ago (2014-09-30 13:17:16 UTC) #49
Dominik Röttsches
NOTRY=true after discussion on #chromium with sergeyv - previous CQ try run was all green ...
6 years, 2 months ago (2014-09-30 13:19:59 UTC) #50
Zhenyao Mo
A revert of this CL (patchset #29 id:660001) has been created in https://codereview.chromium.org/618723003/ by zmo@chromium.org. ...
6 years, 2 months ago (2014-09-30 17:39:39 UTC) #51
Xianzhu
5 years, 6 months ago (2015-06-19 16:49:39 UTC) #52
Message was sent while issue was closed.
Dominik, could you finish the NeedsManualRebaselines left after this CL and
cleanup/restore the disabled expectations? Thanks.

Powered by Google App Engine
This is Rietveld 408576698