Add Segoe UI and its set of linked fonts to the font back list on Windows.

Add Segoe UI and its set of linked fonts to the font back list on Windows.

This is to attempt to fallback to a font which could help render text on Windows installations where the
default font is a raster font like System, etc.

Segoe UI provides fallbacks for Korean, Japanese, Chinese, Armenian, etc.

While this would not cover all cases, it is a reasonable addition.

BUG=467459
TEST=Install Windows 7 Korean edition. Change the system font to system. Run Chrome without this fix. Navigate to some korean websites. Observe that the tab title shows up empty. With this fix the title should render correctly.

Committed: https://crrev.com/4ceae8dcde9e2f60577681fe747bbbb7ce017d3d
Cr-Commit-Position: refs/heads/master@{#321077}

[ananta, 2015-03-17 22:58:55]

ananta@chromium.org changed reviewers:
+ asvitkine@chromium.org, scottmg@chromium.org

[scottmg, 2015-03-17 23:20:05]

lgtm

[Alexei Svitkine (slow), 2015-03-18 03:51:34]

lgtm, but I'm ooo - so please don't set me as reviewer for urgent cls like this

[scottmg, 2015-03-18 03:54:48]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2015-03-18 03:54:57]

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

[commit-bot: I haz the power, 2015-03-18 04:53:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-18 04:53:55]

Patchset 1 (id:??) landed as
https://crrev.com/4ceae8dcde9e2f60577681fe747bbbb7ce017d3d
Cr-Commit-Position: refs/heads/master@{#321077}

[msw, 2015-03-18 15:53:22]

msw@chromium.org changed reviewers:
+ msw@chromium.org

[msw, 2015-03-18 15:53:23]

https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1300: GetFallbackFontFamilies("Segoe UI");
(1) Why does the bug only occur with DirectWrite? (users claim that
--disable-directwrite-for-ui is a workaround, how does that mitigate the
issue?). (2) Why does/should this change affect GDI? (3) What happens here if
"Segoe UI" isn't installed? (it's not included with Windows XP by default,
afaict...) (4) Shouldn't we avoid duplicate fonts in the fallback list?
(especially if Segoe UI is already the default font, I think this will *double*
the number of shaping passes for runs that need fallback?). (5) Why don't we use
Tahoma or Microsoft Sans Serif (available on XP and additionally support Hebrew
and Thai characters, beyond Latin/Greek/Cyrillic).

I'm dubious that this is the correct change...

[scottmg, 2015-03-18 16:13:04]

https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1300: GetFallbackFontFamilies("Segoe UI");
On 2015/03/18 15:53:23, msw wrote:
> (1) Why does the bug only occur with DirectWrite? (users claim that
> --disable-directwrite-for-ui is a workaround, how does that mitigate the
> issue?). (2) Why does/should this change affect GDI? (3) What happens here if
> "Segoe UI" isn't installed? (it's not included with Windows XP by default,
> afaict...) (4) Shouldn't we avoid duplicate fonts in the fallback list?
> (especially if Segoe UI is already the default font, I think this will
*double*
> the number of shaping passes for runs that need fallback?). (5) Why don't we
use
> Tahoma or Microsoft Sans Serif (available on XP and additionally support
Hebrew
> and Thai characters, beyond Latin/Greek/Cyrillic).
> 
> I'm dubious that this is the correct change...

It's definitely quite a mess. We did reproduce corrupted characters in GDI too,
so it's possible we're not reproing the same thing that users are seeing, it's
not clear.

Blink does this, which perhaps we need to consider doing:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I didn't realize duplicating entries in the list was expensive. If there's no
missing glyphs (i.e. Segoe UI is already in the list and helps) then it won't
get here anyway, so maybe it it shouldn't matter in the common case. (?)

Tahoma might be a better choice. We only have reports from Win7 and Win8
currently where Segoe should be available.

[msw, 2015-03-18 17:10:38]

On 2015/03/18 16:13:04, scottmg wrote:
> Blink does this, which perhaps we need to consider doing:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Yikes! (but yeah, something like this has been on my mind for a while, more-so
in the form of extending the default gfx::FontList to contain certain fonts
based on the system and chrome profile languages)

> I didn't realize duplicating entries in the list was expensive. If there's no
> missing glyphs (i.e. Segoe UI is already in the list and helps) then it won't
> get here anyway, so maybe it it shouldn't matter in the common case. (?)

Yeah, it shouldn't affect the common case (the shaping loop should quit if all
glyphs are valid), but this *will* hurt runs requiring fallback, which has been
(and may still be) a performance sore-spot for us.

> Tahoma might be a better choice. We only have reports from Win7 and Win8
> currently where Segoe should be available.

Right, but more importantly, what does this code yield when Segoe UI isn't
available? What are we adding to the fallback list? I haven't seen us hard-code
a font family name like this outside of testing before...

