Optimize Complex Text Shaping and Caching

Optimize Complex Text Shaping and Caching

Change HarfBuzzShaper to return a ShapeResult instead of mutating itself
with the results from the shape operation. This allows the results to be
cached and reused without having the overhead of re-creating the shaper.

In addition this adds a new intermediary shaping step before calling the
HarfBuzzShaper; segmenting TextRuns based on spaces and shapes on a word
by word basis. In itself this adds extra overhead but when combined with
a new cache that caches the ShapeResults per word it allows for a higher
cache hit rate and enables full TextRuns to be shaped without additional
shaping as, in most cases, the full run can be satisfied based on cached
results computed for preferred width calculation or from a previous run.

The new cache replaces both the previous HarfBuzz and Font Width caches.

Word based shaping assumes that words are shaped the same way regardless
of where they occur in a sentence, as such it is currently disabled if a
given font has entry for space in either the GSUB or GPOS tables as that
indicates that the shape of a word may be affected by a space character.

If kerning and ligatures are disabled word based shaping is always used.

The current implementation hasn't been fully optimized and does a lot of
extra work yet it's quite a bit faster than our existing implementation.
Further optimization should allow even bigger wins. Similarly only width
and fillGlyphBuffer operations are currently serviced through the cache.

TEST=PerformanceTests/Layout
BUG=462643
R=behdad@chromium.org, drott@chromium.org, leviw@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198501

[eae, 2015-06-19 22:12:16]

Patchset #1 (id:1) has been deleted

[eae, 2015-06-19 22:21:29]

Patchset #1 (id:20001) has been deleted

[eae, 2015-06-19 23:17:46]

eae@chromium.org changed reviewers:
+ behdad@chromium.org, drott@chromium.org, leviw@chromium.org

[eae, 2015-06-19 23:37:17]

Ready for review.

[eae, 2015-06-19 23:39:46]

Patchset #1 (id:40001) has been deleted

[leviw_travelin_and_unemployed, 2015-06-22 18:52:09]

I didn't page all of this in, but just did a quick pass to familiarize myself.

