Add Unicode Variation Selector support to harfBuzzGetGlyph

Add Unicode Variation Selector support to harfBuzzGetGlyph

This patch adds variation selectors support to the callback function
harfBuzzGetGlyph.

Variation selectors in OpenType uses CMP 14 table. Neither Skia nor
Blink supports this table today. As discussed in crbug.com/180510,
using HB's CMAP 14 support is the most forward looking way to support
variation selectors.

An existing test unicode-variation-selector.html had failing result
images. This is turned to a ref test in this patch, and a case for
non-existent VS is added.

BUG=383580, 461371
TEST=fast/text/unicode-variation-selector.html

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

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

[kojii, 2015-05-27 01:17:12]

kojii@chromium.org changed reviewers:
+ eae@chromium.org

[kojii, 2015-05-27 01:21:40]

Not for landing yet, just to show how hb-ot-font is used, so COMMIT=false for
now.

This CL does not build until hb-ot-font lands:
https://codereview.chromium.org/1143213003/
and will require another CL: https://codereview.chromium.org/1144033004/
and will need rebaseline for unicode-variation-selector.html

If the direction of the fix looks right to you, when the two CLs lands, I'll run
tests, add TestExpectation, and will ask review.

[kojii, 2015-06-04 18:13:18]

PTAL.

Win bot failed for plugins, and re-trying, but this test passed.

Thank you for the other two reviews as always, I hope this should be the last
one for the basic support of UVS.

[eae, 2015-06-04 18:18:23]

eae@chromium.org changed reviewers:
+ behdad@chromium.org

[eae, 2015-06-04 18:18:23]

Looks good to me but I'd like Behdad to take a look.

[behdad_google, 2015-06-04 18:37:07]

behdad@google.com changed reviewers:
+ behdad@google.com

[behdad_google, 2015-06-04 18:37:07]

Hi Koji,

Thanks for putting this together.  I'm a bit concerned about creating a font for
every lookup.  Can you create a font from face (with no sizing info, etc) and
attach it to the face itself and just look it up?

[kojii, 2015-06-05 10:20:59]

Patchset #4 (id:60001) has been deleted

[kojii, 2015-06-05 12:03:05]

Thank you Emli and Behdad for the prompt reviews.

On 2015/06/04 18:37:07, behdad_google wrote:
> Hi Koji,
> 
> Thanks for putting this together.  I'm a bit concerned about creating a font
for
> every lookup.  Can you create a font from face (with no sizing info, etc) and
> attach it to the face itself and just look it up?

Right, I'm not really sure if I understand "attach it to the face" correctly,
but can you look at this (PS4) and see if this matches what you said?

I changed from hb_font_create_sub_font() to hb_font_create(). Without doing
that, hb_font_create_sub_font() increases the ref count and I ended up cycle
references. IIUC that's what you meant by "create a font from face"?

Note that TestExpectations was not changed but rebasing shows diff, sorry about
that.

[kojii, 2015-06-16 01:08:28]

behdad@, could you mind to have a look at this CL? I hope it shouldn't take too
long for you to see if there were more rooms for improvements.

[behdad_google, 2015-06-16 22:23:29]

lgtm!

Koji, I wonder what happens if we always use ot-face for cmap lookups.  That way
we can remove A LOT of caching layers from Blink.

Can you work on measuring the effect of that after landing this?  I have ideas
about speeding that up if performance is an issue, but I'm fairly sure it
wouldn't.  And we would save on memory.

[kojii, 2015-06-17 17:12:51]

eae@, could you PTAL?

On 2015/06/16 22:23:29, behdad_google wrote:
> lgtm!
> 
> Koji, I wonder what happens if we always use ot-face for cmap lookups.  That
way
> we can remove A LOT of caching layers from Blink.
> 
> Can you work on measuring the effect of that after landing this?  I have ideas
> about speeding that up if performance is an issue, but I'm fairly sure it
> wouldn't.  And we would save on memory.

Yeah, this is the first time I looked HarfBuzzFace seriously, I agree that we
should think about the move. Not sure if we can discard the Skia implementation
at this point, one for relying on the newer version of HB -- looks almost ok,
but the related bug is still open, I need to ask eae about this before we make
stronger dependencies -- and another for non-OT/AAT fonts.

But try ot-font first and use Skia when it returns false is something I didn't
think about. Filed crbug.com/501415.

[eae, 2015-06-17 17:18:22]

LGTM

[behdad_google, 2015-06-17 17:36:26]

On 2015/06/17 17:12:51, kojii wrote:
> eae@, could you PTAL?
> 
> On 2015/06/16 22:23:29, behdad_google wrote:
> > lgtm!
> > 
> > Koji, I wonder what happens if we always use ot-face for cmap lookups.  That
> way
> > we can remove A LOT of caching layers from Blink.
> > 
> > Can you work on measuring the effect of that after landing this?  I have
ideas
> > about speeding that up if performance is an issue, but I'm fairly sure it
> > wouldn't.  And we would save on memory.
> 
> Yeah, this is the first time I looked HarfBuzzFace seriously, I agree that we
> should think about the move. Not sure if we can discard the Skia
implementation
> at this point, one for relying on the newer version of HB -- looks almost ok,
> but the related bug is still open, I need to ask eae about this before we make
> stronger dependencies -- and another for non-OT/AAT fonts.

We don't support non-SFNT-based fonts.


> But try ot-font first and use Skia when it returns false is something I didn't
> think about. Filed crbug.com/501415.

Thanks.

The only reason we don't immediately move to the hb-ot-font layer is because for
fonts with bytecode-hinting, we still want the correctly hinted advance width. 
We should experiment with discarding that but that's less clear situation.

[kojii, 2015-06-17 23:33:48]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2015-06-17 23:34:12]

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

[commit-bot: I haz the power, 2015-06-18 02:39:12]

Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197308

[kojii, 2015-06-18 04:38:55]

Reverted at https://codereview.chromium.org/1192473003/
as this CL broke non-official Linux builds which sets use_system_harfbuzz=0

[kojii, 2015-06-18 04:46:54]

See crbug.com/462689

[kojii, 2015-06-19 03:20:49]

The CQ bit was checked by kojii@chromium.org

[kojii, 2015-06-19 03:20:49]

The patchset sent to the CQ was uploaded after l-g-t-m from behdad@google.com,
eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1149693002/#ps140001
(title: "Add -expected back that was accidentally removed during rebase")

[commit-bot: I haz the power, 2015-06-19 03:21:16]

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

[commit-bot: I haz the power, 2015-06-19 03:25:29]

Committed patchset #7 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197426