RenderTextHarfBuzz: Implement font fallback for Win and Linux

RenderTextHarfBuzz: Implement font fallback for Win and Linux

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

[ckocagil, 2014-06-21 20:11:38]

PTAL. Feel free to only leave high level comments instead of a full review.

[msw, 2014-06-30 17:35:54]

Can we cache the fallback families for a given family in a map? Is that
worthwhile for this initial impl?

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
File ui/gfx/font_fallback_linux.cc (right):

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
ui/gfx/font_fallback_linux.cc:16: family.u.s = reinterpret_cast<const unsigned
char*>(font_family.c_str());
Maybe use FcChar8* to match the FC types like platform_font_pango.cc?

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
ui/gfx/font_fallback_linux.cc:19: FcConfigSubstitute(NULL, pattern,
FcMatchPattern);
Check the return value here? (I guess font_render_params_linux.cc doesn't)

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
ui/gfx/font_fallback_linux.cc:29: if (fallback_fonts.empty() ||
fallback_fonts.back() != name)
Do we expect there to be adjacent duplicate entries?

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_mac.cc
File ui/gfx/font_fallback_mac.cc (right):

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_mac...
ui/gfx/font_fallback_mac.cc:9: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
Do we need a stubbed impl for iOS, Android, etc.?

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

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1014: if (!run->HasMissingGlyphs())
Do we want something like RenderTextWin's best_partial_font? Otherwise it seems
like we'll return the last fallback font's results, which may be worse than the
original font.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/render_text...

[ckocagil, 2014-07-04 15:30:16]

PTAL. Note: this doesn't implement the full fallback logic in RenderTextWin,
which uses Uniscribe in addition to linked fonts.

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
File ui/gfx/font_fallback_linux.cc (right):

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
ui/gfx/font_fallback_linux.cc:16: family.u.s = reinterpret_cast<const unsigned
char*>(font_family.c_str());
On 2014/06/30 17:35:53, msw wrote:
> Maybe use FcChar8* to match the FC types like platform_font_pango.cc?

Done.

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
ui/gfx/font_fallback_linux.cc:19: FcConfigSubstitute(NULL, pattern,
FcMatchPattern);
On 2014/06/30 17:35:53, msw wrote:
> Check the return value here? (I guess font_render_params_linux.cc doesn't)

Let's do it here anyway, done.

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
ui/gfx/font_fallback_linux.cc:29: if (fallback_fonts.empty() ||
fallback_fonts.back() != name)
On 2014/06/30 17:35:53, msw wrote:
> Do we expect there to be adjacent duplicate entries?

Yes. Currently we only take the family name into account without looking at the
bold/italic/etc fields. There are can be than one font entry in the list with
the same family name and with varying bold/italic fields.

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_mac.cc
File ui/gfx/font_fallback_mac.cc (right):

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_mac...
ui/gfx/font_fallback_mac.cc:9: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
On 2014/06/30 17:35:54, msw wrote:
> Do we need a stubbed impl for iOS, Android, etc.?

I'll add stubs as we get compile failures in CQ or tree.

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

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/render_text_harfb...
ui/gfx/render_text_harfbuzz.cc:1014: if (!run->HasMissingGlyphs())
On 2014/06/30 17:35:54, msw wrote:
> Do we want something like RenderTextWin's best_partial_font? Otherwise it
seems
> like we'll return the last fallback font's results, which may be worse than
the
> original font.
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/render_text...

Done.

[msw, 2014-07-07 22:52:46]

On 2014/07/04 15:30:16, ckocagil wrote:
> PTAL. Note: this doesn't implement the full fallback logic in RenderTextWin,
> which uses Uniscribe in addition to linked fonts.

Are you referring to the Enh file behavior in ChooseFallbackFont or something
else? Should we move that additional fallback logic to
ui/gfx/font_fallback_win.cc?

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
File ui/gfx/font_fallback_linux.cc (right):

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
ui/gfx/font_fallback_linux.cc:29: if (fallback_fonts.empty() ||
fallback_fonts.back() != name)
On 2014/07/04 15:30:15, ckocagil wrote:
> On 2014/06/30 17:35:53, msw wrote:
> > Do we expect there to be adjacent duplicate entries?
> 
> Yes. Currently we only take the family name into account without looking at
the
> bold/italic/etc fields. There are can be than one font entry in the list with
> the same family name and with varying bold/italic fields.

That seems worth explaining in a short comment here.

https://codereview.chromium.org/331713003/diff/120001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (left):

https://codereview.chromium.org/331713003/diff/120001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:982: hb_buffer_set_language(buffer,
hb_language_get_default());
q: why are you removing this? Is |buffer| already initialized with
hb_language_get_default by hb_buffer_create, making this a trivial no-op?

https://codereview.chromium.org/331713003/diff/120001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/120001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:475: if (glyphs[i] == kMissingGlyphId)
optional nit: missing += glyphs[i] == kMissingGlyphId ? 1 : 0;
(you can remove the curly braces if you do that one-liner)

[ckocagil, 2014-07-12 11:47:54]

Latest comments addressed. Also added a cache to font_fallback_linux to address
your previous comment.

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
File ui/gfx/font_fallback_linux.cc (right):

https://codereview.chromium.org/331713003/diff/80001/ui/gfx/font_fallback_lin...
ui/gfx/font_fallback_linux.cc:29: if (fallback_fonts.empty() ||
fallback_fonts.back() != name)
On 2014/07/07 22:52:45, msw wrote:
> On 2014/07/04 15:30:15, ckocagil wrote:
> > On 2014/06/30 17:35:53, msw wrote:
> > > Do we expect there to be adjacent duplicate entries?
> > 
> > Yes. Currently we only take the family name into account without looking at
> the
> > bold/italic/etc fields. There are can be than one font entry in the list
with
> > the same family name and with varying bold/italic fields.
> 
> That seems worth explaining in a short comment here.

Done.

https://codereview.chromium.org/331713003/diff/120001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (left):

https://codereview.chromium.org/331713003/diff/120001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:982: hb_buffer_set_language(buffer,
hb_language_get_default());
On 2014/07/07 22:52:46, msw wrote:
> q: why are you removing this? Is |buffer| already initialized with
> hb_language_get_default by hb_buffer_create, making this a trivial no-op?

