Change CachingWordShapeIterator to delimit CJK characters

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 94 matching lines @@
         m_startIndex = end;
         return TextRun::Range(start, end - start);
     }
 
     TextRun::Range nextWordRange()
     {
         const unsigned length = m_textRun.length();
         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();
         ASSERT(m_startIndex < length);
         for (unsigned i = m_startIndex + 1; ; i++) {
             if (i == length || m_textRun[i] == ch)
                 return nextRangeToEndOffset(i);
         }
     }
 
@@ skipping 54 matching lines @@
     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