This patch is epic, my friend.

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:395: if (!hb_font_get_glyph(font, spaceCharacter,
0, &space))
This seems worthy of a comment?

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:374: float fromX = 0;
I may have called this minX and maxX.

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/ShapeCache.h (right):

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/ShapeCache.h:14: * THIS SOFTWARE IS PROVIDED BY
GOOGLE INC. ``AS IS'' AND ANY
Are we supposed to make this change?

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/ShapeCache.h:173: if ((m_singleCharMap.size() +
m_map.size()) < s_maxSize) {
Nit: unnecessary braces.

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/ShapeCache.h:184: typedef HashMap<SmallStringKey,
ShapeCacheEntry, SmallStringKeyHash, SmallStringKeyHashTraits> Map;
Nit: Maybe a slightly more descriptive name?

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/ShapeCache.h:186: static const unsigned s_maxSize
= 10000;
Magic #1 replaced by magic #2 :p

It'd be nice if we had *some* sort of explanation about where this came from and
why it's what it is.

[eae, 2015-06-22 19:19:54]

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/F...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/F...
Source/platform/fonts/Font.cpp:395: if (!hb_font_get_glyph(font, spaceCharacter,
0, &space))
On 2015/06/22 18:52:08, leviw wrote:
> This seems worthy of a comment?

Done.

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:374: float fromX = 0;
On 2015/06/22 18:52:08, leviw wrote:
> I may have called this minX and maxX.

This method (as well as offsetForPosition) needs major cleanup, I'd prefer to do
that in a subsequent change though as this patch is already quite massive.

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
File Source/platform/fonts/shaping/ShapeCache.h (right):

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/ShapeCache.h:14: * THIS SOFTWARE IS PROVIDED BY
GOOGLE INC. ``AS IS'' AND ANY
On 2015/06/22 18:52:08, leviw wrote:
> Are we supposed to make this change?

Oops, good catch. Only for _new_ files. Thanks!

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/ShapeCache.h:173: if ((m_singleCharMap.size() +
m_map.size()) < s_maxSize) {
On 2015/06/22 18:52:08, leviw wrote:
> Nit: unnecessary braces.

Done.

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/ShapeCache.h:184: typedef HashMap<SmallStringKey,
ShapeCacheEntry, SmallStringKeyHash, SmallStringKeyHashTraits> Map;
On 2015/06/22 18:52:08, leviw wrote:
> Nit: Maybe a slightly more descriptive name?

Done.

https://codereview.chromium.org/1192223002/diff/60001/Source/platform/fonts/s...
Source/platform/fonts/shaping/ShapeCache.h:186: static const unsigned s_maxSize
= 10000;
On 2015/06/22 18:52:08, leviw wrote:
> Magic #1 replaced by magic #2 :p
> 
> It'd be nice if we had *some* sort of explanation about where this came from
and
> why it's what it is.

Done.

[drott, 2015-06-23 12:45:52]

+1 for Levi's comment, the patch is epic indeed, and revolutionary. Well done
and thank you very much for doing the heavy lifting and crafting this major
change. I think it's very good for us to know and have in the code what
information exactly we want to keep on the word level, and this change will help
us a lot in doing further optimisations, e.g. storing this information closer to
the TextRuns. Plus, the performance improvements you mention sound of course
very promising.

It wasn't easy to review - but I think I have mostly understood it now. Below, I
have a few comments after having spent more time on it, but I think it's pretty
close to ready.

https://codereview.chromium.org/1192223002/diff/100001/LayoutTests/TestExpect...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1192223002/diff/100001/LayoutTests/TestExpect...
LayoutTests/TestExpectations:1005: Bug(eae) fast/text/capitalize-boundaries.html
[ NeedsRebaseline ]
Could you perhaps explain in the changelog why we need rebaselines, or what the
test diffs look like?

After the latest patch iteration I see real failures in 
fast/text/emphasis-complex.html, fast/text/emphasis-ellipsis-complextext.html
for emphasis marks over Arabic text, indicating there might still be something
wrong.

fast/text/complex-preferred-logical-widths.html has a somewhat suspicious length
difference, originating from a difference in the space before the word "lines",
as much as I can tell. Any idea what the reason might be?

There seem to be some +1/-1 differences in vertical selection ranges, too - are
these safe?

https://codereview.chromium.org/1192223002/diff/100001/LayoutTests/fast/text/...
File LayoutTests/fast/text/justify-ideograph.html (right):

https://codereview.chromium.org/1192223002/diff/100001/LayoutTests/fast/text/...
LayoutTests/fast/text/justify-ideograph.html:4: font-family:Ahem;
In a somewhat nitpicky attempt to keep this patch smaller - could we land this
test fix separately?

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/blink_...
File Source/platform/blink_platform.gypi (right):

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/blink_...
Source/platform/blink_platform.gypi:418: 'fonts/shaping/ShapeCache.h',
Nit: Lexicographic order, before Shaper.cpp.

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
File Source/platform/fonts/Font.cpp (right):

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/Font.cpp:117: // FIXME: Populate using CachingWordShaper.
IIRC, in a couple of reviews dpranke asked me to never leave FIXME's dangling
without corresponding bugs, which I find good practice. Could you file an issue
for this issue of emphasis glyphbuffer filling and mark it as blocking 462643?

On the issue itself, I've been thinking about what it would take to solve this:
I think the problem is that we lose the original word/string, right? For
emphasis glyphBuffer filling we need the shapeResult, especially the
glyphToCharacterIndex() method, which is based on the
m_glyphData.characterIndex, but also the original string to do
countGraphemesInCluster. So, the only way to solve this would be to somehow do 
countGraphemesInCluster over the required intervals - when we need emphasis
drawing - and then keep this information with our ShapeCacheEntry / ShapeResult.

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/Font.cpp:384: const FontPlatformData& platformData =
primaryFont()->platformData();
I'd suggest to move the code from below this line to the end of the method to
FontPlatformData and then call something like hasSpaceInLigaturesOrKerning() to
combine this with typeSettingFeatures() to have your local answer for
computeCanShapeWordByWord(). My reasoning would be that the code is more low
level, actually looks at the TTF blob and that it's actually operating on
FontPlatformData.

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
File Source/platform/fonts/shaping/CachingWordShapeIterator.h (right):

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShapeIterator.h:114: if (cacheEntry &&
(!fallbackFonts || fallbackFonts->isEmpty()))
Could you add a similar FIXME + crbug for the fact that we disable caching when
the shaping process reports additional fallback fonts?

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShapeIterator.h:125: unsigned
m_isCacheable : 1;
When reviewing, I struggled to understand what this means from the name of the
variable. It's clear to me now, but perhaps we can find a better name?
m_paramsCacheable, or m_runAndFontCacheable?

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
File Source/platform/fonts/shaping/CachingWordShaperTest.cpp (right):

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShaperTest.cpp:24:
font->setCanShapeWordByWordForTesting(true);
Isn't there a small chance that the font found for the configured
FontDescription does have a space in it's kerning or ligatures tables,
potentially leading to inconsistent test results here? Could we instead assert
for the read and computed value of true here, instead of hard-setting it? Or
could we make sure we load a font known to not have spaces in the relevant
tables?

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShaperTest.cpp:74: const UChar str[] =
{ 0x2F, 0x301, 0x2E, 0x20, 0x2E, 0x0 };
I am not sure, do our source files allow UTF-8? Can we put the string spelled
out in a comment, just to make it easier to read what's expected? (Here and in
the cases below).

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:227: template<TextDirection
direction>
I found this a bit surprising, why did you chose to convert it into a templated
version? Some of the other methods in this neighbourhood keep the direction as a
parameter. For the sake of consistency I would probably suggest to leave it as
parameter? And perhaps decide separately if we want to make them all templated?
But I might have missed something.

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.h (right):

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.h:95: // sub run (represented by
RunInfo structs in the m_runs vector) can have a
...a different text direction?

