Offset-only GlyphBuffer

Instead of storing glyph advances (only to later convert to offsets for the Skia draw calls), refactor GlyphBuffer to store offsets directly. This reduces the amount of buffer post-processing and decreases the API friction when calling into Skia.

GlyphBuffer offsets are relative to the text run origin and are either horizontal/x-only (generated by the simple shaper) or x/y interleaved (generated by the complex shaper). These correspond to the text positioning modes used by Skia.

Thanks to the new buffer format, the refactor also includes:

* zero offset processing when building text blobs (the offsets are essentially memcpy-ed)
* minimal processing (translate by destination point) for the legacy drawPosText path (we could also optimize this path, but the added complexity is probably not justified given the plans to completely switch to text blobs)
* SimpleShaper::adjustSpacing no longer needs to inject synthetic glyphs but simply adjusts the offsets for following glyps
* drawEmphasisMarks uses explicit offsets to glyph-midpoint instead of computing them based on advances

Other things to investigate:

* multi-run buffer builder (to support multi-run text blobs)
* stop storing space glyphs (simply adjust the following glyph offsets instead)

R=jbroman@chromium.org,eae@chromium.org,dominik.rottsches@intel.com
BUG=377845

Review URL: https://codereview.chromium.org/676523003

git-svn-id: svn://svn.chromium.org/blink/trunk@185129 bbb929c8-8fbe-4397-9dbb-9b2b20218538

third_party/WebKit/LayoutTests/TestExpectations

