Simplify text emphasis painting

Simplify text emphasis painting

We're currently drawing emphasis marks in two passes:

  * first pass performs regular text shaping, generating a synthetic
    glyph buffer with real text glyphs and center-aligned offsets

  * in a second pass we generate the actual emphasis mark glyph buffer,
    substituting mark glyphs for the previous text glyphs

This is unnecessarily convoluted: after switching to offset-based glyph
buffers, we can substitute emphasis marks glyphs on the fly and generate
the final buffer in a single pass.

The CL adds the necessary plumbing and updates the shapers to perform
emphasis mark glyph substitution when needed.

R=eae@choromium.org,jbroman@chromium.org,kojii@chromium.org

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

[jbroman, 2015-03-07 18:50:25]

jbroman@chromium.org changed reviewers:
+ eae@chromium.org
- eae@choromium.org

[jbroman, 2015-03-07 18:50:25]

lgtm with nits

Sorry it took me so long to get to this. It's great.

[aside: changing reviewer eae@choromium.org to eae@chromium.org]

https://codereview.chromium.org/986493002/diff/1/Source/platform/fonts/Font.h
File Source/platform/fonts/Font.h (right):

https://codereview.chromium.org/986493002/diff/1/Source/platform/fonts/Font.h...
Source/platform/fonts/Font.h:141: float buildGlyphBuffer(const
TextRunPaintInfo&, GlyphBuffer&, const GlyphData* = nullptr) const;
Please include a name for this argument; it's not obvious here that it's for
emphasis.

https://codereview.chromium.org/986493002/diff/1/Source/platform/fonts/shapin...
File Source/platform/fonts/shaping/SimpleShaper.cpp (right):

https://codereview.chromium.org/986493002/diff/1/Source/platform/fonts/shapin...
Source/platform/fonts/shaping/SimpleShaper.cpp:179: } else {
nit: consider

} else if (...) {

to avoid the extra indentation?

[eae, 2015-03-07 19:31:28]

Very nice! LGTM

[kojii, 2015-03-09 08:06:34]

LGTM too, thank you for working on this.

[Dominik Röttsches, 2015-03-09 10:43:04]

Nice, thanks for picking this up! Maybe we can later add a performance test for
this as well.

[f(malita), 2015-03-09 15:04:56]

On 2015/03/07 18:50:25, jbroman wrote:
> [aside: changing reviewer mailto:eae@choromium.org to mailto:eae@chromium.org]

Good catch, thanks! :)

https://codereview.chromium.org/986493002/diff/1/Source/platform/fonts/Font.h
File Source/platform/fonts/Font.h (right):

https://codereview.chromium.org/986493002/diff/1/Source/platform/fonts/Font.h...
Source/platform/fonts/Font.h:141: float buildGlyphBuffer(const
TextRunPaintInfo&, GlyphBuffer&, const GlyphData* = nullptr) const;
On 2015/03/07 18:50:25, jbroman wrote:
> Please include a name for this argument; it's not obvious here that it's for
> emphasis.

Done.

https://codereview.chromium.org/986493002/diff/1/Source/platform/fonts/shapin...
File Source/platform/fonts/shaping/SimpleShaper.cpp (right):

https://codereview.chromium.org/986493002/diff/1/Source/platform/fonts/shapin...
Source/platform/fonts/shaping/SimpleShaper.cpp:179: } else {
On 2015/03/07 18:50:25, jbroman wrote:
> nit: consider
> 
> } else if (...) {
> 
> to avoid the extra indentation?

Done.

[f(malita), 2015-03-09 15:05:01]

The CQ bit was checked by fmalita@chromium.org

[f(malita), 2015-03-09 15:05:01]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org,
eae@chromium.org, jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/986493002/#ps20001 (title:
"review comments")

[commit-bot: I haz the power, 2015-03-09 15:05:20]

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

[commit-bot: I haz the power, 2015-03-09 16:26:00]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191556