Avoid resetting the metrics-list/character offset for each text box

Source/core/layout/svg/SVGTextLayoutEngine.cpp

 /*
  * Copyright (C) Research In Motion Limited 2010-2012. 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 21 matching lines @@
 // Set to a value > 0 to dump the text fragments
 #define DUMP_TEXT_FRAGMENTS 0
 
 namespace blink {
 
 SVGTextLayoutEngine::SVGTextLayoutEngine(Vector<SVGTextLayoutAttributes*>& layoutAttributes)
     : m_layoutAttributes(layoutAttributes)
     , m_layoutAttributesPosition(0)
     , m_logicalCharacterOffset(0)
     , m_logicalMetricsListOffset(0)
-    , m_visualCharacterOffset(0)
-    , m_visualMetricsListOffset(0)
     , m_x(0)
     , m_y(0)
     , m_dx(0)
     , m_dy(0)
     , m_isVerticalText(false)
     , m_inPathLayout(false)
     , m_textPathCalculator(0)
     , m_textPathLength(0)
     , m_textPathCurrentOffset(0)
     , m_textPathSpacing(0)
@@ skipping 51 matching lines @@
             m_dy += dy;
         }
 
         return;
     }
 
     m_dx = dx;
     m_dy = dy;
 }
 
-void SVGTextLayoutEngine::recordTextFragment(SVGInlineTextBox* textBox, const Vector<SVGTextMetrics>& textMetricsValues)
+void SVGTextLayoutEngine::recordTextFragment(SVGInlineTextBox* textBox)
 {
     ASSERT(!m_currentTextFragment.length);
-    ASSERT(m_visualMetricsListOffset > 0);
 
     // Figure out length of fragment.
-    m_currentTextFragment.length = m_visualCharacterOffset - m_currentTextFragment.characterOffset;
+    m_currentTextFragment.length = m_visualMetricsIterator.characterOffset() - m_currentTextFragment.characterOffset;
 
     // Figure out fragment metrics.
-    const SVGTextMetrics& lastCharacterMetrics = textMetricsValues.at(m_visualMetricsListOffset - 1);
+    const unsigned visualMetricsListOffset = m_visualMetricsIterator.metricsListOffset();
+    const Vector<SVGTextMetrics>& textMetricsValues = m_visualMetricsIterator.metricsList();
+    const SVGTextMetrics& lastCharacterMetrics = textMetricsValues.at(visualMetricsListOffset - 1);
     m_currentTextFragment.width = lastCharacterMetrics.width();
     m_currentTextFragment.height = lastCharacterMetrics.height();
 
     if (m_currentTextFragment.length > 1) {
         // SVGTextLayoutAttributesBuilder assures that the length of the range is equal to the sum of the individual lengths of the glyphs.
         float length = 0;
         if (m_isVerticalText) {
-            for (unsigned i = m_currentTextFragment.metricsListOffset; i < m_visualMetricsListOffset; ++i)
+            for (unsigned i = m_currentTextFragment.metricsListOffset; i < visualMetricsListOffset; ++i)
                 length += textMetricsValues.at(i).height();
             m_currentTextFragment.height = length;
         } else {
-            for (unsigned i = m_currentTextFragment.metricsListOffset; i < m_visualMetricsListOffset; ++i)
+            for (unsigned i = m_currentTextFragment.metricsListOffset; i < visualMetricsListOffset; ++i)
                 length += textMetricsValues.at(i).width();
             m_currentTextFragment.width = length;
         }
     }
 
     textBox->textFragments().append(m_currentTextFragment);
     m_currentTextFragment = SVGTextFragment();
 }
 
 bool SVGTextLayoutEngine::parentDefinesTextLength(LayoutObject* parent) const
@@ skipping 154 matching lines @@
         unsigned fragmentCount = fragments.size();
         for (unsigned i = 0; i < fragmentCount; ++i) {
             ASSERT(fragments[i].lengthAdjustTransform.isIdentity());
             fragments[i].lengthAdjustTransform = textBoxTransformation;
         }
     }
 
     boxes.clear();
 }
 
 void SVGTextLayoutEngine::finishLayout()

[pdr., 2015/05/27 03:11:18]

m_metricsList feels like a use after free waiting to happen. If we need it, can
you clear it here? Maybe m_visualMetricsIterator.clear() or just use an OwnPtr
to store the iterator and free it here.

[fs, 2015/05/27 12:53:13]

I'm not too worried about life-times myself - if the scope of
SVGTextLayoutEngine would be prolonged, there'd be plenty of other reasons to
worry. Added a "clearing" of the iterator here.

 {
     // After all text fragments are stored in their correpsonding SVGInlineTextBoxes, we can layout individual text chunks.
     // Chunk layouting is only performed for line layout boxes, not for path layout, where it has already been done.
     m_chunkLayoutBuilder.layoutTextChunks(m_lineLayoutBoxes);
 
     // Finalize transform matrices, after the chunk layout corrections have been applied, and all fragment x/y positions are finalized.
     if (!m_lineLayoutBoxes.isEmpty()) {
 #if DUMP_TEXT_FRAGMENTS > 0
         fprintf(stderr, "Line layout: ");
         dumpTextBoxes(m_lineLayoutBoxes);
 #endif
 
         finalizeTransformMatrices(m_lineLayoutBoxes);
     }
 
     if (!m_pathLayoutBoxes.isEmpty()) {
 #if DUMP_TEXT_FRAGMENTS > 0
         fprintf(stderr, "Path layout: ");
         dumpTextBoxes(m_pathLayoutBoxes);
 #endif
 
         finalizeTransformMatrices(m_pathLayoutBoxes);
     }
 }
 
+void SVGTextLayoutEngine::updateMetricsIterator(const LayoutSVGInlineText& textLayoutObject, unsigned startCharacterOffset)
+{
+    const Vector<SVGTextMetrics>* metricsList = &textLayoutObject.layoutAttributes()->textMetricsValues();

[f(malita), 2015/05/27 01:53:49]

Let's keep the prev ASSERT(!metricsList->isEmpty()).

[fs, 2015/05/27 12:53:13]

Added.

+    if (&m_visualMetricsIterator.metricsList() != metricsList)

[pdr., 2015/05/27 03:11:18]

Instead of relying on the metrics list address, can we just store an
LayoutSVGInlineText pointer and rely on it changing? This would mean changing
the iterator slightly to be something like SVGInlineTextVisualMetricsIterator.

[fs, 2015/05/27 12:53:13]

Done, except I left the "Visual" part out (it's confusing enough in the context
of SVGTextLayoutEngine, and there's no other metrics to confuse it with.)

+        m_visualMetricsIterator.reset(metricsList);
+
+    if (m_visualMetricsIterator.characterOffset() == startCharacterOffset)
+        return;
+
+    // TODO(fs): We could walk backwards through the metrics list in these cases.
+    if (m_visualMetricsIterator.characterOffset() > startCharacterOffset)
+        m_visualMetricsIterator.reset(metricsList);
+
+    while (m_visualMetricsIterator.characterOffset() < startCharacterOffset)
+        m_visualMetricsIterator.next();
+}
+
 bool SVGTextLayoutEngine::currentLogicalCharacterAttributes(SVGTextLayoutAttributes*& logicalAttributes)
 {
     if (m_layoutAttributesPosition == m_layoutAttributes.size())
         return false;
 
     logicalAttributes = m_layoutAttributes[m_layoutAttributesPosition];
     ASSERT(logicalAttributes);
 
     if (m_logicalCharacterOffset != logicalAttributes->context()->textLength())
         return true;
@@ skipping 31 matching lines @@
         }
 
         // Stop if we found the next valid logical text metrics object.
         return true;
     }
 
     ASSERT_NOT_REACHED();
     return true;
 }
 
-bool SVGTextLayoutEngine::currentVisualCharacterMetrics(SVGInlineTextBox* textBox, const Vector<SVGTextMetrics>& visualMetricsValues, SVGTextMetrics& visualMetrics)
-{
-    ASSERT(!visualMetricsValues.isEmpty());
-    unsigned textMetricsSize = visualMetricsValues.size();
-    unsigned boxStart = textBox->start();
-    unsigned boxLength = textBox->len();
-
-    while (m_visualMetricsListOffset < textMetricsSize) {
-        // Advance to text box start location.
-        if (m_visualCharacterOffset < boxStart) {
-            advanceToNextVisualCharacter(visualMetricsValues[m_visualMetricsListOffset]);
-            continue;
-        }
-
-        // Stop if we've finished processing this text box.
-        if (m_visualCharacterOffset >= boxStart + boxLength)
-            return false;
-
-        visualMetrics = visualMetricsValues[m_visualMetricsListOffset];
-        return true;
-    }
-
-    return false;
-}
-
 void SVGTextLayoutEngine::advanceToNextLogicalCharacter(const SVGTextMetrics& logicalMetrics)
 {
     ++m_logicalMetricsListOffset;
     m_logicalCharacterOffset += logicalMetrics.length();
 }
 
-void SVGTextLayoutEngine::advanceToNextVisualCharacter(const SVGTextMetrics& visualMetrics)
-{
-    ++m_visualMetricsListOffset;
-    m_visualCharacterOffset += visualMetrics.length();
-}
-
 void SVGTextLayoutEngine::layoutTextOnLineOrPath(SVGInlineTextBox* textBox, const LayoutSVGInlineText& text, const ComputedStyle& style)
 {
     if (m_inPathLayout && !m_textPathCalculator)
         return;
 
     LayoutObject* textParent = text.parent();
     bool definesTextLength = textParent ? parentDefinesTextLength(textParent) : false;
 
     const SVGComputedStyle& svgStyle = style.svgStyle();
 
-    m_visualMetricsListOffset = 0;
-    m_visualCharacterOffset = 0;
-
-    const Vector<SVGTextMetrics>& visualMetricsValues = text.layoutAttributes()->textMetricsValues();
-    ASSERT(!visualMetricsValues.isEmpty());
+    // Find the start of the current text box in the metrics list.
+    updateMetricsIterator(text, textBox->start());

[pdr., 2015/05/27 03:11:18]

Do you think something like m_visualMetricsIterator.advanceToTextStart(text,
textBox->start()) might be more clear?

[fs, 2015/05/27 12:53:13]

Done.

 
     const Font& font = style.font();
 
     SVGTextLayoutEngineSpacing spacingLayout(font, style.effectiveZoom());
     SVGTextLayoutEngineBaseline baselineLayout(font, style.effectiveZoom());
 
     bool didStartTextFragment = false;
     bool applySpacingToNextCharacter = false;
 
     float lastAngle = 0;
     float baselineShift = baselineLayout.calculateBaselineShift(style);
     baselineShift -= baselineLayout.calculateAlignmentBaselineShift(m_isVerticalText, &text);
 
     // Main layout algorithm.
-    while (true) {
-        // Find the start of the current text box in this list, respecting ligatures.
-        SVGTextMetrics visualMetrics(SVGTextMetrics::SkippedSpaceMetrics);
-        if (!currentVisualCharacterMetrics(textBox, visualMetricsValues, visualMetrics))
-            break;
-
+    const unsigned boxEndOffset = textBox->start() + textBox->len();
+    while (m_visualMetricsIterator.characterOffset() < boxEndOffset) {
+        const SVGTextMetrics& visualMetrics = m_visualMetricsIterator.metrics();
         if (visualMetrics.isEmpty()) {
-            advanceToNextVisualCharacter(visualMetrics);
+            m_visualMetricsIterator.next();
             continue;
         }
 
         SVGTextLayoutAttributes* logicalAttributes = 0;
         if (!currentLogicalCharacterAttributes(logicalAttributes))
             break;
 
         ASSERT(logicalAttributes);
         SVGTextMetrics logicalMetrics(SVGTextMetrics::SkippedSpaceMetrics);
         if (!currentLogicalCharacterMetrics(logicalAttributes, logicalMetrics))
             break;
 
         SVGCharacterDataMap& characterDataMap = logicalAttributes->characterDataMap();
         SVGCharacterData data;
         SVGCharacterDataMap::iterator it = characterDataMap.find(m_logicalCharacterOffset + 1);
         if (it != characterDataMap.end())
             data = it->value;
 
         float x = data.x;
         float y = data.y;
 
         // When we've advanced to the box start offset, determine using the original x/y values,
         // whether this character starts a new text chunk, before doing any further processing.
-        if (m_visualCharacterOffset == textBox->start())
+        if (m_visualMetricsIterator.characterOffset() == textBox->start())
             textBox->setStartsNewTextChunk(logicalAttributes->context()->characterStartsNewTextChunk(m_logicalCharacterOffset));
 
         float angle = SVGTextLayoutAttributes::isEmptyValue(data.rotate) ? 0 : data.rotate;
 
         // Calculate glyph orientation angle.
-        UChar currentCharacter = text.characterAt(m_visualCharacterOffset);
+        UChar currentCharacter = text.characterAt(m_visualMetricsIterator.characterOffset());
         float orientationAngle = baselineLayout.calculateGlyphOrientationAngle(m_isVerticalText, svgStyle, currentCharacter);
 
         // Calculate glyph advance & x/y orientation shifts.
         float xOrientationShift = 0;
         float yOrientationShift = 0;
         float glyphAdvance = baselineLayout.calculateGlyphAdvanceAndOrientation(m_isVerticalText, visualMetrics, orientationAngle, xOrientationShift, yOrientationShift);
 
         // Assign current text position to x/y values, if needed.
         updateCharacterPositionIfNeeded(x, y);
 
@@ skipping 32 matching lines @@
 
             // Calculate current offset along path.
             textPathOffset = m_textPathCurrentOffset + scaledGlyphAdvance / 2;
 
             // Move to next character.
             m_textPathCurrentOffset += scaledGlyphAdvance + m_textPathSpacing + spacing * m_textPathScaling;
 
             // Skip character, if we're before the path.
             if (textPathOffset < 0) {
                 advanceToNextLogicalCharacter(logicalMetrics);
-                advanceToNextVisualCharacter(visualMetrics);
+                m_visualMetricsIterator.next();
                 continue;
             }
 
             // Stop processing, if the next character lies behind the path.
             if (textPathOffset > m_textPathLength)
                 break;
 
             FloatPoint point;
             bool ok = m_textPathCalculator->pointAndNormalAtLength(textPathOffset, point, angle);
             ASSERT_UNUSED(ok, ok);
@@ skipping 14 matching lines @@
             y += m_dy;
         }
 
         // Determine whether we have to start a new fragment.
         bool shouldStartNewFragment = m_dx || m_dy || m_isVerticalText || m_inPathLayout || angle || angle != lastAngle
             || orientationAngle || applySpacingToNextCharacter || definesTextLength;
 
         // If we already started a fragment, close it now.
         if (didStartTextFragment && shouldStartNewFragment) {
             applySpacingToNextCharacter = false;
-            recordTextFragment(textBox, visualMetricsValues);
+            recordTextFragment(textBox);
         }
 
         // Eventually start a new fragment, if not yet done.
         if (!didStartTextFragment || shouldStartNewFragment) {
             ASSERT(!m_currentTextFragment.characterOffset);
             ASSERT(!m_currentTextFragment.length);
 
             didStartTextFragment = true;
-            m_currentTextFragment.characterOffset = m_visualCharacterOffset;
-            m_currentTextFragment.metricsListOffset = m_visualMetricsListOffset;
+            m_currentTextFragment.characterOffset = m_visualMetricsIterator.characterOffset();
+            m_currentTextFragment.metricsListOffset = m_visualMetricsIterator.metricsListOffset();
             m_currentTextFragment.x = x;
             m_currentTextFragment.y = y;
 
             // Build fragment transformation.
             if (angle)
                 m_currentTextFragment.transform.rotate(angle);
 
             if (xOrientationShift || yOrientationShift)
                 m_currentTextFragment.transform.translate(xOrientationShift, yOrientationShift);
 
@@ skipping 22 matching lines @@
 
             if (m_isVerticalText)
                 xNew -= baselineShift;
             else
                 yNew += baselineShift;
 
             updateCurrentTextPosition(xNew, yNew, glyphAdvance + spacing);
         }
 
         advanceToNextLogicalCharacter(logicalMetrics);
-        advanceToNextVisualCharacter(visualMetrics);
+        m_visualMetricsIterator.next();
         lastAngle = angle;
     }
 
     if (!didStartTextFragment)
         return;
 
     // Close last open fragment, if needed.
-    recordTextFragment(textBox, visualMetricsValues);
+    recordTextFragment(textBox);
 }
 
 }