Drop SVGTextChunk::m_boxes

Source/core/layout/svg/SVGTextChunkBuilder.cpp

Index: Source/core/layout/svg/SVGTextChunkBuilder.cpp
diff --git a/Source/core/layout/svg/SVGTextChunkBuilder.cpp b/Source/core/layout/svg/SVGTextChunkBuilder.cpp
index 2e1109d3d39d867567e803c65efda1a7f0993e55..6fe396ef664e4477a414b92bdd65c0de757090a5 100644
--- a/Source/core/layout/svg/SVGTextChunkBuilder.cpp
+++ b/Source/core/layout/svg/SVGTextChunkBuilder.cpp
@@ -28,57 +28,14 @@
 
 namespace blink {
 
-SVGTextChunkBuilder::SVGTextChunkBuilder()
-{
-}
-
-void SVGTextChunkBuilder::processTextChunks(const Vector<SVGInlineTextBox*>& lineLayoutBoxes)
-{
-    if (lineLayoutBoxes.isEmpty())
-        return;
-
-    bool foundStart = false;
-    unsigned lastChunkStartPosition = 0;
-    unsigned boxPosition = 0;
-    unsigned boxCount = lineLayoutBoxes.size();
-    for (; boxPosition < boxCount; ++boxPosition) {
-        SVGInlineTextBox* textBox = lineLayoutBoxes[boxPosition];
-        if (!textBox->startsNewTextChunk())
-            continue;
-
-        if (!foundStart) {
-            foundStart = true;
-        } else {
-            ASSERT(boxPosition > lastChunkStartPosition);
-            handleTextChunk(lineLayoutBoxes, lastChunkStartPosition, boxPosition);
-        }
-        lastChunkStartPosition = boxPosition;
-    }
-
-    if (!foundStart)
-        return;
-
-    if (boxPosition - lastChunkStartPosition > 0)
-        handleTextChunk(lineLayoutBoxes, lastChunkStartPosition, boxPosition);
-}
+namespace {

[pdr., 2015/05/28 22:00:03]

Is this empty namespace needed?

[fs, 2015/05/29 09:26:36]

Not strictly I suppose, but it provides a neat packaging mechanism for the
symbols that are intended to be CU-local.

 
-SVGTextPathChunkBuilder::SVGTextPathChunkBuilder()
-    : SVGTextChunkBuilder()
-    , m_totalLength(0)
-    , m_totalCharacters(0)
-    , m_totalTextAnchorShift(0)
+SVGTextChunk createTextChunk(const SVGInlineTextBox* textBox)

[pdr., 2015/05/28 22:00:03]

Should this be static?

Can you change the textBox param to be a reference?

[fs, 2015/05/29 09:26:36]

The unnamed namespace will have that effect - and also for
ChunkLengthAccumulator, which is why I chose this way rather than a simple
'static'.
Done.

 {
-}
-
-static SVGTextChunk createTextChunk(const Vector<SVGInlineTextBox*>& lineLayoutBoxes, unsigned boxStart, unsigned boxCount)
-{
-    SVGInlineTextBox* textBox = lineLayoutBoxes[boxStart];
     ASSERT(textBox);
 
     LayoutSVGInlineText& textLayoutObject = toLayoutSVGInlineText(textBox->layoutObject());
-
-    const ComputedStyle& style = toLayoutSVGInlineText(textBox->layoutObject()).styleRef();
-
+    const ComputedStyle& style = textLayoutObject.styleRef();
     const SVGComputedStyle& svgStyle = style.svgStyle();
 
     // Build chunk style flags.
@@ -102,7 +59,7 @@ static SVGTextChunk createTextChunk(const Vector<SVGInlineTextBox*>& lineLayoutB
     case TA_END:
         chunkStyle |= SVGTextChunk::EndAnchor;
         break;
-    };
+    }
 
     // Handle 'lengthAdjust' property.
     float desiredTextLength = 0;
@@ -122,31 +79,122 @@ static SVGTextChunk createTextChunk(const Vector<SVGInlineTextBox*>& lineLayoutB
         case SVGLengthAdjustSpacingAndGlyphs:
             chunkStyle |= SVGTextChunk::LengthAdjustSpacingAndGlyphs;
             break;
-        };
+        }
+    }
+
+    return SVGTextChunk(chunkStyle, desiredTextLength);
+}
+
+class ChunkLengthAccumulator {
+public:
+    ChunkLengthAccumulator(bool isVertical)
+        : m_numCharacters(0)
+        , m_length(0)
+        , m_isVertical(isVertical)
+    {
     }
 
-    SVGTextChunk chunk(chunkStyle, desiredTextLength);
+    typedef Vector<SVGInlineTextBox*>::const_iterator BoxListConstIterator;
 
-    Vector<SVGInlineTextBox*>& boxes = chunk.boxes();
-    for (unsigned i = boxStart; i < boxStart + boxCount; ++i)
-        boxes.append(lineLayoutBoxes[i]);
+    void processRange(BoxListConstIterator boxStart, BoxListConstIterator boxEnd);
+    void reset()
+    {
+        m_numCharacters = 0;
+        m_length = 0;
+    }
+
+    float length() const { return m_length; }
+    unsigned numCharacters() const { return m_numCharacters; }
+
+private:
+    unsigned m_numCharacters;
+    float m_length;
+    const bool m_isVertical;
+};
+
+void ChunkLengthAccumulator::processRange(BoxListConstIterator boxStart, BoxListConstIterator boxEnd)
+{
+    SVGTextFragment* lastFragment = nullptr;
+    for (auto boxIter = boxStart; boxIter != boxEnd; ++boxIter) {
+        for (SVGTextFragment& fragment : (*boxIter)->textFragments()) {
+            m_numCharacters += fragment.length;
+
+            if (m_isVertical)
+                m_length += fragment.height;
+            else
+                m_length += fragment.width;
+
+            if (!lastFragment) {
+                lastFragment = &fragment;
+                continue;
+            }
 
-    return chunk;
+            // Respect gap between chunks.
+            if (m_isVertical)
+                m_length += fragment.y - (lastFragment->y + lastFragment->height);
+            else
+                m_length += fragment.x - (lastFragment->x + lastFragment->width);
+
+            lastFragment = &fragment;
+        }
+    }
 }
 
