Add tab characters support in complex path

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 20 matching lines @@
 #include "platform/fonts/shaping/HarfBuzzShaper.h"
 #include "platform/fonts/shaping/ShapeCache.h"
 #include "wtf/text/CharacterNames.h"
 
 namespace blink {
 
 class CachingWordShapeIterator {
 public:
     CachingWordShapeIterator(ShapeCache* cache, const TextRun& run,
         const Font* font)
-        : m_shapeCache(cache), m_textRun(run), m_font(font), m_startIndex(0)
+        : m_shapeCache(cache), m_textRun(run), m_font(font)
+        , m_widthSoFar(0), m_startIndex(0)
     {
         ASSERT(font);
         const FontDescription& fontDescription = font->fontDescription();
 
         // Word and letter spacing can change the width of a word, as can tabs
         // as we segment solely based on on space characters.
         // If expansion is used (for justified text) the spacing between words
         // change and thus we need to shape the entire run.
         m_wordResultCachable = !fontDescription.wordSpacing()
             && !fontDescription.letterSpacing() && !run.allowTabs()
             && m_textRun.expansion() == 0.0f;
 
         // Shaping word by word is faster as each word is cached. If we cannot
         // use the cache or if the font doesn't support word by word shaping
         // fall back on shaping the entire run.
         m_shapeByWord = m_wordResultCachable && m_font->canShapeWordByWord();
     }
 
     bool next(RefPtr<ShapeResult>* wordResult)
     {
         if (!m_shapeByWord) {
+            if (UNLIKELY(m_textRun.allowTabs()))
+                return nextForAllowTabs(wordResult);
             if (m_startIndex)
                 return false;
             *wordResult = shapeWord(m_textRun, m_font);
             m_startIndex = 1;
             return *wordResult;
         }
 
         unsigned length = m_textRun.length();
         if (m_startIndex < length) {
             if (m_textRun[m_startIndex] == spaceCharacter) {
                 TextRun wordRun = m_textRun.subRun(m_startIndex, 1);
                 *wordResult = shapeWord(wordRun, m_font);
                 m_startIndex++;
                 return true;
             }
 
-            for (unsigned i = m_startIndex; ; i++) {
-                if (i == length || m_textRun[i] == spaceCharacter) {
-                    TextRun wordRun = m_textRun.subRun(m_startIndex,
-                        i - m_startIndex);
-                    *wordResult = shapeWord(wordRun, m_font);
-                    m_startIndex = i;
-                    return true;
-                }
-            }
+            *wordResult = shapeWordUntilCharacter(spaceCharacter);
+            return true;

[eae, 2015/08/10 17:24:07]

Shouldn't this be "return *wordResult != nullptr;"?

[kojii, 2015/08/11 07:22:21]

Done. The fix was moved to nextUntilCharacterOrTab() below so that compiler can
generate jmp instead of call.

Also applied the same fix for spaceCharacter case (moved to nextWord() below.)
These fixes change the behavior of next() slightly, I hope to be more correct.

         }
+
         return false;
     }
 
 private:
     PassRefPtr<ShapeResult> shapeWord(const TextRun& wordRun, const Font* font)
     {
         ShapeCacheEntry* cacheEntry = m_wordResultCachable
             ? m_shapeCache->add(wordRun, ShapeCacheEntry())
             : nullptr;
         if (cacheEntry && cacheEntry->m_shapeResult)
             return cacheEntry->m_shapeResult;
 
         HarfBuzzShaper shaper(font, wordRun);
         RefPtr<ShapeResult> shapeResult = shaper.shapeResult();
         if (!shapeResult)
             return nullptr;
 
         if (cacheEntry)
             cacheEntry->m_shapeResult = shapeResult;
 
         return shapeResult.release();
     }
 
+    PassRefPtr<ShapeResult> shapeWordUntilCharacter(UChar delimiter)
+    {
+        unsigned length = m_textRun.length();
+        ASSERT(m_startIndex < length);
+        for (unsigned i = m_startIndex + 1;; i++) {
+            if (i == length || m_textRun[i] == delimiter) {
+                TextRun wordRun = m_textRun.subRun(m_startIndex,
+                    i - m_startIndex);
+                m_startIndex = i;
+                return shapeWord(wordRun, m_font);
+            }
+        }

[eae, 2015/08/10 17:24:07]

Missing "return nullptr"?

[kojii, 2015/08/11 07:22:20]

The loop exits only with "return", so it's not necessary. I added
ASSERT_NOT_REACHED() for clarification, but compiler claimed that the code is
not reachable.

+    }
+
+    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;
+                }
+            }
+        } else {
+            *wordResult = shapeWordUntilCharacter(tabulationCharacter);

[eae, 2015/08/10 17:24:07]

Shouldn't this still segment on space? I.e. shape until either a space or a
tabulation character is encountered?

[kojii, 2015/08/11 07:22:21]

Good point, we need to do that if we want to cache words in allowTabs(). I
wasn't trying to do so, but after trying, it wasn't as complicated as thought.

PS13 adds this capability. Now we need to check allowTabs() on every next()
call. Is this ok? If you don't think the cost is worth the value, I can revert
to PS12 (which reflects your other comments.)

It turns out that <select> implies allowTabs(), so I needed to rebaseline for
sub-pixel changes caused by the segmenting. Image diff below:
https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

+        }
+        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