@@ -1068,6 +1068,15 @@ crbug.com/381730 http/tests/navigation/pending-location-change-assert.html [ Pas
 crbug.com/381464 fast/dom/Window/Location/window-shadow-location-using-string.html [ Pass Failure ]
 crbug.com/381464 fast/dom/Window/Location/window-shadow-location-using-js-object-with-toString.html [ Pass Failure ]
 
+crbug.com/377845 css1/box_properties/float_on_text_elements.html [ NeedsRebaseline ]
+crbug.com/377845 css2.1/t0905-c5525-fltwidth-00-c-g.html [ NeedsRebaseline ]
+crbug.com/377845 tables/mozilla/marvin/x_tr_align_justify.xml [ NeedsRebaseline ]
+crbug.com/377845 fast/text/basic/generic-family-reset.html [ NeedsRebaseline ]
+crbug.com/377845 virtual/antialiasedtext/fast/text/basic/generic-family-reset.html [ NeedsRebaseline ]
+crbug.com/377845 fast/text/justify-ideograph-vertical.html [ NeedsRebaseline ]
+crbug.com/377845 virtual/antialiasedtext/fast/text/justify-ideograph-vertical.html [ NeedsRebaseline ]
+crbug.com/377845 virtual/textblob/fast/text/justify-ideograph-vertical.html [ NeedsRebaseline ]
+
 crbug.com/377476 fast/js/function-name.html [ NeedsManualRebaseline ]
 crbug.com/377476 fast/js/getOwnPropertyDescriptor.html [ NeedsManualRebaseline ]
 crbug.com/377476 fast/js/mozilla/strict/function-name-arity.html [ NeedsManualRebaseline ]

third_party/WebKit/Source/platform/fonts/Font.h

@@ -137,20 +137,20 @@ public:
 private:
     enum ForTextEmphasisOrNot { NotForTextEmphasis, ForTextEmphasis };
 
-    // Returns the initial in-stream advance.
+    // Returns the total advance.
     float buildGlyphBuffer(const TextRunPaintInfo&, GlyphBuffer&, ForTextEmphasisOrNot = NotForTextEmphasis) const;
-    PassTextBlobPtr buildTextBlob(const GlyphBuffer&, float initialAdvance, const FloatRect& bounds,
-        bool couldUseLCD) const;
-
+    PassTextBlobPtr buildTextBlob(const GlyphBuffer&, const FloatRect& bounds, bool couldUseLCD) const;
     SkPaint textFillPaint(GraphicsContext*, const SimpleFontData*) const;
     SkPaint textStrokePaint(GraphicsContext*, const SimpleFontData*, bool isFilling) const;
     void paintGlyphs(GraphicsContext*, const SimpleFontData*, const Glyph glyphs[], unsigned numGlyphs,
         const SkPoint pos[], const FloatRect& textRect) const;
     void paintGlyphsHorizontal(GraphicsContext*, const SimpleFontData*, const Glyph glyphs[], unsigned numGlyphs,
         const SkScalar xpos[], SkScalar constY, const FloatRect& textRect) const;
+    void paintGlyphsVertical(GraphicsContext*, const SimpleFontData*, const GlyphBuffer&,
+        unsigned from, unsigned numGlyphs, const FloatPoint&, const FloatRect&) const;
     void drawGlyphs(GraphicsContext*, const SimpleFontData*, const GlyphBuffer&, unsigned from, unsigned numGlyphs, const FloatPoint&, const FloatRect& textRect) const;
     void drawTextBlob(GraphicsContext*, const SkTextBlob*, const SkPoint& origin) const;
-    float drawGlyphBuffer(GraphicsContext*, const TextRunPaintInfo&, const GlyphBuffer&, const FloatPoint&) const;
+    void drawGlyphBuffer(GraphicsContext*, const TextRunPaintInfo&, const GlyphBuffer&, const FloatPoint&) const;
     void drawEmphasisMarks(GraphicsContext*, const TextRunPaintInfo&, const GlyphBuffer&, const AtomicString&, const FloatPoint&) const;
     float floatWidthForSimpleText(const TextRun&, HashSet<const SimpleFontData*>* fallbackFonts = 0, IntRectExtent* glyphBounds = 0) const;
     int offsetForPositionForSimpleText(const TextRun&, float position, bool includePartialGlyphs) const;

third_party/WebKit/Source/platform/fonts/GlyphBuffer.h

@@ -31,7 +31,7 @@
 #define GlyphBuffer_h
 
 #include "platform/fonts/Glyph.h"
-#include "platform/geometry/FloatSize.h"
+#include "platform/geometry/FloatPoint.h"
 #include "platform/heap/Heap.h"
 #include "wtf/Vector.h"
 
@@ -43,68 +43,127 @@ class GlyphBuffer {
     STACK_ALLOCATED();
 public:
     bool isEmpty() const { return m_fontData.isEmpty(); }
-    bool hasOffsets() const { return !m_offsets.isEmpty(); }
-    unsigned size() const { return m_fontData.size(); }
+    unsigned size() const
+    {
+        ASSERT(m_fontData.size() == m_glyphs.size());
+        ASSERT(m_fontData.size() == m_offsets.size() || 2 * m_fontData.size() == m_offsets.size());
+        return m_fontData.size();
+    }
+
+    bool hasVerticalOffsets() const
+    {
+        // We exclusively store either horizontal/x-only ofssets -- in which case m_offsets.size == size,
+        // or vertical/xy offsets -- in which case m_offsets.size == size * 2.
+        return size() != m_offsets.size();
+    }
 
-    const Glyph* glyphs(unsigned from) const { return m_glyphs.data() + from; }
-    const float* advances(unsigned from) const { return m_advances.data() + from; }
-    const FloatSize* offsets(unsigned from) const { return m_offsets.data() + from; }
+    const Glyph* glyphs(unsigned from) const
+    {
+        ASSERT(from < size());
+        return m_glyphs.data() + from;
+    }
+
+    // Depending on the GlyphBuffer-wide positioning mode, this either points to an array of
+    // x-only offsets for horizontal positioning ([x1, x2, ... xn]), or interleaved x,y offsets
+    // for full positioning ([x1, y1, x2, y2, ... xn, yn]).
+    const float* offsets(unsigned from) const
+    {
+        ASSERT(from < size());
+        return m_offsets.data() + (hasVerticalOffsets() ? from * 2 : from);
+    }
 
     const SimpleFontData* fontDataAt(unsigned index) const
     {
+        ASSERT(index < size());
         return m_fontData[index];
     }
 
     Glyph glyphAt(unsigned index) const
     {
+        ASSERT(index < size());
         return m_glyphs[index];
     }
 
-    float advanceAt(unsigned index) const
+    float xOffsetAt(unsigned index) const
     {
-        return m_advances[index];
+        ASSERT(index < size());
+        return hasVerticalOffsets() ? m_offsets[index * 2] : m_offsets[index];
+
     }
 
-    void add(Glyph glyph, const SimpleFontData* font, float width)
+    float yOffsetAt(unsigned index) const
     {
-        // should not mix offset/advance-only glyphs
-        ASSERT(!hasOffsets());
+        ASSERT(index < size());
+        ASSERT(hasVerticalOffsets());
+        return m_offsets[index * 2 + 1];
+    }
+
+    void add(Glyph glyph, const SimpleFontData* font, float x)
+    {
+        // cannot mix x-only/xy offsets
+        ASSERT(!hasVerticalOffsets());
 
         m_fontData.append(font);
         m_glyphs.append(glyph);
-        m_advances.append(width);
+        m_offsets.append(x);
     }
 
-    void add(Glyph glyph, const SimpleFontData* font, const FloatSize& offset, float advance)
+    void add(Glyph glyph, const SimpleFontData* font, const FloatPoint& offset)
     {
-        // should not mix offset/advance-only glyphs
-        ASSERT(size() == m_offsets.size());
+        // cannot mix x-only/xy offsets
+        ASSERT(isEmpty() || hasVerticalOffsets());
 
         m_fontData.append(font);
         m_glyphs.append(glyph);
-        m_offsets.append(offset);
-        m_advances.append(advance);
+        m_offsets.append(offset.x());
+        m_offsets.append(offset.y());
     }
 
-    void reverse()
+    void reverseForSimpleRTL(float afterOffset, float totalWidth)
     {
+        ASSERT(!hasVerticalOffsets());
+
+        if (isEmpty())
+            return;
+
         m_fontData.reverse();
         m_glyphs.reverse();
-        m_advances.reverse();
-    }
 
-    void expandLastAdvance(float width)
-    {
-        ASSERT(!isEmpty());
-        float& lastAdvance = m_advances.last();
-        lastAdvance += width;
+        // | .. [X0 X1 ..   Xn]     ..   |
+        // ^                   ^         ^
+        // 0             afterOffset totalWidth
+        //
+        // The input buffer is shaped using RTL advances, but since the right edge is unknown at
+        // that time, offsets are computed as if the advances were LTR. This method performs the
+        // required adjustments by reconstructing advances and positioning offsets in an RTL
+        // progression.
+
+        // FIXME: we should get rid of this (idea: store negative offsets while shaping,
+        //        and adjust the initial advance accordingly -> should yield correctly positioned
+        //        RTL glyphs without any post-shape munging).
+        ASSERT_WITH_SECURITY_IMPLICATION(!m_offsets.isEmpty());
+        float x = totalWidth - m_offsets[0];
+        for (unsigned i = 0; i + 1 < m_offsets.size(); ++i) {
+            float advance = m_offsets[i + 1] - m_offsets[i];
+            ASSERT(advance >= 0);
+            x -= advance;
+            m_offsets[i] = x;
+        }
+        float lastAdvance = afterOffset - m_offsets.last();
+        ASSERT(lastAdvance >= 0);
+        m_offsets.last() = x - lastAdvance;
+
+        m_offsets.reverse();
     }
 
 protected:
     Vector<const SimpleFontData*, 2048> m_fontData;
     Vector<Glyph, 2048> m_glyphs;
-    Vector<float, 2048> m_advances;
-    Vector<FloatSize, 1024> m_offsets;
+
+    // Glyph positioning: either x-only offsets, or interleaved x,y offsets
+    // (depending on the buffer-wide positioning mode). This matches the
+    // glyph positioning format used by Skia.
+    Vector<float, 2048> m_offsets;
 };
 
 } // namespace blink

