Using x-advance for emphasis mark drawing.

Using x-advance for emphasis mark drawing.

According to Behdad's suggestions for complex text CSS emphasis
in https://codereview.chromium.org/130433006#msg11 I would like
to change emphasis glyph placement to half of the x-advance, instead
of half the glyph width. This works better when we land the support
for emphasis marks for complex text with FontHarfBuzz.

BUG=335552
TBR=levi,eae,behdad

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

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

Okay, bots are green now. Would be nice if one of you good review this one, in
preparation for CL #130433006. Thanks!

[behdad, 2014-02-11 16:45:42]

Humm.  Actually, can you explain this a bit?

I suppose we want to center the emphasis glyph in the advance allocated to it. 
Right?  If I understand the code correctly, then you need both the emphasis
glyph width AND the advance set in the buffer.  Am I right that the buffer as we
receive it here is what we filled in earlier with emphasis glyphs and advances?

[behdad, 2014-02-11 16:46:04]

On 2014/02/11 16:45:42, behdad wrote:
> Humm.  Actually, can you explain this a bit?
> 
> I suppose we want to center the emphasis glyph in the advance allocated to it.

> Right?  If I understand the code correctly, then you need both the emphasis
> glyph width AND the advance set in the buffer.  Am I right that the buffer as
we
> receive it here is what we filled in earlier with emphasis glyphs and
advances?

Perhaps just give an example of what we get here in buffer, and what we need to
produce?

[Dominik Röttsches, 2014-02-12 15:08:51]

On 2014/02/11 16:45:42, behdad wrote:
> Humm.  Actually, can you explain this a bit?

> Am I right that the buffer as we
> receive it here is what we filled in earlier with emphasis glyphs and
advances?

Yes. But this is not the only input to this function, we have three types of
inputs here (depending on simple/complex text, or platform):

1) Simple text case: A GlyphBuffer created in
Font::getGlyphsAndAdvancesForSimpleText
   This one is the glyph sequence for simple text, with the glyphs coresponding
to space and separator characters replaced by the ZeroWidthSpace Glyph of the
font, mostly 0.
2) Complex text case, using CoreText: A GlyphBuffer created in
Font::getGlyphsAndAdvancesForComplexText through CoreText
   This one is the glyph sequence for the complex text, with glyphs coresponding
to space and separator characters replaced by 0.
3) Complex text case, using HarfBuzz: A GlyphBuffer created in
Font::getGlyphsAndAdvancesForComplexText through HarfBuzz
   Our synthetic glyph buffer which is now independent and disconnected from the
glyph sequence, for addressing the case of
   distributing the advance to 2 or more grapheme clusters. So here it only
contains 1 or 0 as the synthetic glyphs, and we can't
   measure any individual glyph width any more since we only receive the
placeholder glyph "1".

So, before, the code was measuring glyph width and emphasis mark width and place
the emphasis mark above the middle of the glyph width. 

What I am suggesting with this patch: Instead of placing the emphasis mark on
top of the middle of the glyph width, we should place it above the middle of the
advance for the current glyph, since this allows us to use it for case 3. And I
can't find anything in the spec that would prohibit this - since the spec anyway
talks about grapheme clusters, not glyphs. A minimal change to the previous
approach.

> I suppose we want to center the emphasis glyph in the advance allocated to it.

> Right?

Yes.

> If I understand the code correctly, then you need both the emphasis
> glyph width AND the advance set in the buffer.  

Yes, you still need to know the emphasis mark glyph width and you need to know
the advances of the text that will receive the emphasis mark. Before my patch
you also needed to know the individual glyph widths of the text that receives
the emphasis marks, and that is what I'd like to change. 

The FIXME in the other patch is intended to refactor all this, so that we don't
need these temporary, not so meaningful GlyphBuffers any more for drawing
emphasis marks, but I suggest to that once we have switched to HarfBuzz on Mac.

HTH.

[behdad, 2014-02-12 19:36:11]

