Reorder metrics iteration in LayoutSVGInlineText::updateMetricsList

third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp

Index: third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp
diff --git a/third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp b/third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp
index 3154e73ea24311e011b1b2f6700a7d9c2506d582..27da5076d205ab21e89fdc6d3895b635d7ef8c99 100644
--- a/third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp
+++ b/third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp
@@ -207,7 +207,7 @@ PositionWithAffinity LayoutSVGInlineText::positionForPoint(const LayoutPoint& po
 
 namespace {
 
-inline bool characterStartsSurrogatePair(const TextRun& run, unsigned index)
+inline bool isValidSurrogatePair(const TextRun& run, unsigned index)
 {
     if (!U16_IS_LEAD(run[index]))
         return false;
@@ -216,57 +216,7 @@ inline bool characterStartsSurrogatePair(const TextRun& run, unsigned index)
     return U16_IS_TRAIL(run[index + 1]);
 }
 
-class SVGTextMetricsCalculator {
-public:
-    SVGTextMetricsCalculator(LayoutSVGInlineText&);
-    ~SVGTextMetricsCalculator();
-
-    bool advancePosition();
-    unsigned currentPosition() const { return m_currentPosition; }
-
-    SVGTextMetrics currentCharacterMetrics();
-
-    // TODO(pdr): Character-based iteration is ambiguous and error-prone. It
-    // should be unified under a single concept. See: https://crbug.com/593570
-    bool currentCharacterStartsSurrogatePair() const
-    {
-        return characterStartsSurrogatePair(m_run, m_currentPosition);
-    }
-    bool currentCharacterIsWhiteSpace() const
-    {
-        return m_run[m_currentPosition] == ' ';
-    }
-    unsigned characterCount() const
-    {
-        return static_cast<unsigned>(m_run.charactersLength());
-    }
-
-private:
-    void setupBidiRuns();
-
-    static TextRun constructTextRun(LayoutSVGInlineText&, unsigned position, unsigned length, TextDirection);
-
-    // Ensure |m_subrunRanges| is updated for the current bidi run, or the
-    // complete m_run if no bidi runs are present. Returns the current position
-    // in the subrun which can be used to index into |m_subrunRanges|.
-    unsigned updateSubrunRangesForCurrentPosition();
-
-    // Current character position in m_text.
-    unsigned m_currentPosition;
-
-    LayoutSVGInlineText& m_text;
-    float m_fontScalingFactor;
-    float m_cachedFontHeight;
-    TextRun m_run;
-
-    BidiCharacterRun* m_bidiRun;
-    BidiResolver<TextRunIterator, BidiCharacterRun> m_bidiResolver;
-
-    // Ranges for the current bidi run if present, or the entire run otherwise.
-    Vector<CharacterRange> m_subrunRanges;
-};
-
-TextRun SVGTextMetricsCalculator::constructTextRun(LayoutSVGInlineText& text, unsigned position, unsigned length, TextDirection textDirection)
+TextRun constructTextRun(LayoutSVGInlineText& text, unsigned position, unsigned length, TextDirection textDirection)
 {
     const ComputedStyle& style = text.styleRef();
 
@@ -294,56 +244,12 @@ TextRun SVGTextMetricsCalculator::constructTextRun(LayoutSVGInlineText& text, un
     return run;
 }
 
-SVGTextMetricsCalculator::SVGTextMetricsCalculator(LayoutSVGInlineText& text)
-    : m_currentPosition(0)
-    , m_text(text)
-    , m_fontScalingFactor(m_text.scalingFactor())
-    , m_cachedFontHeight(m_text.scaledFont().getFontMetrics().floatHeight() / m_fontScalingFactor)
-    , m_run(constructTextRun(m_text, 0, m_text.textLength(), m_text.styleRef().direction()))
-    , m_bidiRun(nullptr)
-{
-    setupBidiRuns();
-}
-
-SVGTextMetricsCalculator::~SVGTextMetricsCalculator()
-{
-    m_bidiResolver.runs().deleteRuns();
-}
-
-void SVGTextMetricsCalculator::setupBidiRuns()
-{
-    BidiRunList<BidiCharacterRun>& bidiRuns = m_bidiResolver.runs();
-    bool bidiOverride = isOverride(m_text.styleRef().unicodeBidi());
-    BidiStatus status(LTR, bidiOverride);
-    if (m_run.is8Bit() || bidiOverride) {
-        WTF::Unicode::CharDirection direction = WTF::Unicode::LeftToRight;
-        // If BiDi override is in effect, use the specified direction.
-        if (bidiOverride && !m_text.styleRef().isLeftToRightDirection())
-            direction = WTF::Unicode::RightToLeft;
-        bidiRuns.addRun(new BidiCharacterRun(0, m_run.charactersLength(), status.context.get(), direction));
-    } else {
-        status.last = status.lastStrong = WTF::Unicode::OtherNeutral;
-        m_bidiResolver.setStatus(status);
-        m_bidiResolver.setPositionIgnoringNestedIsolates(TextRunIterator(&m_run, 0));
-        const bool hardLineBreak = false;
-        const bool reorderRuns = false;
-        m_bidiResolver.createBidiRunsForLine(TextRunIterator(&m_run, m_run.length()), NoVisualOverride, hardLineBreak, reorderRuns);
-    }
-    m_bidiRun = bidiRuns.firstRun();
-}
-
-bool SVGTextMetricsCalculator::advancePosition()
-{
-    m_currentPosition += currentCharacterStartsSurrogatePair() ? 2 : 1;
-    return m_currentPosition < characterCount();
-}
-
 // TODO(pdr): We only have per-glyph data so we need to synthesize per-grapheme
 // data. E.g., if 'fi' is shaped into a single glyph, we do not know the 'i'
 // position. The code below synthesizes an average glyph width when characters
 // share a single position. This will incorrectly split combining diacritics.
 // See: https://crbug.com/473476.
-static void synthesizeGraphemeWidths(const TextRun& run, Vector<CharacterRange>& ranges)
+void synthesizeGraphemeWidths(const TextRun& run, Vector<CharacterRange>& ranges)
 {
     unsigned distributeCount = 0;
     for (int rangeIndex = static_cast<int>(ranges.size()) - 1; rangeIndex >= 0; --rangeIndex) {
@@ -352,7 +258,7 @@ static void synthesizeGraphemeWidths(const TextRun& run, Vector<CharacterRange>&
             distributeCount++;
         } else if (distributeCount != 0) {
             // Only count surrogate pairs as a single character.
-            bool surrogatePair = characterStartsSurrogatePair(run, rangeIndex);
+            bool surrogatePair = isValidSurrogatePair(run, rangeIndex);
             if (!surrogatePair)
                 distributeCount++;
 
@@ -373,38 +279,38 @@ static void synthesizeGraphemeWidths(const TextRun& run, Vector<CharacterRange>&
     }
 }
 
-unsigned SVGTextMetricsCalculator::updateSubrunRangesForCurrentPosition()
+} // namespace
+
+void LayoutSVGInlineText::addMetricsFromRun(
+    const TextRun& run, bool& lastCharacterWasWhiteSpace)
 {
-    ASSERT(m_bidiRun);
-    if (m_currentPosition >= static_cast<unsigned>(m_bidiRun->stop())) {
-        m_bidiRun = m_bidiRun->next();
-        // Ensure new subrange ranges are computed below.
-        m_subrunRanges.clear();
-    }
-    ASSERT(m_bidiRun);
-    ASSERT(static_cast<int>(m_currentPosition) < m_bidiRun->stop());
-
-    unsigned positionInRun = m_currentPosition - m_bidiRun->start();
-    if (positionInRun >= m_subrunRanges.size()) {
-        TextRun subRun = constructTextRun(m_text, m_bidiRun->start(),
-            m_bidiRun->stop() - m_bidiRun->start(), m_bidiRun->direction());
-        m_subrunRanges = m_text.scaledFont().individualCharacterRanges(subRun);
-        synthesizeGraphemeWidths(subRun, m_subrunRanges);
-    }
+    Vector<CharacterRange> charRanges = scaledFont().individualCharacterRanges(run);
+    synthesizeGraphemeWidths(run, charRanges);
 
-    ASSERT(m_subrunRanges.size() && positionInRun < m_subrunRanges.size());
-    return positionInRun;
-}
+    const float cachedFontHeight = scaledFont().getFontMetrics().floatHeight() / m_scalingFactor;
+    const bool preserveWhiteSpace = styleRef().whiteSpace() == PRE;
+    const unsigned runLength = static_cast<unsigned>(run.length());
 
-SVGTextMetrics SVGTextMetricsCalculator::currentCharacterMetrics()
-{
-    unsigned currentSubrunPosition = updateSubrunRangesForCurrentPosition();
-    unsigned length = currentCharacterStartsSurrogatePair() ? 2 : 1;
-    float width = m_subrunRanges[currentSubrunPosition].width();
-    return SVGTextMetrics(length, width / m_fontScalingFactor, m_cachedFontHeight);
-}
+    // TODO(pdr): Character-based iteration is ambiguous and error-prone. It
+    // should be unified under a single concept. See: https://crbug.com/593570
+    unsigned characterIndex = 0;
+    while (characterIndex < runLength) {
+        bool currentCharacterIsWhiteSpace = run[characterIndex] == ' ';
+        if (!preserveWhiteSpace && lastCharacterWasWhiteSpace && currentCharacterIsWhiteSpace) {
+            m_metrics.append(SVGTextMetrics(SVGTextMetrics::SkippedSpaceMetrics));
+            characterIndex++;
+            continue;
+        }
 
-} // namespace
+        unsigned length = isValidSurrogatePair(run, characterIndex) ? 2 : 1;
+        float width = charRanges[characterIndex].width() / m_scalingFactor;
+
+        m_metrics.append(SVGTextMetrics(length, width, cachedFontHeight));
+
+        lastCharacterWasWhiteSpace = currentCharacterIsWhiteSpace;
+        characterIndex += length;
+    }
+}
 
 void LayoutSVGInlineText::updateMetricsList(bool& lastCharacterWasWhiteSpace)
 {
@@ -413,23 +319,33 @@ void LayoutSVGInlineText::updateMetricsList(bool& lastCharacterWasWhiteSpace)
     if (!textLength())
         return;
 
-    // TODO(pdr): This loop is too tightly coupled to SVGTextMetricsCalculator.
-    // We should refactor SVGTextMetricsCalculator to be a simple bidi run
-    // iterator and move all subrun logic to a single function.
-    SVGTextMetricsCalculator calculator(*this);
-    bool preserveWhiteSpace = styleRef().whiteSpace() == PRE;
-    do {
-        bool currentCharacterIsWhiteSpace = calculator.currentCharacterIsWhiteSpace();
-        if (!preserveWhiteSpace && lastCharacterWasWhiteSpace && currentCharacterIsWhiteSpace) {
-            m_metrics.append(SVGTextMetrics(SVGTextMetrics::SkippedSpaceMetrics));
-            ASSERT(calculator.currentCharacterMetrics().length() == 1);
-            continue;
-        }
+    TextRun run = constructTextRun(*this, 0, textLength(), styleRef().direction());
+    BidiResolver<TextRunIterator, BidiCharacterRun> bidiResolver;
+    BidiRunList<BidiCharacterRun>& bidiRuns = bidiResolver.runs();
+    bool bidiOverride = isOverride(styleRef().unicodeBidi());
+    BidiStatus status(LTR, bidiOverride);
+    if (run.is8Bit() || bidiOverride) {
+        WTF::Unicode::CharDirection direction = WTF::Unicode::LeftToRight;
+        // If BiDi override is in effect, use the specified direction.
+        if (bidiOverride && !styleRef().isLeftToRightDirection())
+            direction = WTF::Unicode::RightToLeft;
+        bidiRuns.addRun(new BidiCharacterRun(0, run.charactersLength(), status.context.get(), direction));
+    } else {
+        status.last = status.lastStrong = WTF::Unicode::OtherNeutral;
+        bidiResolver.setStatus(status);
+        bidiResolver.setPositionIgnoringNestedIsolates(TextRunIterator(&run, 0));
+        const bool hardLineBreak = false;
+        const bool reorderRuns = false;
+        bidiResolver.createBidiRunsForLine(TextRunIterator(&run, run.length()), NoVisualOverride, hardLineBreak, reorderRuns);
+    }
 
-        m_metrics.append(calculator.currentCharacterMetrics());
+    for (const BidiCharacterRun* bidiRun = bidiRuns.firstRun(); bidiRun; bidiRun = bidiRun->next()) {
+        TextRun subRun = constructTextRun(*this, bidiRun->start(), bidiRun->stop() - bidiRun->start(),
+            bidiRun->direction());
+        addMetricsFromRun(subRun, lastCharacterWasWhiteSpace);
+    }
 
-        lastCharacterWasWhiteSpace = currentCharacterIsWhiteSpace;
-    } while (calculator.advancePosition());
+    bidiResolver.runs().deleteRuns();
 }
 
 void LayoutSVGInlineText::updateScaledFont()