Avoid drawing emphasis marks over ellipsis glyphs

Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp

Index: Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp
diff --git a/Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp b/Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp
index 9869c5c081b9a9faa24b6a667c06ffe3bfe94d93..762674418c904c55c4d94dc5bbbd7241c2ed5de0 100644
--- a/Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp
+++ b/Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp
@@ -986,6 +986,8 @@ void HarfBuzzShaper::fillGlyphBufferForTextEmphasis(GlyphBuffer* glyphBuffer, Ha
     unsigned graphemesInCluster = 1;
     float clusterAdvance = 0;
     uint16_t clusterStart;
+    bool isRTL = m_run.rtl();

[Dominik Röttsches, 2014/11/14 10:10:47]

I wouldn't create a redundant copy here.

[Habib Virji, 2014/11/24 11:52:31]

Reason was it is used quite a lot below. Would remove it if not relevant.

+    float glyphAdv = 0;

[Dominik Röttsches, 2014/11/14 10:10:47]

Please do not use abbreviations in variable naming.
You probably want to call this "advanceSoFar" as in the non-emphasis version of
the function, or better "initialAdvance".

[Habib Virji, 2014/11/24 11:52:30]

Will update to initialAdvance.

 
     // A "cluster" in this context means a cluster as it is used by HarfBuzz:
     // The minimal group of characters and corresponding glyphs, that cannot be broken
@@ -995,23 +997,28 @@ void HarfBuzzShaper::fillGlyphBufferForTextEmphasis(GlyphBuffer* glyphBuffer, Ha
     // then linearly split the sum of corresponding glyph advances by the number of
     // grapheme clusters in order to find positions for emphasis mark drawing.
 
-    if (m_run.rtl())
-        clusterStart = currentRun->startIndex() + currentRun->numCharacters();
-    else
-        clusterStart = currentRun->startIndex() + glyphToCharacterIndexes[0];
+    clusterStart = currentRun->startIndex() + (isRTL ? currentRun->numCharacters() : glyphToCharacterIndexes[0]);

[Dominik Röttsches, 2014/11/14 10:10:47]

As far as I can see, this is functionally equivalent to the old code. Why touch
this?

[Habib Virji, 2014/11/24 11:52:31]

Agreed. The change was to ease in following code, will remove it.

 
     for (unsigned i = 0; i < numGlyphs; ++i) {
         uint16_t currentCharacterIndex = currentRun->startIndex() + glyphToCharacterIndexes[i];
         bool isRunEnd = (i + 1 == numGlyphs);
-        bool isClusterEnd =  isRunEnd || (currentRun->startIndex() + glyphToCharacterIndexes[i + 1] != currentCharacterIndex);
-        clusterAdvance += advances[i];
+        bool isClusterEnd = isRunEnd || (currentRun->startIndex() + glyphToCharacterIndexes[i + 1] != currentCharacterIndex);
 
+        if ((isRTL && currentCharacterIndex >= m_toIndex) || (!isRTL && currentCharacterIndex < m_fromIndex)) {
+            glyphAdv += advances[i];
+            isRTL ? --clusterStart : ++clusterStart;
+            continue;
+        }
+
+        if (glyphAdv) {
+            glyphBuffer->add(0, currentRun->fontData(), glyphAdv);

[Dominik Röttsches, 2014/11/14 10:10:47]

Instead of already adding a glyph here, the first glyph should be shifted by the
value in "initialAdvance".

[Habib Virji, 2014/11/24 11:52:30]

That's the issue I struggled with. Adding initialAdvance to the first glyph
results in emphasis mark on top of middle of the last 4 characters. As show
below, first 3 character glyph advances are stored in initialAdvance, but when
adding emphasis mark on top of first arabic character, it places it on the
middle of last 4 position (3 dots and 1 arabic character). Due to this reason I
had to add empty glyph for ellipsis to position, emphasis mark correctly.

For example:
 .  .. 
...الخ

+            glyphAdv = 0;
+        }
+
+        clusterAdvance += advances[i];
         if (isClusterEnd) {
-            uint16_t clusterEnd;
-            if (m_run.rtl())
-                clusterEnd = currentCharacterIndex;
-            else
-                clusterEnd = isRunEnd ? currentRun->startIndex() + currentRun->numCharacters() : currentRun->startIndex() + glyphToCharacterIndexes[i + 1];
+            uint16_t clusterEnd = isRTL ? currentCharacterIndex :

[Dominik Röttsches, 2014/11/14 10:10:47]

Same as above, code is functionally identical to the previous code, why
reformat?

[Habib Virji, 2014/11/24 11:52:30]

Acknowledged.

+                currentRun->startIndex() + (isRunEnd ? currentRun->numCharacters() : glyphToCharacterIndexes[i + 1]);
 
             graphemesInCluster = countGraphemesInCluster(m_normalizedBuffer.get(), m_normalizedBufferLength, clusterStart, clusterEnd);
             if (!graphemesInCluster || !clusterAdvance)