-void SVGTextPathChunkBuilder::handleTextChunk(const Vector<SVGInlineTextBox*>& boxes, unsigned boxStart, unsigned boxEnd)
+}
+
+SVGTextChunkBuilder::SVGTextChunkBuilder()
 {
-    SVGTextChunk chunk = createTextChunk(boxes, boxStart, boxEnd - boxStart);
+}
+
+void SVGTextChunkBuilder::processTextChunks(const Vector<SVGInlineTextBox*>& lineLayoutBoxes)
+{
+    if (lineLayoutBoxes.isEmpty())
+        return;
 
-    float length = 0;
-    unsigned characters = 0;
-    chunk.calculateLength(length, characters);
+    bool foundStart = false;
+    auto boxIter = lineLayoutBoxes.begin();
+    auto endBox = lineLayoutBoxes.end();
+    auto chunkStartBox = boxIter;
+    for (; boxIter != endBox; ++boxIter) {

[pdr., 2015/05/28 22:00:03]

Not your fault, but can you switch this a fancy for loop?

[fs, 2015/05/29 09:26:36]

I think that should be possible eventually, but for now we'll need access to the
iterators.

+        if (!(*boxIter)->startsNewTextChunk())
+            continue;
+
+        if (!foundStart) {
+            foundStart = true;
+        } else {
+            ASSERT(boxIter != chunkStartBox);
+            handleTextChunk(chunkStartBox, boxIter);
+        }
+        chunkStartBox = boxIter;
+    }
+
+    if (!foundStart)
+        return;
+
+    if (boxIter != chunkStartBox)
+        handleTextChunk(chunkStartBox, boxIter);
+}
+
+SVGTextPathChunkBuilder::SVGTextPathChunkBuilder()
+    : SVGTextChunkBuilder()
+    , m_totalLength(0)
+    , m_totalCharacters(0)
+    , m_totalTextAnchorShift(0)
+{
+}
+
+void SVGTextPathChunkBuilder::handleTextChunk(BoxListConstIterator boxStart, BoxListConstIterator boxEnd)
+{
+    SVGTextChunk chunk = createTextChunk(*boxStart);
+
+    ChunkLengthAccumulator lengthAccumulator(chunk.isVerticalText());
+    lengthAccumulator.processRange(boxStart, boxEnd);
 
     // Handle text-anchor as additional start offset for text paths.
-    m_totalTextAnchorShift += chunk.calculateTextAnchorShift(length);
+    m_totalTextAnchorShift += chunk.calculateTextAnchorShift(lengthAccumulator.length());
 
-    m_totalLength += length;
-    m_totalCharacters += characters;
+    m_totalLength += lengthAccumulator.length();
+    m_totalCharacters += lengthAccumulator.numCharacters();
 }
 
 static void buildSpacingAndGlyphsTransform(bool isVerticalText, float scale, const SVGTextFragment& fragment, AffineTransform& spacingAndGlyphsTransform)
@@ -161,44 +209,47 @@ static void buildSpacingAndGlyphsTransform(bool isVerticalText, float scale, con
     spacingAndGlyphsTransform.translate(-fragment.x, -fragment.y);
 }
 