I wonder if going through additional system fonts (beyond "Message Box") would
be a better solution (respect users' system font choices that should more
gracefully support the users' languages, not hard-code a random family name,
etc.). Regardless, I think we should ensure unique entries in the fallback list,
and test what happens when "Segoe UI" isn't available...

[ananta, 2015-03-18 18:41:12]

https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:1300: GetFallbackFontFamilies("Segoe UI");
On 2015/03/18 15:53:23, msw wrote:
> (1) Why does the bug only occur with DirectWrite? (users claim that
> --disable-directwrite-for-ui is a workaround, how does that mitigate the
> issue?). (2) Why does/should this change affect GDI? (3) What happens here if
> "Segoe UI" isn't installed? (it's not included with Windows XP by default,
> afaict...) (4) Shouldn't we avoid duplicate fonts in the fallback list?
> (especially if Segoe UI is already the default font, I think this will
*double*
> the number of shaping passes for runs that need fallback?). (5) Why don't we
use
> Tahoma or Microsoft Sans Serif (available on XP and additionally support
Hebrew
> and Thai characters, beyond Latin/Greek/Cyrillic).
> 
> I'm dubious that this is the correct change...

The current approach of font fallback does not work on Windows XP anyways. We
use linked fonts which are based off this registry key
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows
NT\CurrentVersion\FontLink\SystemLink.

That key does not exist on Windows XP. 

That being said, as scottmg mentioned the bug does occur with GDI as well if the
system font is set to bitmap fonts like System. 

On a related note on a Windows 7 Korean installation the Microsoft Sans Seriff
font was not present in the list of linked fonts. 
Tahoma is there in the linked fonts for Segoe UI.

We do need a better way to achieve font fallback which makes me wonder why don't
we use skia's fallback logic. Would be better than duplicating the same code in
multiple places.

[msw, 2015-03-19 22:19:04]

On 2015/03/18 18:41:12, ananta wrote:
>
https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz.cc
> File ui/gfx/render_text_harfbuzz.cc (right):
> 
>
https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz...
> ui/gfx/render_text_harfbuzz.cc:1300: GetFallbackFontFamilies("Segoe UI");
> On 2015/03/18 15:53:23, msw wrote:
> > (1) Why does the bug only occur with DirectWrite? (users claim that
> > --disable-directwrite-for-ui is a workaround, how does that mitigate the
> > issue?). (2) Why does/should this change affect GDI? (3) What happens here
if
> > "Segoe UI" isn't installed? (it's not included with Windows XP by default,
> > afaict...) (4) Shouldn't we avoid duplicate fonts in the fallback list?
> > (especially if Segoe UI is already the default font, I think this will
> *double*
> > the number of shaping passes for runs that need fallback?). (5) Why don't we
> use
> > Tahoma or Microsoft Sans Serif (available on XP and additionally support
> Hebrew
> > and Thai characters, beyond Latin/Greek/Cyrillic).
> > 
> > I'm dubious that this is the correct change...
> 
> The current approach of font fallback does not work on Windows XP anyways. We
> use linked fonts which are based off this registry key
> HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows
> NT\CurrentVersion\FontLink\SystemLink.
> 
> That key does not exist on Windows XP. 
> 
> That being said, as scottmg mentioned the bug does occur with GDI as well if
the
> system font is set to bitmap fonts like System. 
> 
> On a related note on a Windows 7 Korean installation the Microsoft Sans Seriff
> font was not present in the list of linked fonts. 
> Tahoma is there in the linked fonts for Segoe UI.
> 
> We do need a better way to achieve font fallback which makes me wonder why
don't
> we use skia's fallback logic. Would be better than duplicating the same code
in
> multiple places.

I still have two concerns that haven't been addressed:
(3) What happens here if Segoe UI" isn't installed? (it's not included with
Windows XP by default, afaict...) (what, if anything, is added to the list?)
(4) Shouldn't we avoid duplicate fonts in the fallback list? (especially if
Segoe UI is already the default font, I think this will *double* the number of
shaping passes for runs that need fallback?) (this seems like a legitimate
concern for perf with non-english text)

[ananta, 2015-03-19 22:26:36]

