Implement Harmony typography spec.

chrome/browser/ui/views/harmony/harmony_typography_provider.cc

Index: chrome/browser/ui/views/harmony/harmony_typography_provider.cc
diff --git a/chrome/browser/ui/views/harmony/harmony_typography_provider.cc b/chrome/browser/ui/views/harmony/harmony_typography_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..85e2c8384d568297c5ee71986a883e5d7977d66e
--- /dev/null
+++ b/chrome/browser/ui/views/harmony/harmony_typography_provider.cc
@@ -0,0 +1,122 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/harmony/harmony_typography_provider.h"
+
+#include "chrome/browser/ui/views/harmony/chrome_typography.h"
+#include "ui/base/resource/resource_bundle.h"
+
+const gfx::FontList& HarmonyTypographyProvider::GetFont(int text_context,
+                                                        int text_style) const {
+  // "Target" font size constants from the Harmony spec.

[Peter Kasting, 2017/04/04 00:47:58]

Nit: I feel weakly like it might be better to define this in the cases below
that actually use them, but not sure.

[tapted, 2017/04/04 06:23:00]

The goal here is to call out the magic numbers that come directly from the spec,
and give them a name.

(the pessimist/pragmatist in me is expecting that there may be a number of cases
coming out in the wash that the spec doesn't cater for, which may be derived
from these. E.g. TouchSelectionMenuRunnerViews::Menu::CreateButton jumps through
hoops to get an 11pt font)

+  constexpr int kHeadlineSize = 20;
+  constexpr int kTitleSize = 15;
+  constexpr int kBodyTextLargeSize = 13;
+  constexpr int kDefaultSize = 12;
+
+#if defined(OS_WIN)
+  constexpr gfx::Font::Weight kButtonFontWeight = gfx::Font::Weight::BOLD;
+#else
+  constexpr gfx::Font::Weight kButtonFontWeight = gfx::Font::Weight::MEDIUM;
+#endif
+
+// The size that a default-constructed gfx::PlatformFont returns in a default
+// OS configuration.

[Peter Kasting, 2017/04/04 00:47:58]

Ultimately, can this be computed (once) at runtime, or centralized to
PlatformFont or something?  Having it here feels a bit magic/layer-violationy,
and it seems like some of the (disabled) tests you previously added want this as
well.

[tapted, 2017/04/04 06:23:00]

I don't think it can be computed at runtime, since it's really an estimate of
what's "most popular" (see next comment), but we can move it to PlatformFont
(done).

+#if defined(OS_MACOSX)
+  constexpr int kPlatformFontSize = 13;
+#else
+  constexpr int kPlatformFontSize = 12;
+#endif
+
+  int size_delta = kDefaultSize - kPlatformFontSize;
+  gfx::Font::Weight font_weight = gfx::Font::Weight::NORMAL;
+  switch (text_context) {
+    case CONTEXT_HEADLINE:
+      size_delta = kHeadlineSize - kPlatformFontSize;
+      break;
+    case views::style::CONTEXT_DIALOG_TITLE:
+      size_delta = kTitleSize - kPlatformFontSize;
+      break;
+    case CONTEXT_BODY_TEXT_LARGE:
+      size_delta = kBodyTextLargeSize - kPlatformFontSize;
+      break;
+    case views::style::CONTEXT_BUTTON:
+      font_weight = kButtonFontWeight;

[Peter Kasting, 2017/04/04 00:47:58]

Nit: Put break; after (no functional effect now, could cause a bug later)

[tapted, 2017/04/04 06:22:59]

Done. (I am ashamed to admit that I don't think this was intentional..)

+    default:
+      break;
+  }
+
+  // Ignore |text_style| since it only affects color in the Harmony spec.
+  return ui::ResourceBundle::GetSharedInstance().GetFontListWithDelta(
+      size_delta, gfx::Font::NORMAL, font_weight);
+}
+
+SkColor HarmonyTypographyProvider::GetColor(int text_context,
+                                            int text_style) const {
+  return SK_ColorBLACK;

[Peter Kasting, 2017/04/04 00:47:58]

Nit: Maybe a TODO?  Prolly not necessary I guess

[tapted, 2017/04/04 06:23:00]

Done.

+}
+
+int HarmonyTypographyProvider::GetLineHeight(int text_context,
+                                             int text_style) const {
+  // "Target" line height constants from the Harmony spec. A default OS
+  // configuration should use these heights. However, if the user overrides OS
+  // defaults, then GetLineHeight() should return the height that would add the
+  // same extra space between lines as the default configuration would have.
+  constexpr int kHeadlineHeight = 32;
+  constexpr int kTitleHeight = 22;
+  constexpr int kBodyHeight = 20;  // For both large and small.
+
+  // Button text should always use the minimum line height for a font to avoid
+  // unnecessarily influencing the height of a button.
+  constexpr int kButtonAbsoluteHeight = 0;
+
+// The platform-specific heights (i.e. gfx::Font::GetHeight()) that result when
+// asking for the target size constants in HarmonyTypographyProvider::GetFont()
+// in a default OS configuration.
+#if defined(OS_MACOSX)

[Peter Kasting, 2017/04/04 00:47:58]

For all platforms, I wonder if these values are really true across OS versions. 
For example, do Win 7 and Win 10 behave the same, do the earliest and latest OS
X versions we support behave the same, etc.

Again, if it's possible to compute this at runtime somehow (have PlatformFont
construct a font with a specific face and size and then measure its height?),
that would be safer than hardcoding these.

[tapted, 2017/04/04 06:23:00]

So, like you say, I'm quite certain that these are not consistent across OS
versions, and we certainly can't assume it's the case for all future OS
versions. Added a TODO to fill this out with OS point version selection.

But we do need a starting point that "looks right" for a majority of users. per
the comment at the top of GetLineHeight -- it's trying to add the same "extra"
space between lines that the default configuration would have when asking for
the line height from the spec.

I can't think of a way to tell by asking the OS at runtime whether their system
font has changed properties because that's what the OS now prefers or that's
what the user prefers.

E.g. on Mac we use [NSFont systemFontSize]. Usually that returns "13". But if a
user has changed the preference, I don't know of another method we could call
that returns "13" to provide what would have been provided by the current OS
version if the user had not overridden it.

And for the font height it's going to also depend on the particular font size -
i.e. what are the characteristics of glyphs in the system font at a particular
size. Since it depends on the font sizes specifically used for Harmony, I think
this is the right place. E.g. on PlatformFont, it might look something like:

kHeightForPopularDefaultFontOfSize20
kHeightForPopularDefaultFontOfSize15
kHeightForPopularDefaultFontOfSize13
kHeightForPopularDefaultFontOfSize12

But if the Harmony spec changes, we shouldn't have to change PlatformFont.


And - for a similar reason - we can't look it up. If a user has configured a
different typeface (e.g. exchanged for one with the same font size), it could
return different results.  E.g. we might still end up with a line spacing of
"20pt" but say a users particular configured font _already_ have a line spacing
of 20pt -- the text would be all squishy since we wouldn't be adding any extra
line spacing. Or say the system font in a particular language is a blockish font
that never has ascenders or descenders: we'd want a smaller line spacing there,
but if we looked it up and it says "height=12pt" we'd add too much.


Aaand, this all assumes there's useful information in the font height when taken
separately from the font size. E.g. an alternative could be just to consider the
fontsize. E.g.

return GetFont(CONTEXT_HEADLINE, kTemplateStyle).GetFontSize() - kHeadlineSize +
kHeadlineHeight;

but this would do nothing to adapt to locales.

[Peter Kasting, 2017/04/04 08:01:17]

I sent email to Alan & co. asking how, theoretically, they'd like us to handle
font size variations, since I feel like I don't sufficiently understand the
design goals.

+  constexpr int kHeadlinePlatformHeight = 25;
+  constexpr int kTitlePlatformHeight = 19;
+  constexpr int kBodyTextLargePlatformHeight = 16;
+  constexpr int kBodyTextSmallPlatformHeight = 15;
+#elif defined(OS_WIN)
+  constexpr int kHeadlinePlatformHeight = 28;
+  constexpr int kTitlePlatformHeight = 20;
+  constexpr int kBodyTextLargePlatformHeight = 17;
+  constexpr int kBodyTextSmallPlatformHeight = 15;
+#else
+  constexpr int kHeadlinePlatformHeight = 24;
+  constexpr int kTitlePlatformHeight = 18;
+  constexpr int kBodyTextLargePlatformHeight = 17;
+  constexpr int kBodyTextSmallPlatformHeight = 15;
+#endif
+
+  // The style of the system font used to determine line heights.
+  constexpr int kTemplateStyle = views::style::STYLE_PRIMARY;
+
+  static const int headline_height =

[Peter Kasting, 2017/04/04 00:47:58]

These statics worry me w.r.t. handling native theme changes that happen while
Chrome is running.

I wonder if instead we need to compute and store these in some kind of native
theme changed listener, and trigger that on startup?

[tapted, 2017/04/04 06:23:00]

We'd need to clear a lot more than these statics - all of
ResourceBundle::font_cache_ as well. Currently that's only supported on ChromeOS
via chromeos::locale_util::SwitchLanguage(..) - no other platform calls
ResourceBundle::ReloadFonts() (and I think ChromeOS only invokes that during a
login flow).

(if nothing invokes ResourceBundle::ReloadFonts, but we tried to re-query these,
then a cached font would be returned by ResourceBundle and give exactly the same
response)

Some kind of ResourceBundleObserver could do this. Added a TODO(). Should we add
a tracking bug for Harmony-on-ChromeOS?

[Peter Kasting, 2017/04/06 06:17:07]

I think we should file a bug saying that when we get a theme change, the
resource bundle should ReloadFonts(), and we should also reload here (either by
noticing the theme change, or by the resource bundle telling some observers that
fonts were reloaded -- possibly the latter just to prevent ordering/race
condition-type problems).

[tapted, 2017/04/06 09:11:09]

Filed http://crbug.com/708943 and noted it here.

+      GetFont(CONTEXT_HEADLINE, kTemplateStyle).GetHeight() -
+      kHeadlinePlatformHeight + kHeadlineHeight;
+  static const int title_height =
+      GetFont(views::style::CONTEXT_DIALOG_TITLE, kTemplateStyle).GetHeight() -
+      kTitlePlatformHeight + kTitleHeight;
+  static const int body_large_height =
+      GetFont(CONTEXT_BODY_TEXT_LARGE, kTemplateStyle).GetHeight() -
+      kBodyTextLargePlatformHeight + kBodyHeight;
+  static const int default_height =
+      GetFont(CONTEXT_BODY_TEXT_SMALL, kTemplateStyle).GetHeight() -
+      kBodyTextSmallPlatformHeight + kBodyHeight;
+
+  switch (text_context) {
+    case CONTEXT_HEADLINE:
+      return headline_height;
+    case views::style::CONTEXT_DIALOG_TITLE:
+      return title_height;
+    case CONTEXT_BODY_TEXT_LARGE:
+      return body_large_height;
+    case views::style::CONTEXT_BUTTON:
+      return kButtonAbsoluteHeight;
+    default:
+      return default_height;
+  }
+}