RenderTextWin: Unroll the loop in LayoutTextRun for clarity

RenderTextWin: Unroll the loop in LayoutTextRun for clarity

BUG=
R=msw
TBR=asvitkine

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282550

[msw, 2014-07-10 22:59:50]

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

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1079: return;
Should we ever add the original font to |successful_substitute_fonts_|?

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1081: best_partial_font_missing_char_count =
missing_count;
We may as well declare and initialize |best_partial_font_missing_char_count =
missing_count| and |best_partial_font = current_font| here, regardless of
missing count condition.

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1093:
successful_substitute_fonts_[original_font.GetFontName()] = run->font;
Isn't this value already set? Can we skip this?

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1221: if (shaping_result == USP_E_SCRIPT_NOT_IN_FONT)
nit: if (shaping_result != S_OK) { DCHECK_EQ(shaping_result,
USP_E_SCRIPT_NOT_IN_FONT); return INT_MAX; }

[ckocagil, 2014-07-10 23:41:33]

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

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1079: return;
On 2014/07/10 22:59:50, msw wrote:
> Should we ever add the original font to |successful_substitute_fonts_|?

I can't see a reason to add the original font. It looks like we already don't,
see the condition at line 1097 on the LHS.

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1081: best_partial_font_missing_char_count =
missing_count;
On 2014/07/10 22:59:50, msw wrote:
> We may as well declare and initialize |best_partial_font_missing_char_count =
> missing_count| and |best_partial_font = current_font| here, regardless of
> missing count condition.

Done.

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1093:
successful_substitute_fonts_[original_font.GetFontName()] = run->font;
On 2014/07/10 22:59:50, msw wrote:
> Isn't this value already set? Can we skip this?

Done.

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1221: if (shaping_result == USP_E_SCRIPT_NOT_IN_FONT)
On 2014/07/10 22:59:50, msw wrote:
> nit: if (shaping_result != S_OK) { DCHECK_EQ(shaping_result,
> USP_E_SCRIPT_NOT_IN_FONT); return INT_MAX; }

Done.

[msw, 2014-07-11 00:08:39]

Despite some duplicated code, this make the current (imperfect) fallback logic a
bit clearer, so lgtm (with a nit).

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

https://codereview.chromium.org/382793004/diff/1/ui/gfx/render_text_win.cc#ne...
ui/gfx/render_text_win.cc:1079: return;
On 2014/07/10 23:41:33, ckocagil wrote:
> On 2014/07/10 22:59:50, msw wrote:
> > Should we ever add the original font to |successful_substitute_fonts_|?
> 
> I can't see a reason to add the original font. It looks like we already don't,
> see the condition at line 1097 on the LHS.

Acknowledged.

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

https://codereview.chromium.org/382793004/diff/20001/ui/gfx/render_text_win.c...
ui/gfx/render_text_win.cc:1067: // Try to shape with the first font in the
fallback list.
nit: maybe mention that it's |original_font|?

[ckocagil, 2014-07-11 01:29:25]

Alexei: TBRing to you, can you take a look when you're back?

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

https://codereview.chromium.org/382793004/diff/20001/ui/gfx/render_text_win.c...
ui/gfx/render_text_win.cc:1067: // Try to shape with the first font in the
fallback list.
On 2014/07/11 00:08:39, msw wrote:
> nit: maybe mention that it's |original_font|?

Done.

[ckocagil, 2014-07-11 01:29:50]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-07-11 01:30:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ckocagil@chromium.org/382793004/40001

[commit-bot: I haz the power, 2014-07-11 04:57:46]

Change committed as 282550

[Alexei Svitkine (slow), 2014-07-15 19:48:20]

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

https://codereview.chromium.org/382793004/diff/40001/ui/gfx/render_text_win.c...
ui/gfx/render_text_win.cc:1102: ShapeTextRunWithFont(run, current_font));
If these two functions are always called together now, just move the inner call
inside the outer and rename the outer function.

I also wonder if the other parts of the logic can be added to the function (i.e.
it could be responsible for updating best_partial_font and
best_partial_font_missing_char_count). It could take those vars as extra params
or perhaps you could introduce a State struct and pass that instead? Would make
it even more readable.

Maybe just make a new short function:

size_t RenderTextWin::TryFallbackFont(const Font& font,
                                      FontFallbackState* state) {
  HRESULT shaping_result = ShapeTextRunWithFont(state->run, font);
  size_t missing_count = (shaping_result != S_OK) ? INT_MAX :
      CountCharsWithMissingGlyphs(state->run);
  if (missing_count < state->best_partial_font_missing_char_count) {
    state->best_partial_font_missing_char_count = missing_count;
    state->best_partial_font = current_font;
  }
  return missing_count;
}

https://codereview.chromium.org/382793004/diff/40001/ui/gfx/render_text_win.c...
ui/gfx/render_text_win.cc:1114: while (fonts.NextFont(&current_font)) {
Now that you're not calling SetNextFont() on |fonts|, can that method be
removed?

https://codereview.chromium.org/382793004/diff/40001/ui/gfx/render_text_win.c...
ui/gfx/render_text_win.cc:1217: DCHECK_EQ(shaping_result,
USP_E_SCRIPT_NOT_IN_FONT);
Nit: The expected value constant should be the first param to DCHECK_EQ.

https://codereview.chromium.org/382793004/diff/40001/ui/gfx/render_text_win.c...
ui/gfx/render_text_win.cc:1218: return INT_MAX;
What does it mean for INT_MAX to be returned by this? Please document in the
header comment.