On 2015/03/19 22:19:04, msw wrote:
> On 2015/03/18 18:41:12, ananta wrote:
> >
>
https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz.cc
> > File ui/gfx/render_text_harfbuzz.cc (right):
> > 
> >
>
https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz...
> > ui/gfx/render_text_harfbuzz.cc:1300: GetFallbackFontFamilies("Segoe UI");
> > On 2015/03/18 15:53:23, msw wrote:
> > > (1) Why does the bug only occur with DirectWrite? (users claim that
> > > --disable-directwrite-for-ui is a workaround, how does that mitigate the
> > > issue?). (2) Why does/should this change affect GDI? (3) What happens here
> if
> > > "Segoe UI" isn't installed? (it's not included with Windows XP by default,
> > > afaict...) (4) Shouldn't we avoid duplicate fonts in the fallback list?
> > > (especially if Segoe UI is already the default font, I think this will
> > *double*
> > > the number of shaping passes for runs that need fallback?). (5) Why don't
we
> > use
> > > Tahoma or Microsoft Sans Serif (available on XP and additionally support
> > Hebrew
> > > and Thai characters, beyond Latin/Greek/Cyrillic).
> > > 
> > > I'm dubious that this is the correct change...
> > 
> > The current approach of font fallback does not work on Windows XP anyways.
We
> > use linked fonts which are based off this registry key
> > HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows
> > NT\CurrentVersion\FontLink\SystemLink.
> > 
> > That key does not exist on Windows XP. 
> > 
> > That being said, as scottmg mentioned the bug does occur with GDI as well if
> the
> > system font is set to bitmap fonts like System. 
> > 
> > On a related note on a Windows 7 Korean installation the Microsoft Sans
Seriff
> > font was not present in the list of linked fonts. 
> > Tahoma is there in the linked fonts for Segoe UI.
> > 
> > We do need a better way to achieve font fallback which makes me wonder why
> don't
> > we use skia's fallback logic. Would be better than duplicating the same code
> in
> > multiple places.
> 
> I still have two concerns that haven't been addressed:
> (3) What happens here if Segoe UI" isn't installed? (it's not included with
> Windows XP by default, afaict...) (what, if anything, is added to the list?)
> (4) Shouldn't we avoid duplicate fonts in the fallback list? (especially if
> Segoe UI is already the default font, I think this will *double* the number of
> shaping passes for runs that need fallback?) (this seems like a legitimate
> concern for perf with non-english text)

The font fallback mechanism does not work on Windows XP as the concept of linked
fonts does not
exist there. It should exist on Vista+. Regarding the concern with duplicate
entries in the fallback list
I will work on a patch over the next few days to address this.

[ananta, 2015-03-19 22:41:28]

On 2015/03/19 22:26:36, ananta wrote:
> On 2015/03/19 22:19:04, msw wrote:
> > On 2015/03/18 18:41:12, ananta wrote:
> > >
> >
>
https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz.cc
> > > File ui/gfx/render_text_harfbuzz.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz...
> > > ui/gfx/render_text_harfbuzz.cc:1300: GetFallbackFontFamilies("Segoe UI");
> > > On 2015/03/18 15:53:23, msw wrote:
> > > > (1) Why does the bug only occur with DirectWrite? (users claim that
> > > > --disable-directwrite-for-ui is a workaround, how does that mitigate the
> > > > issue?). (2) Why does/should this change affect GDI? (3) What happens
here
> > if
> > > > "Segoe UI" isn't installed? (it's not included with Windows XP by
default,
> > > > afaict...) (4) Shouldn't we avoid duplicate fonts in the fallback list?
> > > > (especially if Segoe UI is already the default font, I think this will
> > > *double*
> > > > the number of shaping passes for runs that need fallback?). (5) Why
don't
> we
> > > use
> > > > Tahoma or Microsoft Sans Serif (available on XP and additionally support
> > > Hebrew
> > > > and Thai characters, beyond Latin/Greek/Cyrillic).
> > > > 
> > > > I'm dubious that this is the correct change...
> > > 
> > > The current approach of font fallback does not work on Windows XP anyways.
> We
> > > use linked fonts which are based off this registry key
> > > HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows
> > > NT\CurrentVersion\FontLink\SystemLink.
> > > 
> > > That key does not exist on Windows XP. 
> > > 
> > > That being said, as scottmg mentioned the bug does occur with GDI as well
if
> > the
> > > system font is set to bitmap fonts like System. 
> > > 
> > > On a related note on a Windows 7 Korean installation the Microsoft Sans
> Seriff
> > > font was not present in the list of linked fonts. 
> > > Tahoma is there in the linked fonts for Segoe UI.
> > > 
> > > We do need a better way to achieve font fallback which makes me wonder why
> > don't
> > > we use skia's fallback logic. Would be better than duplicating the same
code
> > in
> > > multiple places.
> > 
> > I still have two concerns that haven't been addressed:
> > (3) What happens here if Segoe UI" isn't installed? (it's not included with
> > Windows XP by default, afaict...) (what, if anything, is added to the list?)
> > (4) Shouldn't we avoid duplicate fonts in the fallback list? (especially if
> > Segoe UI is already the default font, I think this will *double* the number
of
> > shaping passes for runs that need fallback?) (this seems like a legitimate
> > concern for perf with non-english text)
> 
> The font fallback mechanism does not work on Windows XP as the concept of
linked
> fonts does not
> exist there. It should exist on Vista+. Regarding the concern with duplicate
> entries in the fallback list
> I will work on a patch over the next few days to address this.

