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 a84b151747c66eb448ee6e00d5952e2049997bcc..96a0c81afcd7fa5f6c45b23db58ba8fd44231b6d 100644
--- a/Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp
+++ b/Source/platform/fonts/harfbuzz/HarfBuzzShaper.cpp
@@ -1023,6 +1023,8 @@ void HarfBuzzShaper::fillGlyphBufferForTextEmphasis(GlyphBuffer* glyphBuffer, Ha
             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;
+                if (m_run.rtl() && currentCharacterIndex >= m_toIndex)
+                    glyphToAdd = 0;

[Dominik Röttsches, 2014/09/30 12:27:00]

Can m_toIndex be factored into the clusterEnd computation. Then there will not
be a need for special casing here.

[Habib Virji, 2014/09/30 13:28:40]

Reason to add special handling was returning before graphemes cluster loop was
resulting in emphasis marks were not corresponding to letters.
Other issue was clusterEnd, just holds index. What value of index should be set
to get glyphToAdd as zero.

[Dominik Röttsches, 2014/09/30 13:48:24]

Yes, I understood your analysis. Please take a look at graphemesInCluster, which
is used in the loop termination condition: j < graphemesInCluster. Then check
countGraphemesInCluster - there a clusterEnd is specified as argument.
clusterEnd is using all characters of the run - I think the bounds check against
m_toIndex should be in line 1016, where clusterEnd is calculated. Then there is
no need for extra special casing during the loop.

[Dominik Röttsches, 2014/10/01 07:35:35]

I looked at this a bit more closely again - this fix works, but we would still
be generating too large GlyphBuffers. We just keep filling the GlyphBuffer with
0 glyphs, for which no emphasis marks get drawn down the line. This can be okay
as a first iteration - then we can put a FIXME here and say that in fact the
glyphBuffer should not get populated with extra 0s, which get placed there
because we're not taking the m_fromIndex and m_toIndex into account correctly.

If we go for this kind of initial fix: The opposite test cases - and I would ask
you to try that - can be constructed, too: Where the ellipsis is drawn on the
other side, and there is an emphasis drawn over it because we're not taking
m_fromIndex into account either. Then the fix would need to be extended for
that.

The right way to fix this, however, is similar to what the 
fillGlyphBufferFromHarfBuzzRun method is doing: When we're still outside the
bounds of [m_fromIndex, m_toIndex] we should shift the start offset of the glyph
buffer until we reach the start position, and only then add glyphs to it that
are within those bounds. That way the glyphBuffer size is constrained to
actually placing glyphs in it for characters that are within the [m_fromIndex,
m_toIndex] range.

I appreciate your analysis of the issue, and you found the root cause. That's
great -  It'd be great now if you could fix the problem at the source.

Please note that Emil landed some changes to HarfBuzzShaper in
https://codereview.chromium.org/607483002/ on splitting advances and offsets -
make sure you are rebasing on top of that one.

                 glyphBuffer->add(glyphToAdd, currentRun->fontData(), glyphAdvanceX);
             }
             clusterStart = clusterEnd;