RenderText: Try fallback fonts of the Uniscribe font while shaping

RenderText: Try fallback fonts of the Uniscribe font while shaping

BUG=422142
R=asvitkine

Committed: https://crrev.com/f9fd2e2129b8dc0542c76744666a2a1cd06775f2
Cr-Commit-Position: refs/heads/master@{#300351}

[ckocagil, 2014-10-20 19:53:03]

ckocagil@chromium.org changed reviewers:
+ msw@chromium.org

[ckocagil, 2014-10-20 19:53:39]

PTAl. This is intended to be a short-term solution to be merged back to stable.

[msw, 2014-10-20 20:12:03]

https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1055: current_font = Font(fonts[i],
original_font.GetFontSize());
The entire defect may just be that |current_font| was not updated in this loop.
Any good shaping would then use the stale value for
|successful_substitute_fonts_| or |best_partial_font|.

Can you try just adding this line and updating the ShapeTextRunWithFont call
immediately below, without trying additional fallback fonts of the Uniscribe
fallback font? (remove the loop in lines 1068-1082 below and potentially revert
the change above too). See if that fixes the issue... do you need me to test?

[Alexei Svitkine (slow), 2014-10-20 20:14:04]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2014-10-20 20:14:04]

LGTM

https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1073: ShapeTextRunWithFont(run, current_font));
Nit: Indent 2 more.

[ckocagil, 2014-10-20 20:14:11]

On 2014/10/20 20:12:03, msw wrote:
> https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc
> File ui/gfx/render_text_win.cc (right):
> 
>
https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc#ne...
> ui/gfx/render_text_win.cc:1055: current_font = Font(fonts[i],
> original_font.GetFontSize());
> The entire defect may just be that |current_font| was not updated in this
loop.
> Any good shaping would then use the stale value for
> |successful_substitute_fonts_| or |best_partial_font|.
> 
> Can you try just adding this line and updating the ShapeTextRunWithFont call
> immediately below, without trying additional fallback fonts of the Uniscribe
> fallback font? (remove the loop in lines 1068-1082 below and potentially
revert
> the change above too). See if that fixes the issue... do you need me to test?

I will try your suggestion and test locally.

[scottmg, 2014-10-20 20:16:41]

scottmg@chromium.org changed reviewers:
+ scottmg@chromium.org

[scottmg, 2014-10-20 20:16:41]

Repro (apparently) requires a non-English Win XP with chrome set to a different
non-English UI language. I've got a VM with that set up now, building now and
I'll let you know what I see.

Thanks for jumping on this.

[scottmg, 2014-10-20 20:17:15]

(Or just ignore me if you have a simpler repro. :)

[Alexei Svitkine (slow), 2014-10-20 20:21:20]

https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1055: current_font = Font(fonts[i],
original_font.GetFontSize());
On 2014/10/20 20:12:03, msw wrote:
> The entire defect may just be that |current_font| was not updated in this
loop.
> Any good shaping would then use the stale value for
> |successful_substitute_fonts_| or |best_partial_font|.

From my recollection of the original motivation for the previous logic, I
suspect it won't be enough - though happy to hear otherwise.

IIRC, issue is that the good font is from the fallback list of the uniscribe
font, not of the original font. So updating current_font isn't enough I think.

[ckocagil, 2014-10-20 20:24:12]

On 2014/10/20 20:14:11, ckocagil wrote:
> On 2014/10/20 20:12:03, msw wrote:
> > https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc
> > File ui/gfx/render_text_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc#ne...
> > ui/gfx/render_text_win.cc:1055: current_font = Font(fonts[i],
> > original_font.GetFontSize());
> > The entire defect may just be that |current_font| was not updated in this
> loop.
> > Any good shaping would then use the stale value for
> > |successful_substitute_fonts_| or |best_partial_font|.
> > 
> > Can you try just adding this line and updating the ShapeTextRunWithFont call
> > immediately below, without trying additional fallback fonts of the Uniscribe
> > fallback font? (remove the loop in lines 1068-1082 below and potentially
> revert
> > the change above too). See if that fixes the issue... do you need me to
test?
> 
> I will try your suggestion and test locally.

That didn't work. I'll try describing the bug below:

- In LinkedFontsIterator, linked_fonts_ isn't initialized as long as there's a
next font from SetNextFont().
- Old loop called SetNextFont().
- The last "next font" before linked_fonts_ is initialized is the one from
GetUniscribeFallbackFont().
- While linked_fonts_ is being initialized, it tries first original_font_, and
then the current_font_ (which is the one from Uniscribe).

So in this CL I explicitly iterate over the linked fonts of the Uniscribe font.

[Alexei Svitkine (slow), 2014-10-20 20:25:06]

https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1055: current_font = Font(fonts[i],
original_font.GetFontSize());
On 2014/10/20 20:21:20, Alexei Svitkine wrote:
> On 2014/10/20 20:12:03, msw wrote:
> > The entire defect may just be that |current_font| was not updated in this
> loop.
> > Any good shaping would then use the stale value for
> > |successful_substitute_fonts_| or |best_partial_font|.
> 
> From my recollection of the original motivation for the previous logic, I
> suspect it won't be enough - though happy to hear otherwise.
> 
> IIRC, issue is that the good font is from the fallback list of the uniscribe
> font, not of the original font. So updating current_font isn't enough I think.

Although yeah, it looks like the code without the above line would just result
in font linking not working ... so maybe this will work too. I guess the
question is whether it will cover all the cases or not. I'd prefer we keep logic
that matches the original working behavior for the patch that's merged to stable
- and we can investigate if the second linked font query is needed separately on
TOT.

[msw, 2014-10-20 20:28:53]

On 2014/10/20 20:24:12, ckocagil wrote:
> On 2014/10/20 20:14:11, ckocagil wrote:
> > On 2014/10/20 20:12:03, msw wrote:
> > > https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc
> > > File ui/gfx/render_text_win.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/637953010/diff/1/ui/gfx/render_text_win.cc#ne...
> > > ui/gfx/render_text_win.cc:1055: current_font = Font(fonts[i],
> > > original_font.GetFontSize());
> > > The entire defect may just be that |current_font| was not updated in this
> > loop.
> > > Any good shaping would then use the stale value for
> > > |successful_substitute_fonts_| or |best_partial_font|.
> > > 
> > > Can you try just adding this line and updating the ShapeTextRunWithFont
call
> > > immediately below, without trying additional fallback fonts of the
Uniscribe
> > > fallback font? (remove the loop in lines 1068-1082 below and potentially
> > revert
> > > the change above too). See if that fixes the issue... do you need me to
> test?
> > 
> > I will try your suggestion and test locally.
> 
> That didn't work. I'll try describing the bug below:
> 
> - In LinkedFontsIterator, linked_fonts_ isn't initialized as long as there's a
> next font from SetNextFont().
> - Old loop called SetNextFont().
> - The last "next font" before linked_fonts_ is initialized is the one from
> GetUniscribeFallbackFont().
> - While linked_fonts_ is being initialized, it tries first original_font_, and
> then the current_font_ (which is the one from Uniscribe).
> 
> So in this CL I explicitly iterate over the linked fonts of the Uniscribe
font.

Okay, if this matches the old behavior, then it lgtm.
This will regress some [ill-gotten] RTHB perf gains on Win, right?

[ckocagil, 2014-10-20 20:30:43]

On 2014/10/20 20:28:53, msw wrote:
> Okay, if this matches the old behavior, then it lgtm.
> This will regress some [ill-gotten] RTHB perf gains on Win, right?

I'm not sure, I'll have to check the numbers.

[ckocagil, 2014-10-20 20:30:49]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-10-20 20:32:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/637953010/1

[chromium-reviews, 2014-10-20 20:36:09]

I think it would be useful to add a test for this in a followup CL.

I'm thinking we could find a way to mock out the font linking and uniscribe
fallback APIs (i.e. provide fake return values from them from the test for
given inputs) and then the test could run the algorithm and check that the
right fonts were selected. The mocked fallback lists could be hardcoded in
the test based on real Windows font set ups - so that the test wouldn't
need to run on a specific configured Windows machine to test the fix.

Cem, could you take a look at doing that?

On Mon, Oct 20, 2014 at 4:32 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/637953010/1
>
>
> https://codereview.chromium.org/637953010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-10-20 22:13:27]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-10-20 22:14:20]

Patchset 1 (id:??) landed as
https://crrev.com/f9fd2e2129b8dc0542c76744666a2a1cd06775f2
Cr-Commit-Position: refs/heads/master@{#300351}