Replace ENABLE_OPENTYPE_VERTICAL implementation with HarfBuzz

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

[Dominik Röttsches, 2014-10-01 15:33:58]

I want to remove HarfBuzzFaceCoreText before landing this - but generally I
think it's ready for review.

[eae, 2014-10-01 15:39:50]

Yay! LGTM w/nits.

https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbu...
File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right):

https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbu...
Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:132: if (drawVertically) {
Could you move this down to line 150 so that it is before the no-offset
implementation?

[Dominik Röttsches, 2014-10-01 15:53:19]

On 2014/10/01 15:39:50, eae wrote:
> Yay! LGTM w/nits.
> 
>
https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbu...
> File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right):
> 
>
https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbu...
> Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:132: if (drawVertically) {
> Could you move this down to line 150 so that it is before the no-offset
> implementation?

Sure - that should work, since we always upgrade to complex. hasOffsets() is
equivalent to "was shaped with HB", right? Or is there a case where the
GlyphBuffer comes out of HBShaper without offsets?

[Dominik Röttsches, 2014-10-01 15:53:40]

(and thanks for the quick review!)

[eae, 2014-10-01 15:54:16]

On 2014/10/01 15:53:19, Dominik Röttsches wrote:
> On 2014/10/01 15:39:50, eae wrote:
> > Yay! LGTM w/nits.
> > 
> >
>
https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbu...
> > File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right):
> > 
> >
>
https://codereview.chromium.org/617103003/diff/1/Source/platform/fonts/harfbu...
> > Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:132: if (drawVertically) {
> > Could you move this down to line 150 so that it is before the no-offset
> > implementation?
> 
> Sure - that should work, since we always upgrade to complex. hasOffsets() is
> equivalent to "was shaped with HB", right? Or is there a case where the
> GlyphBuffer comes out of HBShaper without offsets?

Correct.

[eae, 2014-10-01 15:54:55]

Much better, thank you.

[behdad_google, 2014-10-01 16:02:29]

behdad@google.com changed reviewers:
+ behdad@google.com

[behdad_google, 2014-10-01 16:02:30]

lgtm

https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/Si...
File Source/platform/fonts/SimpleFontData.h (right):

https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/Si...
Source/platform/fonts/SimpleFontData.h:270: width =
m_verticalData->advanceHeight(this, glyph) + m_syntheticBoldOffset;
We shouldn't need any of these, right?  We don't want the vertical support in
the simple path.

[d.roettsches, 2014-10-01 17:05:00]

d.roettsches@gmail.com changed reviewers:
+ d.roettsches@gmail.com

[d.roettsches, 2014-10-01 17:05:01]

https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/Si...
File Source/platform/fonts/SimpleFontData.h (right):

https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/Si...
Source/platform/fonts/SimpleFontData.h:270: width =
m_verticalData->advanceHeight(this, glyph) + m_syntheticBoldOffset;
You mean the whole "else if (m_verticalData)" branch can go away here since we
don't need the vertical metrics in the simple path and widthForGlyph is the
function they were previously access through? 

You're probably right. I am going to test removing the branch.

[behdad_google, 2014-10-01 17:10:30]

On 2014/10/01 17:05:01, d.roettsches wrote:
>
https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/Si...
> File Source/platform/fonts/SimpleFontData.h (right):
> 
>
https://codereview.chromium.org/617103003/diff/20001/Source/platform/fonts/Si...
> Source/platform/fonts/SimpleFontData.h:270: width =
> m_verticalData->advanceHeight(this, glyph) + m_syntheticBoldOffset;
> You mean the whole "else if (m_verticalData)" branch can go away here since we
> don't need the vertical metrics in the simple path and widthForGlyph is the
> function they were previously access through? 
> 
> You're probably right. I am going to test removing the branch.

Yes, something like that.  I don't think anything vertical-related should be
left in that file (other than the m_verticalData itself).

[d.roettsches, 2014-10-01 17:14:18]

Still a good bunch of failures, need to check what I missed there.

[Dominik Röttsches, 2014-10-03 13:14:33]

Rebased on top of the merging craze.

[Dominik Röttsches, 2014-10-06 15:21:33]

One of the reasons for the selection painting problems is that we still have
some tricky bugs in HarfBuzzShaper::fillGlyphBufferFromHarfBuzzRun, when
setDrawRange was used with non trivial values. As a result, the advances are
glyph sized, which seems reasonable, but the offsets are pointing very far way
from the origin points specified by the advances. I think for the initial
painted glyph specified by m_fromIndex we need to add up all the advanceSoFar,
but also add it to its offset. Then, for the consecutive ones, we shouldn't need
to add anything any more. 
But still needs more investigation, this doesn't fix all cases.

[Dominik Röttsches, 2014-10-07 15:47:32]

Needs to land after https://codereview.chromium.org/627273003 and selection
issues are fixed.

However, Behdad, could you take a look at the Linux failures for example - do
you think the CJK glyphs are placed too low? I wonder why they shift down a
little bit.
When I print from drawGlyphs for test case
vertical-lr-ltr-extend-line-forward-br.html, I get:

Point x: 108.000000 y: 165.000000  <- the incoming placement coordinate, then
for a run of some CJK glyphs
VERTICALLY: pos[0] x: 102.000000 y: 205.000000
VERTICALLY: pos[1] x: 102.000000 y: 245.000000
VERTICALLY: pos[2] x: 102.000000 y: 285.000000
VERTICALLY: pos[3] x: 102.000000 y: 325.000000

So, they are equally spaced at 40px advances, the first glyph origin being
102,205.

Skia docs say drawPosText receives origin coordinates:
"Draw the text, with each character/glyph origin specified by the pos[]
 array. The origin is interpreted by the Align setting in the paint."

Is the pos[i] that we give to Skia the lower right corner of the glyph box, or
the left corner of the glyph box intersecting with the alphabatic baseline? (if
that question makes sense) Are we missing some y offset correction somehow?

If I paint some debug objects, a 3x3 square at the individual glyph coordinates,
plus a 1x60 horizontal square at the glyph coordinates, we can see that the CJK
glyphs actually extend below the debug line. 
http://roettsch.es/vertical_glyph_placement.png

That seems wrong to me. It seems like the CJK glyphs are drawn like they would
be when places on an alphabetic baseline, e.g. when mixed with Latin text. Do we
need to correct by some baseline height value here? Maybe you have a suggestion.
Thanks.

[Dominik Röttsches, 2014-10-07 16:28:21]

Never mind - there was an issue with verticalData in SimpleFontData not getting
instantiated due to leftover #ifdefs :-(. Let's wait for another bot run.

[behdad_google, 2014-10-07 19:56:47]

lgtm

[Dominik Röttsches, 2014-10-08 14:07:11]

Punctuation issues pending: https://github.com/behdad/harfbuzz/issues/63

[Dominik Röttsches, 2014-11-24 16:44:00]

Still needs fixing of emphasis mark orientation. Should always be upright
according to CSS3 Font Decoration Module:
"Marks must remain upright in vertical writing modes: like CJK characters, they
do not rotate to match the writing mode."
http://dev.w3.org/csswg/css-text-decor-3/#text-emphasis-style-property

[Dominik Röttsches, 2014-11-25 14:10:12]

dominik.rottsches@intel.com changed reviewers:
+ fmalita@google.com
- d.roettsches@gmail.com

[Dominik Röttsches, 2014-11-25 14:12:23]

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

[Dominik Röttsches, 2014-11-25 14:12:24]

kbr@, pfeldman@, tkent@: Could you review/rs the modification to
Source/web/tests/OpenTypeVerticalDataTest.cpp - thanks.

[Ken Russell (switch to Gerrit), 2014-11-25 20:56:36]

Source/web/ test change LGTM

[Dominik Röttsches, 2014-11-26 13:46:58]

Patchset #13 (id:240001) has been deleted

[Dominik Röttsches, 2014-11-26 13:47:15]

Patchset #12 (id:220001) has been deleted

[fs, 2014-12-03 09:40:37]

fs@opera.com changed reviewers:
+ fs@opera.com

[fs, 2014-12-03 09:40:39]

https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:813: AffineTransform savedMatrix = gc->getCTM();
In https://codereview.chromium.org/748863004/ this getCTM/setCTM pair (at its
"previous location" that is) was replaced by a GraphicsContextStateSaver, so
this needs to be updated to something like:

GraphicsContextStateSaver stateSaver(*gc, false);
if (drawVertically) {
    stateSaver.save();
    gc->concatCTM(...);
    ...
}

(and the call to setCTM can be dropped.)

[Dominik Röttsches, 2014-12-03 12:55:16]

On 2014/12/03 09:40:39, fs wrote:
>
https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/F...
> File Source/platform/fonts/Font.cpp (right):
> 
>
https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/F...
> Source/platform/fonts/Font.cpp:813: AffineTransform savedMatrix =
gc->getCTM();
> In https://codereview.chromium.org/748863004/ this getCTM/setCTM pair (at its
> "previous location" that is) was replaced by a GraphicsContextStateSaver, so
> this needs to be updated to something like:
> [...]

Thanks for the heads up, I'll rebase. Just to confirm: The
GraphicsContextStateSaver pops all saved states when it goes out of scope? The
first one being when it is created, the second when you call save() on it?

[fs, 2014-12-03 13:02:15]

On 2014/12/03 12:55:16, Dominik Röttsches wrote:
> On 2014/12/03 09:40:39, fs wrote:
> >
>
https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/F...
> > File Source/platform/fonts/Font.cpp (right):
> > 
> >
>
https://codereview.chromium.org/617103003/diff/320002/Source/platform/fonts/F...
> > Source/platform/fonts/Font.cpp:813: AffineTransform savedMatrix =
> gc->getCTM();
> > In https://codereview.chromium.org/748863004/ this getCTM/setCTM pair (at
its
> > "previous location" that is) was replaced by a GraphicsContextStateSaver, so
> > this needs to be updated to something like:
> > [...]
> 
> Thanks for the heads up, I'll rebase. Just to confirm: The
> GraphicsContextStateSaver pops all saved states when it goes out of scope?