third_party/WebKit/Source/platform/fonts/GlyphBufferTest.cpp

@@ -47,10 +47,10 @@ TEST(GlyphBufferTest, StoresGlyphs)
     GlyphBuffer glyphBuffer;
     glyphBuffer.add(42, font1.get(), 10);
     glyphBuffer.add(43, font1.get(), 15);
-    glyphBuffer.add(44, font2.get(), 12);
+    glyphBuffer.add(44, font2.get(), 22);
 
     EXPECT_FALSE(glyphBuffer.isEmpty());
-    EXPECT_FALSE(glyphBuffer.hasOffsets());
+    EXPECT_FALSE(glyphBuffer.hasVerticalOffsets());
     EXPECT_EQ(3u, glyphBuffer.size());
 
     EXPECT_EQ(42, glyphBuffer.glyphAt(0));
@@ -63,27 +63,39 @@ TEST(GlyphBufferTest, StoresGlyphs)
     EXPECT_EQ(44, glyphs[2]);
 }
 
-TEST(GlyphBufferTest, StoresOffsets)
+TEST(GlyphBufferTest, StoresVerticalOffsets)
 {
     RefPtr<SimpleFontData> font1 = TestSimpleFontData::create();
     RefPtr<SimpleFontData> font2 = TestSimpleFontData::create();
 
     GlyphBuffer glyphBuffer;
-    glyphBuffer.add(42, font1.get(), FloatSize(10, 0), 0);
-    glyphBuffer.add(43, font1.get(), FloatSize(15, 0), 0);
-    glyphBuffer.add(44, font2.get(), FloatSize(12, 2), 0);
+    EXPECT_FALSE(glyphBuffer.hasVerticalOffsets());
+
+    glyphBuffer.add(42, font1.get(), FloatPoint(10, 0));
+    glyphBuffer.add(43, font1.get(), FloatPoint(15, 0));
+    glyphBuffer.add(44, font2.get(), FloatPoint(12, 2));
 
     EXPECT_FALSE(glyphBuffer.isEmpty());
-    EXPECT_TRUE(glyphBuffer.hasOffsets());
+    EXPECT_TRUE(glyphBuffer.hasVerticalOffsets());
     EXPECT_EQ(3u, glyphBuffer.size());
 
-    const FloatSize* offsets = glyphBuffer.offsets(0);
-    EXPECT_EQ(FloatSize(10, 0), offsets[0]);
-    EXPECT_EQ(FloatSize(15, 0), offsets[1]);
-    EXPECT_EQ(FloatSize(12, 2), offsets[2]);
+    const float* offsets = glyphBuffer.offsets(0);
+    EXPECT_EQ(10, glyphBuffer.xOffsetAt(0));
+    EXPECT_EQ(0, glyphBuffer.yOffsetAt(0));
+    EXPECT_EQ(15, glyphBuffer.xOffsetAt(1));
+    EXPECT_EQ(0, glyphBuffer.yOffsetAt(1));
+    EXPECT_EQ(12, glyphBuffer.xOffsetAt(2));
+    EXPECT_EQ(2, glyphBuffer.yOffsetAt(2));
+
+    EXPECT_EQ(10, offsets[0]);
+    EXPECT_EQ(0, offsets[1]);
+    EXPECT_EQ(15, offsets[2]);
+    EXPECT_EQ(0, offsets[3]);
+    EXPECT_EQ(12, offsets[4]);
+    EXPECT_EQ(2, offsets[5]);
 }
 