On 2014/02/12 15:08:51, Dominik Röttsches wrote:
> On 2014/02/11 16:45:42, behdad wrote:
> > Humm.  Actually, can you explain this a bit?
> 
> > Am I right that the buffer as we
> > receive it here is what we filled in earlier with emphasis glyphs and
> advances?
> 
> Yes. But this is not the only input to this function, we have three types of
> inputs here (depending on simple/complex text, or platform):
> 
> 1) Simple text case: A GlyphBuffer created in
> Font::getGlyphsAndAdvancesForSimpleText
>    This one is the glyph sequence for simple text, with the glyphs
coresponding
> to space and separator characters replaced by the ZeroWidthSpace Glyph of the
> font, mostly 0.
> 2) Complex text case, using CoreText: A GlyphBuffer created in
> Font::getGlyphsAndAdvancesForComplexText through CoreText
>    This one is the glyph sequence for the complex text, with glyphs
coresponding
> to space and separator characters replaced by 0.
> 3) Complex text case, using HarfBuzz: A GlyphBuffer created in
> Font::getGlyphsAndAdvancesForComplexText through HarfBuzz
>    Our synthetic glyph buffer which is now independent and disconnected from
the
> glyph sequence, for addressing the case of
>    distributing the advance to 2 or more grapheme clusters. So here it only
> contains 1 or 0 as the synthetic glyphs, and we can't
>    measure any individual glyph width any more since we only receive the
> placeholder glyph "1".
> 
> So, before, the code was measuring glyph width and emphasis mark width and
place
> the emphasis mark above the middle of the glyph width. 
> 
> What I am suggesting with this patch: Instead of placing the emphasis mark on
> top of the middle of the glyph width, we should place it above the middle of
the
> advance for the current glyph, since this allows us to use it for case 3. And
I
> can't find anything in the spec that would prohibit this - since the spec
anyway
> talks about grapheme clusters, not glyphs. A minimal change to the previous
> approach.
> 
> > I suppose we want to center the emphasis glyph in the advance allocated to
it.
> 
> > Right?
> 
> Yes.
> 
> > If I understand the code correctly, then you need both the emphasis
> > glyph width AND the advance set in the buffer.  
> 
> Yes, you still need to know the emphasis mark glyph width and you need to know
> the advances of the text that will receive the emphasis mark. Before my patch
> you also needed to know the individual glyph widths of the text that receives
> the emphasis marks, and that is what I'd like to change. 
> 
> The FIXME in the other patch is intended to refactor all this, so that we
don't
> need these temporary, not so meaningful GlyphBuffers any more for drawing
> emphasis marks, but I suggest to that once we have switched to HarfBuzz on
Mac.
> 
> HTH.

Thanks.  That helps a lot!  lgtm.

[leviw_travelin_and_unemployed, 2014-02-13 01:08:46]

LGTM.

[Dominik Röttsches, 2014-02-13 09:10:11]

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

[commit-bot: I haz the power, 2014-02-13 09:10:24]

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

[commit-bot: I haz the power, 2014-02-13 09:10:27]

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

[commit-bot: I haz the power, 2014-02-13 09:10:27]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 succeeded at 276 (offset -11 lines).
  Hunk #2 FAILED at 1072.
  1 out of 2 hunks FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
94fd2e8b5221d241a6800980ad14dbf0a23a1bc3..ff5d9000014e47ede5e4baf0320fef0f892bdffb
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -287,6 +287,8 @@ crbug.com/318980 [ Linux ]
svg/W3C-SVG-1.1/text-tselect-02-f.svg [ Pass ]
 # Probably just needs new baseline, but image_diff is failing - investigate
that.
 crbug.com/318980 [ Linux x86 ] fast/text/international/hindi-spacing.html [
ImageOnlyFailure ]
 
+crbug.com/335552 fast/text/emphasis.html [ NeedsRebaseline ]
+
 # Untriaged Mavericks failures
 
 crbug.com/314947 [ Mavericks ] fast/scrolling/scrollbar-tickmarks-hittest.html
[ Timeout ]
@@ -1070,7 +1072,7 @@ crbug.com/338148 [ Mac Mavericks SnowLeopard Lion
MountainLion Retina ] editing/
 crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/pasteboard/paste-line-endings-009.html [ NeedsRebaseline ]
 crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css3-text/css3-text-decoration/text-decoration-style.html [ NeedsRebaseline
]
 crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
editing/style/smoosh-styles-003.html [ NeedsRebaseline ]
-crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/text/decorations-with-text-combine.html [ NeedsRebaseline ]
+crbug.com/335552 fast/text/decorations-with-text-combine.html [ NeedsRebaseline
]
 crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/css/line-thickness-underline-strikethrough-overline.html [ NeedsRebaseline
]
 crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/dom/clone-node-dynamic-style.html [ NeedsRebaseline ]
 crbug.com/338148 [ Mac Mavericks SnowLeopard Lion MountainLion Retina ]
fast/repaint/shadow-multiple-vertical.html [ NeedsRebaseline ]

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

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

[commit-bot: I haz the power, 2014-02-13 11:39:33]

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

[commit-bot: I haz the power, 2014-02-13 14:21:17]

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

[commit-bot: I haz the power, 2014-02-13 14:21:18]

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

[Dominik Röttsches, 2014-02-13 15:35:25]

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

[commit-bot: I haz the power, 2014-02-13 15:35:50]

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

[commit-bot: I haz the power, 2014-02-13 16:34:14]

Change committed as 167094

[Julien - ping for review, 2014-02-13 16:42:46]

In the future, please avoid using TBR= in your description, use R= instead.

TBR (To Be Reviewed) is used to bypass reviews for trivial and time-critical
changes (build fix, gardening change, ...). If you put TBR in a change, the cq
will process your change without looking at whether you had any l-g-t-m. Thanks!

[Dominik Röttsches, 2014-02-13 19:35:11]

On 2014/02/13 16:42:46, Julien Chaffraix - PST wrote:
> In the future, please avoid using TBR= in your description, use R= instead.

Will do, sorry. Thanks for the reminder.