[eae, 2015-06-23 17:56:44]

On 2015/06/23 12:45:52, drott wrote:
> After the latest patch iteration I see real failures in 
> fast/text/emphasis-complex.html, fast/text/emphasis-ellipsis-complextext.html
> for emphasis marks over Arabic text, indicating there might still be something
> wrong.

Not quite sure why that is given that I didn't change anything but the
expectations between patch 1 and 2 and the tests pass in patch 1...

> fast/text/complex-preferred-logical-widths.html has a somewhat suspicious
length
> difference, originating from a difference in the space before the word
"lines",
> as much as I can tell. Any idea what the reason might be?

Looking into that now, some fonts appear to given slightly different widths for
the space when shaped as a part of a sentence as opposed to in isolation even
when they have no entry in the GPOS/GSUB tables for space.

>
https://codereview.chromium.org/1192223002/diff/100001/LayoutTests/fast/text/...
> LayoutTests/fast/text/justify-ideograph.html:4: font-family:Ahem;
> In a somewhat nitpicky attempt to keep this patch smaller - could we land this
> test fix separately?

Sure


> Source/platform/fonts/Font.cpp:117: // FIXME: Populate using
CachingWordShaper.
> IIRC, in a couple of reviews dpranke asked me to never leave FIXME's dangling
> without corresponding bugs, which I find good practice. Could you file an
issue
> for this issue of emphasis glyphbuffer filling and mark it as blocking 462643?

Will do.


> On the issue itself, I've been thinking about what it would take to solve
this:
> I think the problem is that we lose the original word/string, right? For
> emphasis glyphBuffer filling we need the shapeResult, especially the
> glyphToCharacterIndex() method, which is based on the
> m_glyphData.characterIndex, but also the original string to do
> countGraphemesInCluster. So, the only way to solve this would be to somehow do

> countGraphemesInCluster over the required intervals - when we need emphasis
> drawing - and then keep this information with our ShapeCacheEntry /
ShapeResult.

It's actually pretty easy, earlier versions of this patch had an implementation,
ShapeResult combined with the run has enough information.