-TEST(GlyphBufferTest, StoresAdvances)
+TEST(GlyphBufferTest, StoresOffsets)
 {
     RefPtr<SimpleFontData> font1 = TestSimpleFontData::create();
     RefPtr<SimpleFontData> font2 = TestSimpleFontData::create();
@@ -94,16 +106,17 @@ TEST(GlyphBufferTest, StoresAdvances)
     glyphBuffer.add(44, font2.get(), 20);
 
     EXPECT_FALSE(glyphBuffer.isEmpty());
+    EXPECT_FALSE(glyphBuffer.hasVerticalOffsets());
     EXPECT_EQ(3u, glyphBuffer.size());
 
-    EXPECT_EQ(10, glyphBuffer.advanceAt(0));
-    EXPECT_EQ(15, glyphBuffer.advanceAt(1));
-    EXPECT_EQ(20, glyphBuffer.advanceAt(2));
+    EXPECT_EQ(10, glyphBuffer.xOffsetAt(0));
+    EXPECT_EQ(15, glyphBuffer.xOffsetAt(1));
+    EXPECT_EQ(20, glyphBuffer.xOffsetAt(2));
 
-    const float* advances = glyphBuffer.advances(0);
-    EXPECT_EQ(10, advances[0]);
-    EXPECT_EQ(15, advances[1]);
-    EXPECT_EQ(20, advances[2]);
+    const float* offsets = glyphBuffer.offsets(0);
+    EXPECT_EQ(10, offsets[0]);
+    EXPECT_EQ(15, offsets[1]);
+    EXPECT_EQ(20, offsets[2]);
 }
 
 TEST(GlyphBufferTest, StoresSimpleFontData)
@@ -142,25 +155,45 @@ TEST(GlyphBufferTest, GlyphArrayWithOffset)
     EXPECT_EQ(44, glyphs[1]);
 }
 
