Implement CSS Emphasis Marks for complex text

Implement CSS Emphasis Marks for complex text

BUG=335552
R=behdad,eae,leviw

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

[Dominik Röttsches, 2014-01-16 16:37:31]

Behdad, I started to implement CSS text emphasis for FontHarfBuzz, we could then
apply the same to FontWin. 

However, I would need some help in only drawing emphasis marks for grapheme
clusters. Maybe you can help finding a good way to do that? (I've put my
findings so far in the review comments.)

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

https://codereview.chromium.org/130433006/diff/1/Source/platform/fonts/harfbu...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:796: printf("Glyph: 0x%x,
Character: 0x%x\n", glyphToAdd, ch);
When running this against:
http://roettsch.es/emphasis_simple.html
(just a simple กำ with CSS emphasis) I get currently get three emphasis marks,
but there should be only one for this cluster.

I get 
Glyph: 0xe3, Character: 0xe01
Glyph: 0x12c, Character: 0xe01
Glyph: 0x114, Character: 0xe01

The way this works on mac, for each run, it goes through the list of glyphs,
get's the character for each glyph and checks whether this matches the
U_GC_M_MASK - returns an empty glyph if it does. Which leads to no emphasis mark
being drawn for that glyph index.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The output on Mac is:
Glyph: 0x4ab, Character: 0xe01

In my patch, I get three emphasis marks, on the mac I get one.

The CSS Text Decoration Module says that there should be one emphasis mark per
character, which in this spec is a synonym for a grapheme cluster.

Behdad - would you have an idea how I can filter here? Would I need to scan
backwards and forwards through glyphToCharacterIndexes here to see whether the
glyph belongs to the same grapheme cluster, or are there better ways to figure
this out?

[Dominik Röttsches, 2014-01-16 17:13:09]

On 2014/01/16 16:37:31, Dominik Röttsches wrote:

> Would I need to scan
> backwards and forwards through glyphToCharacterIndexes here to see whether the
> glyph belongs to the same grapheme cluster, or are there better ways to figure
> this out?

I tried this - but scanning for the next different character doesn't really work
if the next character is the same.

[Dominik Röttsches, 2014-01-17 13:03:07]

Never mind - I think I found a solution.

[Dominik Röttsches, 2014-01-17 14:50:22]

There are still selection repainting issues and in my opinion our emphasis marks
with HarfBuzz are too small (for simple and complex text). I'd like to handle
these in separate issues.

[behdad, 2014-01-19 08:58:23]

On 2014/01/17 13:03:07, Dominik Röttsches wrote:
> Never mind - I think I found a solution.

I've got to say I'm not fully following what you did.

[behdad, 2014-01-19 09:00:43]

On 2014/01/19 08:58:23, behdad wrote:
> On 2014/01/17 13:03:07, Dominik Röttsches wrote:
> > Never mind - I think I found a solution.
> 
> I've got to say I'm not fully following what you did.

Let me think about this a bit more and get back to you mid-week (will be
travelling back to Canada tomorrow).

[Dominik Röttsches, 2014-01-20 09:16:33]

On 2014/01/19 08:58:23, behdad wrote:
> On 2014/01/17 13:03:07, Dominik Röttsches wrote:
> > Never mind - I think I found a solution.
> 
> I've got to say I'm not fully following what you did.

I am doing a similar implementation as on Mac. The shaper glue code gets a
special mode in which it replaces glyphs that are not a full grapheme cluster
with 0. Or in other words: Only grapheme clusters should receive an emphasis
mark, that's in the CSS3 decoration module spec. So, the way I am doing this now
is using the cluster information from HarfBuzz and only returning a non-null
character if the next character's cluster info is different from the current one
or if it's the end of the text run. Hope that helps.

[behdad, 2014-01-28 00:49:19]

Ok let me tell you how I think this should be done correctly, and you can decide
where to go with this.

A bit of terminology first:

  - Grapheme clusters: Area between neighboring by Unicode grapheme boundaries,
and I suppose the same as CSS3 character.  Grapheme boundaries are the same as
cursor positions.

  - HarfBuzz / Uniscribe / ... clusters: minimal group of characters and
corresponding glyphs, that cannot be broken down further.  Both HarfBuzz and
Uniscribe only return full grapheme clusters in each cluster.  Ie. HarfBuzz
cluster boundaries are always grapheme clusters.  This is not 100% true, but
true for all practical matters.