-void SVGTextChunkBuilder::handleTextChunk(const Vector<SVGInlineTextBox*>& lineBoxes, unsigned boxStart, unsigned boxEnd)
+void SVGTextChunkBuilder::handleTextChunk(BoxListConstIterator boxStart, BoxListConstIterator boxEnd)
 {
-    SVGTextChunk chunk = createTextChunk(lineBoxes, boxStart, boxEnd - boxStart);
+    SVGTextChunk chunk = createTextChunk(*boxStart);
 
     bool processTextLength = chunk.hasDesiredTextLength();
     bool processTextAnchor = chunk.hasTextAnchor();
     if (!processTextAnchor && !processTextLength)
         return;
 
-    const Vector<SVGInlineTextBox*>& boxes = chunk.boxes();
-    unsigned boxCount = boxes.size();
-    if (!boxCount)
-        return;
+    bool isVerticalText = chunk.isVerticalText();
 
     // Calculate absolute length of whole text chunk (starting from text box 'start', spanning 'length' text boxes).
-    float chunkLength = 0;
-    unsigned chunkCharacters = 0;
-    chunk.calculateLength(chunkLength, chunkCharacters);
+    ChunkLengthAccumulator lengthAccumulator(isVerticalText);
+    lengthAccumulator.processRange(boxStart, boxEnd);
 
-    bool isVerticalText = chunk.isVerticalText();
     if (processTextLength) {
+        float chunkLength = lengthAccumulator.length();
         if (chunk.hasLengthAdjustSpacing()) {
-            float textLengthShift = (chunk.desiredTextLength() - chunkLength) / chunkCharacters;
+            float textLengthShift = (chunk.desiredTextLength() - chunkLength) / lengthAccumulator.numCharacters();
             unsigned atCharacter = 0;
-            for (unsigned boxPosition = 0; boxPosition < boxCount; ++boxPosition) {
-                Vector<SVGTextFragment>& fragments = boxes[boxPosition]->textFragments();
+            for (auto boxIter = boxStart; boxIter != boxEnd; ++boxIter) {
+                Vector<SVGTextFragment>& fragments = (*boxIter)->textFragments();
                 if (fragments.isEmpty())
                     continue;
                 processTextLengthSpacingCorrection(isVerticalText, textLengthShift, fragments, atCharacter);
             }
+
+            // Fragments have been adjusted, we have to recalculate the chunk
+            // length, to be able to apply the text-anchor shift.
+            if (processTextAnchor) {

[pdr., 2015/05/28 22:00:03]

(Probably for another patch)
I think this additional walk can be avoided by using textLengthShift.

[fs, 2015/05/29 09:26:36]

Yepp, I've been thinking so too, but I'll leave that to another CL (need to
unravel this a bit first.)

+                lengthAccumulator.reset();
+                lengthAccumulator.processRange(boxStart, boxEnd);
+            }
         } else {
             ASSERT(chunk.hasLengthAdjustSpacingAndGlyphs());
             float textLengthScale = chunk.desiredTextLength() / chunkLength;
             AffineTransform spacingAndGlyphsTransform;
 
             bool foundFirstFragment = false;
-            for (unsigned boxPosition = 0; boxPosition < boxCount; ++boxPosition) {
-                SVGInlineTextBox* textBox = boxes[boxPosition];
+            for (auto boxIter = boxStart; boxIter != boxEnd; ++boxIter) {
+                SVGInlineTextBox* textBox = *boxIter;
                 Vector<SVGTextFragment>& fragments = textBox->textFragments();
                 if (fragments.isEmpty())
                     continue;
@@ -216,16 +267,9 @@ void SVGTextChunkBuilder::handleTextChunk(const Vector<SVGInlineTextBox*>& lineB
     if (!processTextAnchor)
         return;
 
-    // If we previously applied a lengthAdjust="spacing" correction, we have to recalculate the chunk length, to be able to apply the text-anchor shift.
-    if (processTextLength && chunk.hasLengthAdjustSpacing()) {
-        chunkLength = 0;
-        chunkCharacters = 0;
-        chunk.calculateLength(chunkLength, chunkCharacters);
-    }
-
-    float textAnchorShift = chunk.calculateTextAnchorShift(chunkLength);
-    for (unsigned boxPosition = 0; boxPosition < boxCount; ++boxPosition) {
-        Vector<SVGTextFragment>& fragments = boxes[boxPosition]->textFragments();
+    float textAnchorShift = chunk.calculateTextAnchorShift(lengthAccumulator.length());
+    for (auto boxIter = boxStart; boxIter != boxEnd; ++boxIter) {
+        Vector<SVGTextFragment>& fragments = (*boxIter)->textFragments();
         if (fragments.isEmpty())
             continue;
         processTextAnchorCorrection(isVerticalText, textAnchorShift, fragments);