-TEST(GlyphBufferTest, AdvanceArrayWithOffset)
+TEST(GlyphBufferTest, OffsetArrayWithNonZeroIndex)
 {
     RefPtr<SimpleFontData> font1 = TestSimpleFontData::create();
     RefPtr<SimpleFontData> font2 = TestSimpleFontData::create();
 
-    GlyphBuffer glyphBuffer;
-    glyphBuffer.add(42, font1.get(), 10);
-    glyphBuffer.add(43, font1.get(), 15);
-    glyphBuffer.add(43, font1.get(), 12);
-
-    EXPECT_FALSE(glyphBuffer.isEmpty());
-    EXPECT_EQ(3u, glyphBuffer.size());
+    {
+        GlyphBuffer glyphBuffer;
+        glyphBuffer.add(42, font1.get(), 10);
+        glyphBuffer.add(43, font1.get(), 15);
+        glyphBuffer.add(43, font2.get(), 12);
+
+        EXPECT_FALSE(glyphBuffer.isEmpty());
+        EXPECT_FALSE(glyphBuffer.hasVerticalOffsets());
+        EXPECT_EQ(3u, glyphBuffer.size());
+
+        const float* offsets = glyphBuffer.offsets(1);
+        EXPECT_EQ(15, offsets[0]);
+        EXPECT_EQ(12, offsets[1]);
+    }
 
-    const float* advances = glyphBuffer.advances(1);
-    EXPECT_EQ(15, advances[0]);
-    EXPECT_EQ(12, advances[1]);
+    {
+        GlyphBuffer glyphBuffer;
+        glyphBuffer.add(42, font1.get(), FloatPoint(10, 0));
+        glyphBuffer.add(43, font1.get(), FloatPoint(15, 0));
+        glyphBuffer.add(43, font2.get(), FloatPoint(12, 2));
+
+        EXPECT_FALSE(glyphBuffer.isEmpty());
+        EXPECT_TRUE(glyphBuffer.hasVerticalOffsets());
+        EXPECT_EQ(3u, glyphBuffer.size());
+
+        const float* offsets = glyphBuffer.offsets(1);
+        EXPECT_EQ(15, offsets[0]);
+        EXPECT_EQ(0, offsets[1]);
+        EXPECT_EQ(12, offsets[2]);
+        EXPECT_EQ(2, offsets[3]);
+    }
 }
 
-TEST(GlyphBufferTest, Reverse)
+TEST(GlyphBufferTest, ReverseForSimpleRTL)
 {
     RefPtr<SimpleFontData> font1 = TestSimpleFontData::create();
     RefPtr<SimpleFontData> font2 = TestSimpleFontData::create();
@@ -168,38 +201,24 @@ TEST(GlyphBufferTest, Reverse)
     GlyphBuffer glyphBuffer;
     glyphBuffer.add(42, font1.get(), 10);
     glyphBuffer.add(43, font1.get(), 15);
-    glyphBuffer.add(44, font2.get(), 20);
+    glyphBuffer.add(44, font2.get(), 25);
 
     EXPECT_FALSE(glyphBuffer.isEmpty());
     EXPECT_EQ(3u, glyphBuffer.size());
 
-    glyphBuffer.reverse();
+    glyphBuffer.reverseForSimpleRTL(30, 100);
 
     EXPECT_FALSE(glyphBuffer.isEmpty());
     EXPECT_EQ(3u, glyphBuffer.size());
     EXPECT_EQ(44, glyphBuffer.glyphAt(0));
     EXPECT_EQ(43, glyphBuffer.glyphAt(1));
     EXPECT_EQ(42, glyphBuffer.glyphAt(2));
-    EXPECT_EQ(20, glyphBuffer.advanceAt(0));
-    EXPECT_EQ(15, glyphBuffer.advanceAt(1));
-    EXPECT_EQ(10, glyphBuffer.advanceAt(2));
     EXPECT_EQ(font2.get(), glyphBuffer.fontDataAt(0));
     EXPECT_EQ(font1.get(), glyphBuffer.fontDataAt(1));
     EXPECT_EQ(font1.get(), glyphBuffer.fontDataAt(2));
-}
-
-TEST(GlyphBufferTest, ExpandLastAdvance)
-{
-    RefPtr<SimpleFontData> font1 = TestSimpleFontData::create();
-    RefPtr<SimpleFontData> font2 = TestSimpleFontData::create();
-
-    GlyphBuffer glyphBuffer;
-    glyphBuffer.add(42, font1.get(), 10);
-    glyphBuffer.add(43, font1.get(), 15);
-    glyphBuffer.add(44, font2.get(), 12);
-
-    glyphBuffer.expandLastAdvance(20);
-    EXPECT_EQ(32, glyphBuffer.advanceAt(2));
+    EXPECT_EQ(70, glyphBuffer.xOffsetAt(0));
+    EXPECT_EQ(75, glyphBuffer.xOffsetAt(1));
+    EXPECT_EQ(85, glyphBuffer.xOffsetAt(2));
 }
 
 } // namespace

