Unit tests for HarfBuzzShaper

Unit tests for HarfBuzzShaper

Add unit tests for HarfBuzzShaper covering segmentation based on script
and font data as well as the resolution logic for common, inherited and
script extensions. Operates as close to the API boundary as possible in
that input is a string as given to the Shaper constructor and output is
the final harfbuzz runs as supplied to the hb_buffer_add<enc> function.

R=dominik.rottsches@intel.com

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

[eae, 2015-04-22 13:32:45]

eae@chromium.org changed reviewers:
+ dominik.rottsches@intel.com

[Dominik Röttsches, 2015-04-22 14:17:51]

That's a great addition - I'm glad to have the foundations for this, now. Below,
one consideration on the testing interface, and one suggestion for reducing
repetitive code in the tests.

https://codereview.chromium.org/1088093005/diff/1/Source/platform/fonts/shapi...
File Source/platform/fonts/shaping/HarfBuzzShaper.h (right):

https://codereview.chromium.org/1088093005/diff/1/Source/platform/fonts/shapi...
Source/platform/fonts/shaping/HarfBuzzShaper.h:69: bool
runInfoForTesting(unsigned runIndex, unsigned& startIndex,
You preferred reference parameters here - I was wondering whether we may need
more info on the run here in the future (e.g. perhaps the
glyphToCharacterIndexes map). Should we expose the whole HarfBuzzRun here, or
should we only give out bits of information relevant to the test? WDYT?

https://codereview.chromium.org/1088093005/diff/1/Source/platform/fonts/shapi...
File Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp (right):

https://codereview.chromium.org/1088093005/diff/1/Source/platform/fonts/shapi...
Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:39: FontCachePurgePreventer
fontCachePurgePreventer;
These setup lines seem to be identical in all tests, can they be moved into a
test fixture?
https://code.google.com/p/googletest/wiki/Primer#Test_Fixtures:_Using_the_Sam...

[eae, 2015-04-22 14:23:26]

On 2015/04/22 14:17:51, Dominik Röttsches wrote:
> That's a great addition - I'm glad to have the foundations for this, now.
Below,
> one consideration on the testing interface, and one suggestion for reducing
> repetitive code in the tests.
> 
>
https://codereview.chromium.org/1088093005/diff/1/Source/platform/fonts/shapi...
> File Source/platform/fonts/shaping/HarfBuzzShaper.h (right):
> 
>
https://codereview.chromium.org/1088093005/diff/1/Source/platform/fonts/shapi...
> Source/platform/fonts/shaping/HarfBuzzShaper.h:69: bool
> runInfoForTesting(unsigned runIndex, unsigned& startIndex,
> You preferred reference parameters here - I was wondering whether we may need
> more info on the run here in the future (e.g. perhaps the
> glyphToCharacterIndexes map). Should we expose the whole HarfBuzzRun here, or
> should we only give out bits of information relevant to the test? WDYT?

I'd prefer to expose just what we need for now. When we need something else we
could add that.

> 
>
https://codereview.chromium.org/1088093005/diff/1/Source/platform/fonts/shapi...
> File Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp (right):
> 
>
https://codereview.chromium.org/1088093005/diff/1/Source/platform/fonts/shapi...
> Source/platform/fonts/shaping/HarfBuzzShaperTest.cpp:39:
FontCachePurgePreventer
> fontCachePurgePreventer;
> These setup lines seem to be identical in all tests, can they be moved into a
> test fixture?
>
https://code.google.com/p/googletest/wiki/Primer#Test_Fixtures:_Using_the_Sam...

Was trying to avoid making the test data driven, will look into using fixtures.
Thanks for the tip.

[eae, 2015-04-22 14:30:46]

Good call on using a fixture, made the test much simpler! Thanks!

PTAL

[d.roettsches, 2015-04-22 20:21:12]

LGTM, thanks for the quick update. (Unfortunately I can't login to my Intel
account right now, but I guess as owner you should be able to land it
nevertheless.)

[eae, 2015-04-22 20:29:24]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2015-04-22 20:29:56]

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

[commit-bot: I haz the power, 2015-04-22 20:29:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-22 20:29:58]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Dominik Röttsches, 2015-04-23 08:29:02]

LGTM, sorry for the inconvenience.

[Dominik Röttsches, 2015-04-23 08:29:16]

The CQ bit was checked by dominik.rottsches@intel.com

[commit-bot: I haz the power, 2015-04-23 08:29:40]

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

[commit-bot: I haz the power, 2015-04-23 08:39:09]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194273