ui/gfx: Allow for font-specific rendering settings.

ui/gfx/font_render_params_linux.cc

Index: ui/gfx/font_render_params_linux.cc
diff --git a/ui/gfx/font_render_params_linux.cc b/ui/gfx/font_render_params_linux.cc
index 4a06c59f13a957098855de933fc620776c6e4f7c..a39bf01159bb907b06cd32670c7b63d7070dc5dc 100644
--- a/ui/gfx/font_render_params_linux.cc
+++ b/ui/gfx/font_render_params_linux.cc
@@ -24,61 +24,120 @@ bool SubpixelPositioningRequested(bool renderer) {
                : switches::kEnableBrowserTextSubpixelPositioning);
 }
 
-// Initializes |params| with the system's default settings. |renderer| is true
-// when setting WebKit renderer defaults.
-void LoadDefaults(FontRenderParams* params, bool renderer) {
-  // For non-GTK builds (read: Aura), just use reasonable hardcoded values.
-  params->antialiasing = true;
-  params->autohinter = true;
-  params->use_bitmaps = true;
-  params->hinting = FontRenderParams::HINTING_SLIGHT;
-
-  // Fetch default subpixel rendering settings from FontConfig.
+// Queries FontConfig for rendering settings and updates |params_out| and
+// |family_out| (if non-NULL). Returns false on failure. See
+// GetCustomFontRenderParams() for descriptions of arguments.
+bool QueryFontConfig(

[Daniel Erat, 2014/07/11 17:15:29]

this essentially just combines the code from LoadDefaults() and
PlatformFontPango's FindBestMatchFontFamilyName() (although it also grabs some
additional settings from fontconfig now)

[msw, 2014/07/12 00:27:32]

Acknowledged.

+    const std::vector<std::string>* family_list,
+    const int* pixel_size,
+    const int* point_size,
+    FontRenderParams* params_out,
+    std::string* family_out) {
   FcPattern* pattern = FcPatternCreate();
+  CHECK(pattern);
+
+  if (family_list) {
+    for (std::vector<std::string>::const_iterator it = family_list->begin();
+         it != family_list->end(); ++it) {
+      FcValue value;
+      value.type = FcTypeString;
+      value.u.s = reinterpret_cast<const FcChar8*>(it->c_str());
+      FcPatternAdd(pattern, FC_FAMILY, value, FcTrue /* append */);

[msw, 2014/07/12 00:27:33]

nit: FcPatternAddString might be simpler.

[Daniel Erat, 2014/07/12 01:55:32]

nice! i'd missed these

+    }
+  }
+  if (pixel_size) {
+    FcValue value;
+    value.type = FcTypeDouble;
+    value.u.d = *pixel_size;
+    FcPatternAdd(pattern, FC_PIXEL_SIZE, value, FcTrue /* append */);

[msw, 2014/07/12 00:27:33]

nit: FcPatternAddDouble might be simpler

[Daniel Erat, 2014/07/12 01:55:33]

Done.

+  }
+  if (point_size) {
+    FcValue value;
+    value.type = FcTypeInteger;
+    value.u.i = *point_size;
+    FcPatternAdd(pattern, FC_SIZE, value, FcTrue /* append */);

[msw, 2014/07/12 00:27:32]

nit: FcPatternAddInteger might be simpler.

[Daniel Erat, 2014/07/12 01:55:33]

Done.

+  }
+
   FcConfigSubstitute(NULL, pattern, FcMatchPattern);
   FcDefaultSubstitute(pattern);
   FcResult result;
   FcPattern* match = FcFontMatch(0, pattern, &result);

[msw, 2014/07/12 00:27:32]

nit: s/0/NULL/

[Daniel Erat, 2014/07/12 01:55:32]

Done.

-  DCHECK(match);
-  int fc_rgba = FC_RGBA_RGB;
-  FcPatternGetInteger(match, FC_RGBA, 0, &fc_rgba);
-  FcPatternDestroy(pattern);
-  FcPatternDestroy(match);
-
-  switch (fc_rgba) {
-    case FC_RGBA_RGB:
-      params->subpixel_rendering = FontRenderParams::SUBPIXEL_RENDERING_RGB;
-      break;
-    case FC_RGBA_BGR:
-      params->subpixel_rendering = FontRenderParams::SUBPIXEL_RENDERING_BGR;
-      break;
-    case FC_RGBA_VRGB:
-      params->subpixel_rendering = FontRenderParams::SUBPIXEL_RENDERING_VRGB;
-      break;
-    case FC_RGBA_VBGR:
-      params->subpixel_rendering = FontRenderParams::SUBPIXEL_RENDERING_VBGR;
-      break;
-    default:
-      params->subpixel_rendering = FontRenderParams::SUBPIXEL_RENDERING_NONE;
+  if (!match)

[msw, 2014/07/12 00:27:33]

Should we DCHECK this or is it okay to not actually find any matches?

[Daniel Erat, 2014/07/12 01:55:32]

the documentation is silent, of course, but the implementation looks like it
returns NULL if it fails to find a config (maybe if the caller forgot to
initialize fontconfig?) or if FcFontSetMatchInternal() returns NULL. that method
looks like it returns NULL if it fails to find a match, so it's probably better
to handle this instead of DCHECK-ing.

[msw, 2014/07/12 02:26:40]

Acknowledged.

+    return false;
+
+  if (family_out) {
+    FcChar8* family = NULL;
+    if (FcPatternGetString(match, FC_FAMILY, 0, &family) == FcResultMatch)
+      family_out->assign(reinterpret_cast<const char*>(family));
   }
 
-#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
-  // TODO(derat): Get the autohinter setting from FontConfig. Until then,
-  // maintain backward compatibility with what we were doing previously.
-  params->autohinter = renderer;
-  const LinuxFontDelegate* delegate = LinuxFontDelegate::instance();
-  if (delegate) {
-    params->antialiasing = delegate->UseAntialiasing();
-    params->hinting = delegate->GetHintingStyle();
-    params->subpixel_rendering = delegate->GetSubpixelRenderingStyle();
+  if (params_out) {
+    FcBool fc_antialias = 0;
+    if (FcPatternGetBool(match, FC_ANTIALIAS, 0, &fc_antialias) ==
+        FcResultMatch)
+      params_out->antialiasing = fc_antialias;
+
+    FcBool fc_autohint = 0;
+    if (FcPatternGetBool(match, FC_AUTOHINT, 0, &fc_autohint) == FcResultMatch)
+      params_out->autohinter = fc_autohint;
+
+    FcBool fc_hinting = 0;
+    int fc_hint_style = FC_HINT_NONE;
+    if (FcPatternGetBool(match, FC_HINTING, 0, &fc_hinting) == FcResultMatch &&
+        FcPatternGetInteger(match, FC_HINT_STYLE, 0, &fc_hint_style) ==

[msw, 2014/07/12 00:27:32]

Does it make sense to offer a default hinting setting other than HINTING_NONE if
FC_HINTING is true but FC_HINT_STYLE is not set? Similarly, I'm not sure what's
intended if FC_HINTING is false, but FC_HINT_STYLE is (eg.) FC_HINT_FULL.
Perhaps your current logic requiring both settings to offer any hinting is
reasonable. (why does FontConfig have a separate bool and enum for this?)

[Daniel Erat, 2014/07/12 01:55:33]

(to make it possible for users to choose self-contradicting settings? :-P)

the pango implementation is at
https://git.gnome.org/browse/pango/tree/pango/pangoft2.c#n162 -- maybe we should
just match that:

- read FC_ANTIALIAS bool; default to true if unset
- read FC_HINTING bool; default to true if unset
- read FC_HINT_STYLE int; default to full if unset
- read FC_AUTOHINT bool; default to false if unset

it looks like the hinting bool being false trumps the hinting style enum there,
too.

in general, i'm not particularly worried about us needing to fall back to
defaults for any of these. fontconfig ships with a default config, and i suspect
that users are almost always overlaying crazy stuff on top of it via
~/.fonts.conf instead of deleting stuff from it.

[msw, 2014/07/12 02:26:40]

Acknowledged.

+        FcResultMatch) {
+      params_out->hinting = FontRenderParams::HINTING_NONE;
+      if (fc_hinting) {
+        switch (fc_hint_style) {

[msw, 2014/07/12 00:27:33]

optional nit: add a conversion helper.

[Daniel Erat, 2014/07/12 01:55:33]

Done.

+          case FC_HINT_SLIGHT:
+            params_out->hinting = FontRenderParams::HINTING_SLIGHT;
+            break;
+          case FC_HINT_MEDIUM:
+            params_out->hinting = FontRenderParams::HINTING_MEDIUM;
+            break;
+          case FC_HINT_FULL:
+            params_out->hinting = FontRenderParams::HINTING_FULL;
+            break;
+          default:
+            // Leave hinting disabled otherwise.
+            break;
+        }
+      }
+    }
+
+    int fc_rgba = FC_RGBA_UNKNOWN;
+    if (FcPatternGetInteger(match, FC_RGBA, 0, &fc_rgba) == FcResultMatch) {
+      switch (fc_rgba) {

[msw, 2014/07/12 00:27:32]

optional nit: add a conversion helper.

[Daniel Erat, 2014/07/12 01:55:32]

Done.

+        case FC_RGBA_RGB:
+          params_out->subpixel_rendering =
+              FontRenderParams::SUBPIXEL_RENDERING_RGB;
+          break;
+        case FC_RGBA_BGR:
+          params_out->subpixel_rendering =
+              FontRenderParams::SUBPIXEL_RENDERING_BGR;
+          break;
+        case FC_RGBA_VRGB:
+          params_out->subpixel_rendering =
+              FontRenderParams::SUBPIXEL_RENDERING_VRGB;
+          break;
+        case FC_RGBA_VBGR:
+          params_out->subpixel_rendering =
+              FontRenderParams::SUBPIXEL_RENDERING_VBGR;
+          break;
+        default:
+          params_out->subpixel_rendering =
+              FontRenderParams::SUBPIXEL_RENDERING_NONE;
+      }
+    }
   }
-#endif
 
-  params->subpixel_positioning = SubpixelPositioningRequested(renderer);
+  return true;
+}
 
-  // To enable subpixel positioning, we need to disable hinting.
-  if (params->subpixel_positioning)
-    params->hinting = FontRenderParams::HINTING_NONE;
+// Initializes |params| with the system's default settings. |renderer| is true
+// when setting WebKit renderer defaults.
+void LoadDefaults(FontRenderParams* params, bool renderer) {
+  *params = GetCustomFontRenderParams(renderer, NULL, NULL, NULL, NULL);
 }
 
 }  // namespace
@@ -101,6 +160,52 @@ const FontRenderParams& GetDefaultWebKitFontRenderParams() {
   return default_params;
 }
 
+FontRenderParams GetCustomFontRenderParams(
+    bool renderer,
+    const std::vector<std::string>* family_list,
+    const int* pixel_size,
+    const int* point_size,
+    std::string* family_out) {
+  FontRenderParams params;
+  if (family_out)
+    family_out->clear();
+
+#if defined(OS_CHROMEOS)
+  // Use reasonable defaults.
+  params.antialiasing = true;

[msw, 2014/07/12 00:27:32]

These reasonable defaults previously applied to Desktop Linux Aura as well,
should they continue to do so?

[Daniel Erat, 2014/07/12 01:55:32]

no, i don't think so. i just meant "reasonable for chrome os" when i wrote that
comment, and i was a bit scared that someone would inadvertently change chrome
os while trying to change the linux defaults in the future.

the defaults that i added in FontRenderParam's c'tor line up with these, so i
don't think that linux should be affected.

[msw, 2014/07/12 02:26:40]

Ah, ok. Should we remove these explicit assignments, given the new ctor?

[Daniel Erat, 2014/07/12 02:44:38]

i'd rather continue to be explicit for chrome os, since we know what we want the
settings to be there. i'm more comfortable starting with the defaults from the
c'tor for desktop linux, since there are no "right" settings there and it's very
likely that whatever we use will be overridden by gtk and/or fontconfig.

+  params.autohinter = true;
+  params.use_bitmaps = true;
+  params.hinting = FontRenderParams::HINTING_SLIGHT;
+
+  // Query Fontconfig to get the family name and subpixel rendering setting.

[msw, 2014/07/12 00:27:33]

Why don't we respect the other FontConfig settings on Chrome OS? Should we be
passing in |params| to use its values as defaults?

[Daniel Erat, 2014/07/12 01:55:32]

i'd actually like for us to get away from using fontconfig at all on chrome os
-- among other things, it makes it difficult to get deterministic font rendering
when running a chrome os build of chrome on goobuntu.

i regrettably made us use fontconfig to communicate that subpixel rendering
should be disabled on chromeboxes long ago; perhaps i should've added a flag
instead. i think that we're also dependent on it for specifying alternate fonts
to use for differents scripts, though.

[msw, 2014/07/12 02:26:40]

Can you add a comment here? The reasons for limited FC use are non-obvious.

[Daniel Erat, 2014/07/12 02:44:38]

done. also added a TODO, since i'm still not wholly convinced. maybe it'd be
better if chrome os just used the same fontconfig-dependent path as desktop
linux. i need to talk to jungshik about this to see what his plans are.

+  FontRenderParams fc_params;
+  QueryFontConfig(family_list, pixel_size, point_size, &fc_params, family_out);
+  params.subpixel_rendering = fc_params.subpixel_rendering;
+#else
+  // Start with the delegate's settings, but let Fontconfig have the final say.
+  const LinuxFontDelegate* delegate = LinuxFontDelegate::instance();

[msw, 2014/07/12 00:27:33]

Can we remove LinuxFontDelegate (or most if it) now that we get the right
settings from FontConfig? (if so, that would be fine as a TODO + follow-up)

[Daniel Erat, 2014/07/12 01:55:32]

i'll add a TODO about this since i honestly wasn't sure about it. GtkSettings
exposes rendering settings, but i believe that GTK queries fontconfig when
drawing widgets, so i'm really not sure where the settings come into play, if
anywhere.

[msw, 2014/07/12 02:26:40]

Acknowledged.

+  if (delegate) {
+    params.antialiasing = delegate->UseAntialiasing();
+    params.hinting = delegate->GetHintingStyle();
+    params.subpixel_rendering = delegate->GetSubpixelRenderingStyle();
+  }
+  QueryFontConfig(family_list, pixel_size, point_size, &params, family_out);
+#endif
+
+  params.subpixel_positioning = SubpixelPositioningRequested(renderer);
+
+  // To enable subpixel positioning, we need to disable hinting.
+  if (params.subpixel_positioning)
+    params.hinting = FontRenderParams::HINTING_NONE;
+
+  // Use the first family from the list if FontConfig didn't suggest a family.
+  if (family_out && family_out->empty() &&
+      family_list && !family_list->empty())
+    *family_out = (*family_list)[0];

[Daniel Erat, 2014/07/11 17:15:29]

i didn't bother adding fallback code like this for any other implementations
since PlatformFontPango is the only class that calls GetCustomFontRenderParams()

[msw, 2014/07/12 00:27:33]

Acknowledged.

+
+  return params;
+}
+
 bool GetDefaultWebkitSubpixelPositioning() {
   return SubpixelPositioningRequested(true);
 }