third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.cpp

@@ -949,41 +949,45 @@ void HarfBuzzShaper::setGlyphPositionsForHarfBuzzRun(HarfBuzzRun* currentRun, hb
     m_totalWidth += currentRun->width();
 }
 
-void HarfBuzzShaper::fillGlyphBufferFromHarfBuzzRun(GlyphBuffer* glyphBuffer,
-    HarfBuzzRun* currentRun, float& carryAdvance)
+float HarfBuzzShaper::fillGlyphBufferFromHarfBuzzRun(GlyphBuffer* glyphBuffer,
+    HarfBuzzRun* currentRun, float initialAdvance)
 {
     FloatSize* offsets = currentRun->offsets();
     uint16_t* glyphs = currentRun->glyphs();
     float* advances = currentRun->advances();
     unsigned numGlyphs = currentRun->numGlyphs();
     uint16_t* glyphToCharacterIndexes = currentRun->glyphToCharacterIndexes();
-    FloatSize runStartOffset = FloatSize();
+    float advanceSoFar = initialAdvance;
     if (m_run.rtl()) {
         for (unsigned i = 0; i < numGlyphs; ++i) {
             uint16_t currentCharacterIndex = currentRun->startIndex() + glyphToCharacterIndexes[i];
             if (currentCharacterIndex >= m_toIndex) {
-                carryAdvance += advances[i];
+                advanceSoFar += advances[i];
             } else if (currentCharacterIndex >= m_fromIndex) {
-                runStartOffset = HB_DIRECTION_IS_HORIZONTAL(currentRun->direction()) ? FloatSize(carryAdvance, 0) : FloatSize(0, carryAdvance);
-                glyphBuffer->add(glyphs[i], currentRun->fontData(), runStartOffset + offsets[i], carryAdvance + advances[i]);
-                carryAdvance = 0;
+                FloatPoint runStartOffset = HB_DIRECTION_IS_HORIZONTAL(currentRun->direction()) ?
+                    FloatPoint(advanceSoFar, 0) : FloatPoint(0, advanceSoFar);
+                glyphBuffer->add(glyphs[i], currentRun->fontData(), runStartOffset + offsets[i]);
+                advanceSoFar += advances[i];
             }
         }
     } else {
         for (unsigned i = 0; i < numGlyphs; ++i) {
             uint16_t currentCharacterIndex = currentRun->startIndex() + glyphToCharacterIndexes[i];
             if (currentCharacterIndex < m_fromIndex) {
-                carryAdvance += advances[i];
+                advanceSoFar += advances[i];
             } else if (currentCharacterIndex < m_toIndex) {
-                runStartOffset = HB_DIRECTION_IS_HORIZONTAL(currentRun->direction()) ? FloatSize(carryAdvance, 0) : FloatSize(0, carryAdvance);
-                glyphBuffer->add(glyphs[i], currentRun->fontData(), runStartOffset + offsets[i], carryAdvance + advances[i]);
-                carryAdvance = 0;
+                FloatPoint runStartOffset = HB_DIRECTION_IS_HORIZONTAL(currentRun->direction()) ?
+                    FloatPoint(advanceSoFar, 0) : FloatPoint(0, advanceSoFar);
+                glyphBuffer->add(glyphs[i], currentRun->fontData(), runStartOffset + offsets[i]);
+                advanceSoFar += advances[i];
             }
         }
     }
+
+    return advanceSoFar - initialAdvance;
 }
 
