Change CachingWordShapeIterator to delimit CJK characters

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 6a2b9e69cacfda40f495c8d6bec9bd90532575a4..fc7946df9b84e5250beeab35fc795bba6e306787 100644
--- a/third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h
+++ b/third_party/WebKit/Source/platform/fonts/shaping/CachingWordShapeIterator.h
@@ -112,19 +112,55 @@ private:
         if (m_startIndex >= length)
             return TextRun::Range(0, 0);
 
-        if (m_textRun[m_startIndex] == spaceCharacter
+        if (m_startIndex + 1u == length
+            || m_textRun[m_startIndex] == spaceCharacter
             || m_textRun[m_startIndex] == tabulationCharacter) {
             return TextRun::Range(m_startIndex++, 1);
         }
 
+        // Delimit every CJK character because these scripts do not delimit

[drott, 2015/12/21 10:36:01]

AFAICS we have now three modes in this file: Separate by spaces, separate for
tabulation, and separate by CJK codepoints + combining marks. 

The segmentation logic is getting really hard to read and some member variables
are starting to be wrongly named and misleading. Here we're in "nextWordRange()"
but what is returned is not a word range but a CJK "extended" character. It's
really hard to follow from bool next(RefPtr<ShapeResult>* wordResult) to how the
text actually gets split and I'd hope we can improve that.

For example in line 62 we make a decision on whether we can use caching and use
"m_shapeByWord". Can the logic for whether this is a CJK run be put there? Could
we perhaps make this into a decider which breaking logic to use? m_shapeByMode 
out of { Spaces, Tabulation, CJK characters } or so? It's hard to keep track of
what is going on if the m_shapeByWord variable does not actually tell what
breaking strategy is used.

Can we somehow reflect the breaking strategy be more clearly in the code, then
use the breaking strategy right from the next() method, instead of branching
from nextWord to nextWordRangeForCJK()?

[kojii, 2015/12/21 11:59:56]

I think that's not the correct way to see the options; I'd say:
1. We have tabulation mode on/off
2. We have word caching on/off
3. We have script-based word segmentation, currently support two flavors by code
points.

1 and 2 are independent flags that can be combined any way.
As above, that's not correct, because there are Tabulation + Spaces and
Tabulation + CJK.

I agree that the level of calls are deep. I can merge nextWordRangeForCJK() to
nextWordRnage() if you prefer; it used to be so until the patch before, but
separated thinking non-CJK might not want to see detailed CJK logic. But doing
so can decrease one level.
I think that's conceptual misunderstanding. m_shapeByWord indicates whether word
caching is possible or not. Currently we don't set it properly for CJK, assuming
its low priority, but it may be false for CJK glyphs too in future.

When word caching is enabled, we need script-based segmentation. I'd like to
make it as minimum as possible, but as you know, word segmentation with correct
i18n support is very hard. For instance, this logic does not support South Asian
scripts yet. It's not raised as an important issue so I'm currently ignoring,
but just want your understanding that the current word segmentation is very
simple.
As I said, I can merge the two functions. I'll try it in the next patch.

But I wish your understanding, "segmenting to word" **is** a very complex
problem, we're trying to implement its minimal subset here, but the minimal is
probably more complicated than we might want it to be when i18n is involved.

+        // words by spaces.
+        if (!m_textRun.is8Bit()) {
+            UChar32 ch;
+            unsigned end = m_startIndex;
+            U16_NEXT(m_textRun.characters16(), end, length, ch);
+            if (Character::isCJKIdeographOrSymbol(ch))
+                return nextWordRangeForCJK(end, ch);
+        }
+
         for (unsigned i = m_startIndex + 1; ; i++) {
             if (i == length || m_textRun[i] == spaceCharacter
-                || m_textRun[i] == tabulationCharacter) {
+                || m_textRun[i] == tabulationCharacter
+                || (!m_textRun.is8Bit() && Character::isCJKIdeographOrSymbol(m_textRun[i]))) {
                 return nextRangeToEndOffset(i);
             }
         }
     }
 
+    TextRun::Range nextWordRangeForCJK(unsigned end, UChar32 ch)
+    {
+        const unsigned length = m_textRun.length();
+        bool hasAnySpecificScript = !Character::isCommonOrInheritedScript(ch);
+        for (unsigned i = end; i < length; end = i) {
+            U16_NEXT(m_textRun.characters16(), i, length, ch);
+            // Keep Marks and Modifiers together.
+            if (U_GET_GC_MASK(ch) & (U_GC_M_MASK | U_GC_LM_MASK | U_GC_SK_MASK))
+                continue;
+            // Avoid delimiting COMMON/INHERITED alone, which makes harder to
+            // identify the script.
+            if (Character::isCJKIdeographOrSymbol(ch)) {
+                if (Character::isCommonOrInheritedScript(ch))
+                    continue;
+                if (!hasAnySpecificScript) {
+                    hasAnySpecificScript = true;
+                    continue;
+                }
+            }
+            return nextRangeToEndOffset(end);
+        }
+        return nextRangeToEndOffset(length);
+    }
+
     TextRun::Range nextRangeUntil(UChar ch)
     {
         const unsigned length = m_textRun.length();