RenderTextWin: Use a font that can at least partially display the run, if possible.

RenderTextWin: Use a font that can at least partially display the run, if possible.

BUG=136433
TEST=See instructions on the bug report.

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

[msw, 2012-07-10 20:52:15]

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

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:685: int least_missing_count = INT_MAX;
nit: optionally rename this for more context, like
|best_partial_font_missing_glyph_count| or |least_missing_glyph_count|.

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:783: // If there was a font that was able to partially
display the run, use
nit: make one-liner as "If a font could partially display the run, use that
now." or similar.

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:791: // If no font was able to partially display the
run, replace all glyphs
In the bug, you wrote (and I agree that) "In the failure case, we should also
display the missing glyph "box" character, using the original input font." So,
I'm not certain, but shouldn't this do something slightly different? Isn't the
script_cache that of the last attempted fallback font, and not the original
font?

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:875: int chars_not_missing_glyphs = 0;
Isn't this counting the number glyphs found? (not related to chars?)
If so, rename this |glyphs_found|, and rename the function too.
Otherwise, clarify exactly what's being counted.

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:904: return run->range.length() -
chars_not_missing_glyphs;
Shouldn't this be "return run->glyph_count - glyphs_found;"
Perhaps DCHECK that this value is not negative.

[Alexei Svitkine (slow), 2012-07-10 21:16:35]

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

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:685: int least_missing_count = INT_MAX;
On 2012/07/10 20:52:15, msw wrote:
> nit: optionally rename this for more context, like
> |best_partial_font_missing_glyph_count| or |least_missing_glyph_count|.

Done.

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:783: // If there was a font that was able to partially
display the run, use
On 2012/07/10 20:52:15, msw wrote:
> nit: make one-liner as "If a font could partially display the run, use that
> now." or similar.

Done.

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:791: // If no font was able to partially display the
run, replace all glyphs
On 2012/07/10 20:52:15, msw wrote:
> In the bug, you wrote (and I agree that) "In the failure case, we should also
> display the missing glyph "box" character, using the original input font." So,
> I'm not certain, but shouldn't this do something slightly different? Isn't the
> script_cache that of the last attempted fallback font, and not the original
> font?

You're right that this is not solving that specific issue. I would like to leave
that (more minor) problem for another CL. I think it may be a bit easier to
handle that case once I refactor the fallback code into a separate file.

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:875: int chars_not_missing_glyphs = 0;
On 2012/07/10 20:52:15, msw wrote:
> Isn't this counting the number glyphs found? (not related to chars?)
> If so, rename this |glyphs_found|, and rename the function too.
> Otherwise, clarify exactly what's being counted.

It is counting chars, in fact, due to how the loop is constructed. See the for
loop below; it's iterating over chars.

For each char, it finds the corresponding glyph. A glyph may cover multiple
chars (e.g. a decorated letter may be multiple chars, but a single glyph). So
this is indeed counting chars.

I believe both the function and variable are already named in a way to indicate
that. However, I have clarified the code elsewhere.

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:904: return run->range.length() -
chars_not_missing_glyphs;
On 2012/07/10 20:52:15, msw wrote:
> Shouldn't this be "return run->glyph_count - glyphs_found;"
> Perhaps DCHECK that this value is not negative.

Nope, it shouldn't. See above.

It can't be negative because the loop iterates over |run->range.length()|
characters and can only increments |chars_not_missing_glyphs| one or zero times
each iteration. I've added a DCHECK() though, in case the code changes.

[msw, 2012-07-10 21:40:24]

LGTM, thanks!

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

http://codereview.chromium.org/10702126/diff/1003/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:791: // If no font was able to partially display the
run, replace all glyphs
On 2012/07/10 21:16:35, Alexei Svitkine wrote:
> On 2012/07/10 20:52:15, msw wrote:
> > In the bug, you wrote (and I agree that) "In the failure case, we should
also
> > display the missing glyph "box" character, using the original input font."
So,
> > I'm not certain, but shouldn't this do something slightly different? Isn't
the
> > script_cache that of the last attempted fallback font, and not the original
> > font?
> 
> You're right that this is not solving that specific issue. I would like to
leave
> that (more minor) problem for another CL. I think it may be a bit easier to
> handle that case once I refactor the fallback code into a separate file.

sgtm

[commit-bot: I haz the power, 2012-07-10 21:41:04]

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

[commit-bot: I haz the power, 2012-07-10 23:06:26]

Change committed as 145981