Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 617103003: Replace ENABLE_OPENTYPE_VERTICAL implementation with HarfBuzz (Closed)

Created:
5 years, 11 months ago by Dominik Röttsches
Modified:
5 years, 9 months ago
CC:
blink-reviews, jamesr, pdr+graphicswatchlist_chromium.org, jbroman, mkwst+moarreviews_chromium.org, danakj, Rik, Stephen Chennney, krit, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/blink.git@removeOpenTypeVertical
Project:
blink
Visibility:
Public.

Description

Replace ENABLE_OPENTYPE_VERTICAL implementation with HarfBuzz This allows us to remove a large body of redundant code in blink and improves our vertical text shaping results. Sending vertical text through the complex path also finally fixes fast/writing-mode/text-orientation-basic.html. BUG=408992 R=eae,behdad TEST=fast/writing-mode/text-orientation-basic.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186601

Patch Set 1 #

Total comments: 1

Patch Set 2 : Move GC rotation down #

Total comments: 2

Patch Set 3 : Better decision when to rotate canvas fixes displacement failures, vertical selection is still b0rk… #

Patch Set 4 : Rebased on top of the merging craze. #

Patch Set 5 : Contains carry advances fix (627273003) #

Patch Set 6 : Fixing verticalData instantiation in SimpleFontData.cpp (leftover ifdefs) #

Patch Set 7 : Rebased on master, after 627273003 landed #

Patch Set 8 : Rebased on top of offset-only GlyphBuffer #

Patch Set 9 : Retain SimpleFontData's generated from HarfBuzzFace #

Patch Set 10 : Fixing emphasis mark orientation in vertical mode #

Patch Set 11 : Removed spurious newline #

Patch Set 12 : Fix for accessibility failure, range calculation failed after vertical text goes through complex #

Patch Set 13 : Use glyph size for emphasis mark positioning, not baseline offset #

Patch Set 14 : Required rebaselines #

Patch Set 15 : Fix accessibility test case failure only in HarfBuzzShaper #

Patch Set 16 : Include Mac hashing fix (766683004) #

Patch Set 17 : Additional rebaselines for Windows & Mac #

Patch Set 18 : Adding a Mac rebaseline #

Total comments: 1

Patch Set 19 : GCStateSaver instead of CTM manipulation, integrates extended hashing fix from #766683004 #

Patch Set 20 : Rebase on master, now that FontPlatformData hashing fix landed #

Patch Set 21 : Additional rebaselines for slimmingpaint testsuite #

Patch Set 22 : Additional windows rebaselines for slimmingpaint testsuite #

Patch Set 23 : Additional TestExpectations tweaking for Mac #

Unified diffs Side-by-side diffs Delta from patch set Stats (+191 lines, -441 lines) Patch
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 4 chunks +109 lines, -10 lines 0 comments Download
M LayoutTests/fast/writing-mode/text-orientation-basic.html View 1 2 3 4 5 6 7 1 chunk +3 lines, -4 lines 0 comments Download
M Source/build/features.gypi View 1 2 3 4 5 6 7 1 chunk +0 lines, -1 line 0 comments Download
M Source/platform/fonts/Font.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 7 chunks +29 lines, -57 lines 0 comments Download
M Source/platform/fonts/FontCache.h View 1 2 3 4 5 6 7 3 chunks +1 line, -3 lines 0 comments Download
M Source/platform/fonts/FontCache.cpp View 4 chunks +0 lines, -4 lines 0 comments Download
M Source/platform/fonts/FontDataCache.cpp View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/platform/fonts/FontFaceCreationParams.h View 1 chunk +1 line, -1 line 0 comments Download
M Source/platform/fonts/FontPlatformData.h View 1 2 3 4 5 6 7 1 chunk +0 lines, -2 lines 0 comments Download
M Source/platform/fonts/FontPlatformData.cpp View 1 2 3 4 5 6 7 16 17 18 19 20 21 22 2 chunks +0 lines, -2 lines 0 comments Download
M Source/platform/fonts/GlyphPageTreeNode.cpp View 1 2 3 4 5 6 7 1 chunk +0 lines, -4 lines 0 comments Download
M Source/platform/fonts/SimpleFontData.h View 1 2 3 4 5 6 7 3 chunks +1 line, -10 lines 0 comments Download
M Source/platform/fonts/SimpleFontData.cpp View 1 2 3 4 5 6 7 2 chunks +0 lines, -6 lines 0 comments Download
M Source/platform/fonts/opentype/OpenTypeVerticalData.h View 3 chunks +3 lines, -7 lines 0 comments Download
M Source/platform/fonts/opentype/OpenTypeVerticalData.cpp View 1 2 3 4 5 6 7 8 9 10 6 chunks +0 lines, -313 lines 0 comments Download
M Source/platform/fonts/shaping/HarfBuzzFace.cpp View 1 2 3 4 5 6 7 8 6 chunks +33 lines, -1 line 0 comments Download
M Source/platform/fonts/shaping/HarfBuzzShaper.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 5 chunks +11 lines, -10 lines 0 comments Download
M Source/web/tests/OpenTypeVerticalDataTest.cpp View 2 chunks +0 lines, -4 lines 0 comments Download

Messages

