Separate advance from offset in GlypBuffer

Source/platform/fonts/GlyphBuffer.h

Index: Source/platform/fonts/GlyphBuffer.h
diff --git a/Source/platform/fonts/GlyphBuffer.h b/Source/platform/fonts/GlyphBuffer.h
index 5ca769b548f2922eccef52c32b1749d8b77e8aba..e3b8a016028b426b2be2d0a77fef4f8e45cf8c92 100644
--- a/Source/platform/fonts/GlyphBuffer.h
+++ b/Source/platform/fonts/GlyphBuffer.h
@@ -40,22 +40,24 @@ class SimpleFontData;
 
 class GlyphBuffer {
 public:
-    GlyphBuffer() : m_hasVerticalAdvances(false) { }
+    GlyphBuffer() { }
 
     bool isEmpty() const { return m_fontData.isEmpty(); }
+    bool hasOffsets() const { return !m_offsets.isEmpty(); }
     unsigned size() const { return m_fontData.size(); }
-    bool hasVerticalAdvances() const { return m_hasVerticalAdvances; }
+    void reserveOffsetsCapacity(size_t size) { m_offsets.reserveCapacity(size); }

[jbroman, 2014/09/25 17:03:31]

This seems like an odd method to have. Why not just reserve capacity in all of
the vectors if we know how many glyphs there will be?

[eae, 2014/09/26 05:26:51]

Good point, I'll rename it and change it to operate on all fields.

 
     void clear()
     {
         m_fontData.clear();
         m_glyphs.clear();
+        m_offsets.clear();
         m_advances.clear();
-        m_hasVerticalAdvances = false;
     }
 
     const Glyph* glyphs(unsigned from) const { return m_glyphs.data() + from; }
-    const FloatSize* advances(unsigned from) const { return m_advances.data() + from; }
+    const FloatSize* offsets(unsigned from) const { return m_offsets.data() + from; }
+    const float* advances(unsigned from) const { return m_advances.data() + from; }
 
     const SimpleFontData* fontDataAt(unsigned index) const { return m_fontData[index]; }
 
@@ -64,7 +66,12 @@ public:
         return m_glyphs[index];
     }
 
-    FloatSize advanceAt(unsigned index) const
+    FloatSize offsetAt(unsigned index) const
+    {
+        return m_offsets[index];
+    }
+
+    float advanceAt(unsigned index) const
     {
         return m_advances[index];
     }
@@ -73,42 +80,37 @@ public:
     {

[jbroman, 2014/09/25 17:03:32]

Could we do something to ensure callers don't wind up calling both this and the
other overload that takes an offset? That could result in bizarre situations
where the offsets don't line up properly.

I'm not sure if this should be an ASSERT(!hasOffsets()) here, or some sort of
sanity check after the glyph buffer is done. But if I just saw the interface to
this class I would have no idea that I'm supposed to use only one or the other.

Another alternative would be for there to be a GlyphBuffer and a subclass
ComplexGlyphBuffer (or similar), where the latter adds the offset-related code
and hides the wrong add method.

[eae, 2014/09/26 05:26:51]

Yeah, I was thinking about that, I don't really want to slow things down by
making the add method virtual but it might be the better option.

         m_fontData.append(font);
         m_glyphs.append(glyph);
-        m_advances.append(FloatSize(width, 0));
+        m_advances.append(width);
     }
 
-    void add(Glyph glyph, const SimpleFontData* font, const FloatSize& advance)
+    void add(Glyph glyph, const SimpleFontData* font, const FloatSize& offset, float advance = 0)
     {
         m_fontData.append(font);
         m_glyphs.append(glyph);
+        m_offsets.append(offset);
         m_advances.append(advance);
-        if (advance.height())
-            m_hasVerticalAdvances = true;
     }
 
     void reverse()
     {
         m_fontData.reverse();
         m_glyphs.reverse();
+        m_offsets.reverse();

[jbroman, 2014/09/25 17:11:35]

Are these offsets from the text origin, or from the previous character?

If the former, I don't think this logically reverses the glyph buffer. This
probably works because it so happens we only call reverse in the simple and
Mac-complex paths.

If the latter, then I don't understand how they differ from advances, and we
should probably leave a comment explaining it.

         m_advances.reverse();
     }
 
-    void setAdvanceWidth(unsigned index, float newWidth)
-    {
-        m_advances[index].setWidth(newWidth);
-    }
-
     void expandLastAdvance(float width)
     {
         ASSERT(!isEmpty());
-        FloatSize& lastAdvance = m_advances.last();
-        lastAdvance.setWidth(lastAdvance.width() + width);
+        float& lastAdvance = m_advances.last();
+        lastAdvance += width;
     }
 
 private:
     Vector<const SimpleFontData*, 2048> m_fontData;
     Vector<Glyph, 2048> m_glyphs;
-    Vector<FloatSize, 2048> m_advances;
-    bool m_hasVerticalAdvances;
+    Vector<float, 2048> m_advances;
+    Vector<FloatSize> m_offsets;

[f(malita), 2014/09/25 16:42:05]

Why no inline capacity for m_offsets? Using stack-allocated buffers is much more
efficient than going through the heap allocator (as long as we don't overflow,
which is super unlikely given the current sizing).

We're halving the advance buffer size, so we should be able to reserve space for
1024 offsets without changing sizeof(GlyphBuffer).

[eae, 2014/09/26 05:26:51]

As explained in the CL description the m_offsets field is only used for complex
text, always reserving size for it even for the simple text path where it is not
used seems even more inefficient. In the complex text case it is pre-allocated
to the right size before being used.

 };
 
 } // namespace blink