It pops a (one) saved state when it goes out of scope.

> The first one being when it is created, the second when you call save() on it?

It only saves one - in this case the 'false' argument to the constructor defers
the save until save() is called.

[Dominik Röttsches, 2014-12-03 13:06:33]

> It only saves one - in this case the 'false' argument to the constructor
defers
> the save until save() is called.

Ah, I overlooked that. Thanks!

[Dominik Röttsches, 2014-12-03 14:19:00]

Patchset #19 (id:360001) has been deleted

[Dominik Röttsches, 2014-12-03 15:11:39]

Patchset #19 (id:380001) has been deleted

[eae, 2014-12-03 16:42:38]

Still LGTM, thanks for sticking with it!

[Dominik Röttsches, 2014-12-03 21:50:04]

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

[Dominik Röttsches, 2014-12-03 21:50:04]

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

[Dominik Röttsches, 2014-12-03 21:50:04]

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

[commit-bot: I haz the power, 2014-12-03 21:50:40]

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

[Dominik Röttsches, 2014-12-03 22:09:25]

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

[Dominik Röttsches, 2014-12-05 15:44:32]

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

[commit-bot: I haz the power, 2014-12-05 15:44:51]

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

[commit-bot: I haz the power, 2014-12-05 16:34:04]

Committed patchset #23 (id:480001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186601

[eae, 2014-12-05 17:19:03]

Nice!