>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> Source/platform/fonts/Font.cpp:384: const FontPlatformData& platformData =
> primaryFont()->platformData();
> I'd suggest to move the code from below this line to the end of the method to
> FontPlatformData and then call something like hasSpaceInLigaturesOrKerning()
to
> combine this with typeSettingFeatures() to have your local answer for
> computeCanShapeWordByWord(). My reasoning would be that the code is more low
> level, actually looks at the TTF blob and that it's actually operating on
> FontPlatformData.

Sounds like a good idea, will give it a try.

> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> File Source/platform/fonts/shaping/CachingWordShapeIterator.h (right):
> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> Source/platform/fonts/shaping/CachingWordShapeIterator.h:114: if (cacheEntry
&&
> (!fallbackFonts || fallbackFonts->isEmpty()))
> Could you add a similar FIXME + crbug for the fact that we disable caching
when
> the shaping process reports additional fallback fonts?

Sure.

> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> Source/platform/fonts/shaping/CachingWordShapeIterator.h:125: unsigned
> m_isCacheable : 1;
> When reviewing, I struggled to understand what this means from the name of the
> variable. It's clear to me now, but perhaps we can find a better name?
> m_paramsCacheable, or m_runAndFontCacheable?

Will try to come up with a better name.

> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> File Source/platform/fonts/shaping/CachingWordShaperTest.cpp (right):
> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> Source/platform/fonts/shaping/CachingWordShaperTest.cpp:24:
> font->setCanShapeWordByWordForTesting(true);
> Isn't there a small chance that the font found for the configured
> FontDescription does have a space in it's kerning or ligatures tables,
> potentially leading to inconsistent test results here? Could we instead assert
> for the read and computed value of true here, instead of hard-setting it? Or
> could we make sure we load a font known to not have spaces in the relevant
> tables?

Will make the test more robust.

> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> Source/platform/fonts/shaping/CachingWordShaperTest.cpp:74: const UChar str[]
=
> { 0x2F, 0x301, 0x2E, 0x20, 0x2E, 0x0 };
> I am not sure, do our source files allow UTF-8? Can we put the string spelled
> out in a comment, just to make it easier to read what's expected? (Here and in
> the cases below).
> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):
> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> Source/platform/fonts/shaping/HarfBuzzShaper.cpp:227: template<TextDirection
> direction>
> I found this a bit surprising, why did you chose to convert it into a
templated
> version? Some of the other methods in this neighbourhood keep the direction as
a
> parameter. For the sake of consistency I would probably suggest to leave it as
> parameter? And perhaps decide separately if we want to make them all
templated?
> But I might have missed something.
> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> File Source/platform/fonts/shaping/HarfBuzzShaper.h (right):
> 
>
https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
> Source/platform/fonts/shaping/HarfBuzzShaper.h:95: // sub run (represented by
> RunInfo structs in the m_runs vector) can have a
> ...a different text direction?

[eae, 2015-06-23 22:25:15]

Found a problem with how selections where handled and updated the selection code
to use caching shaper, had hoped to do that in a subsequent patch but this way
it's guaranteed to match the glyph positioning.

PTAL.

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1192223002/diff/100001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:227: template<TextDirection
direction>
On 2015/06/23 12:45:52, drott wrote:
> I found this a bit surprising, why did you chose to convert it into a
templated
> version? Some of the other methods in this neighbourhood keep the direction as
a
> parameter. For the sake of consistency I would probably suggest to leave it as
> parameter? And perhaps decide separately if we want to make them all
templated?
> But I might have missed something.

Less code duplication than having two copies of the loop as we had before
without the overhead of having to check for each glyph. This is the most
performance sensitive of all the methods in here, none of the others are really
on the critical path.,

[eae, 2015-06-24 00:12:19]

See https://blink.gs/results/1192223002 for test results. Looking into the
fast/css/text-overflow-ellipsis-vertical-select.html failure at the moment. 

No idea as of yet why the emphasis tests only fail on mac or why vertical
selection is off on mac but no other platform. If you have any ideas please
speak up.

[eae, 2015-06-24 06:35:45]

emphasis is still broken in some cases as is vertical selection on mac, and only
mac. Will look into both of those issues tomorrow.

