Add tab characters support in complex path

Source/platform/fonts/shaping/HarfBuzzShaper.cpp

Index: Source/platform/fonts/shaping/HarfBuzzShaper.cpp
diff --git a/Source/platform/fonts/shaping/HarfBuzzShaper.cpp b/Source/platform/fonts/shaping/HarfBuzzShaper.cpp
index 22e181f679e9dc53fcc06f0451fa188630533f70..e73837961bc7fd662fec5ecf8f827e8e6379c1af 100644
--- a/Source/platform/fonts/shaping/HarfBuzzShaper.cpp
+++ b/Source/platform/fonts/shaping/HarfBuzzShaper.cpp
@@ -552,13 +552,15 @@ static void normalizeCharacters(const TextRun& run, unsigned length, UChar* dest
     while (position < length) {
         UChar32 character;
         U16_NEXT(source, position, length, character);
-        // Don't normalize tabs as they are not treated as spaces for word-end.
-        if (run.normalizeSpace() && Character::isNormalizedCanvasSpaceCharacter(character))
-            character = spaceCharacter;
-        else if (Character::treatAsSpace(character) && character != tabulationCharacter)
-            character = spaceCharacter;
-        else if (Character::treatAsZeroWidthSpaceInComplexScript(character))
-            character = zeroWidthSpaceCharacter;
+        if (!(character == tabulationCharacter && run.allowTabs())) {
+            // Don't normalize tabs as they are not treated as spaces for word-end.
+            if (run.normalizeSpace() && Character::isNormalizedCanvasSpaceCharacter(character))
+                character = spaceCharacter;
+            else if (Character::treatAsSpace(character) && character != tabulationCharacter)
+                character = spaceCharacter;
+            else if (Character::treatAsZeroWidthSpaceInComplexScript(character))
+                character = zeroWidthSpaceCharacter;
+        }
 
         U16_APPEND(destination, *destinationLength, length, character, error);
         ASSERT_UNUSED(error, !error);
@@ -732,8 +734,34 @@ struct CandidateRun {
     UScriptCode script;
 };
 
+static void collectCandidateRunsForTabulationCharacters(UTF16TextIterator& iterator,
+    Vector<CandidateRun>* runs, unsigned& startIndexOfCurrentRun,
+    UChar32& character, const SimpleFontData* currentFontData)
+{
+    ASSERT(character == tabulationCharacter);
+    for (iterator.advance(); iterator.consume(character); iterator.advance()) {
+        if (character != tabulationCharacter)
+            break;
+    }
+    CandidateRun run = { tabulationCharacter, startIndexOfCurrentRun, static_cast<unsigned>(iterator.offset()), currentFontData, USCRIPT_COMMON };
+    runs->append(run);
+    startIndexOfCurrentRun = iterator.offset();
+}
+
+static inline bool updateCharacterInfo(const Font* font, UChar32 character, const TextRun& textRun,

[eae, 2015/07/20 17:39:24]

Update makes it sound like it has state, how about something like this:

struct RunGlypInfo {
  const SimpleFontData* fontData;
  UScriptCode script;
}

RunGlypInfo glyphInfoForChar(cont char* font, UChar32 character, TextRun&
textRun)
{
...
}

[kojii, 2015/07/21 01:11:47]

Glad you suggested this! I once did this but it made the patch larger, and my
mentor told me to make patch smaller. I'll prioritize cleaner code than smaller
patch in future ;-)

+    const SimpleFontData*& fontData, UScriptCode& script)
+{
+    fontData = font->glyphDataForCharacter(character, false, textRun.normalizeSpace()).fontData;
+    UErrorCode errorCode = U_ZERO_ERROR;
+    script = uscript_getScript(character, &errorCode);
+    if (U_FAILURE(errorCode))
+        return false;
+    return true;
+}
+
 static inline bool collectCandidateRuns(const UChar* normalizedBuffer,
-    size_t bufferLength, const Font* font, Vector<CandidateRun>* runs, bool isSpaceNormalize)
+    size_t bufferLength, const Font* font, Vector<CandidateRun>* runs,
+    const TextRun& textRun)
 {
     UTF16TextIterator iterator(normalizedBuffer, bufferLength);
     UChar32 character;
@@ -742,21 +770,33 @@ static inline bool collectCandidateRuns(const UChar* normalizedBuffer,
     if (!iterator.consume(character))
         return false;
 
-    const SimpleFontData* nextFontData = font->glyphDataForCharacter(character, false, isSpaceNormalize).fontData;
-    UErrorCode errorCode = U_ZERO_ERROR;
-    UScriptCode nextScript = uscript_getScript(character, &errorCode);
-    if (U_FAILURE(errorCode))
+    const SimpleFontData* nextFontData;
+    UScriptCode nextScript;
+    if (!updateCharacterInfo(font, character, textRun, nextFontData, nextScript))
         return false;
 
     do {
+        // Tab characters need to go to its own run because the run doesn't use Harfbuzz.
+        if (character == tabulationCharacter && textRun.allowTabs()) {

[eae, 2015/07/20 17:39:24]

How about adding UNLIKELY macros around the tab checks to prime to branch
predictor?

if (UNLIKELY(character == tabulationCharacter && textRun.allowTabs())) {

[kojii, 2015/07/21 01:11:47]

Thought UNLIKELY() is the past. Thanks, learned one more thing.

+            collectCandidateRunsForTabulationCharacters(iterator, runs,
+                startIndexOfCurrentRun, character, nextFontData);
+            if (iterator.atEnd())
+                break;
+            if (!updateCharacterInfo(font, character, textRun, nextFontData, nextScript))
+                return false;
+        }
+
         const UChar* currentCharacterPosition = iterator.characters();
         const SimpleFontData* currentFontData = nextFontData;
         UScriptCode currentScript = nextScript;
 
         UChar32 lastCharacter = character;
         for (iterator.advance(); iterator.consume(character); iterator.advance()) {
-            if (Character::treatAsZeroWidthSpace(character))
+            if (Character::treatAsZeroWidthSpace(character)) {
+                if (character == tabulationCharacter && textRun.allowTabs())

[eae, 2015/07/20 17:39:24]

I'd move this check out of the treatAsZeroWidthSpace block,

if (character == tabulationCharacter && textRun.allowTabs()) {
...
} else if (Character::treatAsZeroWidthSpace(character)) {
...
}

[kojii, 2015/07/21 01:11:47]

That's better to read. treatAsZeroWidthSpace() checks tab, so we check tab twice
then. It's rare code so I tried less CPU cycles but I'm fine either way.

+                    break;
                 continue;
+            }
             if ((U_GET_GC_MASK(character) & U_GC_M_MASK)
                 && (Character::isUnicodeVariationSelector(character)
                     || currentFontData->canRenderCombiningCharacterSequence(
@@ -764,9 +804,7 @@ static inline bool collectCandidateRuns(const UChar* normalizedBuffer,
                         iterator.glyphEnd() - currentCharacterPosition)))
                 continue;
 
-            nextFontData = font->glyphDataForCharacter(character, false, isSpaceNormalize).fontData;
-            nextScript = uscript_getScript(character, &errorCode);
-            if (U_FAILURE(errorCode))
+            if (!updateCharacterInfo(font, character, textRun, nextFontData, nextScript))
                 return false;
             if (lastCharacter == zeroWidthJoinerCharacter)
                 currentFontData = nextFontData;
@@ -780,7 +818,7 @@ static inline bool collectCandidateRuns(const UChar* normalizedBuffer,
         runs->append(run);
 
         startIndexOfCurrentRun = iterator.offset();
-    } while (iterator.consume(character));
+    } while (!iterator.atEnd());
 
     return true;
 }
@@ -893,7 +931,7 @@ bool HarfBuzzShaper::createHarfBuzzRuns()
 
     Vector<CandidateRun> candidateRuns;
     if (!collectCandidateRuns(m_normalizedBuffer.get(),
-        m_normalizedBufferLength, m_font, &candidateRuns, m_textRun.normalizeSpace()))
+        m_normalizedBufferLength, m_font, &candidateRuns, m_textRun))
         return false;
 
     if (!resolveCandidateRuns(candidateRuns))
@@ -902,10 +940,16 @@ bool HarfBuzzShaper::createHarfBuzzRuns()
     size_t length = candidateRuns.size();
     for (size_t i = 0; i < length; ) {
         CandidateRun& run = candidateRuns[i];
+        if (run.character == tabulationCharacter && m_textRun.allowTabs()) {

[eae, 2015/07/20 17:39:24]

Is this really needed? Isn't the extra condition in the loop below enough to
keep the tab runs separate?

[kojii, 2015/07/21 01:11:47]

Yeah, this check is needed if the first run is tab, and the second is not, and
the second has the same font and USCRIPT_COMMON; e.g., "\t//".

Maybe I could use USCRIPT_INVALID or something some sentry-like value for tab
runs.

+            addHarfBuzzRun(run.start, run.end, run.fontData, run.script);
+            ++i;
+            continue;
+        }
         CandidateRun lastMatchingRun = run;
         for (i++; i < length; i++) {
             if (candidateRuns[i].script != run.script
-                || candidateRuns[i].fontData != run.fontData)
+                || candidateRuns[i].fontData != run.fontData
+                || candidateRuns[i].character == tabulationCharacter)
                 break;
             lastMatchingRun = candidateRuns[i];
         }
@@ -958,12 +1002,12 @@ static const uint16_t* toUint16(const UChar* src)
 }
 
 static inline void addToHarfBuzzBufferInternal(hb_buffer_t* buffer,
-    const FontDescription& fontDescription, const UChar* normalizedBuffer,
-    unsigned startIndex, unsigned numCharacters)
+    const FontDescription& fontDescription, const UChar* characters,
+    unsigned numCharacters)
 {
     if (fontDescription.variant() == FontVariantSmallCaps
-        && u_islower(normalizedBuffer[startIndex])) {
-        String upperText = String(normalizedBuffer + startIndex, numCharacters)
+        && u_islower(characters[0])) {
+        String upperText = String(characters, numCharacters)
             .upper();
         // TextRun is 16 bit, therefore upperText is 16 bit, even after we call
         // makeUpper().
@@ -971,7 +1015,7 @@ static inline void addToHarfBuzzBufferInternal(hb_buffer_t* buffer,
         hb_buffer_add_utf16(buffer, toUint16(upperText.characters16()),
             numCharacters, 0, numCharacters);
     } else {
-        hb_buffer_add_utf16(buffer, toUint16(normalizedBuffer + startIndex),
+        hb_buffer_add_utf16(buffer, toUint16(characters),
             numCharacters, 0, numCharacters);
     }
 }
@@ -991,6 +1035,13 @@ PassRefPtr<ShapeResult> HarfBuzzShaper::shapeHarfBuzzRuns()
     for (unsigned i = 0; i < m_harfBuzzRuns.size(); ++i) {
         unsigned runIndex = m_textRun.rtl() ? m_harfBuzzRuns.size() - i - 1 : i;
         const HarfBuzzRun* currentRun = &m_harfBuzzRuns[runIndex];
+        ASSERT(m_normalizedBufferLength >= currentRun->m_startIndex + currentRun->m_numCharacters);
+        const UChar* characters = m_normalizedBuffer.get() + currentRun->m_startIndex;
+
+        if (characters[0] == tabulationCharacter && m_textRun.allowTabs()) {
+            shapeResultForTabulationCharacters(result.get(), i, currentRun);
+            continue;
+        }
 
         const SimpleFontData* currentFontData = currentRun->m_fontData;
         FontPlatformData* platformData = const_cast<FontPlatformData*>(&currentFontData->platformData());
@@ -1008,8 +1059,7 @@ PassRefPtr<ShapeResult> HarfBuzzShaper::shapeHarfBuzzRuns()
         hb_buffer_add_utf16(harfBuzzBuffer.get(), &preContext, 1, 1, 0);
 
         addToHarfBuzzBufferInternal(harfBuzzBuffer.get(),
-            fontDescription, m_normalizedBuffer.get(), currentRun->m_startIndex,
-            currentRun->m_numCharacters);
+            fontDescription, characters, currentRun->m_numCharacters);
 
         if (fontDescription.isVerticalAnyUpright())
             face->setScriptForVerticalGlyphSubstitution(harfBuzzBuffer.get());
@@ -1067,6 +1117,7 @@ void HarfBuzzShaper::shapeResult(ShapeResult* result, unsigned index,
         float advance = harfBuzzPositionToFloat(glyphPositions[i].x_advance - glyphPositions[i].y_advance);
         unsigned currentCharacterIndex = currentRun->m_startIndex + glyphInfos[i].cluster;
         RELEASE_ASSERT(m_normalizedBufferLength > currentCharacterIndex);
+        ASSERT(m_normalizedBuffer[currentCharacterIndex] != tabulationCharacter || !m_textRun.allowTabs());
         bool isClusterEnd = runEnd || glyphInfos[i].cluster != glyphInfos[i + 1].cluster;
         float spacing = 0;
 
@@ -1103,6 +1154,27 @@ void HarfBuzzShaper::shapeResult(ShapeResult* result, unsigned index,
         result->m_fallbackFonts.add(const_cast<SimpleFontData*>(fallbackFont));
 }
 
+void HarfBuzzShaper::shapeResultForTabulationCharacters(ShapeResult* result, unsigned index, const HarfBuzzRun* currentRun)
+{
+    const SimpleFontData& fontData = *currentRun->m_fontData;
+    ShapeResult::RunInfo* run = new ShapeResult::RunInfo(currentRun->m_fontData,
+        currentRun->m_direction, currentRun->m_script, currentRun->m_startIndex,
+        currentRun->m_numCharacters, currentRun->m_numCharacters);
+    result->m_runs[index] = run;
+    result->m_numGlyphs += currentRun->m_numCharacters;
+    float position = m_textRun.xPos() + result->m_width;
+    float positionStart = position;
+    for (size_t i = 0; i < currentRun->m_numCharacters; ++i) {
+        ASSERT(m_normalizedBuffer[currentRun->m_startIndex + i] == tabulationCharacter);
+        float advance = m_font->tabWidth(fontData, m_textRun.tabSize(), position);
+        run->m_glyphData[i].characterIndex = i;
+        run->setGlyphAndPositions(i, fontData.spaceGlyph(), advance, 0, 0);
+        position += advance;
+    }
+    run->m_width = position - positionStart;
+    result->m_width += run->m_width;
+}
+
 float HarfBuzzShaper::adjustSpacing(ShapeResult::RunInfo* run, size_t glyphIndex, unsigned currentCharacterIndex, float& offset, float& totalAdvance)
 {
     float spacing = 0;