Refactor word segmentation in CachingWordShapeIterator

third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h

Index: third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h
diff --git a/third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h b/third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h
index 42ba703dbc07a75d2c60526340f590568a3f2493..ce44d9ca98de9a85eb971cdc768c6132dda4419a 100644
--- a/third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h
+++ b/third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h
@@ -98,35 +98,70 @@ private:
         return shapeResult.release();
     }
 
-    bool nextWord(RefPtr<ShapeResult>* wordResult)
+    bool shapeRange(RefPtr<ShapeResult>* result, std::pair<unsigned, unsigned> range)
     {
-        unsigned length = m_textRun.length();
-        if (m_startIndex < length) {
-            if (m_textRun[m_startIndex] == spaceCharacter
-                || m_textRun[m_startIndex] == tabulationCharacter) {
-                TextRun wordRun = m_textRun.subRun(m_startIndex, 1);
-                *wordResult = shapeWord(wordRun, m_font);
-                m_startIndex++;
-                return *wordResult;
+        if (!range.second)
+            return false;
+
+        if (!range.first && range.second == static_cast<unsigned>(m_textRun.length())) {
+            *result = shapeWord(m_textRun, m_font);
+            return *result;
+        }
+
+        TextRun subRun = m_textRun.subRun(range.first, range.second);
+        *result = shapeWord(subRun, m_font);
+        return *result;
+    }
+
+    std::pair<unsigned, unsigned> nextWordRange()

[drott, 2015/12/16 12:40:58]

I'd suggest making std::pair<unsigned, unsigned> a struct, with a default
constructor when returning it and use this consistently in every place where you
use this type. Perhaps even declare it in TextRun and make the subRun() method
take this type?

[kojii, 2015/12/16 12:53:34]

That one I was wondering, thank you for pointing it out. I'll try that.

+    {
+        const unsigned start = m_startIndex;
+        const unsigned length = m_textRun.length();
+        if (start >= length)
+            return std::make_pair(0, 0);
+
+        if (m_textRun[start] == spaceCharacter
+            || m_textRun[start] == tabulationCharacter) {
+            m_startIndex++;
+            return std::make_pair(start, 1);
+        }
+
+        for (unsigned i = start + 1; ; i++) {

[drott, 2015/12/16 12:40:59]

Can this loop and use nextRangeUntil as well?

[kojii, 2015/12/16 12:53:34]

This needs two characters, and possibly more other space characters. We can
split this out to yet another function to make it clearer.

+            if (i == length || m_textRun[i] == spaceCharacter
+                || m_textRun[i] == tabulationCharacter) {
+                m_startIndex = i;
+                return std::make_pair(start, i - start);
             }
+        }
+    }
 
-            return nextUntilCharacterOrTab(wordResult, spaceCharacter);
+    bool nextWord(RefPtr<ShapeResult>* wordResult)
+    {
+        return shapeRange(wordResult, nextWordRange());
+    }
+
+    std::pair<unsigned, unsigned> nextRangeUntil(UChar ch)

[drott, 2015/12/16 12:40:59]

What's the difference between nextRangeUntil and nextRangeWhile? They look
identical to me.

[kojii, 2015/12/16 12:53:34]

"Until" exists loop when "==" while "While" exists when "!=".

+    {
+        const unsigned start = m_startIndex;
+        const unsigned length = m_textRun.length();
+        ASSERT(m_startIndex < length);
+        for (unsigned i = start + 1; ; i++) {
+            if (i == length || m_textRun[i] == ch) {
+                m_startIndex = i;
+                return std::make_pair(start, i - start);
+            }
         }
-        return false;
     }
 
-    bool nextUntilCharacterOrTab(RefPtr<ShapeResult>* wordResult, UChar delimiter)
+    std::pair<unsigned, unsigned> nextRangeWhile(UChar ch)
     {
-        unsigned length = m_textRun.length();
+        const unsigned start = m_startIndex;
+        const unsigned length = m_textRun.length();
         ASSERT(m_startIndex < length);
-        for (unsigned i = m_startIndex + 1; ; i++) {
-            if (i == length || m_textRun[i] == delimiter
-                || m_textRun[i] == tabulationCharacter) {
-                TextRun wordRun = m_textRun.subRun(m_startIndex,
-                    i - m_startIndex);
+        for (unsigned i = start + 1; ; i++) {
+            if (i == length || m_textRun[i] != ch) {
                 m_startIndex = i;
-                *wordResult = shapeWord(wordRun, m_font);
-                return *wordResult;
+                return std::make_pair(start, i - start);
             }
         }
     }
@@ -138,23 +173,17 @@ private:
             return false;
 
         if (UNLIKELY(m_textRun[m_startIndex] == tabulationCharacter)) {
-            for (unsigned i = m_startIndex + 1; ; i++) {
-                if (i == length || m_textRun[i] != tabulationCharacter) {
-                    *wordResult = ShapeResult::createForTabulationCharacters(
-                        m_font, m_textRun, m_widthSoFar, i - m_startIndex);
-                    m_startIndex = i;
-                    break;
-                }
-            }
+            *wordResult = ShapeResult::createForTabulationCharacters(
+                m_font, m_textRun, m_widthSoFar,
+                nextRangeWhile(tabulationCharacter).second);
         } else if (!m_shapeByWord) {
-            if (!nextUntilCharacterOrTab(wordResult, 0))
+            if (!shapeRange(wordResult, nextRangeUntil(tabulationCharacter)))
                 return false;
         } else {
             if (!nextWord(wordResult))
                 return false;
         }
-        if (!*wordResult)
-            return false;
+        ASSERT(*wordResult);

[drott, 2015/12/16 12:40:59]

What has changed to be able upgrade this to an assertion?

[kojii, 2015/12/16 12:53:34]

Actually, nothing; even before the patch, this should never become nullptr,
because we check above and returns false already if |*wordResult|. Figured out
that this is not needed during the refactoring.

         m_widthSoFar += (*wordResult)->width();
         return true;
     }