Implement font fallback in RenderTextWin, try #2.

Implement font fallback in RenderTextWin, try #2.

The original change (http://codereview.chromium.org/8565011/) was
reverted due to failing NativeTextfieldViewsTest.HitInsideTextAreaTest.

The test was too strict when comparing cursor bounds and failed due
to font fallback causing height differences in cursor bounds. This
updated CL makes the test only check the x value of cursor bounds.

Original CL description:

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, 104234
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=110749

[Alexei Svitkine (slow), 2011-11-15 15:41:09]

Mike, can you take a look at the new change to RenderTextWin::GetCursorBounds()?

Let me know if you prefer a different solution here.

[msw, 2011-11-15 19:50:02]

Is this the right solution or should the test be changed?
If this is the right way to go, then this change LGTM.

FYI, in the future it helps if you re-use the issue for minor updates after a
revert; with git you can use "git cl issue <old issue #>" to reuse the old issue
on a new branch.

[Alexei Svitkine (slow), 2011-11-15 21:51:15]

On 2011/11/15 19:50:02, msw wrote:
> Is this the right solution or should the test be changed?

I was hoping you would provide that insight.

Perhaps Xiaomei can shed some light here?

Basically, the issue is that with font fallback, different runs of text now end
up using different fonts, which also causes the font.GetHeight() values to be
different which is being used to calculate the caret bounds.

NativeTextfieldViewsTest.HitInsideTextAreaTest expects that the bounds for two
different caret positions to be the same, but now fails due to the carets being
of different height.

Xiaomei, could you to shed some light on what the premise behind this test is. I
don't quite understand the meaning of the first two parameters to SelectionModel
and why the test expects SelectionModel(2, 1, ...) and SelectionModel(4, 3, ...)
to have the same cursor bounds in this case.

Thanks in advance.

[msw, 2011-11-15 22:14:35]

On 2011/11/15 21:51:15, Alexei Svitkine wrote:
> On 2011/11/15 19:50:02, msw wrote:
> > Is this the right solution or should the test be changed?
> 
> I was hoping you would provide that insight.

I guess the test should be more robust; rather than assuming that the cursor
height is the same for two positions, it should probably measure and compare
against each. It shouldn't fail because font fallback used a different text size
(if that's the correct font fallback behavior). 

This article points out some font sizing issues with font fallback, so I'm
curious about our results with either approach. I'd presume that we'd want to
use the size derived from ScriptStringAnalyse.
http://blogs.msdn.com/b/michkap/archive/2008/02/06/7490625.aspx

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

> I guess the test should be more robust; rather than assuming that the cursor
> height is the same for two positions, it should probably measure and compare
> against each.

On the surface, I agree - but I'd like to understand the test better before
changing it in this way. Hopefully Xiaomei can chime in here.

-Alexei

[Alexei Svitkine (slow), 2011-11-16 19:18:16]

On 2011/11/16 16:14:17, Alexei Svitkine wrote:
> > I guess the test should be more robust; rather than assuming that the cursor
> > height is the same for two positions, it should probably measure and compare
> > against each.
> 
> On the surface, I agree - but I'd like to understand the test better before
> changing it in this way. Hopefully Xiaomei can chime in here.
> 
> -Alexei

[xji, 2011-11-16 21:38:06]

On 2011/11/16 19:18:16, Alexei Svitkine wrote:
> On 2011/11/16 16:14:17, Alexei Svitkine wrote:
> > > I guess the test should be more robust; rather than assuming that the
cursor
> > > height is the same for two positions, it should probably measure and
compare
> > > against each.
> > 
> > On the surface, I agree - but I'd like to understand the test better before
> > changing it in this way. Hopefully Xiaomei can chime in here.
> > 
> > -Alexei

The main purpose of the test is to test the 2 cursors are in the same x-axis.
For example, given text "abCD" where CD are hebrew characters, which display as
"abDC". SelecionModel(2, 1, tailing) means cursor is logically at position 2
(which is after "ab"), and visually at the trailing edge of 1st character, which
is visually at "ab|".
SelectionModel(4, 3, trailing) means cursor is logically at position 4 (which is
after "abCD"), and visually at the trailing edge of 3rd character, which is
visually at "ab|DC".
these 2 cursors should be displayed at the same x positions.

when I first wrote the test, I missed the different fonts part.
Looking at it again, I think the test should change to just compare cursor
bound's x-axis.

As to whether to use run's font height as cursor bound's height or use default
font's height as cursor bound's height, I am not so sure. But I think use run's
font is a better option.

(I am on vacation this week and next week, so slow to reply).

[Alexei Svitkine (slow), 2011-11-16 21:45:27]

On Wed, Nov 16, 2011 at 4:38 PM, <xji@chromium.org> wrote:

> On 2011/11/16 19:18:16, Alexei Svitkine wrote:
>
>> On 2011/11/16 16:14:17, Alexei Svitkine wrote:
>> > > I guess the test should be more robust; rather than assuming that the
>>
> cursor
>
>> > > height is the same for two positions, it should probably measure and
>>
> compare
>
>> > > against each.
>> >
>> > On the surface, I agree - but I'd like to understand the test better
>> before
>> > changing it in this way. Hopefully Xiaomei can chime in here.
>> >
>> > -Alexei
>>
>
> The main purpose of the test is to test the 2 cursors are in the same
> x-axis.
> For example, given text "abCD" where CD are hebrew characters, which
> display as
> "abDC". SelecionModel(2, 1, tailing) means cursor is logically at position
> 2
> (which is after "ab"), and visually at the trailing edge of 1st character,
> which
> is visually at "ab|".
> SelectionModel(4, 3, trailing) means cursor is logically at position 4
> (which is
> after "abCD"), and visually at the trailing edge of 3rd character, which is
> visually at "ab|DC".
> these 2 cursors should be displayed at the same x positions.
>
> when I first wrote the test, I missed the different fonts part.
> Looking at it again, I think the test should change to just compare cursor
> bound's x-axis.
>
> As to whether to use run's font height as cursor bound's height or use
> default
> font's height as cursor bound's height, I am not so sure. But I think use
> run's
> font is a better option.
>
> (I am on vacation this week and next week, so slow to reply).
>

Thanks for the explanation. (Apologies for not realising you were on
vacation.)

I'll update the test to just compare x positions and add a comment with
your explanation so that the test intention is clearer.

-Alexei

[xji, 2011-11-17 23:46:45]

On 2011/11/16 21:45:27, Alexei Svitkine wrote:
> On Wed, Nov 16, 2011 at 4:38 PM, <mailto:xji@chromium.org> wrote:
> 
> > On 2011/11/16 19:18:16, Alexei Svitkine wrote:
> >
> >> On 2011/11/16 16:14:17, Alexei Svitkine wrote:
> >> > > I guess the test should be more robust; rather than assuming that the
> >>
> > cursor
> >
> >> > > height is the same for two positions, it should probably measure and
> >>
> > compare
> >
> >> > > against each.
> >> >
> >> > On the surface, I agree - but I'd like to understand the test better
> >> before
> >> > changing it in this way. Hopefully Xiaomei can chime in here.
> >> >
> >> > -Alexei
> >>
> >
> > The main purpose of the test is to test the 2 cursors are in the same
> > x-axis.
> > For example, given text "abCD" where CD are hebrew characters, which
> > display as
> > "abDC". SelecionModel(2, 1, tailing) means cursor is logically at position
> > 2
> > (which is after "ab"), and visually at the trailing edge of 1st character,
> > which
> > is visually at "ab|".
> > SelectionModel(4, 3, trailing) means cursor is logically at position 4
> > (which is
> > after "abCD"), and visually at the trailing edge of 3rd character, which is
> > visually at "ab|DC".
> > these 2 cursors should be displayed at the same x positions.
> >
> > when I first wrote the test, I missed the different fonts part.
> > Looking at it again, I think the test should change to just compare cursor
> > bound's x-axis.
> >
> > As to whether to use run's font height as cursor bound's height or use
> > default
> > font's height as cursor bound's height, I am not so sure. But I think use
> > run's
> > font is a better option.
> >
> > (I am on vacation this week and next week, so slow to reply).
> >
> 
> Thanks for the explanation. (Apologies for not realising you were on
> vacation.)
> 
> I'll update the test to just compare x positions and add a comment with
> your explanation so that the test intention is clearer.

Thanks for doing that!

> 
> -Alexei

[Alexei Svitkine (slow), 2011-11-18 16:31:32]

Mike, can you review the latest patch?

Besides changing the test per discussion with xji@, I've also made the following
changes since the older version of this patch:

- Added TextRun::~TextRun() destructor responsible for calling
ScriptFreeCache(). The previous version of this patch only free theses for all
the runs in RenderTextWin::~RenderTextWin(), which caused a leak when the runs
were cleared by RenderTextWin::ItemizeLogicalText().

- Deleted unused variables text_is_dirty_ and style_is_dirty_.

Scott, can you OWNERS rubber-stamp?

[msw, 2011-11-18 18:25:17]

LGTM!

[sky, 2011-11-18 18:43:14]

Rubber stamp LGTM

[commit-bot: I haz the power, 2011-11-18 18:43:37]

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

[commit-bot: I haz the power, 2011-11-18 20:33:59]

Try job failure for 8570003-28001 (retry) (retry) on linux_rel for step
"ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-11-18 20:40:57]

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