Refactor word segmentation in CachingWordShapeIterator

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

 /*
  * Copyright (C) 2015 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 80 matching lines @@
         RefPtr<ShapeResult> shapeResult = shaper.shapeResult();
         if (!shapeResult)
             return nullptr;
 
         if (cacheEntry)
             cacheEntry->m_shapeResult = shapeResult;
 
         return shapeResult.release();
     }
 
+    bool shapeRange(RefPtr<ShapeResult>* result, std::pair<unsigned, unsigned> range)
+    {
+        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);
+            }
+        }
+    }
+
     bool nextWord(RefPtr<ShapeResult>* wordResult)
     {
-        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;
-            }
-
-            return nextUntilCharacterOrTab(wordResult, spaceCharacter);
-        }
-        return false;
+        return shapeRange(wordResult, nextWordRange());
     }
 
-    bool nextUntilCharacterOrTab(RefPtr<ShapeResult>* wordResult, UChar delimiter)
+    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 "!=".

     {
-        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);
             }
         }
     }
+
+    std::pair<unsigned, unsigned> nextRangeWhile(UChar ch)
+    {
+        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);
+            }
+        }
+    }
 
     bool nextForAllowTabs(RefPtr<ShapeResult>* wordResult)
     {
         unsigned length = m_textRun.length();
         if (m_startIndex >= length)
             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;
     }
 
     ShapeCache* m_shapeCache;
     const TextRun& m_textRun;
     const Font* m_font;
     float m_widthSoFar; // Used only when allowTabs()
     unsigned m_startIndex : 30;
     unsigned m_wordResultCachable : 1;
     unsigned m_shapeByWord : 1;
 };
 
 } // namespace blink
 
 #endif // CachingWordShapeIterator_h