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 1005 matching lines @@
                 clusterEnd = isRunEnd ? currentRun->startIndex() + currentRun->numCharacters() : currentRun->startIndex() + 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;
+                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;
             clusterAdvance = 0;
         }
     }
 }
 
 bool HarfBuzzShaper::fillGlyphBuffer(GlyphBuffer* glyphBuffer)
 {
@@ skipping 107 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