RenderTextHarfBuzz: Check CreateSkiaTypeface return value against NULL

RenderTextHarfBuzz: Check CreateSkiaTypeface return value against NULL

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

[msw, 2014-07-31 15:14:25]

Can you add a test that this case no longer crashes and also verifies the edge
case return values of GetClusterAt and GetGraphemeBounds?

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

https://codereview.chromium.org/435583003/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1019: size_t best_font_missing = -1;
Should this be std::numeric_limits<size_t>::max()? Otherwise I'm not sure your
checks below for "current_missing < best_font_missing" will ever be true...

https://codereview.chromium.org/435583003/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1023: if (ShapeRunWithFont(run,
primary_font_name)) {
Hmm, I really hope that the primary font wouldn't yield a NULL skia_face...

https://codereview.chromium.org/435583003/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1064: if (!best_font.empty() &&
ShapeRunWithFont(run, best_font))
Can we avoid this extra shaping if (current_missing <= best_font_missing)?

[ckocagil, 2014-08-02 12:14:04]

On 2014/07/31 15:14:25, msw wrote:
> Can you add a test that this case no longer crashes and also verifies the edge
> case return values of GetClusterAt and GetGraphemeBounds?

Done.

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

https://codereview.chromium.org/435583003/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1019: size_t best_font_missing = -1;
On 2014/07/31 15:14:25, msw wrote:
> Should this be std::numeric_limits<size_t>::max()? Otherwise I'm not sure your
> checks below for "current_missing < best_font_missing" will ever be true...

Either is fine (well-defined by the lang and portable) since size_t is unsigned,
but I'm still changing it to avoid any confusion.

https://codereview.chromium.org/435583003/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1023: if (ShapeRunWithFont(run,
primary_font_name)) {
On 2014/07/31 15:14:25, msw wrote:
> Hmm, I really hope that the primary font wouldn't yield a NULL skia_face...

So do I, but IMO we should check that possibility anyway.

https://codereview.chromium.org/435583003/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:1064: if (!best_font.empty() &&
ShapeRunWithFont(run, best_font))
On 2014/07/31 15:14:25, msw wrote:
> Can we avoid this extra shaping if (current_missing <= best_font_missing)?

But if the primary font and the fallback fonts all miss 1 glyph, we'd use a
fallback font and not the primary font. Instead I now keep the name of the last
font, PTAL.

[msw, 2014-08-02 18:05:33]

LGTM with nits. I had some ideas, but don't address them now:

1) ShapeRunWithFont could clear its run data at the top, then bail if the
supplied font can't be initialized (so ShapeRun wouldn't need to set zeroes at
the bottom, which is a potentially incomplete way of clearing the run data).

2) We could construct multiple run objects, shaping each with the various fonts
(ie. |current_shaped_run|, |best_shaped_run|), then just use the best one
instead of re-shaping a single run instance. (maybe this would help with
performance?).
But those are just thoughts for potential followup.

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1031: best_font = primary_font_name;
nit: s/primary_font_name/current_font/

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1047: best_font = uniscribe_font.GetFontName();
nit: s/uniscribe_font.GetFontName()/current_font/

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1064: best_font = fonts[i];
nit: s/fonts[i]/current_font/

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2265: // Try shaping with fonts that don't exist
on the system.
Consider: "// Ensure that shaping with a non-existent font does not cause a
crash."

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2268: 
nit: remove blank line

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2293: 
nit: remove blank line

[ckocagil, 2014-08-03 03:36:55]

Nits addressed, checking CQ box. For your suggestions, PTAL at
https://code.google.com/p/chromium/issues/detail?id=399919

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_harfb...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1031: best_font = primary_font_name;
On 2014/08/02 18:05:33, msw wrote:
> nit: s/primary_font_name/current_font/

Done.

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1047: best_font = uniscribe_font.GetFontName();
On 2014/08/02 18:05:33, msw wrote:
> nit: s/uniscribe_font.GetFontName()/current_font/

Done.

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1064: best_font = fonts[i];
On 2014/08/02 18:05:33, msw wrote:
> nit: s/fonts[i]/current_font/

Done.

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2265: // Try shaping with fonts that don't exist
on the system.
On 2014/08/02 18:05:33, msw wrote:
> Consider: "// Ensure that shaping with a non-existent font does not cause a
> crash."

Done.

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2268: 
On 2014/08/02 18:05:33, msw wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/435583003/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2293: 
On 2014/08/02 18:05:33, msw wrote:
> nit: remove blank line

Done.

[ckocagil, 2014-08-03 03:37:12]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-08-03 03:37:47]

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

[commit-bot: I haz the power, 2014-08-03 05:50:34]

Change committed as 287250