Filter font list, use Fontconfig and build on Ozone

Filter font list, use Fontconfig and build on Ozone

Since we dropped support for rendering Type 1 fonts in Chrome, we should
filter the list of fonts that can be configured as defaults to only
display fonts that have the right font format, i.e. TrueType or CFF.

A a side effect, this allows us to build the font list enumeration for
ozone as well, as it is only dependent on Fontconfig after this CL, not
dependent on Pango anymore.

For backwards compability with the old implementation, we're adding
three Fontconfig alias families Sans, Serif and Monospace to the list,
since our default settings on Linux included the "Monospace" family for
the fixed width font.

BUG=630508, 457307
Committed: https://crrev.com/e70ca0a2e161e47a7f36f94c19e5a9c6448a15b9
Cr-Commit-Position: refs/heads/master@{#414677}

[drott, 2016-08-25 13:36:57]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 13:37:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-08-25 13:37:51]

drott@chromium.org changed reviewers:
+ derat@chromium.org, jln@chromium.org

[drott, 2016-08-25 13:40:00]

drott@chromium.org changed reviewers:
+ alexst@chromium.org, rjkroege@chromium.org, spang@chromium.org

[drott, 2016-08-25 13:42:06]

Ozone owners added just in case. PTAL.

[drott, 2016-08-25 13:45:48]

drott@chromium.org changed reviewers:
+ behdad@chromium.org

[drott, 2016-08-25 13:46:53]

Description was changed from

==========
Implement font list using Fontconfig and build on Ozone

Since we dropped support for rendering Type 1 fonts in Chrome, we should
filter the list of fonts that can be configured as defaults to only
display fonts that have the right font format, i.e. TrueType or CFF.

A a side effect, this allows us to build the font list enumeration for
ozone as well, as it is only dependent on Fontconfig after this CL, not
dependent on Pango anymore.

For backwards compability with the old implementation, we're adding
three Fontconfig alias families Sans, Serif and Monospace to the list,
since our default settings on Linux included the "Monospace" family for
the fixed width font.

BUG=630508,457307
==========

to

==========
Filter font list, use Fontconfig and build on Ozone

Since we dropped support for rendering Type 1 fonts in Chrome, we should
filter the list of fonts that can be configured as defaults to only
display fonts that have the right font format, i.e. TrueType or CFF.

A a side effect, this allows us to build the font list enumeration for
ozone as well, as it is only dependent on Fontconfig after this CL, not
dependent on Pango anymore.

For backwards compability with the old implementation, we're adding
three Fontconfig alias families Sans, Serif and Monospace to the list,
since our default settings on Linux included the "Monospace" family for
the fixed width font.

BUG=630508,457307
==========

[Daniel Erat, 2016-08-25 13:49:57]

generally lgtm, but i'll let behdad comment on the fontconfig parts.

https://codereview.chromium.org/2278143002/diff/1/content/common/font_list_fo...
File content/common/font_list_fontconfig.cc (right):

https://codereview.chromium.org/2278143002/diff/1/content/common/font_list_fo...
content/common/font_list_fontconfig.cc:7: #include <fontconfig/fontconfig.h>
nit: i think that system headers usually come after stl ones

https://codereview.chromium.org/2278143002/diff/1/content/common/font_list_fo...
content/common/font_list_fontconfig.cc:9: #include <set>
nit: #include <memory> too

https://codereview.chromium.org/2278143002/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/2278143002/diff/1/content/content_common.gypi...
content/content_common.gypi:328: 'common/font_list_fontconfig.cc',
nit: keep this alphabetized

[commit-bot: I haz the power, 2016-08-25 13:53:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 13:53:32]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[drott, 2016-08-25 14:13:05]

Review comments by derat@ addressed

[drott, 2016-08-25 14:13:06]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 14:13:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-08-25 14:17:42]

Review comments by derat@ addressed, trying to fix mac and Android builds

[drott, 2016-08-25 14:17:43]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-25 14:17:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-08-25 14:18:52]

On 2016/08/25 at 13:49:57, derat wrote:
> generally lgtm, but i'll let behdad comment on the fontconfig parts.

Thanks for the quick review, comments addressed in the new upload. Since Behdad
is ooo still for a few days, perhaps you could preliminarily comment on the
Fontconfig parts?

[commit-bot: I haz the power, 2016-08-25 17:00:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 17:00:20]

Dry run: This issue passed the CQ dry run.

[drott, 2016-08-25 18:51:33]

