linux: Add tests for FontRenderParams on Linux.

linux: Add tests for FontRenderParams on Linux.

Add some tests for FontRenderParams's interactions with
Fontconfig, which is used on desktop Linux and Chrome OS.

Also re-add platform_font_pango_unittest.cc for gn.

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

[Daniel Erat, 2014-07-15 22:09:45]

will, i'm just adding you for the net/base dependency.

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

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:33: std::string
CreateEditStanza(const std::string& name,
an alternative would be creating a testdata directory and storing the configs
there, but i have a preference for keeping the data within the test when
possible.

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:65: // Add the original config's
fonts to the test config.
the other approach here would be creating a testdata directory and storing some
freely-distributable font there and then calling FcConfigAppFontAddFile() to
load it. that might actually be slower than just using the existing cache,
though, and this doesn't seem like it adds any dependencies
(platform_font_pango_unittest.cc already appears to depend on arial and times
new roman being installed; these tests just need a single arbitrary font to be
present to generate a match).

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:71: const std::string path =
net::EscapeForHTML(
ui/base appears to use this for escaping, so i hope it's okay to add to
ui/gfx... i couldn't find anything in base/ for doing this.

[Daniel Erat, 2014-07-15 22:12:21]

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/BUILD.gn#newcode417
ui/gfx/BUILD.gn:417: "platform_font_pango_unittest.cc",
this looks like it got dropped in the gn configs.

[msw, 2014-07-16 00:53:31]

+CC jochen and jam for Fontconfig testing experience, as per r161351
(content/shell/app/webkit_test_platform_support_linux.cc)

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/BUILD.gn
File ui/gfx/BUILD.gn (right):

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/BUILD.gn#newcode417
ui/gfx/BUILD.gn:417: "platform_font_pango_unittest.cc",
On 2014/07/15 22:12:21, Daniel Erat wrote:
> this looks like it got dropped in the gn configs.

Acknowledged.

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

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:33: std::string
CreateEditStanza(const std::string& name,
On 2014/07/15 22:09:45, Daniel Erat wrote:
> an alternative would be creating a testdata directory and storing the configs
> there, but i have a preference for keeping the data within the test when
> possible.

Test .conf files might be clearer (especially if we test more features in the
future); they might also lead to more [test] code sharing with
content/shell/app/webkit_test_platform_support_linux.cc, which appears to use
content/shell/renderer/test_runner/resources/fonts/fonts.conf. That said, your
self-contained approach does seem okay for now. I don't have a strong opinion,
but maybe ping another reviewer?

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:61: FontRenderParamsTest() {
Should this be done on SetUp instead of the ctor?

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:65: // Add the original config's
fonts to the test config.
On 2014/07/15 22:09:45, Daniel Erat wrote:
> the other approach here would be creating a testdata directory and storing
some
> freely-distributable font there and then calling FcConfigAppFontAddFile() to
> load it. that might actually be slower than just using the existing cache,
> though, and this doesn't seem like it adds any dependencies
> (platform_font_pango_unittest.cc already appears to depend on arial and times
> new roman being installed; these tests just need a single arbitrary font to be
> present to generate a match).

It looks like content/shell/app/webkit_test_platform_support_linux.cc already
expects a whole hardcoded list of |kFonts| dirs/files to be accessible and fires
off some warnings/errors if they're not. It also loads a couple other i18n fonts
using FcConfigAppFontAddFile. Can we follow that existing pattern instead?

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:71: const std::string path =
net::EscapeForHTML(
On 2014/07/15 22:09:45, Daniel Erat wrote:
> ui/base appears to use this for escaping, so i hope it's okay to add to
> ui/gfx... i couldn't find anything in base/ for doing this.

Maybe just make a base::FilePath and get its ::value() instead?
(we should probably avoid adding a direct net dependency if possible)

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:77: FcStrList* cache_dirs =
FcConfigGetCacheDirs(NULL);
The <cachedir>s are used to *store* or read the cache of font information. Will
this test have any effect on the actual system font caches by listing these
directories? Should you use the TempDir instead or can we omit this altogether?

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:89: LoadFontconfigData(config_data);
Could this class instead offer any needed font/dir/cache elements via a helper
(that returns a string), which individual tests append to their configs?

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:92: // Swap in the new config. A
primer on undocumented FcConfig
Ha, this is good to include!

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:104: virtual ~FontRenderParamsTest()
{
Should this be done on TearDown instead of the dtor?

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:131: LoadFontconfigData(
Both tests don't modify the same FontRenderParamsTest::config_ instance, right?
Still, perhaps they could use test-local FcConfig instances?

[willchan no longer on Chromium, 2014-07-16 01:35:39]

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

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:71: const std::string path =
net::EscapeForHTML(
On 2014/07/16 00:53:31, msw wrote:
> On 2014/07/15 22:09:45, Daniel Erat wrote:
> > ui/base appears to use this for escaping, so i hope it's okay to add to
> > ui/gfx... i couldn't find anything in base/ for doing this.
> 
> Maybe just make a base::FilePath and get its ::value() instead?
> (we should probably avoid adding a direct net dependency if possible)

That'd be nice if possible :) Let me know if I need to approve a net dependency,
but it'd be nice to avoid it.

[Daniel Erat, 2014-07-16 03:19:47]

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

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:61: FontRenderParamsTest() {
On 2014/07/16 00:53:31, msw wrote:
> Should this be done on SetUp instead of the ctor?

these are mostly equivalent, but i believe that c'tor/d'tor are preferred:
https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de...

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:65: // Add the original config's
fonts to the test config.
On 2014/07/16 00:53:31, msw wrote:
> On 2014/07/15 22:09:45, Daniel Erat wrote:
> > the other approach here would be creating a testdata directory and storing
> some
> > freely-distributable font there and then calling FcConfigAppFontAddFile() to
> > load it. that might actually be slower than just using the existing cache,
> > though, and this doesn't seem like it adds any dependencies
> > (platform_font_pango_unittest.cc already appears to depend on arial and
times
> > new roman being installed; these tests just need a single arbitrary font to
be
> > present to generate a match).
> 
> It looks like content/shell/app/webkit_test_platform_support_linux.cc already
> expects a whole hardcoded list of |kFonts| dirs/files to be accessible and
fires
> off some warnings/errors if they're not. It also loads a couple other i18n
fonts
> using FcConfigAppFontAddFile. Can we follow that existing pattern instead?

thanks for pointing that out. i'll switch to that pattern, which means the
font/cache stuff is unneeded. i might send out another change first to try to
make a ui/gfx/fontconfig_test_support or similar file that content/ can use.

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:71: const std::string path =
net::EscapeForHTML(
On 2014/07/16 01:35:38, willchan wrote:
> On 2014/07/16 00:53:31, msw wrote:
> > On 2014/07/15 22:09:45, Daniel Erat wrote:
> > > ui/base appears to use this for escaping, so i hope it's okay to add to
> > > ui/gfx... i couldn't find anything in base/ for doing this.
> > 
> > Maybe just make a base::FilePath and get its ::value() instead?
> > (we should probably avoid adding a direct net dependency if possible)
> 
> That'd be nice if possible :) Let me know if I need to approve a net
dependency,
> but it'd be nice to avoid it.

this is going into an XML file, so i need to ampersand-escape things like quotes
and angled brackets. putting it into a base::FilePath and then converting it
back to a string won't help there. :-P

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:77: FcStrList* cache_dirs =
FcConfigGetCacheDirs(NULL);
On 2014/07/16 00:53:31, msw wrote:
> The <cachedir>s are used to *store* or read the cache of font information.
Will
> this test have any effect on the actual system font caches by listing these
> directories? Should you use the TempDir instead or can we omit this
altogether?

the already-built caches are needed to prevent fontconfig from needing to read
all of the font files at the beginning of each test (which takes 15+ seconds on
my z620).

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:89: LoadFontconfigData(config_data);
On 2014/07/16 00:53:31, msw wrote:
> Could this class instead offer any needed font/dir/cache elements via a helper
> (that returns a string), which individual tests append to their configs?

it could, but i'm not sure if there's much benefit. all of the tests will need
fonts to be installed to be able to generate matches (unless there's a future
test for the no-match case).

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:104: virtual ~FontRenderParamsTest()
{
On 2014/07/16 00:53:31, msw wrote:
> Should this be done on TearDown instead of the dtor?

see c'tor comment

https://codereview.chromium.org/394963002/diff/20001/ui/gfx/font_render_param...
ui/gfx/font_render_params_linux_unittest.cc:131: LoadFontconfigData(
On 2014/07/16 00:53:31, msw wrote:
> Both tests don't modify the same FontRenderParamsTest::config_ instance,
right?
> Still, perhaps they could use test-local FcConfig instances?

see the googletest faq that i linked to -- each TEST_F generates a new instance
of FontRenderParamsTest

[willchan no longer on Chromium, 2014-07-16 04:04:40]

lgtm then

[msw, 2014-07-16 17:15:23]

lgtm as-is, but I can take another look if you change the font list pattern
here.

[Daniel Erat, 2014-07-16 19:37:44]

i've moved the code that's shared between content/ and my new test into
ui/gfx/test/fontconfig_util_linux.{h,cc}. mind taking another look?

[Daniel Erat, 2014-07-16 19:38:16]

(moved will to cc since the net/ dependency is gone now)

[msw, 2014-07-16 22:38:07]

lgtm with nits, but you may need additional owners.

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
File ui/gfx/test/fontconfig_util_linux.cc (right):

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:48:
"/usr/share/fonts/truetype/kochi/kochi-gothic.ttf",
nit: Why reorder these? They previously were alphabetical, right? Maybe replace
the use of kSystemFontsForFontconfig[0] in the new test file with a hardcoded
index 2 or a separate const value if this font doesn't work for the test there?
Or maybe this is fine out of order.

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:50: NULL,
Can users do arraysize(kSystemFontsForFontconfig) instead? Or maybe create a
separate const here, like chrome/common/url_constants.cc's "const size_t
kNumberOfChromeHostURLs = arraysize(kChromeHostURLs)";

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:107: bool
LoadConfigDataIntoFontconfig(const base::FilePath& temp_dir,
nit: consider encapsulating the TempDir creation here.

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:114: VLOG(1) << "Writing config to " <<
path.value() << ":\n" << data;
nit: is logging the entire file content helpful or necessary?

[Daniel Erat, 2014-07-16 23:10:19]

+brettw for BUILD.gn and content/

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
File ui/gfx/test/fontconfig_util_linux.cc (right):

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:48:
"/usr/share/fonts/truetype/kochi/kochi-gothic.ttf",
On 2014/07/16 22:38:07, msw wrote:
> nit: Why reorder these? They previously were alphabetical, right? Maybe
replace
> the use of kSystemFontsForFontconfig[0] in the new test file with a hardcoded
> index 2 or a separate const value if this font doesn't work for the test
there?
> Or maybe this is fine out of order.

sure, changed the order back and added a new helper to
font_render_params_linux_unittest.cc that loads a font by base name. (i was
planning to add another test in a later change that matches by family, so i need
this anyway.)

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:50: NULL,
On 2014/07/16 22:38:07, msw wrote:
> Can users do arraysize(kSystemFontsForFontconfig) instead? Or maybe create a
> separate const here, like chrome/common/url_constants.cc's "const size_t
> kNumberOfChromeHostURLs = arraysize(kChromeHostURLs)";

i wasn't sure this would work with a const extern value, but it does, so i've
updated this.

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:107: bool
LoadConfigDataIntoFontconfig(const base::FilePath& temp_dir,
On 2014/07/16 22:38:07, msw wrote:
> nit: consider encapsulating the TempDir creation here.

i believe that fontconfig watches config files for changes, and i wasn't sure
about what would happen if i deleted the temp dir immediately after loading the
config. see e.g. "The <rescan> element holds an <int> element which indicates
the default interval between automatic checks for font configuration changes.
Fontconfig will validate all of the configuration files and directories and
automatically rebuild the internal datastructures when this interval passes." at
http://www.freedesktop.org/software/fontconfig/fontconfig-user.html

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:114: VLOG(1) << "Writing config to " <<
path.value() << ":\n" << data;
On 2014/07/16 22:38:07, msw wrote:
> nit: is logging the entire file content helpful or necessary?

i found it useful when writing a test that generated its own configs, but it's
easy enough for the developer to add there own logging statement here when they
need it instead of running with --vmodule, i guess. i don't have much of an
opinion one way or the other; deleted for now

[msw, 2014-07-16 23:19:24]

still lgtm

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
File ui/gfx/test/fontconfig_util_linux.cc (right):

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:107: bool
LoadConfigDataIntoFontconfig(const base::FilePath& temp_dir,
On 2014/07/16 23:10:18, Daniel Erat wrote:
> On 2014/07/16 22:38:07, msw wrote:
> > nit: consider encapsulating the TempDir creation here.
> 
> i believe that fontconfig watches config files for changes, and i wasn't sure
> about what would happen if i deleted the temp dir immediately after loading
the
> config. see e.g. "The <rescan> element holds an <int> element which indicates
> the default interval between automatic checks for font configuration changes.
> Fontconfig will validate all of the configuration files and directories and
> automatically rebuild the internal datastructures when this interval passes."
at
> http://www.freedesktop.org/software/fontconfig/fontconfig-user.html

Hmmm, Maybe it doesn't automatically rescan the file if <rescan> isn't supplied,
or maybe we could call FcConfigSetRescanInverval to ensure this doesn't happen,
or maybe you can just ignore my original suggestion...

https://codereview.chromium.org/394963002/diff/100001/ui/gfx/test/fontconfig_...
ui/gfx/test/fontconfig_util_linux.cc:114: VLOG(1) << "Writing config to " <<
path.value() << ":\n" << data;
On 2014/07/16 23:10:18, Daniel Erat wrote:
> On 2014/07/16 22:38:07, msw wrote:
> > nit: is logging the entire file content helpful or necessary?
> 
> i found it useful when writing a test that generated its own configs, but it's
> easy enough for the developer to add there own logging statement here when
they
> need it instead of running with --vmodule, i guess. i don't have much of an
> opinion one way or the other; deleted for now

It's not output by default, so it's probably fine if you like it.

[Daniel Erat, 2014-07-17 02:23:11]

The CQ bit was checked by derat@chromium.org

[Daniel Erat, 2014-07-17 02:23:31]

The CQ bit was unchecked by derat@chromium.org

[brettw, 2014-07-17 17:19:39]

build/config/linux LGTM

[Daniel Erat, 2014-07-17 17:59:34]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-17 18:00:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/394963002/50008

[commit-bot: I haz the power, 2014-07-17 22:11:50]

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

[commit-bot: I haz the power, 2014-07-17 22:32:46]

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

[commit-bot: I haz the power, 2014-07-17 22:32:47]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Daniel Erat, 2014-07-17 22:43:40]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-17 22:47:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/394963002/50008

[commit-bot: I haz the power, 2014-07-17 23:09:28]

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

[commit-bot: I haz the power, 2014-07-17 23:32:40]

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

[commit-bot: I haz the power, 2014-07-17 23:32:41]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Daniel Erat, 2014-07-18 00:04:00]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2014-07-18 00:07:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/derat@chromium.org/394963002/50008

[commit-bot: I haz the power, 2014-07-18 00:34:54]

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

[commit-bot: I haz the power, 2014-07-18 01:44:27]

Change committed as 283969