Note that the above two are certainly NOT the same.  Easy example: the "fi"
ligature in Latin, will be returned as one HarfBuzz cluster, having two
characters ('f' and 'i') but quite possible only one glyph (the 'fi' ligature
glyph).  Both ends of the sequence are cursor positions / grapheme boundaries,
but there's also a cursor position / grapheme boundary in the middle of the
ligature (between 'f' and i').  HarfBuzz provides something called "ligature
carets" that can help locate the spatial position of that middle cusor position
in the glyph, but no system currently uses that and we'll leave that out for
now.

So, normally, what systems do to position cursor inside a HarfBuzz cluster is:
count the number of grapheme clusters corresponding to the cluster, and position
cursor positions linearly on the width of the cluster.  In this example, there
are two cursor positions corresponding to this cluster: before 'f', and before
'i' (the one after 'i' belongs to the next cluster by convention), and since
this is a left-to-right run of text, we position the first cursor position to
the left of the glyph, and the second one halfway through the advance width of
glyph.

Note that the cluster may have more than one glyph.  In general, we may have N
characters, corresponding to M glyphs, with K grapheme clusters.  We know K <=
N, but that's the only invariant.  You can also assume M, N, and K are all
non-zero if you are careful.

Now, it looks like you want one emphasis mark per logical grapheme cluster.  Ie.
one in between every two neighboring cursor positions.

HTH,
behdad

[Dominik Röttsches, 2014-01-28 16:26:45]

Behdad, thanks a lot for the detailed instructions! I'll look into implementing
that.

> So, normally, what systems do to position cursor inside a HarfBuzz cluster is:
> count the number of grapheme clusters corresponding to the cluster, and
position
> cursor positions linearly on the width of the cluster.  

How do I identify this relation? So, in a sense a question regarding the
HarfBuzz usage:
The HarfBuzz-cluster info is in glyphInfo.cluster - how do I map this to the
grapheme clusters? Would you have a pointer here?
I'll also look into that and try to find out myself.

Thanks again!

[leviw_travelin_and_unemployed, 2014-01-28 18:17:31]

On 2014/01/28 16:26:45, Dominik Röttsches wrote:
> Behdad, thanks a lot for the detailed instructions!

+1!

[behdad, 2014-01-28 20:45:48]

On 2014/01/28 16:26:45, Dominik Röttsches wrote:

> How do I identify this relation? So, in a sense a question regarding the
> HarfBuzz usage:
> The HarfBuzz-cluster info is in glyphInfo.cluster - how do I map this to the
> grapheme clusters? Would you have a pointer here?
> I'll also look into that and try to find out myself.

Ok, lets start with an example.  Imagine the input is this Unicode sequence:

  0041,0066,0300,0066,0069,005A

That's A,f,COMBINING GRAVE ACCENT,f,i,z.  Here are the indices into the text:

Characters:
0 -> before A
1 -> before first f
2 -> before grave accent
3 -> before second f
4 -> before i
5 -> before z
6 -> after z

With the right font, this gets shaped into four glyphs, with their cluster
mapping and advance widths:

Glyphs:
0 -> glyph A             cluster=0  advance=800
1 -> glyph ffi           cluster=1  advance=2000
2 -> glyph grave accent  cluster=1  advance=0
3 -> glyph z             cluster=5  advance=700

Walking over the glyphs and clusters values we get the following HarfBuzz
clusters, with their combined advance width.  End values are excluded from the
cluster:

HB clusters:
0 -> text-range=0..1 glyph-range=0..1 advance=800  text="A"
1 -> text-range=1..5 glyph-range=1..3 advance=2000 text="f,grave-accent,f,i"
2 -> text-range=5..6 glyph-range=3..4 advance=700  text="z"

Now, we also need to get Unicode graphemes.  You can get that from ICU.  The
Unicode graphemes for this text sequence are:

Grapheme clusters:
0 -> text-range=0..1 text="A"
1 -> text-range=1..3 text="f,grave-accent"
2 -> text-range=3..4 text="f"
3 -> text-range=4..5 text="i"
4 -> text-range=5..6 text="z"

Putting the two together we get:

HB-cluster-0: grapheme-clusters=0..1 text="A"
HB-cluster-1: grapheme-clusters=1..4 text="f,grave-accent,f,i"
HB-cluster-2: grapheme-clusters=4..5 text="z"

Here's number-of-grapheme-clusters and advance-width per HB-cluster:

HB-cluster-0: #grapheme-clusters=1 advance=800
HB-cluster-1: #grapheme-clusters=3 advance=2000
HB-cluster-2: #grapheme-clusters=1 advance=700

As such, we get the following emphasis marks that need to be rendered, with
coordinates relative to left-side of the HB-cluster:

HB-cluster-0:
- One at 400 (.5*800/1)
HB-cluster-1:
- Three at 333, 1000, 1667 (.5*2000/3, 1.5*2000/3, 2.5*2000/3)
HB-cluster-2:
- One at 350 (.5*700/1)

For RTL, it's similar, but in reverse...  When you walk the HB glyphs, the
cluster values go down.

Pango doesn't fully implement this.  It assumes that every Unicode char is one
grapheme.  So it doesn't run the Unicode grapheme cluster thing, so it's buggy
in comparison.  Here's the code anyway:

  https://git.gnome.org/browse/pango/tree/pango/pango-glyph-item.c#n772

HTH!

[Dominik Röttsches, 2014-01-28 22:34:00]

On 2014/01/28 20:45:48, behdad wrote:

> HTH!

Wow, it does help a lot. Alright - I'll look into it. Thanks!!

[Dominik Röttsches, 2014-02-06 18:12:20]

Behdad, I tried to implement the algorithm you described now and it works quite
well (if I understood everything correctly) for the example you provided and a
couple of other cases that I tried (see the new test case).

The implementation generates a synthetic new GlyphBuffer, as an interim
solution. This would allow us to switch to HarfBuzz on Mac with a progression
towards better emphasis mark drawing, instead of a regression of losing emphasis
mark drawing completely. Afterwards, I'd say, we can refactor this into a
solution which would not need the helper-GlyphBuffer.

In this patch, I also changed the emphasis mark drawing in FontFastPath to be
based on advances, not on width. This would be inline with your algorithm.
Before landing, I would split this part out into a separate CL.

Could you and Levi perhaps review this? Appreciate your help!

[behdad, 2014-02-06 18:45:55]

On 2014/02/06 18:12:20, Dominik Röttsches wrote:
> Behdad, I tried to implement the algorithm you described now and it works
quite
> well (if I understood everything correctly) for the example you provided and a
> couple of other cases that I tried (see the new test case).
> 
> The implementation generates a synthetic new GlyphBuffer, as an interim
> solution. This would allow us to switch to HarfBuzz on Mac with a progression
> towards better emphasis mark drawing, instead of a regression of losing
emphasis
> mark drawing completely. Afterwards, I'd say, we can refactor this into a
> solution which would not need the helper-GlyphBuffer.
> 
> In this patch, I also changed the emphasis mark drawing in FontFastPath to be
> based on advances, not on width. This would be inline with your algorithm.
> Before landing, I would split this part out into a separate CL.
> 
> Could you and Levi perhaps review this? Appreciate your help!

Excellent!  I'll get on it.  Thanks!

[behdad, 2014-02-08 02:45:09]

lgtm

Really surprised at how short implementing this actually was.

https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:222: // FIXME: Perhaps turn
this into a classic character break iterator?
What does this mean?

https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:965: clusterEnd = runEnd ?
currentRun->startIndex() + currentRun->numCharacters() :
currentRun->startIndex() + glyphToCharacterIndexes[i + 1];
Looks to me like you can easily merge lines 962..965 with lines 955..957.

https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:970:
glyphBuffer->add(glyphToAdd, currentRun->fontData(),
createGlyphBufferAdvance(glyphAdvanceX, 0));
I think we should try to horizontally center the emphasis mark over the
glyphAdvanceX.

[behdad, 2014-02-08 02:45:09]

lgtm

lgtm

Really surprised at how short implementing this actually was.

https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:222: // FIXME: Perhaps turn
this into a classic character break iterator?
What does this mean?

https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:965: clusterEnd = runEnd ?
currentRun->startIndex() + currentRun->numCharacters() :
currentRun->startIndex() + glyphToCharacterIndexes[i + 1];
Looks to me like you can easily merge lines 962..965 with lines 955..957.

https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:970:
glyphBuffer->add(glyphToAdd, currentRun->fontData(),
createGlyphBufferAdvance(glyphAdvanceX, 0));
I think we should try to horizontally center the emphasis mark over the
glyphAdvanceX.

[leviw_travelin_and_unemployed, 2014-02-10 00:10:37]

Thanks for the great work!

https://codereview.chromium.org/130433006/diff/270001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp (right):

https://codereview.chromium.org/130433006/diff/270001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:203: HarfBuzzShaper shaper(this,
run, ForTextEmphasis); // TODO: forTextEmphasis argument? Nullify space &
separator glyphs
I'm not sure I understand this TODO. Can you make it a little clearer?

