linux/chromeos: Improve querying for Fontconfig defaults.

linux/chromeos: Improve queries for Fontconfig defaults.

r302898 made FontRenderParams avoid calling FcFontMatch()
for empty queries, instead just sticking to the default
pattern. Many users don't have sane default patterns,
though, so instead call FcConfigSubstituteWithPat() to load
settings that aren't tied to particular font families.

Also use scoped_ptr to simplify FcPattern management and add
myself as an owner for font_render_params*.

BUG=442443, 435277, 423056
Committed: https://crrev.com/2ff93446c3405557b6024d4e65eb5677195456cd
Cr-Commit-Position: refs/heads/master@{#309560}

[Daniel Erat, 2014-12-17 22:31:44]

derat@chromium.org changed reviewers:
+ behdad@chromium.org, msw@chromium.org

[Daniel Erat, 2014-12-17 22:31:44]

Let me know what you guys think about doing this.

[msw, 2014-12-17 22:42:23]

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:129: // patterns configured
(http://crbug.com/442443, http://crbug.com/435277,
optional nit: to avoid wrapping onto the next line: "configured:
http://crbug.com/442443, http://crbug.com/435277, etc."

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:102: // The match _should_ take
affect on desktop Linux, though:
nit: "take effect"

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:277: // Desktop Linux performs a
match instead of using the default pattern.
Shouldn't Linux fall back on the default pattern if there is no font match?
(ie. shouldn't this test still pass as-is?)

[Daniel Erat, 2014-12-18 00:15:46]

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_linux.cc (right):

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux.cc:129: // patterns configured
(http://crbug.com/442443, http://crbug.com/435277,
On 2014/12/17 22:42:23, msw wrote:
> optional nit: to avoid wrapping onto the next line: "configured:
> http://crbug.com/442443, http://crbug.com/435277, etc."

Done.

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:102: // The match _should_ take
affect on desktop Linux, though:
On 2014/12/17 22:42:23, msw wrote:
> nit: "take effect"

oh my, that's embarrassing. i know the difference between those words. thanks.
:-/

https://codereview.chromium.org/811123002/diff/40001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:277: // Desktop Linux performs a
match instead of using the default pattern.
On 2014/12/17 22:42:23, msw wrote:
> Shouldn't Linux fall back on the default pattern if there is no font match?
> (ie. shouldn't this test still pass as-is?)

that's what i was thinking at first, but i saw failures when i tried it. for
example, "antialias" was true, which i think means that fontconfig is falling
back to its built-in defaults. but now that you mentioned it and i thought about
it a bit more, i realized that the problem is probably that fontconfig matches
require there to be at least one font installed, and this test wasn't calling
LoadSystemFont() initially. now that i've added that, it passes. without these
changes.

[msw, 2014-12-18 01:10:49]

lgtm with a nit

https://codereview.chromium.org/811123002/diff/60001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/811123002/diff/60001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:226:
ASSERT_TRUE(LoadSystemFont("arial.ttf"));
nit: We previously tried to avoid loading fonts for these tests wherever
possible. I realize it's needed for ForceFullHintingWhenAntialiasingIsDisabled,
but perhaps we should continue to avoid it here?

[Daniel Erat, 2014-12-18 17:17:59]

https://codereview.chromium.org/811123002/diff/60001/ui/gfx/font_render_param...
File ui/gfx/font_render_params_linux_unittest.cc (right):

https://codereview.chromium.org/811123002/diff/60001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:226:
ASSERT_TRUE(LoadSystemFont("arial.ttf"));
On 2014/12/18 01:10:49, msw wrote:
> nit: We previously tried to avoid loading fonts for these tests wherever
> possible. I realize it's needed for
ForceFullHintingWhenAntialiasingIsDisabled,
> but perhaps we should continue to avoid it here?

sure, done. just added it here since just about all of the other tests do it
now, but yeah, it doesn't seem to be necessary.

[behdad_google, 2014-12-19 19:59:05]

behdad@google.com changed reviewers:
+ behdad@google.com

[behdad_google, 2014-12-19 19:59:06]

Wouldn't this reintroduce the original bug we were trying to fix with the
no-match change?  Eg. if the default font enables bytecode hinting, we don't
want to use that setting for every font.  Or am I missing something?

[Daniel Erat, 2014-12-19 20:52:37]

On 2014/12/19 19:59:06, behdad_google wrote:
> Wouldn't this reintroduce the original bug we were trying to fix with the
> no-match change?  Eg. if the default font enables bytecode hinting, we don't
> want to use that setting for every font.  Or am I missing something?

i was mostly trying to fix this on chrome os initially. some background:

as discussed on http://crbug.com/420355 and http://crbug.com/421247,
FontRenderParams was calling FcFontMatch() for empty queries (which are used to
get the default rendering settings that blink uses for web fonts). Fontconfig
matched Arimo in this case, which was using the BCI instead of the autohinter.
avoiding the FcFontMatch() call meant that we could add <match target="pattern">
configs on chrome os that enabled autohinting. those settings would be used for
web fonts, so the problem was avoided there.

unfortunately, i don't think that many linux users use <match target="pattern">
stanzas to set their defaults. as a result, r302898 meant that the rendering
settings used for web fonts could change suddenly, since their <match
target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be ignored. this is
why i've been considering landing this change to at least go back to the earlier
behavior on linux.

in any case, users who aren't enabling the autohinter via <match
target="pattern">[1] will end up with broken fonts. i filed
http://crbug.com/435277 to track a lower-level fix for the problem, but i got
some pushback. if you'd like to weigh in there with your opinion about how this
should be resolved, i'd appreciate it greatly. thanks!

1. e.g. via something like this:

  <match target="pattern">
    <edit name="autohint" mode="assign"><bool>true</bool></edit>
  </match>
  <match target="font">
    <edit name="antialias" mode="assign"><bool>true</bool></edit>
    <edit name="autohint" mode="assign"><bool>false</bool></edit>
    <edit name="hinting" mode="assign"><bool>true</bool></edit>
    <edit name="hintstyle" mode="assign"><const>hintfull</const></edit>
    <edit name="rgba" mode="assign"><const>none</const></edit>
  </match>

[behdad_google, 2014-12-19 21:37:19]

On 2014/12/19 20:52:37, Daniel Erat wrote:
> On 2014/12/19 19:59:06, behdad_google wrote:
> > Wouldn't this reintroduce the original bug we were trying to fix with the
> > no-match change?  Eg. if the default font enables bytecode hinting, we don't
> > want to use that setting for every font.  Or am I missing something?
> 
> i was mostly trying to fix this on chrome os initially. some background:
> 
> as discussed on http://crbug.com/420355 and http://crbug.com/421247,
> FontRenderParams was calling FcFontMatch() for empty queries (which are used
to
> get the default rendering settings that blink uses for web fonts). Fontconfig
> matched Arimo in this case, which was using the BCI instead of the autohinter.
> avoiding the FcFontMatch() call meant that we could add <match
target="pattern">
> configs on chrome os that enabled autohinting. those settings would be used
for
> web fonts, so the problem was avoided there.
> 
> unfortunately, i don't think that many linux users use <match
target="pattern">
> stanzas to set their defaults. as a result, r302898 meant that the rendering
> settings used for web fonts could change suddenly, since their <match
> target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be ignored. this is
> why i've been considering landing this change to at least go back to the
earlier
> behavior on linux.

Ok lets fix that properly then!

What you need is this: when you get your query pattern, make a copy and call it
font pattern, clear the FC_FAMILY in it (and if needed set one nonexisting
value), then call FcFontRenderPrepare (config, query, font);

That will cause the generic font config stanzas to be applied, but nothing
specific to a specific font.

[Daniel Erat, 2014-12-19 22:02:11]

On 2014/12/19 21:37:19, behdad_google wrote:
> On 2014/12/19 20:52:37, Daniel Erat wrote:
> > On 2014/12/19 19:59:06, behdad_google wrote:
> > > Wouldn't this reintroduce the original bug we were trying to fix with the
> > > no-match change?  Eg. if the default font enables bytecode hinting, we
don't
> > > want to use that setting for every font.  Or am I missing something?
> > 
> > i was mostly trying to fix this on chrome os initially. some background:
> > 
> > as discussed on http://crbug.com/420355 and http://crbug.com/421247,
> > FontRenderParams was calling FcFontMatch() for empty queries (which are used
> to
> > get the default rendering settings that blink uses for web fonts).
Fontconfig
> > matched Arimo in this case, which was using the BCI instead of the
autohinter.
> > avoiding the FcFontMatch() call meant that we could add <match
> target="pattern">
> > configs on chrome os that enabled autohinting. those settings would be used
> for
> > web fonts, so the problem was avoided there.
> > 
> > unfortunately, i don't think that many linux users use <match
> target="pattern">
> > stanzas to set their defaults. as a result, r302898 meant that the rendering
> > settings used for web fonts could change suddenly, since their <match
> > target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be ignored. this
is
> > why i've been considering landing this change to at least go back to the
> earlier
> > behavior on linux.
> 
> Ok lets fix that properly then!
> 
> What you need is this: when you get your query pattern, make a copy and call
it
> font pattern, clear the FC_FAMILY in it (and if needed set one nonexisting
> value), then call FcFontRenderPrepare (config, query, font);
> 
> That will cause the generic font config stanzas to be applied, but nothing
> specific to a specific font.

that wouldn't fix http://crbug.com/435277, would it? i think that users who
disable the autohinter by default will still have deformed fonts. is that just a
solution for the default-config-specifies-font-matches issue, and an alternative
to this patch?

and i'm still a bit shaky on fontconfig usage. is something like the following
what you're recommending?

  [initialize |query_pattern| based on |query|, a FontRenderParamsQuery...]

  FcConfigSubstitute(NULL, query_pattern, FcMatchPattern);
  FcDefaultSubstitute(query_pattern);

  FcResult result;
  FcPattern* font_pattern = FcFontMatch(NULL, query_pattern, &result);
  if (!font_pattern) { 
    FcPatternDestroy(query_pattern);
    return false;
  }

  FcPattern* result_pattern = font_pattern;
  if (query.is_empty()) {
    FcPatternRemove(font_pattern, FC_FAMILY, 0);
    result_pattern = FcFontRenderPrepare(NULL, query_pattern, font_pattern);
    FcPatternDestroy(font_pattern);
  }

  [read values from |result_pattern|]

[behdad_google, 2014-12-19 22:18:31]

On 2014/12/19 22:02:11, Daniel Erat wrote:
> On 2014/12/19 21:37:19, behdad_google wrote:
> > On 2014/12/19 20:52:37, Daniel Erat wrote:
> > > On 2014/12/19 19:59:06, behdad_google wrote:
> > > > Wouldn't this reintroduce the original bug we were trying to fix with
the
> > > > no-match change?  Eg. if the default font enables bytecode hinting, we
> don't
> > > > want to use that setting for every font.  Or am I missing something?
> > > 
> > > i was mostly trying to fix this on chrome os initially. some background:
> > > 
> > > as discussed on http://crbug.com/420355 and http://crbug.com/421247,
> > > FontRenderParams was calling FcFontMatch() for empty queries (which are
used
> > to
> > > get the default rendering settings that blink uses for web fonts).
> Fontconfig
> > > matched Arimo in this case, which was using the BCI instead of the
> autohinter.
> > > avoiding the FcFontMatch() call meant that we could add <match
> > target="pattern">
> > > configs on chrome os that enabled autohinting. those settings would be
used
> > for
> > > web fonts, so the problem was avoided there.
> > > 
> > > unfortunately, i don't think that many linux users use <match
> > target="pattern">
> > > stanzas to set their defaults. as a result, r302898 meant that the
rendering
> > > settings used for web fonts could change suddenly, since their <match
> > > target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be ignored.
this
> is
> > > why i've been considering landing this change to at least go back to the
> > earlier
> > > behavior on linux.
> > 
> > Ok lets fix that properly then!
> > 
> > What you need is this: when you get your query pattern, make a copy and call
> it
> > font pattern, clear the FC_FAMILY in it (and if needed set one nonexisting
> > value), then call FcFontRenderPrepare (config, query, font);
> > 
> > That will cause the generic font config stanzas to be applied, but nothing
> > specific to a specific font.
> 
> that wouldn't fix http://crbug.com/435277, would it?

> i think that users who
> disable the autohinter by default will still have deformed fonts. is that just
a
> solution for the default-config-specifies-font-matches issue, and an
alternative
> to this patch?

Yes.

For "disable bci for webfonts", we control what flags get passed down, so we can
override whatever we want for webfonts.  The part of code that this CL touches
concerns with detecting default rendering settings from fontconfig.

> and i'm still a bit shaky on fontconfig usage. is something like the following
> what you're recommending?

More like:

  [initialize |query_pattern| based on |query|, a FontRenderParamsQuery...]

  FcConfigSubstitute(NULL, query_pattern, FcMatchPattern);
  FcDefaultSubstitute(query_pattern);

  FcResult result;

  FcPattern* result_pattern;
  if (query.is_empty()) {
    FcPattern* font_pattern = FcPatternDuplicate(query_pattern);
    if (!font_pattern) { 
      FcPatternDestroy(query_pattern);
      return false;
    }
    FcPatternRemove(font_pattern, FC_FAMILY, 0);
    result_pattern = FcFontRenderPrepare(NULL, query_pattern, font_pattern);
    FcPatternDestroy(font_pattern);
  }
  else
  {
    FcPattern* font_pattern = FcFontMatch(NULL, query_pattern, &result);
    if (!font_pattern) { 
      FcPatternDestroy(query_pattern);
      return false;
    }
    result_pattern = font_pattern;
  }

  [read values from |result_pattern|]

[Daniel Erat, 2014-12-19 22:26:01]

On 2014/12/19 22:18:31, behdad_google wrote:
> On 2014/12/19 22:02:11, Daniel Erat wrote:
> > On 2014/12/19 21:37:19, behdad_google wrote:
> > > On 2014/12/19 20:52:37, Daniel Erat wrote:
> > > > On 2014/12/19 19:59:06, behdad_google wrote:
> > > > > Wouldn't this reintroduce the original bug we were trying to fix with
> the
> > > > > no-match change?  Eg. if the default font enables bytecode hinting, we
> > don't
> > > > > want to use that setting for every font.  Or am I missing something?
> > > > 
> > > > i was mostly trying to fix this on chrome os initially. some background:
> > > > 
> > > > as discussed on http://crbug.com/420355 and http://crbug.com/421247,
> > > > FontRenderParams was calling FcFontMatch() for empty queries (which are
> used
> > > to
> > > > get the default rendering settings that blink uses for web fonts).
> > Fontconfig
> > > > matched Arimo in this case, which was using the BCI instead of the
> > autohinter.
> > > > avoiding the FcFontMatch() call meant that we could add <match
> > > target="pattern">
> > > > configs on chrome os that enabled autohinting. those settings would be
> used
> > > for
> > > > web fonts, so the problem was avoided there.
> > > > 
> > > > unfortunately, i don't think that many linux users use <match
> > > target="pattern">
> > > > stanzas to set their defaults. as a result, r302898 meant that the
> rendering
> > > > settings used for web fonts could change suddenly, since their <match
> > > > target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be ignored.
> this
> > is
> > > > why i've been considering landing this change to at least go back to the
> > > earlier
> > > > behavior on linux.
> > > 
> > > Ok lets fix that properly then!
> > > 
> > > What you need is this: when you get your query pattern, make a copy and
call
> > it
> > > font pattern, clear the FC_FAMILY in it (and if needed set one nonexisting
> > > value), then call FcFontRenderPrepare (config, query, font);
> > > 
> > > That will cause the generic font config stanzas to be applied, but nothing
> > > specific to a specific font.
> > 
> > that wouldn't fix http://crbug.com/435277, would it?
> 
> > i think that users who
> > disable the autohinter by default will still have deformed fonts. is that
just
> a
> > solution for the default-config-specifies-font-matches issue, and an
> alternative
> > to this patch?
> 
> Yes.
> 
> For "disable bci for webfonts", we control what flags get passed down, so we
can
> override whatever we want for webfonts.  The part of code that this CL touches
> concerns with detecting default rendering settings from fontconfig.
> 
> > and i'm still a bit shaky on fontconfig usage. is something like the
following
> > what you're recommending?
> 
> More like:
> 
>   [initialize |query_pattern| based on |query|, a FontRenderParamsQuery...]
> 
>   FcConfigSubstitute(NULL, query_pattern, FcMatchPattern);
>   FcDefaultSubstitute(query_pattern);
> 
>   FcResult result;
> 
>   FcPattern* result_pattern;
>   if (query.is_empty()) {
>     FcPattern* font_pattern = FcPatternDuplicate(query_pattern);
>     if (!font_pattern) { 
>       FcPatternDestroy(query_pattern);
>       return false;
>     }
>     FcPatternRemove(font_pattern, FC_FAMILY, 0);

this is the part that confuses me. when query.is_empty() is true, it means that
we didn't have any families to begin with. could the earlier
FcConfigSubstitute() or FcDefaultSubstitute() calls have added families to
|query_pattern|, or does only FcFontMatch() do that?

>     result_pattern = FcFontRenderPrepare(NULL, query_pattern, font_pattern);
>     FcPatternDestroy(font_pattern);
>   }
>   else
>   {
>     FcPattern* font_pattern = FcFontMatch(NULL, query_pattern, &result);
>     if (!font_pattern) { 
>       FcPatternDestroy(query_pattern);
>       return false;
>     }
>     result_pattern = font_pattern;
>   }
> 
>   [read values from |result_pattern|]

[behdad_google, 2014-12-19 22:29:26]

On 2014/12/19 22:26:01, Daniel Erat wrote:
> On 2014/12/19 22:18:31, behdad_google wrote:
> > On 2014/12/19 22:02:11, Daniel Erat wrote:
> > > On 2014/12/19 21:37:19, behdad_google wrote:
> > > > On 2014/12/19 20:52:37, Daniel Erat wrote:
> > > > > On 2014/12/19 19:59:06, behdad_google wrote:
> > > > > > Wouldn't this reintroduce the original bug we were trying to fix
with
> > the
> > > > > > no-match change?  Eg. if the default font enables bytecode hinting,
we
> > > don't
> > > > > > want to use that setting for every font.  Or am I missing something?
> > > > > 
> > > > > i was mostly trying to fix this on chrome os initially. some
background:
> > > > > 
> > > > > as discussed on http://crbug.com/420355 and http://crbug.com/421247,
> > > > > FontRenderParams was calling FcFontMatch() for empty queries (which
are
> > used
> > > > to
> > > > > get the default rendering settings that blink uses for web fonts).
> > > Fontconfig
> > > > > matched Arimo in this case, which was using the BCI instead of the
> > > autohinter.
> > > > > avoiding the FcFontMatch() call meant that we could add <match
> > > > target="pattern">
> > > > > configs on chrome os that enabled autohinting. those settings would be
> > used
> > > > for
> > > > > web fonts, so the problem was avoided there.
> > > > > 
> > > > > unfortunately, i don't think that many linux users use <match
> > > > target="pattern">
> > > > > stanzas to set their defaults. as a result, r302898 meant that the
> > rendering
> > > > > settings used for web fonts could change suddenly, since their <match
> > > > > target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be ignored.
> > this
> > > is
> > > > > why i've been considering landing this change to at least go back to
the
> > > > earlier
> > > > > behavior on linux.
> > > > 
> > > > Ok lets fix that properly then!
> > > > 
> > > > What you need is this: when you get your query pattern, make a copy and
> call
> > > it
> > > > font pattern, clear the FC_FAMILY in it (and if needed set one
nonexisting
> > > > value), then call FcFontRenderPrepare (config, query, font);
> > > > 
> > > > That will cause the generic font config stanzas to be applied, but
nothing
> > > > specific to a specific font.
> > > 
> > > that wouldn't fix http://crbug.com/435277, would it?
> > 
> > > i think that users who
> > > disable the autohinter by default will still have deformed fonts. is that
> just
> > a
> > > solution for the default-config-specifies-font-matches issue, and an
> > alternative
> > > to this patch?
> > 
> > Yes.
> > 
> > For "disable bci for webfonts", we control what flags get passed down, so we
> can
> > override whatever we want for webfonts.  The part of code that this CL
touches
> > concerns with detecting default rendering settings from fontconfig.
> > 
> > > and i'm still a bit shaky on fontconfig usage. is something like the
> following
> > > what you're recommending?
> > 
> > More like:
> > 
> >   [initialize |query_pattern| based on |query|, a FontRenderParamsQuery...]
> > 
> >   FcConfigSubstitute(NULL, query_pattern, FcMatchPattern);
> >   FcDefaultSubstitute(query_pattern);
> > 
> >   FcResult result;
> > 
> >   FcPattern* result_pattern;
> >   if (query.is_empty()) {
> >     FcPattern* font_pattern = FcPatternDuplicate(query_pattern);
> >     if (!font_pattern) { 
> >       FcPatternDestroy(query_pattern);
> >       return false;
> >     }
> >     FcPatternRemove(font_pattern, FC_FAMILY, 0);
> 
> this is the part that confuses me. when query.is_empty() is true, it means
that
> we didn't have any families to begin with. could the earlier
> FcConfigSubstitute() or FcDefaultSubstitute() calls have added families to
> |query_pattern|,

Yes, that's how "default" fallback list gets added.  Try:

$ fc-pattern --config


> or does only FcFontMatch() do that?
> 
> >     result_pattern = FcFontRenderPrepare(NULL, query_pattern, font_pattern);
> >     FcPatternDestroy(font_pattern);
> >   }
> >   else
> >   {
> >     FcPattern* font_pattern = FcFontMatch(NULL, query_pattern, &result);
> >     if (!font_pattern) { 
> >       FcPatternDestroy(query_pattern);
> >       return false;
> >     }
> >     result_pattern = font_pattern;
> >   }
> > 
> >   [read values from |result_pattern|]

[Daniel Erat, 2014-12-19 23:12:20]

On 2014/12/19 22:29:26, behdad_google wrote:
> On 2014/12/19 22:26:01, Daniel Erat wrote:
> > On 2014/12/19 22:18:31, behdad_google wrote:
> > > On 2014/12/19 22:02:11, Daniel Erat wrote:
> > > > On 2014/12/19 21:37:19, behdad_google wrote:
> > > > > On 2014/12/19 20:52:37, Daniel Erat wrote:
> > > > > > On 2014/12/19 19:59:06, behdad_google wrote:
> > > > > > > Wouldn't this reintroduce the original bug we were trying to fix
> with
> > > the
> > > > > > > no-match change?  Eg. if the default font enables bytecode
hinting,
> we
> > > > don't
> > > > > > > want to use that setting for every font.  Or am I missing
something?
> > > > > > 
> > > > > > i was mostly trying to fix this on chrome os initially. some
> background:
> > > > > > 
> > > > > > as discussed on http://crbug.com/420355 and http://crbug.com/421247,
> > > > > > FontRenderParams was calling FcFontMatch() for empty queries (which
> are
> > > used
> > > > > to
> > > > > > get the default rendering settings that blink uses for web fonts).
> > > > Fontconfig
> > > > > > matched Arimo in this case, which was using the BCI instead of the
> > > > autohinter.
> > > > > > avoiding the FcFontMatch() call meant that we could add <match
> > > > > target="pattern">
> > > > > > configs on chrome os that enabled autohinting. those settings would
be
> > > used
> > > > > for
> > > > > > web fonts, so the problem was avoided there.
> > > > > > 
> > > > > > unfortunately, i don't think that many linux users use <match
> > > > > target="pattern">
> > > > > > stanzas to set their defaults. as a result, r302898 meant that the
> > > rendering
> > > > > > settings used for web fonts could change suddenly, since their
<match
> > > > > > target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be
ignored.
> > > this
> > > > is
> > > > > > why i've been considering landing this change to at least go back to
> the
> > > > > earlier
> > > > > > behavior on linux.
> > > > > 
> > > > > Ok lets fix that properly then!
> > > > > 
> > > > > What you need is this: when you get your query pattern, make a copy
and
> > call
> > > > it
> > > > > font pattern, clear the FC_FAMILY in it (and if needed set one
> nonexisting
> > > > > value), then call FcFontRenderPrepare (config, query, font);
> > > > > 
> > > > > That will cause the generic font config stanzas to be applied, but
> nothing
> > > > > specific to a specific font.
> > > > 
> > > > that wouldn't fix http://crbug.com/435277, would it?
> > > 
> > > > i think that users who
> > > > disable the autohinter by default will still have deformed fonts. is
that
> > just
> > > a
> > > > solution for the default-config-specifies-font-matches issue, and an
> > > alternative
> > > > to this patch?
> > > 
> > > Yes.
> > > 
> > > For "disable bci for webfonts", we control what flags get passed down, so
we
> > can
> > > override whatever we want for webfonts.  The part of code that this CL
> touches
> > > concerns with detecting default rendering settings from fontconfig.
> > > 
> > > > and i'm still a bit shaky on fontconfig usage. is something like the
> > following
> > > > what you're recommending?
> > > 
> > > More like:
> > > 
> > >   [initialize |query_pattern| based on |query|, a
FontRenderParamsQuery...]
> > > 
> > >   FcConfigSubstitute(NULL, query_pattern, FcMatchPattern);
> > >   FcDefaultSubstitute(query_pattern);
> > > 
> > >   FcResult result;
> > > 
> > >   FcPattern* result_pattern;
> > >   if (query.is_empty()) {
> > >     FcPattern* font_pattern = FcPatternDuplicate(query_pattern);
> > >     if (!font_pattern) { 
> > >       FcPatternDestroy(query_pattern);
> > >       return false;
> > >     }
> > >     FcPatternRemove(font_pattern, FC_FAMILY, 0);
> > 
> > this is the part that confuses me. when query.is_empty() is true, it means
> that
> > we didn't have any families to begin with. could the earlier
> > FcConfigSubstitute() or FcDefaultSubstitute() calls have added families to
> > |query_pattern|,
> 
> Yes, that's how "default" fallback list gets added.  Try:
> 
> $ fc-pattern --config
> 
> 
> > or does only FcFontMatch() do that?
> > 
> > >     result_pattern = FcFontRenderPrepare(NULL, query_pattern,
font_pattern);
> > >     FcPatternDestroy(font_pattern);
> > >   }
> > >   else
> > >   {
> > >     FcPattern* font_pattern = FcFontMatch(NULL, query_pattern, &result);
> > >     if (!font_pattern) { 
> > >       FcPatternDestroy(query_pattern);
> > >       return false;
> > >     }
> > >     result_pattern = font_pattern;
> > >   }
> > > 
> > >   [read values from |result_pattern|]

thanks for the explanation. i'm not sure that this is working how i'd expect,
though. using https://github.com/derat/font-config-info as a testbed, i'm doing
the following:

  printf("Fontconfig (default pattern):\n");
  FcPattern* query = FcPatternCreate();
  assert(query);
  FcConfigSubstitute(NULL, query, FcMatchPattern);
  FcDefaultSubstitute(query);
  PrintFontconfigPattern(query, 0);

  printf("Fontconfig (default match):\n");
  FcResult result;
  FcPattern* match = FcFontMatch(NULL, query, &result);
  assert(match);
  PrintFontconfigPattern(match, 1);

  printf("Fontconfig (non-family defaults):\n");
  FcPattern* query_without_family = FcPatternDuplicate(query);
  FcPatternRemove(query_without_family, FC_FAMILY, 0);
  FcPattern* defaults = FcFontRenderPrepare(NULL, query, query_without_family);
  PrintFontconfigPattern(defaults, 0);

my ~/.fonts.conf contains:

  <match target="font">
    <edit name="antialias" mode="assign"><bool>true</bool></edit>
    <edit name="autohint" mode="assign"><bool>false</bool></edit>
    <edit name="hinting" mode="assign"><bool>true</bool></edit>
    <edit name="hintstyle" mode="assign"><const>hintfull</const></edit>
    <edit name="rgba" mode="assign"><const>none</const></edit>
  </match>
  <match target="font">
    <test name="family"><string>DejaVu Sans</string></test>
    <edit name="antialias"><bool>false</bool></edit>
  </match>

i'd expect the "non-family defaults" section to enable autohinting, since i'm
only disabling it in the DejaVu Sans-specific stanza, but instead:

Fontconfig (default pattern):
antialias            [no match]
hinting              1
autohint             0
embeddedbitmap       1
hintstyle            3 (full)
rgba                 [no match]

Fontconfig (default match):
family               DejaVu Sans
pixelsize            12.50 pixels
size                 12 points
antialias            0
hinting              1
autohint             0
embeddedbitmap       1
hintstyle            3 (full)
rgba                 5 (none)

Fontconfig (non-family defaults):
antialias            0
hinting              1
autohint             0
embeddedbitmap       1
hintstyle            3 (full)
rgba                 5 (none)

so obviously i still don't understand how this is supposed to work. :-)

[Daniel Erat, 2014-12-19 23:13:09]

On 2014/12/19 23:12:20, Daniel Erat wrote:
> On 2014/12/19 22:29:26, behdad_google wrote:
> > On 2014/12/19 22:26:01, Daniel Erat wrote:
> > > On 2014/12/19 22:18:31, behdad_google wrote:
> > > > On 2014/12/19 22:02:11, Daniel Erat wrote:
> > > > > On 2014/12/19 21:37:19, behdad_google wrote:
> > > > > > On 2014/12/19 20:52:37, Daniel Erat wrote:
> > > > > > > On 2014/12/19 19:59:06, behdad_google wrote:
> > > > > > > > Wouldn't this reintroduce the original bug we were trying to fix
> > with
> > > > the
> > > > > > > > no-match change?  Eg. if the default font enables bytecode
> hinting,
> > we
> > > > > don't
> > > > > > > > want to use that setting for every font.  Or am I missing
> something?
> > > > > > > 
> > > > > > > i was mostly trying to fix this on chrome os initially. some
> > background:
> > > > > > > 
> > > > > > > as discussed on http://crbug.com/420355 and
http://crbug.com/421247,
> > > > > > > FontRenderParams was calling FcFontMatch() for empty queries
(which
> > are
> > > > used
> > > > > > to
> > > > > > > get the default rendering settings that blink uses for web fonts).
> > > > > Fontconfig
> > > > > > > matched Arimo in this case, which was using the BCI instead of the
> > > > > autohinter.
> > > > > > > avoiding the FcFontMatch() call meant that we could add <match
> > > > > > target="pattern">
> > > > > > > configs on chrome os that enabled autohinting. those settings
would
> be
> > > > used
> > > > > > for
> > > > > > > web fonts, so the problem was avoided there.
> > > > > > > 
> > > > > > > unfortunately, i don't think that many linux users use <match
> > > > > > target="pattern">
> > > > > > > stanzas to set their defaults. as a result, r302898 meant that the
> > > > rendering
> > > > > > > settings used for web fonts could change suddenly, since their
> <match
> > > > > > > target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be
> ignored.
> > > > this
> > > > > is
> > > > > > > why i've been considering landing this change to at least go back
to
> > the
> > > > > > earlier
> > > > > > > behavior on linux.
> > > > > > 
> > > > > > Ok lets fix that properly then!
> > > > > > 
> > > > > > What you need is this: when you get your query pattern, make a copy
> and
> > > call
> > > > > it
> > > > > > font pattern, clear the FC_FAMILY in it (and if needed set one
> > nonexisting
> > > > > > value), then call FcFontRenderPrepare (config, query, font);
> > > > > > 
> > > > > > That will cause the generic font config stanzas to be applied, but
> > nothing
> > > > > > specific to a specific font.
> > > > > 
> > > > > that wouldn't fix http://crbug.com/435277, would it?
> > > > 
> > > > > i think that users who
> > > > > disable the autohinter by default will still have deformed fonts. is
> that
> > > just
> > > > a
> > > > > solution for the default-config-specifies-font-matches issue, and an
> > > > alternative
> > > > > to this patch?
> > > > 
> > > > Yes.
> > > > 
> > > > For "disable bci for webfonts", we control what flags get passed down,
so
> we
> > > can
> > > > override whatever we want for webfonts.  The part of code that this CL
> > touches
> > > > concerns with detecting default rendering settings from fontconfig.
> > > > 
> > > > > and i'm still a bit shaky on fontconfig usage. is something like the
> > > following
> > > > > what you're recommending?
> > > > 
> > > > More like:
> > > > 
> > > >   [initialize |query_pattern| based on |query|, a
> FontRenderParamsQuery...]
> > > > 
> > > >   FcConfigSubstitute(NULL, query_pattern, FcMatchPattern);
> > > >   FcDefaultSubstitute(query_pattern);
> > > > 
> > > >   FcResult result;
> > > > 
> > > >   FcPattern* result_pattern;
> > > >   if (query.is_empty()) {
> > > >     FcPattern* font_pattern = FcPatternDuplicate(query_pattern);
> > > >     if (!font_pattern) { 
> > > >       FcPatternDestroy(query_pattern);
> > > >       return false;
> > > >     }
> > > >     FcPatternRemove(font_pattern, FC_FAMILY, 0);
> > > 
> > > this is the part that confuses me. when query.is_empty() is true, it means
> > that
> > > we didn't have any families to begin with. could the earlier
> > > FcConfigSubstitute() or FcDefaultSubstitute() calls have added families to
> > > |query_pattern|,
> > 
> > Yes, that's how "default" fallback list gets added.  Try:
> > 
> > $ fc-pattern --config
> > 
> > 
> > > or does only FcFontMatch() do that?
> > > 
> > > >     result_pattern = FcFontRenderPrepare(NULL, query_pattern,
> font_pattern);
> > > >     FcPatternDestroy(font_pattern);
> > > >   }
> > > >   else
> > > >   {
> > > >     FcPattern* font_pattern = FcFontMatch(NULL, query_pattern, &result);
> > > >     if (!font_pattern) { 
> > > >       FcPatternDestroy(query_pattern);
> > > >       return false;
> > > >     }
> > > >     result_pattern = font_pattern;
> > > >   }
> > > > 
> > > >   [read values from |result_pattern|]
> 
> thanks for the explanation. i'm not sure that this is working how i'd expect,
> though. using https://github.com/derat/font-config-info as a testbed, i'm
doing
> the following:
> 
>   printf("Fontconfig (default pattern):\n");
>   FcPattern* query = FcPatternCreate();
>   assert(query);
>   FcConfigSubstitute(NULL, query, FcMatchPattern);
>   FcDefaultSubstitute(query);
>   PrintFontconfigPattern(query, 0);
> 
>   printf("Fontconfig (default match):\n");
>   FcResult result;
>   FcPattern* match = FcFontMatch(NULL, query, &result);
>   assert(match);
>   PrintFontconfigPattern(match, 1);
> 
>   printf("Fontconfig (non-family defaults):\n");
>   FcPattern* query_without_family = FcPatternDuplicate(query);
>   FcPatternRemove(query_without_family, FC_FAMILY, 0);
>   FcPattern* defaults = FcFontRenderPrepare(NULL, query,
query_without_family);
>   PrintFontconfigPattern(defaults, 0);
> 
> my ~/.fonts.conf contains:
> 
>   <match target="font">
>     <edit name="antialias" mode="assign"><bool>true</bool></edit>
>     <edit name="autohint" mode="assign"><bool>false</bool></edit>
>     <edit name="hinting" mode="assign"><bool>true</bool></edit>
>     <edit name="hintstyle" mode="assign"><const>hintfull</const></edit>
>     <edit name="rgba" mode="assign"><const>none</const></edit>
>   </match>
>   <match target="font">
>     <test name="family"><string>DejaVu Sans</string></test>
>     <edit name="antialias"><bool>false</bool></edit>
>   </match>
> 
> i'd expect the "non-family defaults" section to enable autohinting, since i'm
> only disabling it in the DejaVu Sans-specific stanza, but instead:
> 
> Fontconfig (default pattern):
> antialias            [no match]
> hinting              1
> autohint             0
> embeddedbitmap       1
> hintstyle            3 (full)
> rgba                 [no match]
> 
> Fontconfig (default match):
> family               DejaVu Sans
> pixelsize            12.50 pixels
> size                 12 points
> antialias            0
> hinting              1
> autohint             0
> embeddedbitmap       1
> hintstyle            3 (full)
> rgba                 5 (none)
> 
> Fontconfig (non-family defaults):
> antialias            0
> hinting              1
> autohint             0
> embeddedbitmap       1
> hintstyle            3 (full)
> rgba                 5 (none)
> 
> so obviously i still don't understand how this is supposed to work. :-)

sorry, big typo there: i meant "i'd expect the 'non-family defaults' section to
enable *antialiasing*".

[behdad_google, 2014-12-20 07:10:18]

On 2014/12/19 23:13:09, Daniel Erat wrote:
> On 2014/12/19 23:12:20, Daniel Erat wrote:
> > On 2014/12/19 22:29:26, behdad_google wrote:
> > > On 2014/12/19 22:26:01, Daniel Erat wrote:
> > > > On 2014/12/19 22:18:31, behdad_google wrote:
> > > > > On 2014/12/19 22:02:11, Daniel Erat wrote:
> > > > > > On 2014/12/19 21:37:19, behdad_google wrote:
> > > > > > > On 2014/12/19 20:52:37, Daniel Erat wrote:
> > > > > > > > On 2014/12/19 19:59:06, behdad_google wrote:
> > > > > > > > > Wouldn't this reintroduce the original bug we were trying to
fix
> > > with
> > > > > the
> > > > > > > > > no-match change?  Eg. if the default font enables bytecode
> > hinting,
> > > we
> > > > > > don't
> > > > > > > > > want to use that setting for every font.  Or am I missing
> > something?
> > > > > > > > 
> > > > > > > > i was mostly trying to fix this on chrome os initially. some
> > > background:
> > > > > > > > 
> > > > > > > > as discussed on http://crbug.com/420355 and
> http://crbug.com/421247,
> > > > > > > > FontRenderParams was calling FcFontMatch() for empty queries
> (which
> > > are
> > > > > used
> > > > > > > to
> > > > > > > > get the default rendering settings that blink uses for web
fonts).
> > > > > > Fontconfig
> > > > > > > > matched Arimo in this case, which was using the BCI instead of
the
> > > > > > autohinter.
> > > > > > > > avoiding the FcFontMatch() call meant that we could add <match
> > > > > > > target="pattern">
> > > > > > > > configs on chrome os that enabled autohinting. those settings
> would
> > be
> > > > > used
> > > > > > > for
> > > > > > > > web fonts, so the problem was avoided there.
> > > > > > > > 
> > > > > > > > unfortunately, i don't think that many linux users use <match
> > > > > > > target="pattern">
> > > > > > > > stanzas to set their defaults. as a result, r302898 meant that
the
> > > > > rendering
> > > > > > > > settings used for web fonts could change suddenly, since their
> > <match
> > > > > > > > target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be
> > ignored.
> > > > > this
> > > > > > is
> > > > > > > > why i've been considering landing this change to at least go
back
> to
> > > the
> > > > > > > earlier
> > > > > > > > behavior on linux.
> > > > > > > 
> > > > > > > Ok lets fix that properly then!
> > > > > > > 
> > > > > > > What you need is this: when you get your query pattern, make a
copy
> > and
> > > > call
> > > > > > it
> > > > > > > font pattern, clear the FC_FAMILY in it (and if needed set one
> > > nonexisting
> > > > > > > value), then call FcFontRenderPrepare (config, query, font);
> > > > > > > 
> > > > > > > That will cause the generic font config stanzas to be applied, but
> > > nothing
> > > > > > > specific to a specific font.
> > > > > > 
> > > > > > that wouldn't fix http://crbug.com/435277, would it?
> > > > > 
> > > > > > i think that users who
> > > > > > disable the autohinter by default will still have deformed fonts. is
> > that
> > > > just
> > > > > a
> > > > > > solution for the default-config-specifies-font-matches issue, and an
> > > > > alternative
> > > > > > to this patch?
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > For "disable bci for webfonts", we control what flags get passed down,
> so
> > we
> > > > can
> > > > > override whatever we want for webfonts.  The part of code that this CL
> > > touches
> > > > > concerns with detecting default rendering settings from fontconfig.
> > > > > 
> > > > > > and i'm still a bit shaky on fontconfig usage. is something like the
> > > > following
> > > > > > what you're recommending?
> > > > > 
> > > > > More like:
> > > > > 
> > > > >   [initialize |query_pattern| based on |query|, a
> > FontRenderParamsQuery...]
> > > > > 
> > > > >   FcConfigSubstitute(NULL, query_pattern, FcMatchPattern);
> > > > >   FcDefaultSubstitute(query_pattern);
> > > > > 
> > > > >   FcResult result;
> > > > > 
> > > > >   FcPattern* result_pattern;
> > > > >   if (query.is_empty()) {
> > > > >     FcPattern* font_pattern = FcPatternDuplicate(query_pattern);
> > > > >     if (!font_pattern) { 
> > > > >       FcPatternDestroy(query_pattern);
> > > > >       return false;
> > > > >     }
> > > > >     FcPatternRemove(font_pattern, FC_FAMILY, 0);
> > > > 
> > > > this is the part that confuses me. when query.is_empty() is true, it
means
> > > that
> > > > we didn't have any families to begin with. could the earlier
> > > > FcConfigSubstitute() or FcDefaultSubstitute() calls have added families
to
> > > > |query_pattern|,
> > > 
> > > Yes, that's how "default" fallback list gets added.  Try:
> > > 
> > > $ fc-pattern --config
> > > 
> > > 
> > > > or does only FcFontMatch() do that?
> > > > 
> > > > >     result_pattern = FcFontRenderPrepare(NULL, query_pattern,
> > font_pattern);
> > > > >     FcPatternDestroy(font_pattern);
> > > > >   }
> > > > >   else
> > > > >   {
> > > > >     FcPattern* font_pattern = FcFontMatch(NULL, query_pattern,
&result);
> > > > >     if (!font_pattern) { 
> > > > >       FcPatternDestroy(query_pattern);
> > > > >       return false;
> > > > >     }
> > > > >     result_pattern = font_pattern;
> > > > >   }
> > > > > 
> > > > >   [read values from |result_pattern|]
> > 
> > thanks for the explanation. i'm not sure that this is working how i'd
expect,
> > though. using https://github.com/derat/font-config-info as a testbed, i'm
> doing
> > the following:
> > 
> >   printf("Fontconfig (default pattern):\n");
> >   FcPattern* query = FcPatternCreate();
> >   assert(query);
> >   FcConfigSubstitute(NULL, query, FcMatchPattern);
> >   FcDefaultSubstitute(query);
> >   PrintFontconfigPattern(query, 0);
> > 
> >   printf("Fontconfig (default match):\n");
> >   FcResult result;
> >   FcPattern* match = FcFontMatch(NULL, query, &result);
> >   assert(match);
> >   PrintFontconfigPattern(match, 1);
> > 
> >   printf("Fontconfig (non-family defaults):\n");
> >   FcPattern* query_without_family = FcPatternDuplicate(query);
> >   FcPatternRemove(query_without_family, FC_FAMILY, 0);
> >   FcPattern* defaults = FcFontRenderPrepare(NULL, query,
> query_without_family);
> >   PrintFontconfigPattern(defaults, 0);
> > 
> > my ~/.fonts.conf contains:
> > 
> >   <match target="font">
> >     <edit name="antialias" mode="assign"><bool>true</bool></edit>
> >     <edit name="autohint" mode="assign"><bool>false</bool></edit>
> >     <edit name="hinting" mode="assign"><bool>true</bool></edit>
> >     <edit name="hintstyle" mode="assign"><const>hintfull</const></edit>
> >     <edit name="rgba" mode="assign"><const>none</const></edit>
> >   </match>
> >   <match target="font">
> >     <test name="family"><string>DejaVu Sans</string></test>
> >     <edit name="antialias"><bool>false</bool></edit>
> >   </match>
> > 
> > i'd expect the "non-family defaults" section to enable autohinting, since
i'm
> > only disabling it in the DejaVu Sans-specific stanza, but instead:
> > 
> > Fontconfig (default pattern):
> > antialias            [no match]
> > hinting              1
> > autohint             0
> > embeddedbitmap       1
> > hintstyle            3 (full)
> > rgba                 [no match]
> > 
> > Fontconfig (default match):
> > family               DejaVu Sans
> > pixelsize            12.50 pixels
> > size                 12 points
> > antialias            0
> > hinting              1
> > autohint             0
> > embeddedbitmap       1
> > hintstyle            3 (full)
> > rgba                 5 (none)
> > 
> > Fontconfig (non-family defaults):
> > antialias            0
> > hinting              1
> > autohint             0
> > embeddedbitmap       1
> > hintstyle            3 (full)
> > rgba                 5 (none)
> > 
> > so obviously i still don't understand how this is supposed to work. :-)
> 
> sorry, big typo there: i meant "i'd expect the 'non-family defaults' section
to
> enable *antialiasing*".

What if you add a nonexisting family to the font_pattern, something like "."?

My guess is that when font_pattern doesn't have FC_FAMILY, then RenderPrepare
will copy the FC_FAMILY from query pattern.

Actually, possibly a better approach is to call FcConfigSubstituteWithPat
(config, font_pattern, query_pattern, FcMatchFont); instead of
FcFontRenderPrepare.  Yeah, try that instead please.

[Daniel Erat, 2014-12-20 14:24:13]

On 2014/12/20 07:10:18, behdad_google wrote:
> On 2014/12/19 23:13:09, Daniel Erat wrote:
> > On 2014/12/19 23:12:20, Daniel Erat wrote:
> > > On 2014/12/19 22:29:26, behdad_google wrote:
> > > > On 2014/12/19 22:26:01, Daniel Erat wrote:
> > > > > On 2014/12/19 22:18:31, behdad_google wrote:
> > > > > > On 2014/12/19 22:02:11, Daniel Erat wrote:
> > > > > > > On 2014/12/19 21:37:19, behdad_google wrote:
> > > > > > > > On 2014/12/19 20:52:37, Daniel Erat wrote:
> > > > > > > > > On 2014/12/19 19:59:06, behdad_google wrote:
> > > > > > > > > > Wouldn't this reintroduce the original bug we were trying to
> fix
> > > > with
> > > > > > the
> > > > > > > > > > no-match change?  Eg. if the default font enables bytecode
> > > hinting,
> > > > we
> > > > > > > don't
> > > > > > > > > > want to use that setting for every font.  Or am I missing
> > > something?
> > > > > > > > > 
> > > > > > > > > i was mostly trying to fix this on chrome os initially. some
> > > > background:
> > > > > > > > > 
> > > > > > > > > as discussed on http://crbug.com/420355 and
> > http://crbug.com/421247,
> > > > > > > > > FontRenderParams was calling FcFontMatch() for empty queries
> > (which
> > > > are
> > > > > > used
> > > > > > > > to
> > > > > > > > > get the default rendering settings that blink uses for web
> fonts).
> > > > > > > Fontconfig
> > > > > > > > > matched Arimo in this case, which was using the BCI instead of
> the
> > > > > > > autohinter.
> > > > > > > > > avoiding the FcFontMatch() call meant that we could add <match
> > > > > > > > target="pattern">
> > > > > > > > > configs on chrome os that enabled autohinting. those settings
> > would
> > > be
> > > > > > used
> > > > > > > > for
> > > > > > > > > web fonts, so the problem was avoided there.
> > > > > > > > > 
> > > > > > > > > unfortunately, i don't think that many linux users use <match
> > > > > > > > target="pattern">
> > > > > > > > > stanzas to set their defaults. as a result, r302898 meant that
> the
> > > > > > rendering
> > > > > > > > > settings used for web fonts could change suddenly, since their
> > > <match
> > > > > > > > > target="font"> stanzas (e.g. from /etc/fonts/conf.d) would be
> > > ignored.
> > > > > > this
> > > > > > > is
> > > > > > > > > why i've been considering landing this change to at least go
> back
> > to
> > > > the
> > > > > > > > earlier
> > > > > > > > > behavior on linux.
> > > > > > > > 
> > > > > > > > Ok lets fix that properly then!
> > > > > > > > 
> > > > > > > > What you need is this: when you get your query pattern, make a
> copy
> > > and
> > > > > call
> > > > > > > it
> > > > > > > > font pattern, clear the FC_FAMILY in it (and if needed set one
> > > > nonexisting
> > > > > > > > value), then call FcFontRenderPrepare (config, query, font);
> > > > > > > > 
> > > > > > > > That will cause the generic font config stanzas to be applied,
but
> > > > nothing
> > > > > > > > specific to a specific font.
> > > > > > > 
> > > > > > > that wouldn't fix http://crbug.com/435277, would it?
> > > > > > 
> > > > > > > i think that users who
> > > > > > > disable the autohinter by default will still have deformed fonts.
is
> > > that
> > > > > just
> > > > > > a
> > > > > > > solution for the default-config-specifies-font-matches issue, and
an
> > > > > > alternative
> > > > > > > to this patch?
> > > > > > 
> > > > > > Yes.
> > > > > > 
> > > > > > For "disable bci for webfonts", we control what flags get passed
down,
> > so
> > > we
> > > > > can
> > > > > > override whatever we want for webfonts.  The part of code that this
CL
> > > > touches
> > > > > > concerns with detecting default rendering settings from fontconfig.
> > > > > > 
> > > > > > > and i'm still a bit shaky on fontconfig usage. is something like
the
> > > > > following
> > > > > > > what you're recommending?
> > > > > > 
> > > > > > More like:
> > > > > > 
> > > > > >   [initialize |query_pattern| based on |query|, a
> > > FontRenderParamsQuery...]
> > > > > > 
> > > > > >   FcConfigSubstitute(NULL, query_pattern, FcMatchPattern);
> > > > > >   FcDefaultSubstitute(query_pattern);
> > > > > > 
> > > > > >   FcResult result;
> > > > > > 
> > > > > >   FcPattern* result_pattern;
> > > > > >   if (query.is_empty()) {
> > > > > >     FcPattern* font_pattern = FcPatternDuplicate(query_pattern);
> > > > > >     if (!font_pattern) { 
> > > > > >       FcPatternDestroy(query_pattern);
> > > > > >       return false;
> > > > > >     }
> > > > > >     FcPatternRemove(font_pattern, FC_FAMILY, 0);
> > > > > 
> > > > > this is the part that confuses me. when query.is_empty() is true, it
> means
> > > > that
> > > > > we didn't have any families to begin with. could the earlier
> > > > > FcConfigSubstitute() or FcDefaultSubstitute() calls have added
families
> to
> > > > > |query_pattern|,
> > > > 
> > > > Yes, that's how "default" fallback list gets added.  Try:
> > > > 
> > > > $ fc-pattern --config
> > > > 
> > > > 
> > > > > or does only FcFontMatch() do that?
> > > > > 
> > > > > >     result_pattern = FcFontRenderPrepare(NULL, query_pattern,
> > > font_pattern);
> > > > > >     FcPatternDestroy(font_pattern);
> > > > > >   }
> > > > > >   else
> > > > > >   {
> > > > > >     FcPattern* font_pattern = FcFontMatch(NULL, query_pattern,
> &result);
> > > > > >     if (!font_pattern) { 
> > > > > >       FcPatternDestroy(query_pattern);
> > > > > >       return false;
> > > > > >     }
> > > > > >     result_pattern = font_pattern;
> > > > > >   }
> > > > > > 
> > > > > >   [read values from |result_pattern|]
> > > 
> > > thanks for the explanation. i'm not sure that this is working how i'd
> expect,
> > > though. using https://github.com/derat/font-config-info as a testbed, i'm
> > doing
> > > the following:
> > > 
> > >   printf("Fontconfig (default pattern):\n");
> > >   FcPattern* query = FcPatternCreate();
> > >   assert(query);
> > >   FcConfigSubstitute(NULL, query, FcMatchPattern);
> > >   FcDefaultSubstitute(query);
> > >   PrintFontconfigPattern(query, 0);
> > > 
> > >   printf("Fontconfig (default match):\n");
> > >   FcResult result;
> > >   FcPattern* match = FcFontMatch(NULL, query, &result);
> > >   assert(match);
> > >   PrintFontconfigPattern(match, 1);
> > > 
> > >   printf("Fontconfig (non-family defaults):\n");
> > >   FcPattern* query_without_family = FcPatternDuplicate(query);
> > >   FcPatternRemove(query_without_family, FC_FAMILY, 0);
> > >   FcPattern* defaults = FcFontRenderPrepare(NULL, query,
> > query_without_family);
> > >   PrintFontconfigPattern(defaults, 0);
> > > 
> > > my ~/.fonts.conf contains:
> > > 
> > >   <match target="font">
> > >     <edit name="antialias" mode="assign"><bool>true</bool></edit>
> > >     <edit name="autohint" mode="assign"><bool>false</bool></edit>
> > >     <edit name="hinting" mode="assign"><bool>true</bool></edit>
> > >     <edit name="hintstyle" mode="assign"><const>hintfull</const></edit>
> > >     <edit name="rgba" mode="assign"><const>none</const></edit>
> > >   </match>
> > >   <match target="font">
> > >     <test name="family"><string>DejaVu Sans</string></test>
> > >     <edit name="antialias"><bool>false</bool></edit>
> > >   </match>
> > > 
> > > i'd expect the "non-family defaults" section to enable autohinting, since
> i'm
> > > only disabling it in the DejaVu Sans-specific stanza, but instead:
> > > 
> > > Fontconfig (default pattern):
> > > antialias            [no match]
> > > hinting              1
> > > autohint             0
> > > embeddedbitmap       1
> > > hintstyle            3 (full)
> > > rgba                 [no match]
> > > 
> > > Fontconfig (default match):
> > > family               DejaVu Sans
> > > pixelsize            12.50 pixels
> > > size                 12 points
> > > antialias            0
> > > hinting              1
> > > autohint             0
> > > embeddedbitmap       1
> > > hintstyle            3 (full)
> > > rgba                 5 (none)
> > > 
> > > Fontconfig (non-family defaults):
> > > antialias            0
> > > hinting              1
> > > autohint             0
> > > embeddedbitmap       1
> > > hintstyle            3 (full)
> > > rgba                 5 (none)
> > > 
> > > so obviously i still don't understand how this is supposed to work. :-)
> > 
> > sorry, big typo there: i meant "i'd expect the 'non-family defaults' section
> to
> > enable *antialiasing*".
> 
> What if you add a nonexisting family to the font_pattern, something like "."?
> 
> My guess is that when font_pattern doesn't have FC_FAMILY, then RenderPrepare
> will copy the FC_FAMILY from query pattern.
> 
> Actually, possibly a better approach is to call FcConfigSubstituteWithPat
> (config, font_pattern, query_pattern, FcMatchFont); instead of
> FcFontRenderPrepare.  Yeah, try that instead please.

that looks like it gets the default-family-specific target="font" configs
instead of the non-family-specific ones. with the following:

  printf("Fontconfig (non-family defaults):\n");
  FcPattern* query_without_family = FcPatternDuplicate(query);
  FcPatternRemove(query_without_family, FC_FAMILY, 0);
  FcConfigSubstituteWithPat(NULL, query_without_family, query, FcMatchFont);
  PrintFontconfigPattern(query_without_family, 0);

and the config from above (target="font" with no tests and antialias=true,
target="font" with DejaVu Sans test and antialias=false, i see:

Fontconfig (non-family defaults):
antialias            0
hinting              1
autohint             0
embeddedbitmap       1
hintstyle            3 (full)
rgba                 5 (none)

i.e. the family-specific antialias=false setting appears to be used.

i get the same results when i use FcFontRenderPrepare() after adding an invalid
family name.

[behdad_google, 2014-12-20 19:20:31]

Can you attach a full test program please?

Also, which version of fontconfig?

[Daniel Erat, 2014-12-21 13:58:26]

On 2014/12/20 19:20:31, behdad_google wrote:
> Can you attach a full test program please?
> 
> Also, which version of fontconfig?

sure, i linked to it above: https://github.com/derat/font-config-info

trusty looks like it's at 2.11.0.

[behdad_google, 2014-12-21 19:23:58]

On 2014/12/21 13:58:26, Daniel Erat wrote:
> On 2014/12/20 19:20:31, behdad_google wrote:
> > Can you attach a full test program please?
> > 
> > Also, which version of fontconfig?
> 
> sure, i linked to it above: https://github.com/derat/font-config-info
> 
> trusty looks like it's at 2.11.0.

Thanks.  Didn't occur to me that you are committing these to font-config-info as
we speak.

I can reproduce your observation.  Let me debug and see what's going on.

[behdad_google, 2014-12-21 19:41:52]

On 2014/12/21 19:23:58, behdad_google wrote:
> On 2014/12/21 13:58:26, Daniel Erat wrote:
> > On 2014/12/20 19:20:31, behdad_google wrote:
> > > Can you attach a full test program please?
> > > 
> > > Also, which version of fontconfig?
> > 
> > sure, i linked to it above: https://github.com/derat/font-config-info
> > 
> > trusty looks like it's at 2.11.0.
> 
> Thanks.  Didn't occur to me that you are committing these to font-config-info
as
> we speak.
> 
> I can reproduce your observation.  Let me debug and see what's going on.

Your main problem was using FcPatternRemove when you really needed FcPatternDel.
 Fixing that both approach work.

Sent you a pull-request.

FcPatternPrint is your friend.  It took me two minutes to see what's wrong using
that function.

[Daniel Erat, 2014-12-22 18:13:28]

thanks again for the help, behdad! mind reviewing the latest version of this
change?

[behdad_google, 2014-12-23 04:09:24]

lgtm

[Daniel Erat, 2014-12-23 15:00:09]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-12-23 15:00:27]

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

[commit-bot: I haz the power, 2014-12-23 18:18:37]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-12-23 18:21:11]

Patchset 7 (id:??) landed as
https://crrev.com/2ff93446c3405557b6024d4e65eb5677195456cd
Cr-Commit-Position: refs/heads/master@{#309560}

[Daniel Erat, 2014-12-23 19:11:16]

On 2014/12/23 18:21:11, I haz the power (commit-bot) wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/2ff93446c3405557b6024d4e65eb5677195456cd
> Cr-Commit-Position: refs/heads/master@{#309560}

Behdad, thinking about this a bit more: would it also make sense to use
FcPatternDel() to clear FC_SIZE and FC_PIXEL_SIZE, to ensure that we're not
affected by configs that e.g. turn off antialiasing for certain size ranges? For
example:

  <match target="font">
    <edit name="antialias" mode="assign"><bool>true</bool></edit>
  </match>
  <match target="font">
    <test name="size" qual="any" compare="more"><double>10</double></test>
    <test name="size" qual="any" compare="less"><double>20</double></test>
    <edit name="antialias"><bool>false</bool></edit>
  </match>

If we clear these, are there any other properties that should also be cleared?

(On the other hand, maybe all of this is hopeless unless we start querying
Fontconfig for each run of text that's using web fonts -- we'll get it wrong at
times otherwise. :-/)

[behdad_google, 2014-12-30 23:43:26]

On 2014/12/23 19:11:16, Daniel Erat wrote:
> On 2014/12/23 18:21:11, I haz the power (commit-bot) wrote:
> > Patchset 7 (id:??) landed as
> > https://crrev.com/2ff93446c3405557b6024d4e65eb5677195456cd
> > Cr-Commit-Position: refs/heads/master@{#309560}
> 
> Behdad, thinking about this a bit more: would it also make sense to use
> FcPatternDel() to clear FC_SIZE and FC_PIXEL_SIZE, to ensure that we're not
> affected by configs that e.g. turn off antialiasing for certain size ranges?
For
> example:
> 
>   <match target="font">
>     <edit name="antialias" mode="assign"><bool>true</bool></edit>
>   </match>
>   <match target="font">
>     <test name="size" qual="any" compare="more"><double>10</double></test>
>     <test name="size" qual="any" compare="less"><double>20</double></test>
>     <edit name="antialias"><bool>false</bool></edit>
>   </match>
> 
> If we clear these, are there any other properties that should also be cleared?
> 
> (On the other hand, maybe all of this is hopeless unless we start querying
> Fontconfig for each run of text that's using web fonts -- we'll get it wrong
at
> times otherwise. :-/)

Sure, you have a valid point.  The more you try to fix this the more impossible
it will become.  At the end of the day, the only way to be completely correct is
to add the font to fontconfig and do a real match...