The svg failures on windows are due to kerning with one specific font. Behdad is
looking into that.
fast/css/text-overflow-ellipsis-vertical-select.html is due to slimming paint,
pdr is investigating.
The remaining few one pixel changes are due to differences in rounding and
snapping and are fine.

[drott, 2015-06-24 07:56:58]

> No idea as of yet why the emphasis tests only fail on mac or why vertical
> selection is off on mac but no other platform. If you have any ideas please
> speak up.

I am not 100% sure - but perhaps this helps: The emphasis marks are basically
placed placed by linear interpolation, there are placed for each grapheme in a
cluster. If two many are drawn, this is due to this equation in
fillGlyphBufferForTextEmphasis going wrong. On Mac, we did previously discuss an
issue with the HarfBuzz cluster information being off, Behdad fixed it but
mentioned that there might be additional issues:

https://github.com/behdad/harfbuzz/issues/49

Interestingly, I reported this after facing a selection issue, so it might help
with the vertical selection as well.

An alternative slightly far-fetched idea for the selection issues: Maybe it
could have something do with m_spaceShape being odd on Mac - just based on the
trouble we had with printable / non-printable.

[drott, 2015-06-30 15:07:57]

https://codereview.chromium.org/1192223002/diff/160001/Source/platform/fonts/...
File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):

https://codereview.chromium.org/1192223002/diff/160001/Source/platform/fonts/...
Source/platform/fonts/shaping/HarfBuzzShaper.cpp:64: struct ShapeResult::RunInfo
{
One more comment perhaps - can we move ShapeResult to its own file? Perhaps that
makes it easier to separate our downstream interface to HarfBuzz and the
Blink-internal caching logic.

[eae, 2015-06-30 17:12:12]

On 2015/06/30 15:07:57, drott wrote:
>
https://codereview.chromium.org/1192223002/diff/160001/Source/platform/fonts/...
> File Source/platform/fonts/shaping/HarfBuzzShaper.cpp (right):
> 
>
https://codereview.chromium.org/1192223002/diff/160001/Source/platform/fonts/...
> Source/platform/fonts/shaping/HarfBuzzShaper.cpp:64: struct
ShapeResult::RunInfo
> {
> One more comment perhaps - can we move ShapeResult to its own file? Perhaps
that
> makes it easier to separate our downstream interface to HarfBuzz and the
> Blink-internal caching logic.

I'm not sure it's the right interface yet which is why it is local to the
HarfBuzzShaper.cpp file, to ensure that no outside code starts depending on the
implementation and instead treat it as an opaque type.

[eae, 2015-06-30 19:03:43]

Patchset #7 (id:180001) has been deleted

[eae, 2015-06-30 23:45:14]

Patchset #7 (id:200001) has been deleted

[drott, 2015-07-01 09:10:27]

> I'm not sure it's the right interface yet which is why it is local to the
> HarfBuzzShaper.cpp file, to ensure that no outside code starts depending on
the
> implementation and instead treat it as an opaque type.

Makes sense, thanks for explaining the reasoning.

[eae, 2015-07-07 02:01:10]

Finally managed to fixed emphasis painting (yay!), still some problems with
vertical selection on mac, ellipsis painting and
complex-text-spaces-selection.html but getting closer! Perf still looks great
with a 400%+ win on most tests.

[eae, 2015-07-07 20:30:22]

Patchset #8 (id:240001) has been deleted

[eae, 2015-07-07 22:22:07]

This is now ready for review. The latest patch-set fixes selection and ellipsis
painting. The mac vertical text selection differences are due to the tests
running without subpixel antialiasing and does not reproduce in stand-alone
content_shell or chrome.

[drott, 2015-07-08 13:00:59]

Impressive work!

LGTM. Just have one nit on the test SetUp().

Out of curiosity: What was the fix for ellipsis drawing? "Take directionality
into account for single glyph cache entries" is the heading for patch set 8 but
since it's a rebase as well, I think, I couldn't extract the difference to 7.
Well done for figuring out those details - the ellipsis code is a nuisance :-)

https://codereview.chromium.org/1192223002/diff/280001/Source/platform/fonts/...
File Source/platform/fonts/shaping/CachingWordShaperTest.cpp (right):

https://codereview.chromium.org/1192223002/diff/280001/Source/platform/fonts/...
Source/platform/fonts/shaping/CachingWordShaperTest.cpp:24:
font->setCanShapeWordByWordForTesting(true);
Probably a rare case, but I feel like if the a case is hit, it would be pretty
hard to debug, that's why I bring it up again.

How about just the following instead:
ASSERT_TRUE(font->canShapeWordByWord()); // We need a font that is suitable for
word-based caching for testing.

If the acquired font here would have space-ligatures, it would cause test
failures in CommonAccentLeftToRightFillGlyphBuffer and
CommonAccentRightToLeftFillGlyphBuffer that are due to the fact that word-based
caching was forced-on even though the font doesn't support it. But the intention
of the tests is only to guarantee that non-caching and caching path produce the
same results under the assumption the font canShapeWordByWord().

[eae, 2015-07-08 14:20:17]

On 2015/07/08 13:00:59, drott wrote:
> Impressive work!
> 
> LGTM. Just have one nit on the test SetUp().
> 
> Out of curiosity: What was the fix for ellipsis drawing? "Take directionality
> into account for single glyph cache entries" is the heading for patch set 8
but
> since it's a rebase as well, I think, I couldn't extract the difference to 7.
> Well done for figuring out those details - the ellipsis code is a nuisance :-)
> 
>
https://codereview.chromium.org/1192223002/diff/280001/Source/platform/fonts/...
> File Source/platform/fonts/shaping/CachingWordShaperTest.cpp (right):
> 
>
https://codereview.chromium.org/1192223002/diff/280001/Source/platform/fonts/...
> Source/platform/fonts/shaping/CachingWordShaperTest.cpp:24:
> font->setCanShapeWordByWordForTesting(true);
> Probably a rare case, but I feel like if the a case is hit, it would be pretty
> hard to debug, that's why I bring it up again.
> 
> How about just the following instead:
> ASSERT_TRUE(font->canShapeWordByWord()); // We need a font that is suitable
for
> word-based caching for testing.

