Commit 5307bd51 authored by Etienne Bergeron's avatar Etienne Bergeron Committed by Commit Bot

Re-enable layout provider unittests

This CL is re-enabling the layout unittest on windows.

Fonts size have been adjusted on MAC. They are now about
1 pixel larger than other platform, which make more sense
than the previous values. I cannot explain the choice of
the previous values.

These tests were flaky because some code in chrome tests were
modifying a system-wide flag that change the way fonts are
rendered.

We are assuming the default configuration: Anti-Aliasing is on.

Other unittests should not turn on/off the system-wide flag.
Modifying that flag may leave the developer computer or the
build bot in a bad state. Tests are likely to crash and may
not restore correctly the state; they may run in parallel and
have incorrect behavior.

This change needs to be landed after fixing flakiness:
  1) https://chromium-review.googlesource.com/c/chromium/src/+/1570527/
  2) https://chromium-review.googlesource.com/c/chromium/src/+/1569969

NOTE TO SHERRIFS:
 If any Layout test are flaky, please disable the test
 and assign the bug to me. I think I've found any cause of flakiness. If not,
 I'll dig more.

BUG=759870, 701241
R=tapted@chromium.org
CC=​​​​​​​​​robliao@chromium.org,asvitkine@chromium.org

Change-Id: Iab8f4adb58e565600bedecb1365e05bfdf931079
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1570653
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: default avatarEtienne Bergeron <etienneb@chromium.org>
Reviewed-by: default avatarMichael Wasserman <msw@chromium.org>
Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658689}
parent aed20f9f
......@@ -110,18 +110,17 @@ SkColor GetHarmonyTextColorForNonStandardNativeTheme(
#if defined(OS_WIN)
// static
int ChromeTypographyProvider::GetPlatformFontHeight(int font_context) {
const bool windows_10 = base::win::GetVersion() >= base::win::Version::WIN10;
switch (font_context) {
case CONTEXT_HEADLINE:
return windows_10 ? 27 : 28;
return 27;
case views::style::CONTEXT_DIALOG_TITLE:
return windows_10 ? 20 : 21;
return 20;
case CONTEXT_BODY_TEXT_LARGE:
case CONTEXT_TAB_HOVER_CARD_TITLE:
case views::style::CONTEXT_MESSAGE_BOX_BODY_TEXT:
return 18;
case CONTEXT_BODY_TEXT_SMALL:
return windows_10 ? 16 : 15;
return 16;
}
NOTREACHED();
return 0;
......
......@@ -39,17 +39,22 @@ class LayoutProviderTest : public testing::Test {
public:
LayoutProviderTest() {}
#if defined(OS_WIN)
protected:
static void SetUpTestCase() {
// The expected case is to have DirectWrite enabled; the fallback gives
// different font heights. However, only use DirectWrite on Windows 10 and
// later, since it's known to have flaky results on Windows 7. See
// http://crbug.com/759870.
if (base::win::GetVersion() >= base::win::Version::WIN10)
gfx::win::InitializeDirectWrite();
}
#if defined(OS_WIN)
gfx::win::InitializeDirectWrite();
// Ensures anti-aliasing is activated.
BOOL antialiasing = TRUE;
BOOL result =
SystemParametersInfo(SPI_GETFONTSMOOTHING, 0, &antialiasing, 0);
ASSERT_NE(result, FALSE);
ASSERT_NE(antialiasing, FALSE)
<< "The test requires that fonts smoothing (anti-aliasing) is "
"activated. If this assert is failing you need to manually activate "
"the flag in your system fonts settings.";
#endif
}
private:
DISALLOW_COPY_AND_ASSIGN(LayoutProviderTest);
......@@ -58,23 +63,24 @@ class LayoutProviderTest : public testing::Test {
// Check legacy font sizes. No new code should be using these constants, but if
// these tests ever fail it probably means something in the old UI will have
// changed by mistake.
// Disabled since this relies on machine configuration. http://crbug.com/701241.
TEST_F(LayoutProviderTest, DISABLED_LegacyFontSizeConstants) {
TEST_F(LayoutProviderTest, LegacyFontSizeConstants) {
ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
gfx::FontList label_font = rb.GetFontListWithDelta(ui::kLabelFontSizeDelta);
EXPECT_EQ(12, label_font.GetFontSize());
#if defined(OS_WIN)
EXPECT_EQ(16, label_font.GetHeight());
EXPECT_EQ(13, label_font.GetBaseline());
#else
EXPECT_EQ(15, label_font.GetHeight());
EXPECT_EQ(12, label_font.GetBaseline());
#endif
EXPECT_EQ(12, label_font.GetFontSize());
EXPECT_EQ(9, label_font.GetCapHeight());
// Note some Windows bots report 11,13,11,9 for the above.
// TODO(tapted): Smoke them out and figure out why.
#if defined(OS_MACOSX)
EXPECT_EQ(10, label_font.GetExpectedTextWidth(1));
EXPECT_EQ(7, label_font.GetExpectedTextWidth(1));
#else
EXPECT_EQ(6, label_font.GetExpectedTextWidth(1));
// Some Windows bots may say 5.
#endif
gfx::FontList title_font = rb.GetFontListWithDelta(ui::kTitleFontSizeDelta);
......@@ -104,7 +110,7 @@ TEST_F(LayoutProviderTest, DISABLED_LegacyFontSizeConstants) {
if (base::mac::IsOS10_9()) {
EXPECT_EQ(7, title_font.GetExpectedTextWidth(1));
} else {
EXPECT_EQ(12, title_font.GetExpectedTextWidth(1));
EXPECT_EQ(8, title_font.GetExpectedTextWidth(1));
}
#else
EXPECT_EQ(8, title_font.GetExpectedTextWidth(1));
......@@ -142,8 +148,7 @@ TEST_F(LayoutProviderTest, DISABLED_LegacyFontSizeConstants) {
// platform starts returning 18 in a standard configuration then the
// TypographyProvider must add 4 instead. We do this so that Chrome adapts
// correctly to _non-standard_ system font configurations on user machines.
// Disabled since this relies on machine configuration. http://crbug.com/701241.
TEST_F(LayoutProviderTest, DISABLED_RequestFontBySize) {
TEST_F(LayoutProviderTest, RequestFontBySize) {
#if defined(OS_MACOSX)
constexpr int kBase = 13;
#else
......@@ -311,8 +316,7 @@ TEST_F(LayoutProviderTest, TypographyLineHeight) {
// Ensure that line heights reported in a default bot configuration match the
// Harmony spec. This test will only run if it detects that the current machine
// has the default OS configuration.
// Flaky. http://crbug.com/759870
TEST_F(LayoutProviderTest, DISABLED_ExplicitTypographyLineHeight) {
TEST_F(LayoutProviderTest, ExplicitTypographyLineHeight) {
std::unique_ptr<views::LayoutProvider> layout_provider =
ChromeLayoutProvider::CreateLayoutProvider();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment