Always try metafile font fallback in the case of missing glyphs in RenderTextWin.

Always try metafile font fallback in the case of missing glyphs in RenderTextWin.

Before this patch, if ScriptShape() returned E_OK but there
were missing glyphs, we would only try font linking and not
metafile font fallback. For Armenian text (and probably
others), ScriptShape() returns E_OK with missing glyphs,
but font linking won't find the right font. The metafile
approach is needed.

This change makes sure the metafile font fallback path
is always taken first, before trying font linking to
find the missing font.

BUG=105550, 122974
TEST=Under Windows with use_canvas_skia=1 enabled, go to
http://akumb.am. Make sure the tab title is displayed
correctly.

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

[msw, 2012-04-12 19:58:32]

Why swap the order of linked fonts and the meta file font?
How does this impact perf and font fallback quality?

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

http://codereview.chromium.org/10010047/diff/1002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:644: // If there are no missing glyphs, we're done.
nit: Try to avoid "we" and similar words, here and elsewhere.
Consider // Skip font substitution if there are no missing glyphs.

http://codereview.chromium.org/10010047/diff/1002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:648: // If there are missing glyphs, first, try
finding a fallback font using a
nit: Remove the comma after "first,".

http://codereview.chromium.org/10010047/diff/1002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:655: DeriveFontIfNecessary(font_size, run->font_style,
&run->font);
This 3-line block is duplicated with line 685-687.
I know that was already the case, but can you combine them?

http://codereview.chromium.org/10010047/diff/1002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:662: // We didn't find a font using a meta file, try
to find one using font
nit: "We"

[Alexei Svitkine (slow), 2012-04-12 20:40:24]

On 2012/04/12 19:58:32, msw wrote:
> Why swap the order of linked fonts and the meta file font?

It doesn't really swap the order. The previous code assumed for a given run,
only one of those approaches would be taken.

That is, if the return value indicated the glyphs weren't in the font, it would
always use the metafile approach and if the return value was E_OK with missing
glyphs, it would always use font linking.

> How does this impact perf and font fallback quality?

Perf-wise, this will be slightly worse for CJK fonts that come from font
linking, since we'll need to do a metafile lookup first. But that cost isn't
terrible.

Quality wise, it's an improvement. As the CL description states, this makes
Armenian actually show up properly, and likely other scripts. Note that WebKit
does it in the same order.

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

http://codereview.chromium.org/10010047/diff/1002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:644: // If there are no missing glyphs, we're done.
On 2012/04/12 19:58:32, msw wrote:
> nit: Try to avoid "we" and similar words, here and elsewhere.
> Consider // Skip font substitution if there are no missing glyphs.

Done.

http://codereview.chromium.org/10010047/diff/1002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:648: // If there are missing glyphs, first, try
finding a fallback font using a
On 2012/04/12 19:58:32, msw wrote:
> nit: Remove the comma after "first,".

Done.

http://codereview.chromium.org/10010047/diff/1002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:655: DeriveFontIfNecessary(font_size, run->font_style,
&run->font);
On 2012/04/12 19:58:32, msw wrote:
> This 3-line block is duplicated with line 685-687.
> I know that was already the case, but can you combine them?

Done.

http://codereview.chromium.org/10010047/diff/1002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:662: // We didn't find a font using a meta file, try
to find one using font
On 2012/04/12 19:58:32, msw wrote:
> nit: "We"

Done.

[Alexei Svitkine (slow), 2012-04-12 21:11:42]

Adding missing render_text.h changes.

[msw, 2012-04-12 21:32:14]

The font replacement code is generally a bit messy and should probably cache
successful replacement fonts to try in later runs, etc. but that's out of scope
here... Just a thought.
Please consider my inline comment and nits.

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

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:660: // Couldn't find a replacement font using the
meta file, try to find one
nit: consider "The meta file did not yield a replacement font,"

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:732: void
RenderTextWin::UpdateRunFont(internal::TextRun* run, int font_size) {
I think it'd be a better to supply the substitute font as an argument here
(instead of |font_size|), and use the run's existing font size with
DeriveFontIfNecessary, as done with |run->font_style|.

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.h
File ui/gfx/render_text_win.h (right):

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.h#ne...
ui/gfx/render_text_win.h:99: void UpdateRunFont(internal::TextRun* run, int
font_size);
nit: Consider renaming this ApplyFontSubstitute or similar.

[Alexei Svitkine (slow), 2012-04-12 22:11:43]

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

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:732: void
RenderTextWin::UpdateRunFont(internal::TextRun* run, int font_size) {
On 2012/04/12 21:32:14, msw wrote:
> I think it'd be a better to supply the substitute font as an argument here
> (instead of |font_size|), and use the run's existing font size with
> DeriveFontIfNecessary, as done with |run->font_style|.

I agree it would be cleaner from an interface POV, but it would result in an
extra Font copy operation (since ChooseFallbackFont() would need to output to a
temp font), which (I just checked) is not cheap for PlatformFontWin. :(

(Although, I'm going to look at fixing this.)

Would you be okay with a TODO to clean this up, or do you prefer I block landing
this pending looking at making the PlatformFontWin copy constructor fast?

[Alexei Svitkine (slow), 2012-04-12 22:16:26]

Actually, forget my question. I'll just do the refactoring you suggest and make
PlatformFontWin copying fast in a separate CL tomorrow morning and make sure to
land the font CL before this.

[Alexei Svitkine (slow), 2012-04-13 13:56:15]

I've addressed all your comments.

(I was actually incorrect about gfx::Font copies - they are in fact cheap. I was
looking at the PlatformFontWin(Font) constructor, which is not cheap, but that
does not actually get invoked when you copy a gfx::Font, since gfx::Font keeps a
scoped_refptr to the PlatformFont and just copies that.)

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

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:660: // Couldn't find a replacement font using the
meta file, try to find one
On 2012/04/12 21:32:14, msw wrote:
> nit: consider "The meta file did not yield a replacement font,"

Done.

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:732: void
RenderTextWin::UpdateRunFont(internal::TextRun* run, int font_size) {
On 2012/04/12 21:32:14, msw wrote:
> I think it'd be a better to supply the substitute font as an argument here
> (instead of |font_size|), and use the run's existing font size with
> DeriveFontIfNecessary, as done with |run->font_style|.

Done.

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.h
File ui/gfx/render_text_win.h (right):

http://codereview.chromium.org/10010047/diff/4002/ui/gfx/render_text_win.h#ne...
ui/gfx/render_text_win.h:99: void UpdateRunFont(internal::TextRun* run, int
font_size);
On 2012/04/12 21:32:14, msw wrote:
> nit: Consider renaming this ApplyFontSubstitute or similar.

Done.

[msw, 2012-04-13 18:27:42]

LGTM; thanks for digging in to clean this up and check perf!

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

http://codereview.chromium.org/10010047/diff/13001/ui/gfx/render_text_win.cc#...
ui/gfx/render_text_win.cc:650: // TODO(msw|asvitkine): Keep a cache of previous
successful replacement
optional one-liner nit:
// TODO(msw|asvitkine): Cache previous successful replacement fonts?

http://codereview.chromium.org/10010047/diff/13001/ui/gfx/render_text_win.cc#...
ui/gfx/render_text_win.cc:668: // None of the linked fonts worked - break out of
the loop.
optional nit: use a comma/semicolon or rewrite to remove the dash:
// Break out of the loop if none of the substitute fonts worked.

[commit-bot: I haz the power, 2012-04-13 18:36:58]

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

[commit-bot: I haz the power, 2012-04-13 20:03:39]

Change committed as 132250