Use font linking based on the original run font in RenderTextWin.

Use font linking based on the original run font in RenderTextWin.

BUG=105550
TEST=On a Windows 7 system, go to a Japanese website and check that
the Japanese characters in the tab title are rendered using Meiryo
font and not MS UI Gothic. (Which matches the GDI path.)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133692

[msw, 2012-04-23 18:22:14]

lgtm

[commit-bot: I haz the power, 2012-04-23 18:34:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10143009/1

[Alexei Svitkine (slow), 2012-04-23 19:59:25]

Actually, the original version of this CL breaks font linking with custom fonts
(that my other CL was fixing). The issue is that if the original font is used to
get the font link list, that list may not exist for a custom font.

I've updated this CL to use the original font to get the font link list first,
but to fall back to the other font if the the first list is empty.

Can you take another look?

[msw, 2012-04-23 20:20:59]

http://codereview.chromium.org/10143009/diff/5003/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/10143009/diff/5003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:671: // font is a custom font that has no linked fonts
in the Registry.
nit: Should registry be capitalized? Are you referring to the Windows Registry?

http://codereview.chromium.org/10143009/diff/5003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:673: linked_fonts = GetLinkedFonts(run->font);
Have you considered appending the linked fonts for the Uniscribe fallback to the
list gathered for |original_font|, regardless of those available for the
original font? (more font options = better)?

[Alexei Svitkine (slow), 2012-04-23 21:13:14]

http://codereview.chromium.org/10143009/diff/5003/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/10143009/diff/5003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:671: // font is a custom font that has no linked fonts
in the Registry.
On 2012/04/23 20:20:59, msw wrote:
> nit: Should registry be capitalized? Are you referring to the Windows
Registry?

Yes, the Windows Registry. It's capitalized in the comment for
QueryLinkedFontsFromRegistry(), so I've kept it consistent...

http://codereview.chromium.org/10143009/diff/5003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:673: linked_fonts = GetLinkedFonts(run->font);
On 2012/04/23 20:20:59, msw wrote:
> Have you considered appending the linked fonts for the Uniscribe fallback to
the
> list gathered for |original_font|, regardless of those available for the
> original font? (more font options = better)?

I've thought about it, but decided to keep it up my sleeve for now until/if
there's evidence that it would actually help things. Here's my reasoning:

First, it's clear that there's a benefit to the approach I'm taking here (when
the first list is empty) since it makes fallback work with a custom font.

But I can't really think of a scenario where it would help merging the lists
when the first one is not empty already (other than users munging around in
their Registry font linking settings, which is unsupported).

On the other hand, if the system truly doesn't have fonts to render something,
then traversing two link chains may just hurt performance without helping
anything.

So I'd rather keep that idea up my sleeve until/if there's a real scenario that
may benefit from it.

[msw, 2012-04-24 00:05:11]

LGTM, please add a comment.

http://codereview.chromium.org/10143009/diff/5003/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/10143009/diff/5003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:673: linked_fonts = GetLinkedFonts(run->font);
On 2012/04/23 21:13:14, Alexei Svitkine wrote:
> On 2012/04/23 20:20:59, msw wrote:
> > Have you considered appending the linked fonts for the Uniscribe fallback to
> the
> > list gathered for |original_font|, regardless of those available for the
> > original font? (more font options = better)?
> 
> I've thought about it, but decided to keep it up my sleeve for now until/if
> there's evidence that it would actually help things. Here's my reasoning:
> 
> First, it's clear that there's a benefit to the approach I'm taking here (when
> the first list is empty) since it makes fallback work with a custom font.
> 
> But I can't really think of a scenario where it would help merging the lists
> when the first one is not empty already (other than users munging around in
> their Registry font linking settings, which is unsupported).
> 
> On the other hand, if the system truly doesn't have fonts to render something,
> then traversing two link chains may just hurt performance without helping
> anything.
> 
> So I'd rather keep that idea up my sleeve until/if there's a real scenario
that
> may benefit from it.

Cool, how about a TODO or less directed/actionable comment mentioning that
possibility for future investigations?

[commit-bot: I haz the power, 2012-04-24 14:12:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10143009/19001

[commit-bot: I haz the power, 2012-04-24 15:47:31]

Change committed as 133692