-void HarfBuzzShaper::fillGlyphBufferForTextEmphasis(GlyphBuffer* glyphBuffer, HarfBuzzRun* currentRun)
+float HarfBuzzShaper::fillGlyphBufferForTextEmphasis(GlyphBuffer* glyphBuffer, HarfBuzzRun* currentRun, float initialAdvance)
 {
     // FIXME: Instead of generating a synthetic GlyphBuffer here which is then used by the
     // drawEmphasisMarks method of FontFastPath, we should roll our own emphasis mark drawing function.
@@ -1008,6 +1012,7 @@ void HarfBuzzShaper::fillGlyphBufferForTextEmphasis(GlyphBuffer* glyphBuffer, Ha
     else
         clusterStart = currentRun->startIndex() + glyphToCharacterIndexes[0];
 
+    float advanceSoFar = initialAdvance;
     for (unsigned i = 0; i < numGlyphs; ++i) {
         uint16_t currentCharacterIndex = currentRun->startIndex() + glyphToCharacterIndexes[i];
         bool isRunEnd = (i + 1 == numGlyphs);
@@ -1029,44 +1034,33 @@ void HarfBuzzShaper::fillGlyphBufferForTextEmphasis(GlyphBuffer* glyphBuffer, Ha
             for (unsigned j = 0; j < graphemesInCluster; ++j) {
                 // Do not put emphasis marks on space, separator, and control characters.
                 Glyph glyphToAdd = Character::canReceiveTextEmphasis(m_run[currentCharacterIndex]) ? 1 : 0;
-                glyphBuffer->add(glyphToAdd, currentRun->fontData(), glyphAdvanceX);
+                // The emphasis code expects mid-glyph offsets.
+                glyphBuffer->add(glyphToAdd, currentRun->fontData(), advanceSoFar + glyphAdvanceX / 2);
+                advanceSoFar += glyphAdvanceX;
             }
             clusterStart = clusterEnd;
             clusterAdvance = 0;
         }
     }
+
+    return advanceSoFar - initialAdvance;
 }
 
 bool HarfBuzzShaper::fillGlyphBuffer(GlyphBuffer* glyphBuffer)
 {
+    ASSERT(glyphBuffer);
+
     unsigned numRuns = m_harfBuzzRuns.size();
-    float carryAdvance = 0;
-    if (m_run.rtl()) {
-        for (int runIndex = numRuns - 1; runIndex >= 0; --runIndex) {
-            HarfBuzzRun* currentRun = m_harfBuzzRuns[runIndex].get();
-            if (!currentRun->hasGlyphToCharacterIndexes()) {
-                // FIXME: bug 337886, 359664
-                continue;
-            }
-            if (m_forTextEmphasis == ForTextEmphasis) {
-                fillGlyphBufferForTextEmphasis(glyphBuffer, currentRun);
-            } else {
-                fillGlyphBufferFromHarfBuzzRun(glyphBuffer, currentRun, carryAdvance);
-            }
-        }
-    } else {
-        for (unsigned runIndex = 0; runIndex < numRuns; ++runIndex) {
-            HarfBuzzRun* currentRun = m_harfBuzzRuns[runIndex].get();
-            if (!currentRun->hasGlyphToCharacterIndexes()) {
-                // FIXME: bug 337886, 359664
-                continue;
-            }
-            if (m_forTextEmphasis == ForTextEmphasis) {
-                fillGlyphBufferForTextEmphasis(glyphBuffer, currentRun);
-            } else {
-                fillGlyphBufferFromHarfBuzzRun(glyphBuffer, currentRun, carryAdvance);
-            }
+    float advanceSoFar = 0;
+    for (unsigned runIndex = 0; runIndex < numRuns; ++runIndex) {
+        HarfBuzzRun* currentRun = m_harfBuzzRuns[m_run.ltr() ? runIndex : numRuns - runIndex - 1].get();
+        if (!currentRun->hasGlyphToCharacterIndexes()) {
+            // FIXME: bug 337886, 359664
+            continue;
         }
+        advanceSoFar += (m_forTextEmphasis == ForTextEmphasis)
+            ? fillGlyphBufferForTextEmphasis(glyphBuffer, currentRun, advanceSoFar)
+            : fillGlyphBufferFromHarfBuzzRun(glyphBuffer, currentRun, advanceSoFar);
     }
     return glyphBuffer->size();
 }

third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzShaper.h

@@ -129,8 +129,8 @@ private:
     bool createHarfBuzzRuns();
     bool shapeHarfBuzzRuns();
     bool fillGlyphBuffer(GlyphBuffer*);
-    void fillGlyphBufferFromHarfBuzzRun(GlyphBuffer*, HarfBuzzRun*, float& carryAdvance);
-    void fillGlyphBufferForTextEmphasis(GlyphBuffer*, HarfBuzzRun* currentRun);
+    float fillGlyphBufferFromHarfBuzzRun(GlyphBuffer*, HarfBuzzRun*, float initialAdvance);
+    float fillGlyphBufferForTextEmphasis(GlyphBuffer*, HarfBuzzRun*, float initialAdvance);
     void setGlyphPositionsForHarfBuzzRun(HarfBuzzRun*, hb_buffer_t*);
     void addHarfBuzzRun(unsigned startCharacter, unsigned endCharacter, const SimpleFontData*, UScriptCode);
 

third_party/WebKit/Source/platform/fonts/shaping/SimpleShaper.cpp

@@ -98,8 +98,7 @@ void SimpleShaper::cacheFallbackFont(const SimpleFontData* fontData,
     m_fallbackFonts->add(fontData);
 }
 
-float SimpleShaper::adjustSpacing(float width, const CharacterData& charData,
-    const SimpleFontData& fontData, GlyphBuffer* glyphBuffer)
+float SimpleShaper::adjustSpacing(float width, const CharacterData& charData)
 {
     // Account for letter-spacing.
     if (width)
@@ -112,18 +111,7 @@ float SimpleShaper::adjustSpacing(float width, const CharacterData& charData,
             if (!treatAsSpace && !m_isAfterExpansion) {
                 // Take the expansion opportunity before this ideograph.
                 m_expansion -= m_expansionPerOpportunity;
-                float expansionAtThisOpportunity = m_expansionPerOpportunity;
-                m_runWidthSoFar += expansionAtThisOpportunity;
-                if (glyphBuffer) {
-                    if (glyphBuffer->isEmpty()) {
-                        if (m_forTextEmphasis)
-                            glyphBuffer->add(fontData.zeroWidthSpaceGlyph(), &fontData, m_expansionPerOpportunity);
-                        else
-                            glyphBuffer->add(fontData.spaceGlyph(), &fontData, expansionAtThisOpportunity);
-                    } else {
-                        glyphBuffer->expandLastAdvance(expansionAtThisOpportunity);
-                    }
-                }
+                m_runWidthSoFar += m_expansionPerOpportunity;
             }
             if (m_run.allowsTrailingExpansion()
                 || (m_run.ltr() && charData.characterOffset + charData.clusterLength < static_cast<size_t>(m_run.length()))
@@ -202,20 +190,26 @@ unsigned SimpleShaper::advanceInternal(TextIterator& textIterator, GlyphBuffer*
         }
 
         if (hasExtraSpacing && !spaceUsedAsZeroWidthSpace)
-            width = adjustSpacing(width, charData, *fontData, glyphBuffer);
+            width = adjustSpacing(width, charData);
 
         if (m_bounds)
             updateGlyphBounds(glyphData, width, !charData.characterOffset);
 
-        if (m_forTextEmphasis && !Character::canReceiveTextEmphasis(charData.character))
-            glyph = 0;
+        if (m_forTextEmphasis) {
+            if (!Character::canReceiveTextEmphasis(charData.character))
+                glyph = 0;
+
+            // The emphasis code expects mid-glyph offsets.
+            width /= 2;
+            m_runWidthSoFar += width;
+        }
+
+        if (glyphBuffer)
+            glyphBuffer->add(glyph, fontData, m_runWidthSoFar);
 
         // Advance past the character we just dealt with.
         textIterator.advance(charData.clusterLength);
         m_runWidthSoFar += width;
-
-        if (glyphBuffer)
-            glyphBuffer->add(glyph, fontData, width);
     }
 
     unsigned consumedCharacters = textIterator.currentCharacter() - m_currentCharacter;

third_party/WebKit/Source/platform/fonts/shaping/SimpleShaper.h

@@ -82,7 +82,7 @@ private:
     GlyphData glyphDataForCharacter(CharacterData&, bool normalizeSpace = false);
     float characterWidth(UChar32, const GlyphData&) const;
     void cacheFallbackFont(const SimpleFontData*, const SimpleFontData* primaryFont);
-    float adjustSpacing(float, const CharacterData&, const SimpleFontData&, GlyphBuffer*);
+    float adjustSpacing(float, const CharacterData&);
     void updateGlyphBounds(const GlyphData&, float width, bool firstCharacter);
 
     template <typename TextIterator>