https://codereview.chromium.org/130433006/diff/270001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/130433006/diff/270001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:954: for (unsigned i = 0; i <
numGlyphs; ++i) {
This block of code is becoming more complicated than I can follow. Can you
extract a couple handily-named methods to help me understand?

[Dominik Röttsches, 2014-02-11 11:38:32]

On 2014/02/08 02:45:09, behdad wrote:

>
https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:970:
> glyphBuffer->add(glyphToAdd, currentRun->fontData(),
> createGlyphBufferAdvance(glyphAdvanceX, 0));
> I think we should try to horizontally center the emphasis mark over the
> glyphAdvanceX.

Yes, this was and is still handled further up, in FontFastPath. There's the
division for placing it in the middle. As you suggest, I am changing the
placement from half-glyphwidth to half-advance. And that's not split into the
separate CL https://codereview.chromium.org/152093006.

[Dominik Röttsches, 2014-02-11 11:39:27]

Also - thanks to both of you for the positive feedback and your quick reviews!

[Dominik Röttsches, 2014-02-11 13:14:26]

New patch, hopefully most issues addressed:

>
https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:970:
> glyphBuffer->add(glyphToAdd, currentRun->fontData(),
> createGlyphBufferAdvance(glyphAdvanceX, 0));
> I think we should try to horizontally center the emphasis mark over the
> glyphAdvanceX.

It's now rebased and dependent on https://codereview.chromium.org/152093006
which changes to half-advance based placement of the emphasis mark glyphs.
This should also address Behdad's above comment.

>
https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:965: clusterEnd = runEnd ?
> currentRun->startIndex() + currentRun->numCharacters() :
> currentRun->startIndex() + glyphToCharacterIndexes[i + 1];
> Looks to me like you can easily merge lines 962..965 with lines 955..957.

I stared at this for a while and tried to figure out an elegant to merge the two
regions but I didn't find much.

I found one simplification:
clusterEnd = currentCharacterIndex;

The other: I could of course pull the clusterEnd position calculation up and out
of the if (isClusterEnd), but we only need this if
isClusterEnd is true - so I think it makes more sense if we only compute it when
we need it?

I renamed runEnd to isRunEnd to make it clear that it's just a boolean.

If you had other ways of merging in mind, could you please explain them in a bit
more detail?

>
https://codereview.chromium.org/130433006/diff/220001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:222: // FIXME: Perhaps turn
> this into a classic character break iterator?
> What does this mean?

I removed the FIXME and left it as as cursorMovementIterator. I think this fits
with your explanations in the sense that the emphasis mark glyphs should be
drawn between adjacent cursor positions.

The cursorMovementIterator() function in TextBreakIteratorICU.cpp generates a
slightly modified ICU CharacterBreakIterator. (More details in the comment,
codesearch link: http://goo.gl/2fegFF ). I was wondering whether such a
CharacterBreakIterator or the cursorMovementIterator is a better fit for our
case. My conclusion is, the cursorMovementIterator fits.


>
https://codereview.chromium.org/130433006/diff/270001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/FontHarfBuzz.cpp:203: HarfBuzzShaper
shaper(this,
> run, ForTextEmphasis); // TODO: forTextEmphasis argument? Nullify space &
> separator glyphs
> I'm not sure I understand this TODO. Can you make it a little clearer?

This TODO was actually already addressed, sorry for the leftover noise.

>
https://codereview.chromium.org/130433006/diff/270001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:954: for (unsigned i = 0; i
<
> numGlyphs; ++i) {
> This block of code is becoming more complicated than I can follow. Can you
> extract a couple handily-named methods to help me understand?

I did not manage to extract smaller methods out of the new block since most of
the magic here is in index position calculation. I wouldn't see readability
improving by moving these calculations out and passing a lot of numbers as
arguments.

What I did though: I refactored the new block into a new suitably named
function, and left the previous fillGlyphBufferFromHarfBuzzRun function
untouched.

I also added comments trying to explain the algorithm more. And, as mentioned
above, simplified one of the index calculations. 

If we find a time when we're both on IRC, I am happy to walk you through the new
code, if that helps, I can also try to separately post a walkthrough based on
Behdad's example.

[Dominik Röttsches, 2014-02-11 13:38:36]

On 2014/02/10 00:10:37, Levi wrote:

> This block of code is becoming more complicated than I can follow. Can you
> extract a couple handily-named methods to help me understand?

To illustrate how this works, I'll try walking you through the algorithm, for
the string as in Behdad's example:

"Af̀fiZ"

We're iterating (using index i) over the glyphs, with corresponding cluster /
glyphToCharacterIndexes info:

0 -> glyph A             glyphToCharacterIndexes=0  advance=800
1 -> glyph ffi           glyphToCharacterIndexes=1  advance=2000
2 -> glyph grave accent  glyphToCharacterIndexes=1  advance=0
3 -> glyph z             glyphToCharacterIndexes=5  advance=700

HarfBuzz provides a reverse mapping for a glyph to the corresponding beginning
character of the HarfBuzz cluster that it originates from. This is in
glyphToCharacterIndexes[].

At the start, currentCharacterIndex points at A. A's glyphToCharacterIndex is 0,
while the glyphToCharacterIndex of the next glyph after A is 1, so we are at the
end of a cluster, so we set isClusterEnd to true.

Now we need to find out how many graphemes are in this cluster. It's simple in
this case since it's a 1:1:1 situation of one glyph being mapped to one
character and to one grapheme cluster.
clusterStart is the index position of A, clusterEnd is the glyphToCharacterIndex
of the glyph after A, so 1 in this case. We count using countGraphemesInCluster,
which results in 1. This means, the advance that we accumulated for A (summed up
in clusterAdvance += advances[i];) only needs to be distributed to one grapheme
cluster. So we add one placeholder glyph to the glyphbuffer with the full
advance of 800.

We move on by setting clusterStart to the previous clusterEnd and incrementing
i.

Now we're looking at glyph "ffi" with glyphToCharacterIndex = 1. The
glyphToCharacterIndex position of the next glyph after "ffi", "glyph grave
accent", is identical so we're not yet at a clusterEnd. We accumulate the
advance for "ffi" and iterate i to the next glyph "glyph grave accent". Now the
following glyph "z" has a different glyphToCharacterIndex (of 5), this means
we're at the end of the "ffi + grace accent" cluster. The clusterStart is 1, the
clusterEnd is 5. 

Again, we calculate how many grapheme clusters there are in this
HarfBuzz-cluster, which in this case are three:  f̀,f and i. So the advance of
2000 for f̀fi gets linearly distributed among the three grapheme clusters.
Because of the way the placement of the emphasis marks is handled in
drawEmphasisMarks in FontFastPath we need to add 3 placeholder glyphas, each
with an advance of 2000 / 3.

Now we're done with the "ffi" and "glyph grave accent" glyph, again we set
clusterStart to the previous clusterEnd, and iterate i to point at the next
glyph z. 

The case for z is similar to "A". There's a simple glyph to grapheme to
character mapping and we just need to add one placeholder glyph using the full
advance of 700.

HTH.

[behdad, 2014-02-11 18:53:43]

https://codereview.chromium.org/130433006/diff/430001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/130433006/diff/430001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1040: graphemesInCluster =
countGraphemesInCluster(m_run, clusterStart, clusterEnd);
Do you handle the case of this returning -1 or 0?

[behdad, 2014-02-11 20:17:41]

https://codereview.chromium.org/130433006/diff/430001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/130433006/diff/430001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1040: graphemesInCluster =
countGraphemesInCluster(m_run, clusterStart, clusterEnd);
On 2014/02/11 18:53:44, behdad wrote:
> Do you handle the case of this returning -1 or 0?

Ok, I see why you don't care about that right now.  It's problematic though, if,
eg, a combining mark uses a different font than it's base.  But I guess we can
address that with followup fixes.

https://codereview.chromium.org/130433006/diff/430001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1041: float glyphAdvanceX =
clusterAdvance / graphemesInCluster;
To handle some of the cases stated above, I suggest you skip insertion if
clusterAdvance is zero.

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

On 2014/02/11 20:17:41, behdad wrote:
>
https://codereview.chromium.org/130433006/diff/430001/Source/platform/fonts/h...
> File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):
> 
>
https://codereview.chromium.org/130433006/diff/430001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1040: graphemesInCluster =
> countGraphemesInCluster(m_run, clusterStart, clusterEnd);
> On 2014/02/11 18:53:44, behdad wrote:
> > Do you handle the case of this returning -1 or 0?
> 
> Ok, I see why you don't care about that right now.  It's problematic though,
if,
> eg, a combining mark uses a different font than it's base.  But I guess we can
> address that with followup fixes.
> 
>
https://codereview.chromium.org/130433006/diff/430001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:1041: float glyphAdvanceX =
> clusterAdvance / graphemesInCluster;
> To handle some of the cases stated above, I suggest you skip insertion if
> clusterAdvance is zero.

Thanks, good points. I updated the patch to make this case more explicit and
changed countGraphemesInCluster slightly so that it can't return -1, also fixed
choice of types in that function. Should be a bit cleaner now.

[leviw_travelin_and_unemployed, 2014-02-13 01:07:54]

https://codereview.chromium.org/130433006/diff/500001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.h (right):

https://codereview.chromium.org/130433006/diff/500001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.h:61: HarfBuzzShaper(const Font*,
const TextRun&, bool forTextEmphasis = false);
Don't you want to use the enum you defined above, instead of a boolean param?