Added back.

I had no idea what this did, so I decided to remove it and possibly add it back
when I got to the TODO. But I know what it does now, it is used to enable
language specific shaping features (even though Arabic and Urdu use the same
script, they look different).

https://codereview.chromium.org/331713003/diff/120001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/120001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:475: if (glyphs[i] == kMissingGlyphId)
On 2014/07/07 22:52:46, msw wrote:
> optional nit: missing += glyphs[i] == kMissingGlyphId ? 1 : 0;
> (you can remove the curly braces if you do that one-liner)

Done.

[msw, 2014-07-12 17:41:11]

Thanks again for working on this!

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:263: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
In another patch, it might make sense to expose a cross-platform
LinkedFontsIterator. Otherwise, each time we shape a run, we're going through
the whole fallback process, even though the initial font might be perfectly
fine. LinkedFontsIterator would allow more fonts to be requested as needed.

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_win.h
File ui/gfx/font_fallback_win.h (right):

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.h:81: // Finds a fallback font to use to render the
specified |text| with respect to
nit: remove "to use", "to render" is sufficient

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:957:
GetFallbackFontFamilies(primary_font.GetFontName());
Let's only call this if |primary_font| doesn't shape all the glyphs (for perf)

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:973: if (GetUniscribeFallbackFont(NULL,
primary_font,
Could font_fallback_win insert any Uniscribe fallback to the vector returned by
GetFallbackFontFamilies? That would avoid platform-specific code here. (it would
also let you restore the single loop over |fonts|)

[ckocagil, 2014-07-12 19:53:26]

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:263: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
On 2014/07/12 17:41:11, msw wrote:
> In another patch, it might make sense to expose a cross-platform
> LinkedFontsIterator. Otherwise, each time we shape a run, we're going through
> the whole fallback process, even though the initial font might be perfectly
> fine. LinkedFontsIterator would allow more fonts to be requested as needed.

I don't see how this would improve performance (we seem to cache the important
bits on Win and Linux). It would simplify the code however.

Is the code below similar to what you have in mind?
FallbackFonts fonts(primary_font, text, text_length);
while (fonts.NextFont(&current_font))
  ...;

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_win.h
File ui/gfx/font_fallback_win.h (right):

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.h:81: // Finds a fallback font to use to render the
specified |text| with respect to
On 2014/07/12 17:41:11, msw wrote:
> nit: remove "to use", "to render" is sufficient

Done.

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:957:
GetFallbackFontFamilies(primary_font.GetFontName());
On 2014/07/12 17:41:11, msw wrote:
> Let's only call this if |primary_font| doesn't shape all the glyphs (for perf)

Done.

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:973: if (GetUniscribeFallbackFont(NULL,
primary_font,
On 2014/07/12 17:41:11, msw wrote:
> Could font_fallback_win insert any Uniscribe fallback to the vector returned
by
> GetFallbackFontFamilies? That would avoid platform-specific code here. (it
would
> also let you restore the single loop over |fonts|)

This would issue a call to GetUniscribeFallbackFont every time we shape
something, and that isn't a trivial function. We currently don't have a good way
to cache the Uniscribe lookup results.

We could merge the ifdef with the loop below it though, but there's still the
issue of hiding (or getting rid of) the passed HDC.

[msw, 2014-07-12 22:03:38]

Sorry, I got overly ambitious with my last round of comments.
This patch is already a good step in the right direction.
LGTM modulo the NULL HDC question in render_text_harfbuzz.cc.

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:263: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
On 2014/07/12 19:53:26, ckocagil wrote:
> On 2014/07/12 17:41:11, msw wrote:
> > In another patch, it might make sense to expose a cross-platform
> > LinkedFontsIterator. Otherwise, each time we shape a run, we're going
through
> > the whole fallback process, even though the initial font might be perfectly
> > fine. LinkedFontsIterator would allow more fonts to be requested as needed.
> 
> I don't see how this would improve performance (we seem to cache the important
> bits on Win and Linux). It would simplify the code however.

Your comment in RenderTextHarfBuzz illustrates my concern, where my request to
make GetFallbackFontFamilies call GetUniscribeFallbackFont, would make us "issue
a call to GetUniscribeFallbackFont every time we shape something". So, perhaps
what you have in the latest patch set is good for now.

> Is the code below similar to what you have in mind?
> FallbackFonts fonts(primary_font, text, text_length);
> while (fonts.NextFont(&current_font))
>   ...;

Yes, (but eventually using the the whole |font_list()| instead of the
|primary_font|). Anyway, let's use the latest patch set as-is for this first
implementation.

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/200001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:973: if (GetUniscribeFallbackFont(NULL,
primary_font,
On 2014/07/12 19:53:26, ckocagil wrote:
> On 2014/07/12 17:41:11, msw wrote:
> > Could font_fallback_win insert any Uniscribe fallback to the vector returned
> by
> > GetFallbackFontFamilies? That would avoid platform-specific code here. (it
> would
> > also let you restore the single loop over |fonts|)
> 
> This would issue a call to GetUniscribeFallbackFont every time we shape
> something, and that isn't a trivial function. We currently don't have a good
way
> to cache the Uniscribe lookup results.

You're right, forget this for now, we can consider more cleanup later.

> We could merge the ifdef with the loop below it though, but there's still the
> issue of hiding (or getting rid of) the passed HDC.

When we address this later, we could either have font_fallback_win cache its own
HDC (if it doesn't need to actually share one with RenderText). Otherwise,
perhaps we could pass the RenderText HDC to the LinkedFontsIterator instance on
windows, like:

  FallbackFonts fonts(primary_font, text, text_length);
#if defined(OS_WIN)
  fonts.set_hdc(cached_hdc_);
#endif
  while (fonts.NextFont(&current_font))
    ...

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:967: if (GetUniscribeFallbackFont(NULL,
primary_font,
Is it okay that we're passing in NULL here?

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:983: std::vector<std::string> fonts =
GetFallbackFontFamilies(primary_font_name);
I think in a followup, we'll want to use the other fonts in the FontList
|font_list()|, and merge those with the fallback fonts for the primary font, and
perhaps even the fallback fonts of the other fonts of the FontList.

[Daniel Erat, 2014-07-14 14:58:21]

lgtm for linux with some nits about string arguments that should be const
references and a suggestion for merging this with another in-flight change in
the future

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/font_fallback.h
File ui/gfx/font_fallback.h (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/font_fallback.h#...
ui/gfx/font_fallback.h:15: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family);
argument should be const std::string&

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/font_fallback_li...
File ui/gfx/font_fallback_linux.cc (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/font_fallback_li...
ui/gfx/font_fallback_linux.cc:19: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
in a later change, it may make sense to unify this with the
GetCustomFontRenderParams() code that i'm adding in
https://codereview.chromium.org/382273002/. that method also does a fontconfig
query (but additionally including the size), but returns the primary font
instead of the fallbacks.

PlatformFontPango calls that function to find the primary family to use from the
the list read from a pango description, but perhaps it could call this function
instead or GetCustomFontRenderParams() could be updated to return the full list
of families.

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.h (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.h:115: void
ShapeRunWithFont(internal::TextRunHarfBuzz* run, std::string font);
const std::string&

[ckocagil, 2014-07-16 23:42:22]

Comments addressed.

Mike, please take a quick look at the font_fallback_win.cc changes, thanks!

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/font_fallback.h
File ui/gfx/font_fallback.h (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/font_fallback.h#...
ui/gfx/font_fallback.h:15: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family);
On 2014/07/14 14:58:20, Daniel Erat wrote:
> argument should be const std::string&

Done.

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/font_fallback_li...
File ui/gfx/font_fallback_linux.cc (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/font_fallback_li...
ui/gfx/font_fallback_linux.cc:19: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
On 2014/07/14 14:58:20, Daniel Erat wrote:
> in a later change, it may make sense to unify this with the
> GetCustomFontRenderParams() code that i'm adding in
> https://codereview.chromium.org/382273002/. that method also does a fontconfig
> query (but additionally including the size), but returns the primary font
> instead of the fallbacks.
> 
> PlatformFontPango calls that function to find the primary family to use from
the
> the list read from a pango description, but perhaps it could call this
function
> instead or GetCustomFontRenderParams() could be updated to return the full
list
> of families.

Acknowledged.

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:967: if (GetUniscribeFallbackFont(NULL,
primary_font,
On 2014/07/12 22:03:38, msw wrote:
> Is it okay that we're passing in NULL here?

It should be okay in the sense that MSDN documents its use, but performance-wise
I'm not sure. Even though it should be a simple operation and I have no idea
about GDI internals, GDI might have locks to guard accesses to its shared
object.

So I removed this argument and we now use a separate DC in font_fallback_win.cc.

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.h (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.h:115: void
ShapeRunWithFont(internal::TextRunHarfBuzz* run, std::string font);
On 2014/07/14 14:58:20, Daniel Erat wrote:
> const std::string&

Done.

[msw, 2014-07-17 00:08:04]

LGTM with a nit.

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/220001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:967: if (GetUniscribeFallbackFont(NULL,
primary_font,
On 2014/07/16 23:42:22, ckocagil wrote:
> On 2014/07/12 22:03:38, msw wrote:
> > Is it okay that we're passing in NULL here?
> 
> It should be okay in the sense that MSDN documents its use, but
performance-wise
> I'm not sure. Even though it should be a simple operation and I have no idea
> about GDI internals, GDI might have locks to guard accesses to its shared
> object.
> 
> So I removed this argument and we now use a separate DC in
font_fallback_win.cc.

Nice.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:286: hdc = CreateCompatibleDC(NULL);
nit: can you inline this with the static decl?

[msw, 2014-07-17 00:08:37]

+CC Alexei in case he wishes to take a look.

[ckocagil, 2014-07-17 00:10:49]

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:286: hdc = CreateCompatibleDC(NULL);
On 2014/07/17 00:08:04, msw wrote:
> nit: can you inline this with the static decl?

Wouldn't that cause a static initializer to be added?

[msw, 2014-07-17 00:15:35]

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:286: hdc = CreateCompatibleDC(NULL);
On 2014/07/17 00:10:49, ckocagil wrote:
> On 2014/07/17 00:08:04, msw wrote:
> > nit: can you inline this with the static decl?
> 
> Wouldn't that cause a static initializer to be added?

I thought this was okay for function-scoped statics.
Maybe just ignore my comment and land this as-is.
(I'm hoping we can enable harfbuzz by default asap)

[Daniel Erat, 2014-07-17 02:22:24]

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:286: hdc = CreateCompatibleDC(NULL);
On 2014/07/17 00:15:35, msw wrote:
> On 2014/07/17 00:10:49, ckocagil wrote:
> > On 2014/07/17 00:08:04, msw wrote:
> > > nit: can you inline this with the static decl?
> > 
> > Wouldn't that cause a static initializer to be added?
> 
> I thought this was okay for function-scoped statics.
> Maybe just ignore my comment and land this as-is.
> (I'm hoping we can enable harfbuzz by default asap)

function-scoped static initializers are fine, per my reading of
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/%22eli...

[Alexei Svitkine (slow), 2014-07-17 21:22:00]

Looks good, some comments for you below.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_li...
File ui/gfx/font_fallback_linux.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_li...
ui/gfx/font_fallback_linux.cc:19: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
The param should be passed by const ref. (How does this even compile given the
mismatch with the header?)

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_li...
ui/gfx/font_fallback_linux.cc:24: std::vector<std::string> fallback_fonts;
Nit: Can you add directly to |cache_value|? (And rename |cache_value| to
|fallback_fonts|?

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:265: internal::LinkedFontsIterator
linked_fonts(Font(font_family, 10));
Please add a comment about the hard-coded 10.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:279: //
http://msdn.microsoft.com/en-us/library/windows/desktop/dn280480.aspx
This comment should either be in the header or within the body of the function
now that the function is exported.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:968: &(GetLayoutText()[run->range.start()]),
run->range.length(),
Nit: Can you extract this to a local variable to make this if more concise.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:999: const std::string& font) {
Name |font| param more meaningfully, e.g. |font_family|.

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

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1006: Font current_font = Font(fonts[0],
original_font.GetFontSize());
Nit: You can just do:

Font current_font(...);

[ckocagil, 2014-07-17 23:12:01]

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_li...
File ui/gfx/font_fallback_linux.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_li...
ui/gfx/font_fallback_linux.cc:19: std::vector<std::string>
GetFallbackFontFamilies(std::string font_family) {
On 2014/07/17 21:22:00, Alexei Svitkine wrote:
> The param should be passed by const ref. (How does this even compile given the
> mismatch with the header?)

Done. (it shouldn't compile, I didn't build on Linux after making it a const
ref)

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_li...
ui/gfx/font_fallback_linux.cc:24: std::vector<std::string> fallback_fonts;
On 2014/07/17 21:22:00, Alexei Svitkine wrote:
> Nit: Can you add directly to |cache_value|? (And rename |cache_value| to
> |fallback_fonts|?

Done.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
File ui/gfx/font_fallback_win.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:265: internal::LinkedFontsIterator
linked_fonts(Font(font_family, 10));
On 2014/07/17 21:22:00, Alexei Svitkine wrote:
> Please add a comment about the hard-coded 10.

Done.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:279: //
http://msdn.microsoft.com/en-us/library/windows/desktop/dn280480.aspx
On 2014/07/17 21:22:00, Alexei Svitkine wrote:
> This comment should either be in the header or within the body of the function
> now that the function is exported.

Done, moved into the body.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/font_fallback_wi...
ui/gfx/font_fallback_win.cc:286: hdc = CreateCompatibleDC(NULL);
Oh, now I read the thread and came to the same conclusion. Done!

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:968: &(GetLayoutText()[run->range.start()]),
run->range.length(),
On 2014/07/17 21:22:00, Alexei Svitkine wrote:
> Nit: Can you extract this to a local variable to make this if more concise.

Done.

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:999: const std::string& font) {
On 2014/07/17 21:22:00, Alexei Svitkine wrote:
> Name |font| param more meaningfully, e.g. |font_family|.

Done.

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

https://codereview.chromium.org/331713003/diff/240001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1006: Font current_font = Font(fonts[0],
original_font.GetFontSize());
On 2014/07/17 21:22:00, Alexei Svitkine wrote:
> Nit: You can just do:
> 
> Font current_font(...);

Done.

[Alexei Svitkine (slow), 2014-07-18 13:08:48]

LGTM with one more suggestion

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

https://codereview.chromium.org/331713003/diff/260001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1000:
GetFallbackFontFamilies(original_font.GetFontName());
Nit: Can this be moved down right above the loop over it? That way, if original
font or the cached or substitute font works, we don't need to pay the cost of
querying this / copying the vector.

https://codereview.chromium.org/331713003/diff/260001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1006: Font current_font(fonts[0],
original_font.GetFontSize());
With the suggestion above, this can just be initialized to |original_font|.

[ckocagil, 2014-07-18 15:50:12]

Comments addressed. I'll check the commit box after the try jobs pass. Thanks
for the reviews everyone!

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

https://codereview.chromium.org/331713003/diff/260001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1000:
GetFallbackFontFamilies(original_font.GetFontName());
On 2014/07/18 13:08:48, Alexei Svitkine wrote:
> Nit: Can this be moved down right above the loop over it? That way, if
original
> font or the cached or substitute font works, we don't need to pay the cost of
> querying this / copying the vector.

Done.

https://codereview.chromium.org/331713003/diff/260001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1006: Font current_font(fonts[0],
original_font.GetFontSize());
On 2014/07/18 13:08:48, Alexei Svitkine wrote:
> With the suggestion above, this can just be initialized to |original_font|.

Done.

[msw, 2014-07-18 17:09:18]

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

https://codereview.chromium.org/331713003/diff/280001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1003: Font current_font(original_font.GetFontName(),
original_font.GetFontSize());
nit: just do "Font current_font = original_font;"

https://codereview.chromium.org/331713003/diff/280001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1032: if (GetUniscribeFallbackFont(run->font,
run_text, run_length,
Use original_font here? ShapeTextRunWithFont modifies run->font.

[ckocagil, 2014-07-18 17:25:31]

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

https://codereview.chromium.org/331713003/diff/280001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1003: Font current_font(original_font.GetFontName(),
original_font.GetFontSize());
On 2014/07/18 17:09:18, msw wrote:
> nit: just do "Font current_font = original_font;"

Done.

https://codereview.chromium.org/331713003/diff/280001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1032: if (GetUniscribeFallbackFont(run->font,
run_text, run_length,
On 2014/07/18 17:09:18, msw wrote:
> Use original_font here? ShapeTextRunWithFont modifies run->font.

AFAICT this is identical to the logic before my unroll patch (r282550). This
doesn't make much sense to me, so I assume this was a mistake from before and
I'm changing it to original_font.

[msw, 2014-07-18 17:37:10]

still lgtm, thanks!

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

https://codereview.chromium.org/331713003/diff/280001/ui/gfx/render_text_win....
ui/gfx/render_text_win.cc:1032: if (GetUniscribeFallbackFont(run->font,
run_text, run_length,
On 2014/07/18 17:25:30, ckocagil wrote:
> On 2014/07/18 17:09:18, msw wrote:
> > Use original_font here? ShapeTextRunWithFont modifies run->font.
> 
> AFAICT this is identical to the logic before my unroll patch (r282550). This
> doesn't make much sense to me, so I assume this was a mistake from before and
> I'm changing it to original_font.

Acknowledged.

[ckocagil, 2014-07-18 19:27:40]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-07-18 19:30:09]

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

[commit-bot: I haz the power, 2014-07-18 22:08:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[ckocagil, 2014-07-18 23:31:28]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-07-18 23:32:08]

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

[commit-bot: I haz the power, 2014-07-19 03:01:42]

Change committed as 284297

[ebrahimgnu, 2014-07-19 21:02:43]

https://codereview.chromium.org/331713003/diff/340001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/340001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1036: run->positions[i].set(run->width +
x_offset, y_offset);
Seems this caused regression of
https://code.google.com/p/chromium/issues/detail?id=391203 again :(

[msw, 2014-07-19 23:31:41]

https://codereview.chromium.org/331713003/diff/340001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/331713003/diff/340001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:1036: run->positions[i].set(run->width +
x_offset, y_offset);
On 2014/07/19 21:02:43, ebraminio wrote:
> Seems this caused regression of
> https://code.google.com/p/chromium/issues/detail?id=391203 again :(

I'll fix this in https://codereview.chromium.org/408733002/
(I'm sure it was just a merge error)