Ideally I'd like to set up the font using a specific, named, font. We don't have
any good way of doing that without making platform/ depend on the entire
core/css stack though. We really should add a Font constructor that takes a font
filename or something similar. For now we always fall back on the last-resort
fallback font.

Anyway, adding an assert, as suggested, is a good idea and will help us catch
cases where the font is not as expected. Thanks!

[jbroman, 2015-07-08 14:39:39]

On 2015/07/08 14:20:17, eae wrote:
> Ideally I'd like to set up the font using a specific, named, font. We don't
have
> any good way of doing that without making platform/ depend on the entire
> core/css stack though. We really should add a Font constructor that takes a
font
> filename or something similar. For now we always fall back on the last-resort
> fallback font.
> 
> Anyway, adding an assert, as suggested, is a good idea and will help us catch
> cases where the font is not as expected. Thanks!

For what it's worth, this worked for me in a unit test (that never landed
because
the CL it was in was abandoned):
https://gist.github.com/jeremyroman/255e8b820c7d64479af1

Maybe something like that would work as a unit test utility?

[eae, 2015-07-08 14:43:31]

> For what it's worth, this worked for me in a unit test (that never landed
> because
> the CL it was in was abandoned):
> https://gist.github.com/jeremyroman/255e8b820c7d64479af1
> 
> Maybe something like that would work as a unit test utility?

Oh, interesting. Thanks jbroman!

[eae, 2015-07-08 15:43:48]

The CQ bit was checked by eae@chromium.org

[eae, 2015-07-08 15:43:54]

The patchset sent to the CQ was uploaded after l-g-t-m from drott@chromium.org
Link to the patchset: https://codereview.chromium.org/1192223002/#ps260002
(title: "Change CachingWordShaperTest as suggested")

[commit-bot: I haz the power, 2015-07-08 15:44:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1192223002/260002

[commit-bot: I haz the power, 2015-07-08 15:50:20]

Committed patchset #10 (id:260002) as
https://src.chromium.org/viewvc/blink?view=rev&revision=198501