[Dominik Röttsches, 2014-02-13 15:45:48]

On 2014/02/13 01:07:54, Levi wrote:
>
https://codereview.chromium.org/130433006/diff/500001/Source/platform/fonts/h...
> File Source/platform/fonts/harfbuzz/HarfBuzzShaper.h (right):
> 
>
https://codereview.chromium.org/130433006/diff/500001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.h:61: HarfBuzzShaper(const
Font*,
> const TextRun&, bool forTextEmphasis = false);
> Don't you want to use the enum you defined above, instead of a boolean param?

I took this pattern from the ComplexTextController Mac example, but I'd agree
it's cleaner to use the enum also in the class, so I changed it. Now I ended up
with a bit of legal, but funny looking syntax to avoid a style checker issue.

HarfBuzzShaper(const Font*, const TextRun&, ForTextEmphasisOrNot =
NotForTextEmphasis);

(i.e. default value without parameter name). 

Style checker issue was:

  Source/platform/fonts/harfbuzz/HarfBuzzShaper.h:61:  The parameter name
"forTextEmphasis" adds no information, so it should be removed. 
[readability/parameter_name] [5]

Would this be ready to land from your point of view, now, Levi?

[leviw_travelin_and_unemployed, 2014-02-14 01:31:14]

On 2014/02/13 15:45:48, Dominik Röttsches wrote:
> On 2014/02/13 01:07:54, Levi wrote:
> >
>
https://codereview.chromium.org/130433006/diff/500001/Source/platform/fonts/h...
> > File Source/platform/fonts/harfbuzz/HarfBuzzShaper.h (right):
> > 
> >
>
https://codereview.chromium.org/130433006/diff/500001/Source/platform/fonts/h...
> > Source/platform/fonts/harfbuzz/HarfBuzzShaper.h:61: HarfBuzzShaper(const
> Font*,
> > const TextRun&, bool forTextEmphasis = false);
> > Don't you want to use the enum you defined above, instead of a boolean
param?
> 
> I took this pattern from the ComplexTextController Mac example, but I'd agree
> it's cleaner to use the enum also in the class, so I changed it. Now I ended
up
> with a bit of legal, but funny looking syntax to avoid a style checker issue.
> 
> HarfBuzzShaper(const Font*, const TextRun&, ForTextEmphasisOrNot =
> NotForTextEmphasis);
> 
> (i.e. default value without parameter name). 
> 
> Style checker issue was:
> 
>   Source/platform/fonts/harfbuzz/HarfBuzzShaper.h:61:  The parameter name
> "forTextEmphasis" adds no information, so it should be removed. 
> [readability/parameter_name] [5]
> 
> Would this be ready to land from your point of view, now, Levi?

