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..3476470fdf8c1c09486fd9d86a350546973ffb5e 100644
--- a/Source/platform/fonts/GlyphBuffer.h
+++ b/Source/platform/fonts/GlyphBuffer.h
@@ -40,23 +40,21 @@ class SimpleFontData;
 
 class GlyphBuffer {
 public:
-    GlyphBuffer() : m_hasVerticalAdvances(false) { }
+    GlyphBuffer() { }

[jbroman, 2014/09/26 13:02:11]

We can probably remove the default constructors since they're implicitly defined
this way. Any reason to explicitly declare them?

 
     bool isEmpty() const { return m_fontData.isEmpty(); }
+    virtual bool hasOffsets() const { return false; }
     unsigned size() const { return m_fontData.size(); }
-    bool hasVerticalAdvances() const { return m_hasVerticalAdvances; }
 
-    void clear()
+    virtual void clear()
     {
         m_fontData.clear();
         m_glyphs.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 float* advances(unsigned from) const { return m_advances.data() + from; }
     const SimpleFontData* fontDataAt(unsigned index) const { return m_fontData[index]; }
 
     Glyph glyphAt(unsigned index) const
@@ -64,7 +62,7 @@ public:
         return m_glyphs[index];
     }
 
-    FloatSize advanceAt(unsigned index) const
+    float advanceAt(unsigned index) const
     {
         return m_advances[index];
     }
@@ -73,42 +71,70 @@ public:
     {
         m_fontData.append(font);

[jbroman, 2014/09/26 13:02:11]

I'm not sure how expensive adding
ASSERT(!hasOffsets());
here would be. It's possible that this could get called on a
GlyphBufferWithOffsets instance through the base pointer, and it would be nice
to catch that. But if it kills performance of debug builds too much, perhaps not
worth it.

         m_glyphs.append(glyph);
-        m_advances.append(FloatSize(width, 0));
-    }
-
-    void add(Glyph glyph, const SimpleFontData* font, const FloatSize& advance)
-    {
-        m_fontData.append(font);
-        m_glyphs.append(glyph);
-        m_advances.append(advance);
-        if (advance.height())
-            m_hasVerticalAdvances = true;
+        m_advances.append(width);
     }
 
-    void reverse()
+    virtual void reverse()
     {
         m_fontData.reverse();
         m_glyphs.reverse();
         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:
+protected:
     Vector<const SimpleFontData*, 2048> m_fontData;
     Vector<Glyph, 2048> m_glyphs;
-    Vector<FloatSize, 2048> m_advances;
-    bool m_hasVerticalAdvances;
+    Vector<float, 2048> m_advances;
+};
+
+
+class GlyphBufferWithOffsets: public GlyphBuffer {

[jbroman, 2014/09/26 13:02:11]

super-nit: space before :

+public:
+    GlyphBufferWithOffsets() { }
+
+    virtual bool hasOffsets() const { return true; }

[jbroman, 2014/09/26 13:02:11]

"override", here and the other virtual methods? (We can apparently use the
lowercase one now!)

+
+    virtual void clear()
+    {
+        GlyphBuffer::clear();
+        m_offsets.clear();
+    }
+
+    const FloatSize* offsets(unsigned from) const { return m_offsets.data() + from; }
+
+    FloatSize offsetAt(unsigned index) const
+    {
+        return m_offsets[index];
+    }
+
+    void add(Glyph glyph, const SimpleFontData* font, float width)
+    {
+        ASSERT_NOT_REACHED();

[jbroman, 2014/09/26 13:02:11]

We should be able to catch this case at compile time. Consider hiding this
method.

Once it becomes permitted, I'd use "= delete" for this. In the meantime, making
it private should do the trick.

+    }
+
+    void add(Glyph glyph, const SimpleFontData* font, const FloatSize& offset, float advance)
+    {
+        m_fontData.append(font);
+        m_glyphs.append(glyph);
+        m_offsets.append(offset);
+        m_advances.append(advance);
+    }
+
+    virtual void reverse()
+    {
+        GlyphBuffer::reverse();
+        m_offsets.reverse();
+    }
+
+private:
+    Vector<FloatSize, 1024> m_offsets;
 };
 
 } // namespace blink