I am going to land this and ask Behdad to take a look when he's back from
vacation since I'd like to have this in before the branch point.

[drott, 2016-08-25 18:51:37]

The CQ bit was checked by drott@chromium.org

[drott, 2016-08-25 18:51:38]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2278143002/#ps40001
(title: "Review comments by derat@ addressed, trying to fix mac and Android
builds")

[commit-bot: I haz the power, 2016-08-25 18:52:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 19:03:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 19:03:44]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rjkroege, 2016-08-25 19:17:16]

I don't really understand this change. Can you explain why it's desirable?

https://codereview.chromium.org/2278143002/diff/40001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/2278143002/diff/40001/content/common/BUILD.gn...
content/common/BUILD.gn:157: if (is_linux) {
I think you've conflated use_ozone and is_linux. They are currently equivalent
but that might change someday.

If use_pango is no longer supported, should it be removed elsewhere?

[drott, 2016-08-25 19:21:41]

drott@chromium.org changed reviewers:
+ avi@chromium.org, pfeldman@chromium.org, piman@chromium.org,
sievers@chromium.org

[drott, 2016-08-25 19:29:44]

On 2016/08/25 at 19:17:16, rjkroege wrote:
> I don't really understand this change. Can you explain why it's desirable?

We don't render Type 1 fonts correctly and we don't want to support this font
format anymore, because it breaks optimize our TrueType/OpenType code paths. But
the pango font results list contains a few of those, which is one part of bug
630508 - so currently you're allowed to select a font that later won't render
(Only a few Type 1 / PostScript fonts that come with Tex packages etc.). But the
pango font enumeration API does not support filtering by font format and it's
difficult to retrieve the format information from the pango results. That's why
I am moving this to Fontconfig, which is practically what Pango does internally,
see also issue 457307. Also, the pango dependency is sort of legacy and long
term we want to get rid of it.

> https://codereview.chromium.org/2278143002/diff/40001/content/common/BUILD.gn
> File content/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2278143002/diff/40001/content/common/BUILD.gn...
> content/common/BUILD.gn:157: if (is_linux) {
> I think you've conflated use_ozone and is_linux. They are currently equivalent
but that might change someday.

Yes, because the Pango library is not generally available on ozone, but
fontconfig is, as much as I could gather from Ozone's dependency tree: Ozone
requires skia, which already requires fontconfig in turn. If Ozone wants to
support another platform that does not have fontconfig, the build conditions can
certainly be changed again. Also note that this actually brings the capability
to enumerate fonts to Ozone. The previous implementation was returning an empty
list.

> If use_pango is no longer supported, should it be removed elsewhere?

It's still used in a few places, which is tracked in bug 457307. But here we can
remove it.

Hope this make the change clearer.

[drott, 2016-08-25 19:30:33]

content/ OWNERS avi@, pfeldman@, sievers@, piman@, PTAL.

[spang, 2016-08-25 19:40:07]

On 2016/08/25 19:29:44, drott wrote:
> On 2016/08/25 at 19:17:16, rjkroege wrote:
> > I don't really understand this change. Can you explain why it's desirable?
> 
> We don't render Type 1 fonts correctly and we don't want to support this font
> format anymore, because it breaks optimize our TrueType/OpenType code paths.
But
> the pango font results list contains a few of those, which is one part of bug
> 630508 - so currently you're allowed to select a font that later won't render
> (Only a few Type 1 / PostScript fonts that come with Tex packages etc.). But
the
> pango font enumeration API does not support filtering by font format and it's
> difficult to retrieve the format information from the pango results. That's
why
> I am moving this to Fontconfig, which is practically what Pango does
internally,
> see also issue 457307. Also, the pango dependency is sort of legacy and long
> term we want to get rid of it.
> 
> >
https://codereview.chromium.org/2278143002/diff/40001/content/common/BUILD.gn
> > File content/common/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/2278143002/diff/40001/content/common/BUILD.gn...
> > content/common/BUILD.gn:157: if (is_linux) {
> > I think you've conflated use_ozone and is_linux. They are currently
equivalent
> but that might change someday.
> 
> Yes, because the Pango library is not generally available on ozone, but
> fontconfig is, as much as I could gather from Ozone's dependency tree: Ozone
> requires skia, which already requires fontconfig in turn. If Ozone wants to
> support another platform that does not have fontconfig, the build conditions
can
> certainly be changed again. Also note that this actually brings the capability
> to enumerate fonts to Ozone. The previous implementation was returning an
empty
> list.
> 
> > If use_pango is no longer supported, should it be removed elsewhere?
> 
> It's still used in a few places, which is tracked in bug 457307. But here we
can
> remove it.
> 
> Hope this make the change clearer.

lgtm

Removing pango on CrOS has been open for a while, thanks :)

Can we also remove cairo yet?

[Avi (use Gerrit), 2016-08-25 19:54:54]

https://codereview.chromium.org/2278143002/diff/40001/content/common/font_lis...
File content/common/font_list_fontconfig.cc (right):

https://codereview.chromium.org/2278143002/diff/40001/content/common/font_lis...
content/common/font_list_fontconfig.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
Copyright 2016 ...

https://codereview.chromium.org/2278143002/diff/40001/content/common/font_lis...
content/common/font_list_fontconfig.cc:61: for
(std::set<std::string>::const_iterator iter = sorted_families.begin();
for (const auto& family : sorted_families) ?

[drott, 2016-08-25 20:08:31]

Review comments by avi@ addressed

[drott, 2016-08-25 20:08:32]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[drott, 2016-08-25 20:09:13]

Thanks for the review, CL updated.

https://codereview.chromium.org/2278143002/diff/40001/content/common/font_lis...
File content/common/font_list_fontconfig.cc (right):

https://codereview.chromium.org/2278143002/diff/40001/content/common/font_lis...
content/common/font_list_fontconfig.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2016/08/25 at 19:54:54, Avi wrote:
> Copyright 2016 ...

Done.

https://codereview.chromium.org/2278143002/diff/40001/content/common/font_lis...
content/common/font_list_fontconfig.cc:61: for
(std::set<std::string>::const_iterator iter = sorted_families.begin();
On 2016/08/25 at 19:54:54, Avi wrote:
> for (const auto& family : sorted_families) ?

Much nicer indeed, thanks.

[commit-bot: I haz the power, 2016-08-25 20:09:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-08-25 20:13:07]

lgtm with fix.

https://codereview.chromium.org/2278143002/diff/60001/content/common/font_lis...
File content/common/font_list_fontconfig.cc (right):

https://codereview.chromium.org/2278143002/diff/60001/content/common/font_lis...
content/common/font_list_fontconfig.cc:1: // Copyright (c) 2016 The Chromium
Authors. All rights reserved.
No (c), just

// Copyright 2016 ...

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[drott, 2016-08-25 20:22:48]

Copyright line fixed

[drott, 2016-08-25 20:22:49]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[drott, 2016-08-25 20:22:56]

The CQ bit was checked by drott@chromium.org

[drott, 2016-08-25 20:22:57]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
spang@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2278143002/#ps10006
(title: "Copyright line fixed")

[commit-bot: I haz the power, 2016-08-25 20:23:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 23:08:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-25 23:08:38]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[drott, 2016-08-26 07:15:57]

The CQ bit was checked by drott@chromium.org

[commit-bot: I haz the power, 2016-08-26 07:16:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 09:25:00]

Committed patchset #5 (id:10006)

[commit-bot: I haz the power, 2016-08-26 09:26:42]

Description was changed from

==========
Filter font list, use Fontconfig and build on Ozone

Since we dropped support for rendering Type 1 fonts in Chrome, we should
filter the list of fonts that can be configured as defaults to only
display fonts that have the right font format, i.e. TrueType or CFF.

A a side effect, this allows us to build the font list enumeration for
ozone as well, as it is only dependent on Fontconfig after this CL, not
dependent on Pango anymore.

For backwards compability with the old implementation, we're adding
three Fontconfig alias families Sans, Serif and Monospace to the list,
since our default settings on Linux included the "Monospace" family for
the fixed width font.

BUG=630508,457307
==========

to

==========
Filter font list, use Fontconfig and build on Ozone

Since we dropped support for rendering Type 1 fonts in Chrome, we should
filter the list of fonts that can be configured as defaults to only
display fonts that have the right font format, i.e. TrueType or CFF.

A a side effect, this allows us to build the font list enumeration for
ozone as well, as it is only dependent on Fontconfig after this CL, not
dependent on Pango anymore.

For backwards compability with the old implementation, we're adding
three Fontconfig alias families Sans, Serif and Monospace to the list,
since our default settings on Linux included the "Monospace" family for
the fixed width font.

BUG=630508,457307

Committed: https://crrev.com/e70ca0a2e161e47a7f36f94c19e5a9c6448a15b9
Cr-Commit-Position: refs/heads/master@{#414677}
==========

[commit-bot: I haz the power, 2016-08-26 09:26:44]

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