Yeah, that syntax surprised me at first, but I've grown to like it :)

LGTM. Bombs away!

[leviw_travelin_and_unemployed, 2014-02-14 01:31:20]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-02-14 01:31:34]

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

[commit-bot: I haz the power, 2014-02-14 01:31:36]

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

[commit-bot: I haz the power, 2014-02-14 01:31:37]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 96.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
aa565a3508331e387938dd54adf466858fdde64b..6366dcc8865f799cd17394a5dd880a18542ed174
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -96,6 +96,7 @@ crbug.com/306222
virtual/gpu/fast/hidpi/image-srcset-relative-svg-canvas-2x.html
 
 crbug.com/335552 fast/text/emphasis.html [ NeedsRebaseline ]
 crbug.com/335552 fast/text/decorations-with-text-combine.html [ NeedsRebaseline
]
+crbug.com/335552 fast/text/emphasis-complex.html [ NeedsRebaseline ]
 
 crbug.com/304953 fast/css/font-face-download-error.html [ Pass Timeout ]

[Dominik Röttsches, 2014-02-14 11:01:44]

On 2014/02/14 01:31:20, Levi wrote:
> The CQ bit was checked by mailto:leviw@chromium.org

Thanks for the review and submitting to cq. I found one small problem: My
assumption that countGraphemesInCluster only needs 8bit support turned out to be
wrong, since m_run can be 8bit. That showed in fast/text/emphasis.html. So if
you could check the small change for enabling 8bit support in the count function
and cq+ again, that'd be great.

