"Bootstrap" a toolkit-views Typography spec.

ui/gfx/render_text_unittest.cc

 // Copyright (c) 2012 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 "ui/gfx/render_text.h"
 
 #include <limits.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 2322 matching lines @@
 
   render_text->SetMinLineHeight(default_size.height() / 2);
   EXPECT_EQ(default_size.ToString(), render_text->GetStringSizeF().ToString());
 
   render_text->SetMinLineHeight(default_size.height() * 2);
   SizeF taller_size = render_text->GetStringSizeF();
   EXPECT_EQ(default_size.height() * 2, taller_size.height());
   EXPECT_EQ(default_size.width(), taller_size.width());
 }
 
+// Check that, for Latin characters, typesetting text in the default fonts and
+// sizes does not discover any glyphs that would exceed the line spacing
+// recommended by gfx::Font.
+// Disabled since this relies on machine configuration. http://crbug.com/701241.

[sky, 2017/03/20 16:33:11]

The bug points at the harmony bug, which isn't at all obvious how it relates to
this test. What's the plan to enable this test? I would expect the bots to be
configured in similar ways, but maybe you're referring to font differences
between OS version? If it's the later, it seems like fonts may always differ
between major OS version.

[tapted, 2017/03/20 22:31:42]

The plan would be to bring this up at the same time as the tests in
c/b/ui/views/harmony/layout_delegate_unittest.cc that also reference
http://crbug.com/701241

I hadn't referenced http://crbug.com/701241 in the CL description (added it
now). http://crbug.com/701241 is "Bring up views::Typography "change detector"
tests" and talks about the rationale for starting with these tests disabled, and
the plan to bring them up. 

Specifically for this RenderTextTest, it's referencing the 'standard' font sizes
from the Harmony spec with the goal to validate the assumption that it is safe
to rely on the result of gfx::Font::GetFontSize() on a "default OS
configuration" as a means to derive an appropriate line spacing.

For example, if typesetting that Latin string gives font glyphs that are "too
tall" then the assumption that we can add a constant to gfx::Font::GetFontSize()
to obtain the "in-spec" line spacing may be wrong (or may indicate a bug in how
GetFontSize() is calculated, which involves a bit of arithmetic and
platform-specific assumptions).

I could remove the EXPECT_EQ(<integer literal>, ..) checks below and this test
might not suffer from the flakiness I'm concerned about. But then it's harder to
identify whether the failure is due to a misconfigured bot. However, once the
tests in layout_delegate_unittest.cc are robust on all our bots, then it should
be safe just to switch this on regardless.



You can see in an earlier patchset -

https://codereview.chromium.org/2734113006/diff/240001/ui/views/style/typogra...
- the lengths I was going to to get the tests green. There was one bot run on
Windows where the base font size was 11 rather than 12 - I need to smoke it out
and any like it.

The OS version also plays a role, especially on Mac - Apple changed the system
typeface from Lucida Grande to "Helvetica Neue" to "San Francisco" across 3
consecutive versions :|


So, yup, fonts are likely to differ between OS versions. But we need a way to
detect this change in any new OS release. The Harmony typography provider will
need platform, and perhaps OS-version-specific, logic to ensure that the
"default" configuration gives a line height of, e.g., 20. (But we can't just say
"line height is 20" because in a *non*-default configuration -- when a user has
configured larger fonts -- that number will be wrong).

+TEST_P(RenderTextTest, DISABLED_DefaultLineHeights) {
+  RenderText* render_text = GetRenderText();
+  render_text->SetText(
+      ASCIIToUTF16("A quick brown fox jumped over the lazy dog!"));
+
+#if defined(OS_MACOSX)
+  const FontList body2_font = FontList().DeriveWithSizeDelta(-1);
+#else
+  const FontList body2_font;
+#endif
+
+  const FontList headline_font = body2_font.DeriveWithSizeDelta(8);
+  const FontList title_font = body2_font.DeriveWithSizeDelta(3);
+  const FontList body1_font = body2_font.DeriveWithSizeDelta(1);
+#if defined(OS_WIN)
+  const FontList button_font =
+      body2_font.DeriveWithWeight(gfx::Font::Weight::BOLD);
+#else
+  const FontList button_font =
+      body2_font.DeriveWithWeight(gfx::Font::Weight::MEDIUM);
+#endif
+
+  EXPECT_EQ(12, body2_font.GetFontSize());
+  EXPECT_EQ(20, headline_font.GetFontSize());
+  EXPECT_EQ(15, title_font.GetFontSize());
+  EXPECT_EQ(13, body1_font.GetFontSize());
+  EXPECT_EQ(12, button_font.GetFontSize());
+
+  for (const auto& font :
+       {headline_font, title_font, body1_font, body2_font, button_font}) {
+    render_text->SetFontList(font);
+    EXPECT_EQ(font.GetHeight(), render_text->GetStringSizeF().height());
+  }
+}
+
 TEST_P(RenderTextTest, SetFontList) {
   RenderText* render_text = GetRenderText();
   render_text->SetFontList(
       FontList(base::StringPrintf("Arial,%s, 13px", kSymbolFontName)));
   const std::vector<Font>& fonts = render_text->font_list().GetFonts();
   ASSERT_EQ(2U, fonts.size());
   EXPECT_EQ("Arial", fonts[0].GetFontName());
   EXPECT_EQ(kSymbolFontName, fonts[1].GetFontName());
   EXPECT_EQ(13, render_text->font_list().GetFontSize());
 }
@@ skipping 2031 matching lines @@
                         ::testing::Values(RENDER_TEXT_HARFBUZZ),
                         PrintRenderTextBackend());
 #endif
 
 INSTANTIATE_TEST_CASE_P(,
                         RenderTextHarfBuzzTest,
                         ::testing::Values(RENDER_TEXT_HARFBUZZ),
                         PrintRenderTextBackend());
 
 }  // namespace gfx