Avoid drawing emphasis marks over ellipsis glyphs

Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp

 /*
  * Copyright (c) 2012 Google Inc. All rights reserved.
  * Copyright (C) 2013 BlackBerry Limited. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 968 matching lines @@
 {
     // FIXME: Instead of generating a synthetic GlyphBuffer here which is then used by the
     // drawEmphasisMarks method of FontFastPath, we should roll our own emphasis mark drawing function.
 
     float* advances = currentRun->advances();
     unsigned numGlyphs = currentRun->numGlyphs();
     uint16_t* glyphToCharacterIndexes = currentRun->glyphToCharacterIndexes();
     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
     // down further from a text shaping point of view.
     // A cluster can contain multiple glyphs and grapheme clusters, with mutually
     // overlapping boundaries. Below we count grapheme clusters per HarfBuzz clusters,
     // 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);
+        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)
                 continue;
 
             float glyphAdvanceX = clusterAdvance / graphemesInCluster;
             for (unsigned j = 0; j < graphemesInCluster; ++j) {
                 // Do not put emphasis marks on space, separator, and control characters.
                 Glyph glyphToAdd = Character::canReceiveTextEmphasis(m_run[currentCharacterIndex]) ? 1 : 0;
                 glyphBuffer->add(glyphToAdd, currentRun->fontData(), glyphAdvanceX);

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

So you'd need to use your accumulated initial advance here, then reset it to 0.

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

This is what i tried, but due to wrong position of emphasis mark, did not try
out.

             }
             clusterStart = clusterEnd;
             clusterAdvance = 0;
         }
     }
 }
 
 bool HarfBuzzShaper::fillGlyphBuffer(GlyphBuffer* glyphBuffer)
 {
     unsigned numRuns = m_harfBuzzRuns.size();
@@ skipping 110 matching lines @@
             point.x() + fromX, point.x() + toX,
             point.y(), height);
     }
 
     return Font::pixelSnappedSelectionRect(
         point.x() + toX, point.x() + fromX,
         point.y(), height);
 }
 
 } // namespace blink