Implement font fallback in RenderTextWin.

Implement font fallback in RenderTextWin.

This is done by using a metafile to capture the font
that Uniscribe would use to render the text (since
there is no API to get this from Uniscribe itself).

Makes SCRIPT_CACHE be per-run, since different runs
may have different fonts and the SCRIPT_CACHE cannot
be re-used between these.

This is similar to what is done in WebKit in FontCacheWin.cpp

BUG=90426
TEST=Run chrome.exe --use-pure-views and paste some Hebrew
text into the omnibox. It should show up properly.

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

[Alexei Svitkine (slow), 2011-11-14 15:39:43]

msw: review
sky: OWNERS approval

[sky, 2011-11-14 16:30:40]

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

http://codereview.chromium.org/8565011/diff/3002/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:75: = reinterpret_cast<const
EMREXTCREATEFONTINDIRECTW*>(record);
= on previous line.

http://codereview.chromium.org/8565011/diff/3002/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:499: SelectObject(hdc, run->font.GetNativeFont());
Do we need to worry about selecting the old font on exit?

[Alexei Svitkine (slow), 2011-11-14 16:41:37]

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

http://codereview.chromium.org/8565011/diff/3002/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:75: = reinterpret_cast<const
EMREXTCREATEFONTINDIRECTW*>(record);
On 2011/11/14 16:30:41, sky wrote:
> = on previous line.

Done.

http://codereview.chromium.org/8565011/diff/3002/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:499: SelectObject(hdc, run->font.GetNativeFont());
On 2011/11/14 16:30:41, sky wrote:
> Do we need to worry about selecting the old font on exit?

It's not an issue since this is a temporary dc.

[msw, 2011-11-14 19:43:31]

LGTM with questions/nits.

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

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:73: LOGFONT* font =
reinterpret_cast<LOGFONT*>(log_font);
Can you just assign a reinterpret_cast'ed value to log_font directly?

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:84: bool ChooseFallbackFont(HDC hdc,
It appears that this has been adapted from WebCore's
FontCache::getFontDataForCharacters. Can you add a comment to that effect?

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:498: ScriptFreeCache(&run->script_cache);
Is it okay to call ScriptFreeCache here then re-use the value in the next call
to ScriptShape?

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:502: run->script_analysis.eScript = SCRIPT_UNDEFINED;
I'm not sure, but I think we only ought to set SCRIPT_UNDEFINED if
ChooseFallbackFont fails; and we might want to leave some comment giving context
to the problems with using SCRIPT_UNDEFINED at all.

[Alexei Svitkine (slow), 2011-11-14 19:56:58]

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

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:73: LOGFONT* font =
reinterpret_cast<LOGFONT*>(log_font);
On 2011/11/14 19:43:31, msw wrote:
> Can you just assign a reinterpret_cast'ed value to log_font directly?

Done.

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:84: bool ChooseFallbackFont(HDC hdc,
On 2011/11/14 19:43:31, msw wrote:
> It appears that this has been adapted from WebCore's
> FontCache::getFontDataForCharacters. Can you add a comment to that effect?

Done.

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:498: ScriptFreeCache(&run->script_cache);
On 2011/11/14 19:43:31, msw wrote:
> Is it okay to call ScriptFreeCache here then re-use the value in the next call
> to ScriptShape?

Yes, this is fine. ScriptFreeCache() will free it and set it to NULL, which is
the same as the initial value for script_cache. (The next call to ScriptShape
will allocate a new cache.)

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:502: run->script_analysis.eScript = SCRIPT_UNDEFINED;
On 2011/11/14 19:43:31, msw wrote:
> I'm not sure, but I think we only ought to set SCRIPT_UNDEFINED if
> ChooseFallbackFont fails; and we might want to leave some comment giving
context
> to the problems with using SCRIPT_UNDEFINED at all.

I think it make sense the way it is now. Here's why:

If, for example, there's no font at all on the system that can display a glyph
then ChooseFallbackFont() will still return the font that Uniscribe would use to
render the text (as boxes, presumably). If we only set it on the failure case,
then this code would never break out of the loop in such a case.

On the other hand, if ChooseFallbackFont() succeeds in finding a font that will
work, then setting it to SCRIPT_UNDEFINED here is harmless because eScript will
be overwritten by the next call to ScriptShape().

[msw, 2011-11-14 20:05:25]

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

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:498: ScriptFreeCache(&run->script_cache);
On 2011/11/14 19:56:58, Alexei Svitkine wrote:
> On 2011/11/14 19:43:31, msw wrote:
> > Is it okay to call ScriptFreeCache here then re-use the value in the next
call
> > to ScriptShape?
> 
> Yes, this is fine. ScriptFreeCache() will free it and set it to NULL, which is
> the same as the initial value for script_cache. (The next call to ScriptShape
> will allocate a new cache.)

Gotcha, thanks for clarifying.

http://codereview.chromium.org/8565011/diff/6001/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:502: run->script_analysis.eScript = SCRIPT_UNDEFINED;
On 2011/11/14 19:56:58, Alexei Svitkine wrote:
> On 2011/11/14 19:43:31, msw wrote:
> > I'm not sure, but I think we only ought to set SCRIPT_UNDEFINED if
> > ChooseFallbackFont fails; and we might want to leave some comment giving
> context
> > to the problems with using SCRIPT_UNDEFINED at all.
> 
> I think it make sense the way it is now. Here's why:
> 
> If, for example, there's no font at all on the system that can display a glyph
> then ChooseFallbackFont() will still return the font that Uniscribe would use
to
> render the text (as boxes, presumably). If we only set it on the failure case,
> then this code would never break out of the loop in such a case.
> 
> On the other hand, if ChooseFallbackFont() succeeds in finding a font that
will
> work, then setting it to SCRIPT_UNDEFINED here is harmless because eScript
will
> be overwritten by the next call to ScriptShape().

Fair enough, but if ChooseFallbackFont doesn't find a suitable font, we may
still hit the same (potential crash) problems described in my comment's URLs
that you are removing. I'd like to leave some context here, in case this does
cause problems. According to the first link:

If you [use SCRIPT_UNDEFINED], note Mozilla bug 341500. Apparently, Uniscribe
occasionally crashes if you disable shaping and there is a UTF-16 surrogate pair
(representing a character above U+FFFF)....

[Alexei Svitkine (slow), 2011-11-14 20:15:24]

> Fair enough, but if ChooseFallbackFont doesn't find a suitable font, we may
> still hit the same (potential crash) problems described in my comment's URLs
> that you are removing. I'd like to leave some context here, in case this does
> cause problems. According to the first link:
> 
> If you [use SCRIPT_UNDEFINED], note Mozilla bug 341500. Apparently, Uniscribe
> occasionally crashes if you disable shaping and there is a UTF-16 surrogate
pair
> (representing a character above U+FFFF)....

I've re-added the comment with that info.

[msw, 2011-11-14 20:17:32]

Thanks. Also, can you set BUG=90426?

[Alexei Svitkine (slow), 2011-11-14 20:25:00]

On 2011/11/14 20:17:32, msw wrote:
> Thanks. Also, can you set BUG=90426?

Done.

[sky, 2011-11-14 21:44:47]

LGTM

[commit-bot: I haz the power, 2011-11-14 21:53:29]

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

[commit-bot: I haz the power, 2011-11-14 23:22:17]

Change committed as 109977