[eae, 2014-02-14 11:13:29]

On 2014/02/14 11:01:44, Dominik Röttsches wrote:
> On 2014/02/14 01:31:20, Levi wrote:
> > The CQ bit was checked by mailto:leviw@chromium.org
> 
> Thanks for the review and submitting to cq. I found one small problem: My
> assumption that countGraphemesInCluster only needs 8bit support turned out to
be
> wrong, since m_run can be 8bit. That showed in fast/text/emphasis.html. So if
> you could check the small change for enabling 8bit support in the count
function
> and cq+ again, that'd be great.

Ah, good catch! Glad the test caught it.

[eae, 2014-02-14 11:15:35]

https://codereview.chromium.org/130433006/diff/640001/Source/platform/fonts/h...
File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/130433006/diff/640001/Source/platform/fonts/h...
Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:225: stringFor8BitRun =
String::make16BitFrom8BitSource(subRun.characters8(), subRun.length());
I don't suppose we could have this method use m_normalizedBuffer instead?
Converting back and forth is quite expensive.

[Dominik Röttsches, 2014-02-14 13:04:14]

On 2014/02/14 11:15:35, eae wrote:
>
https://codereview.chromium.org/130433006/diff/640001/Source/platform/fonts/h...
> File Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp (right):
> 
>
https://codereview.chromium.org/130433006/diff/640001/Source/platform/fonts/h...
> Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp:225: stringFor8BitRun =
> String::make16BitFrom8BitSource(subRun.characters8(), subRun.length());
> I don't suppose we could have this method use m_normalizedBuffer instead?
> Converting back and forth is quite expensive.