Regarding the concern with duplicate entries for Segoe UI and its associated
fonts in the fallback list,
if I am reading the fallback code correctly, we enumerate the fallback fonts in
the list and return when
we find the first match. We will enumerate the whole list only if there is a
failure to find a fallback font
for the text being rendered? and how much of a concern is that?

scottmg posted a request on the bug requesting the affected users to try out the
Canary build to see if the font
issues were addressed. If they weren't then we were on the wrong track anyways.
Once we hear back from them I will 
continue working on this. On a related note QA verified that the fix works on
the VM where the problem was occurring.

[msw, 2015-03-19 22:49:02]

On 2015/03/19 22:41:28, ananta wrote:
> On 2015/03/19 22:26:36, ananta wrote:
> > On 2015/03/19 22:19:04, msw wrote:
> > > On 2015/03/18 18:41:12, ananta wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz.cc
> > > > File ui/gfx/render_text_harfbuzz.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1009533003/diff/1/ui/gfx/render_text_harfbuzz...
> > > > ui/gfx/render_text_harfbuzz.cc:1300: GetFallbackFontFamilies("Segoe
UI");
> > > > On 2015/03/18 15:53:23, msw wrote:
> > > > > (1) Why does the bug only occur with DirectWrite? (users claim that
> > > > > --disable-directwrite-for-ui is a workaround, how does that mitigate
the
> > > > > issue?). (2) Why does/should this change affect GDI? (3) What happens
> here
> > > if
> > > > > "Segoe UI" isn't installed? (it's not included with Windows XP by
> default,
> > > > > afaict...) (4) Shouldn't we avoid duplicate fonts in the fallback
list?
> > > > > (especially if Segoe UI is already the default font, I think this will
> > > > *double*
> > > > > the number of shaping passes for runs that need fallback?). (5) Why
> don't
> > we
> > > > use
> > > > > Tahoma or Microsoft Sans Serif (available on XP and additionally
support
> > > > Hebrew
> > > > > and Thai characters, beyond Latin/Greek/Cyrillic).
> > > > > 
> > > > > I'm dubious that this is the correct change...
> > > > 
> > > > The current approach of font fallback does not work on Windows XP
anyways.
> > We
> > > > use linked fonts which are based off this registry key
> > > > HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows
> > > > NT\CurrentVersion\FontLink\SystemLink.
> > > > 
> > > > That key does not exist on Windows XP. 
> > > > 
> > > > That being said, as scottmg mentioned the bug does occur with GDI as
well
> if
> > > the
> > > > system font is set to bitmap fonts like System. 
> > > > 
> > > > On a related note on a Windows 7 Korean installation the Microsoft Sans
> > Seriff
> > > > font was not present in the list of linked fonts. 
> > > > Tahoma is there in the linked fonts for Segoe UI.
> > > > 
> > > > We do need a better way to achieve font fallback which makes me wonder
why
> > > don't
> > > > we use skia's fallback logic. Would be better than duplicating the same
> code
> > > in
> > > > multiple places.
> > > 
> > > I still have two concerns that haven't been addressed:
> > > (3) What happens here if Segoe UI" isn't installed? (it's not included
with
> > > Windows XP by default, afaict...) (what, if anything, is added to the
list?)
> > > (4) Shouldn't we avoid duplicate fonts in the fallback list? (especially
if
> > > Segoe UI is already the default font, I think this will *double* the
number
> of
> > > shaping passes for runs that need fallback?) (this seems like a legitimate
> > > concern for perf with non-english text)
> > 
> > The font fallback mechanism does not work on Windows XP as the concept of
> linked
> > fonts does not
> > exist there. It should exist on Vista+. Regarding the concern with duplicate
> > entries in the fallback list
> > I will work on a patch over the next few days to address this.
> 
> Regarding the concern with duplicate entries for Segoe UI and its associated
> fonts in the fallback list,
> if I am reading the fallback code correctly, we enumerate the fallback fonts
in
> the list and return when
> we find the first match. We will enumerate the whole list only if there is a
> failure to find a fallback font
> for the text being rendered? and how much of a concern is that?
> 
> scottmg posted a request on the bug requesting the affected users to try out
the
> Canary build to see if the font
> issues were addressed. If they weren't then we were on the wrong track
anyways.
> Once we hear back from them I will 
> continue working on this. On a related note QA verified that the fix works on
> the VM where the problem was occurring.

RenderTextHarfBuzz::ShapeRun returns early when shaping yields no missing
glyphs, but as long as some glyphs are missing, it will try shaping with all the
fallback fonts and then use the font that yields the fewest missing glyphs in
the run. My concern is for slowing down shaping of text runs that mix characters
from various languages, or mix symbols with text.