Drop SVGTextChunk::m_boxes

Source/core/layout/svg/SVGTextChunkBuilder.cpp

 /*
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
  * This library is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 10 matching lines @@
 #include "core/layout/svg/SVGTextChunkBuilder.h"
 
 #include "core/layout/svg/LayoutSVGInlineText.h"
 #include "core/layout/svg/SVGTextChunk.h"
 #include "core/layout/svg/line/SVGInlineTextBox.h"
 #include "core/svg/SVGLengthContext.h"
 #include "core/svg/SVGTextContentElement.h"
 
 namespace blink {
 
-SVGTextChunkBuilder::SVGTextChunkBuilder()
+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.

+
+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.

 {
-}
-
-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);
-}
-
-SVGTextPathChunkBuilder::SVGTextPathChunkBuilder()
-    : SVGTextChunkBuilder()
-    , m_totalLength(0)
-    , m_totalCharacters(0)
-    , m_totalTextAnchorShift(0)
-{
-}
-
-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.
     unsigned chunkStyle = SVGTextChunk::DefaultStyle;
 
     // Handle 'direction' property.
     if (!style.isLeftToRightDirection())
         chunkStyle |= SVGTextChunk::RightToLeftText;
 
     // Handle 'writing-mode' property.
     if (svgStyle.isVerticalWritingMode())
         chunkStyle |= SVGTextChunk::VerticalText;
 
     // Handle 'text-anchor' property.
     switch (svgStyle.textAnchor()) {
     case TA_START:
         break;
     case TA_MIDDLE:
         chunkStyle |= SVGTextChunk::MiddleAnchor;
         break;
     case TA_END:
         chunkStyle |= SVGTextChunk::EndAnchor;
         break;
-    };
+    }
 
     // Handle 'lengthAdjust' property.
     float desiredTextLength = 0;
     if (SVGTextContentElement* textContentElement = SVGTextContentElement::elementFromLayoutObject(textLayoutObject.parent())) {
         SVGLengthContext lengthContext(textContentElement);
         if (textContentElement->textLengthIsSpecifiedByUser())
             desiredTextLength = textContentElement->textLength()->currentValue()->value(lengthContext);
         else
             desiredTextLength = 0;
 
         switch (textContentElement->lengthAdjust()->currentValue()->enumValue()) {
         case SVGLengthAdjustUnknown:
             break;
         case SVGLengthAdjustSpacing:
             chunkStyle |= SVGTextChunk::LengthAdjustSpacing;
             break;
         case SVGLengthAdjustSpacingAndGlyphs:
             chunkStyle |= SVGTextChunk::LengthAdjustSpacingAndGlyphs;
             break;
-        };
+        }
     }
 
-    SVGTextChunk chunk(chunkStyle, desiredTextLength);
-
-    Vector<SVGInlineTextBox*>& boxes = chunk.boxes();
-    for (unsigned i = boxStart; i < boxStart + boxCount; ++i)
-        boxes.append(lineLayoutBoxes[i]);
-
-    return chunk;
+    return SVGTextChunk(chunkStyle, desiredTextLength);
 }
 
-void SVGTextPathChunkBuilder::handleTextChunk(const Vector<SVGInlineTextBox*>& boxes, unsigned boxStart, unsigned boxEnd)
+class ChunkLengthAccumulator {
+public:
+    ChunkLengthAccumulator(bool isVertical)
+        : m_numCharacters(0)
+        , m_length(0)
+        , m_isVertical(isVertical)
+    {
+    }
+
+    typedef Vector<SVGInlineTextBox*>::const_iterator BoxListConstIterator;
+
+    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)
 {
-    SVGTextChunk chunk = createTextChunk(boxes, boxStart, boxEnd - boxStart);
+    SVGTextFragment* lastFragment = nullptr;
+    for (auto boxIter = boxStart; boxIter != boxEnd; ++boxIter) {
+        for (SVGTextFragment& fragment : (*boxIter)->textFragments()) {
+            m_numCharacters += fragment.length;
 
-    float length = 0;
-    unsigned characters = 0;
-    chunk.calculateLength(length, characters);
+            if (m_isVertical)
+                m_length += fragment.height;
+            else
+                m_length += fragment.width;
+
+            if (!lastFragment) {
+                lastFragment = &fragment;
+                continue;
+            }
+
+            // 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;
+        }
+    }
+}
+
+}
+
+SVGTextChunkBuilder::SVGTextChunkBuilder()
+{
+}
+
+void SVGTextChunkBuilder::processTextChunks(const Vector<SVGInlineTextBox*>& lineLayoutBoxes)
+{
+    if (lineLayoutBoxes.isEmpty())
+        return;
+
+    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)
 {
     spacingAndGlyphsTransform.translate(fragment.x, fragment.y);
 
     if (isVerticalText)
         spacingAndGlyphsTransform.scaleNonUniform(1, scale);
     else
         spacingAndGlyphsTransform.scaleNonUniform(scale, 1);
 
     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;
 
                 if (!foundFirstFragment) {
                     foundFirstFragment = true;
                     buildSpacingAndGlyphsTransform(isVerticalText, textLengthScale, fragments.first(), spacingAndGlyphsTransform);
                 }
 
                 m_textBoxTransformations.set(textBox, spacingAndGlyphsTransform);
             }
         }
     }
 
     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);
     }
 }
 
 void SVGTextChunkBuilder::processTextLengthSpacingCorrection(bool isVerticalText, float textLengthShift, Vector<SVGTextFragment>& fragments, unsigned& atCharacter)
 {
     unsigned fragmentCount = fragments.size();
     for (unsigned i = 0; i < fragmentCount; ++i) {
@@ skipping 32 matching lines @@
             continue;
 
         for (SVGTextFragment& fragment : textBox->textFragments()) {
             ASSERT(fragment.lengthAdjustTransform.isIdentity());
             fragment.lengthAdjustTransform = textBoxTransformation;
         }
     }
 }
 
 }