Total messages: 42 (14 generated)
Dominik Röttsches
I want to remove HarfBuzzFaceCoreText before landing this - but generally I think it's ready ...
5 years, 11 months ago (2014-10-01 15:33:58 UTC) #1
eae
Yay! LGTM w/nits. https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right): https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp#newcode132 Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:132: if (drawVertically) { Could you move ...
5 years, 11 months ago (2014-10-01 15:39:50 UTC) #2
Dominik Röttsches
On 2014/10/01 15:39:50, eae wrote: > Yay! LGTM w/nits. > > https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp > File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp ...
5 years, 11 months ago (2014-10-01 15:53:19 UTC) #3
Dominik Röttsches
(and thanks for the quick review!)
5 years, 11 months ago (2014-10-01 15:53:40 UTC) #4
eae
On 2014/10/01 15:53:19, Dominik Röttsches wrote: > On 2014/10/01 15:39:50, eae wrote: > > Yay! ...
5 years, 11 months ago (2014-10-01 15:54:16 UTC) #5
eae
Much better, thank you.
5 years, 11 months ago (2014-10-01 15:54:55 UTC) #6
behdad_google
lgtm https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/SimpleFontData.h File Source/platform/fonts/SimpleFontData.h (right): https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/SimpleFontData.h#newcode270 Source/platform/fonts/SimpleFontData.h:270: width = m_verticalData->advanceHeight(this, glyph) + m_syntheticBoldOffset; We shouldn't ...
5 years, 11 months ago (2014-10-01 16:02:30 UTC) #8
d.roettsches
https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/SimpleFontData.h File Source/platform/fonts/SimpleFontData.h (right): https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/SimpleFontData.h#newcode270 Source/platform/fonts/SimpleFontData.h:270: width = m_verticalData->advanceHeight(this, glyph) + m_syntheticBoldOffset; You mean the ...
5 years, 11 months ago (2014-10-01 17:05:01 UTC) #10
behdad_google
On 2014/10/01 17:05:01, d.roettsches wrote: > https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/SimpleFontData.h > File Source/platform/fonts/SimpleFontData.h (right): > > https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/SimpleFontData.h#newcode270 > ...
5 years, 11 months ago (2014-10-01 17:10:30 UTC) #11
d.roettsches
Still a good bunch of failures, need to check what I missed there.
5 years, 11 months ago (2014-10-01 17:14:18 UTC) #12
Dominik Röttsches
Rebased on top of the merging craze.
5 years, 11 months ago (2014-10-03 13:14:33 UTC) #13
Dominik Röttsches
One of the reasons for the selection painting problems is that we still have some ...
5 years, 11 months ago (2014-10-06 15:21:33 UTC) #14
Dominik Röttsches
Needs to land after https://codereview.chromium.org/627273003 and selection issues are fixed. However, Behdad, could you take ...
5 years, 11 months ago (2014-10-07 15:47:32 UTC) #15
Dominik Röttsches
Never mind - there was an issue with verticalData in SimpleFontData not getting instantiated due ...
5 years, 11 months ago (2014-10-07 16:28:21 UTC) #16
behdad_google
lgtm
5 years, 11 months ago (2014-10-07 19:56:47 UTC) #17
Dominik Röttsches
Punctuation issues pending: https://github.com/behdad/harfbuzz/issues/63
5 years, 11 months ago (2014-10-08 14:07:11 UTC) #18
Dominik Röttsches
Still needs fixing of emphasis mark orientation. Should always be upright according to CSS3 Font ...
5 years, 10 months ago (2014-11-24 16:44:00 UTC) #19
Dominik Röttsches
kbr@, pfeldman@, tkent@: Could you review/rs the modification to Source/web/tests/OpenTypeVerticalDataTest.cpp - thanks.
5 years, 9 months ago (2014-11-25 14:12:24 UTC) #22
Ken Russell (switch to Gerrit)
Source/web/ test change LGTM
5 years, 9 months ago (2014-11-25 20:56:36 UTC) #23
fs
https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/Font.cpp File Source/platform/fonts/Font.cpp (right): https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/Font.cpp#newcode813 Source/platform/fonts/Font.cpp:813: AffineTransform savedMatrix = gc->getCTM(); In https://codereview.chromium.org/748863004/ this getCTM/setCTM pair ...
5 years, 9 months ago (2014-12-03 09:40:39 UTC) #27
Dominik Röttsches
On 2014/12/03 09:40:39, fs wrote: > https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/Font.cpp > File Source/platform/fonts/Font.cpp (right): > > https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/Font.cpp#newcode813 > ...
5 years, 9 months ago (2014-12-03 12:55:16 UTC) #28
fs
On 2014/12/03 12:55:16, Dominik Röttsches wrote: > On 2014/12/03 09:40:39, fs wrote: > > > ...
5 years, 9 months ago (2014-12-03 13:02:15 UTC) #29
Dominik Röttsches
> It only saves one - in this case the 'false' argument to the constructor ...
5 years, 9 months ago (2014-12-03 13:06:33 UTC) #30
eae
Still LGTM, thanks for sticking with it!
5 years, 9 months ago (2014-12-03 16:42:38 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/617103003/480001
5 years, 9 months ago (2014-12-03 21:50:40 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/617103003/480001
5 years, 9 months ago (2014-12-05 15:44:51 UTC) #40
commit-bot: I haz the power
Committed patchset #23 (id:480001) as https://src.chromium.org/viewvc/blink?view=rev&revision=186601
5 years, 9 months ago (2014-12-05 16:34:04 UTC) #41
eae
5 years, 9 months ago (2014-12-05 17:19:03 UTC) #42
Message was sent while issue was closed.
Nice!

Powered by Google App Engine
This is Rietveld 408576698