Very good point, thanks. For some reason, I was under the impression that the
glyphToCharacterIndex mapping indices are actually pointing to positions in
m_run's characters, but looking at collectHarfBuzzRuns etc, HarfBuzz is
initialized with m_normalizedBuffer indeed, so the indices point to this one.
So, in order for this to be correct even in cases where the buffer is different
after normalization, we actually have to use m_normalizedBuffer. Patch updated.

[Dominik Röttsches, 2014-02-14 13:12:16]

On 2014/02/14 13:04:14, Dominik Röttsches wrote:

> ...collectHarfBuzzRuns...

Sry, _createHarfBuzzRuns_ after your patch, Emil. :-)

[eae, 2014-02-18 16:48:00]

On 2014/02/14 13:12:16, Dominik Röttsches wrote:
> On 2014/02/14 13:04:14, Dominik Röttsches wrote:
> 
> > ...collectHarfBuzzRuns...
> 
> Sry, _createHarfBuzzRuns_ after your patch, Emil. :-)

LGTM!

[Dominik Röttsches, 2014-02-18 18:34:54]

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

[commit-bot: I haz the power, 2014-02-18 18:35:07]

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

[commit-bot: I haz the power, 2014-02-18 20:05:15]

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

[commit-bot: I haz the power, 2014-02-18 20:05:15]

Retried try job too often on win_layout for step(s) webkit_lint
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[Dominik Röttsches, 2014-02-18 21:09:28]

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

[commit-bot: I haz the power, 2014-02-18 21:09:45]

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

[commit-bot: I haz the power, 2014-02-19 00:57:52]

Change committed as 167385

[Dominik Röttsches, 2014-02-19 07:45:59]

On 2014/02/18 16:48:00, eae wrote:

> LGTM!

Thanks, Emil!