Take Accept-Language into account in CJK font fallback for Android/Win

Take Accept-Language into account in CJK font fallback for Android/Win

This patch changes the CJK font fallback for Android and Windows when
neither the content (the lang attribute) nor the UI locale can
disambiguate the Unified Han script.

The first entry in the Accept-Language user preference that can
disambiguate the Han script is taken into account.

BUG=411407
Committed: https://crrev.com/3021a101c7438ae3d7dff6369955a6d4ab196e77
Cr-Commit-Position: refs/heads/master@{#375433}

[kojii, 2016-02-12 03:33:57]

Description was changed from

==========
WIP: accept-lang-android

BUG=
==========

to

==========
Take Accept-Language into account in CJK font fallback for Android

This patch changes the CJK font fallback for Android when neither the
content (the lang attribute) nor the UI locale can disambiguate the
Unified Han script.

The first entry in the Accept-Language user preference that can
disambiguate the Han script is taken into account.

BUG=411407
==========

[kojii, 2016-02-12 08:56:16]

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

[kojii, 2016-02-12 08:56:16]

PTAL.

[drott, 2016-02-12 12:07:30]

Thanks Koji, this implementation is coming along quite quickly.

This is a bit of a review of both the Android and the Windows implementation. 

If we had the matchFamilyStyleCharacter font fallback Skia API on all platforms,
we should be actually able to unify our FontCache implementations. That's the
direction I am following up on, at least mid-to-long-term. So that's why I have
concerns about adding more platform specific code to our FontCache
implementations, that we would later need to look into unifying. So perhaps we
can do some of this already today, and reduce the platform specific code to a
minimum.

That's why I would propose to have a new target for the accept-languages change
notifications, sth. like AcceptLanguagesHanResolver that we can then use on all
platforms (in FontFallbackWin and in FontCacheAndroid). Plus: We would be able
to write a quick unittest for whether it outputs the right script codes and
bcp47 locale strings for a set of languages.

What do you think about this idea?

https://codereview.chromium.org/1693563003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/android/FontCacheAndroid.cpp
(right):

https://codereview.chromium.org/1693563003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/android/FontCacheAndroid.cpp:100: void
FontCache::acceptLanguagesChanged(const String& acceptLanguages)
Could we move this listener to a new singleton called sth like
AcceptLanguagesHanResolver, this would have roughly three methods: 

acceptLanguagesChanged(const String& acceptLanguages)
and then two methods for retrieval:
UScriptCode preferredHanScript()
char* preferredHanBcp47Locale()

The second method can internally call the first, and then practially do:
toSkFontMgrLocaleForHan.

Then the preferredHanBcp47Locale() could be used in
getFamilyNameForCharacterBelow directly, or in computeLocal for Han here.

[kojii, 2016-02-12 12:54:26]

I'm fine to split the logic out to a separate singleton class. It uses a bit of
unnecessary cycles/memory on some platforms, but I guess you think it's ok?

I'm not a big fan of matchFamilyStyleCharacter() API though, because it takes
locale strings. BCP-47 is too flexible to handle all the cases properly, so Skia
makes one assumption, ICU makes another assumption, and they're not compatible.
Whenever I need to handle locale string, I have to check which assumptions the
callers/APIs makes and adjust slightly. That probably explains why we name
"SkFontMgrLocale" because "locale" is ambiguous.

USCRIPT_* gives much cleaner definition, and faster to process. It's good enough
to disambiguate CJK if handled properly. It may or may not be able to
disambiguate Arabic, I'm interested in learning but I don't have enough
knowledge atm.

Anyway, I'll try to split the logic out to a singleton class. I'd name without
"Han" in its class name because in future we might want to use it to
disambiguate Arabic or other scripts too, I guess you're ok with it.

[kojii, 2016-02-12 13:05:44]

Ah, um, one more difference today; Win uses ICU to get the default locale while
Android uses Platform. They return slightly different locale in a mixed locale
environment, so I'm not sure whether the merging the two is ok or not.

I can keep that out of the merged logic, but it'd add slight perf hit on every
lookup. Hmm...

[drott, 2016-02-12 13:17:58]

On 2016/02/12 12:54:26, kojii wrote:
> I'm fine to split the logic out to a separate singleton class. It uses a bit
of
> unnecessary cycles/memory on some platforms, but I guess you think it's ok?

Memory: Storing one or two static members for a locale is fine, I believe.
Cycles? I would think the code in this new class would only be executed when
accept-languages changes, similar to before.

I much prefer keeping our font code clean over saving few CPU instructions.
We've worked hard to clean up platform/fonts/* after the WebKit fork. I hope
you're not too annoyed with me about constantly coming back to this.

> I'm not a big fan of matchFamilyStyleCharacter() API though, because it takes
> locale strings. BCP-47 is too flexible to handle all the cases properly, so
Skia
> makes one assumption, ICU makes another assumption, and they're not
compatible.
> Whenever I need to handle locale string, I have to check which assumptions the
> callers/APIs makes and adjust slightly. That probably explains why we name
> "SkFontMgrLocale" because "locale" is ambiguous.

It would be great to discuss how we can improve this, could you file a bug for
this (perhaps on the Skia bug tracker?) and explain this a bit there, with an
example or two how this bcp-47 argument leads to ambiguity?

> USCRIPT_* gives much cleaner definition, and faster to process. It's good
enough
> to disambiguate CJK if handled properly. It may or may not be able to
> disambiguate Arabic, I'm interested in learning but I don't have enough
> knowledge atm.
> 
> Anyway, I'll try to split the logic out to a singleton class. I'd name without
> "Han" in its class name because in future we might want to use it to
> disambiguate Arabic or other scripts too, I guess you're ok with it.

Sure, goo idea, thanks!

[drott, 2016-02-12 13:20:15]

On 2016/02/12 13:05:44, kojii wrote:
> Ah, um, one more difference today; Win uses ICU to get the default locale
while
> Android uses Platform. They return slightly different locale in a mixed locale
> environment, so I'm not sure whether the merging the two is ok or not.

Okay, would you have an example? I don't fully understand/know what such a
difference would look like. 

> I can keep that out of the merged logic, but it'd add slight perf hit on every
> lookup. Hmm...

I'd say you decide what's best there, you have a better understanding of this
difference, I think an #ifdef in the new class would be acceptable if the rest
of the parsing can stay the same.

[kojii, 2016-02-12 17:05:37]

Description was changed from

==========
Take Accept-Language into account in CJK font fallback for Android

This patch changes the CJK font fallback for Android when neither the
content (the lang attribute) nor the UI locale can disambiguate the
Unified Han script.

The first entry in the Accept-Language user preference that can
disambiguate the Han script is taken into account.

BUG=411407
==========

to

==========
Take Accept-Language into account in CJK font fallback for Android/Win

This patch changes the CJK font fallback for Android and Windows when
neither the content (the lang attribute) nor the UI locale can
disambiguate the Unified Han script.

The first entry in the Accept-Language user preference that can
disambiguate the Han script is taken into account.

BUG=411407
==========

[kojii, 2016-02-12 18:37:04]

PTAL.

[drott, 2016-02-15 08:07:58]

LGTM, very nice. Thanks for reworking this.

[kojii, 2016-02-15 08:39:12]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-02-15 08:39:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1693563003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1693563003/100001

[commit-bot: I haz the power, 2016-02-15 09:57:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:49:22]

Description was changed from

==========
Take Accept-Language into account in CJK font fallback for Android/Win

This patch changes the CJK font fallback for Android and Windows when
neither the content (the lang attribute) nor the UI locale can
disambiguate the Unified Han script.

The first entry in the Accept-Language user preference that can
disambiguate the Han script is taken into account.

BUG=411407
==========

to

==========
Take Accept-Language into account in CJK font fallback for Android/Win

This patch changes the CJK font fallback for Android and Windows when
neither the content (the lang attribute) nor the UI locale can
disambiguate the Unified Han script.

The first entry in the Accept-Language user preference that can
disambiguate the Han script is taken into account.

BUG=411407

Committed: https://crrev.com/3021a101c7438ae3d7dff6369955a6d4ab196e77
Cr-Commit-Position: refs/heads/master@{#375433}
==========

[commit-bot: I haz the power, 2016-02-16 22:49:22]

Patchset 6 (id:??) landed as
https://crrev.com/3021a101c7438ae3d7dff6369955a6d4ab196e77
Cr-Commit-Position: refs/heads/master@{#375433}