Reland: Remove MaterialDesignController::GetMode() and old MD mode enum values.

De-parameterizes one test that doesn't care about the MD mode.

Removes a death test that won't consistently pass now and will soon be
completely meaningless.  (This was the source of the previous landing failure.)

Bug: none
Change-Id: I30fd5ed1ff06a14d757dbccdf8ac1d51cc0ac19e
Reviewed-on: https://chromium-review.googlesource.com/c/1300109Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603097}

ash/public/cpp/ash_layout_constants.cc

@@ -18,9 +18,8 @@ gfx::Size GetAshLayoutSize(AshLayoutSize size) {
   // |kBrowserMaximizedCaptionButtonHeight| should be kept in sync with those
   // for TAB_HEIGHT in // chrome/browser/ui/layout_constants.cc.
   // TODO: Ideally these values should be obtained from a common location.
-  constexpr int kBrowserMaximizedCaptionButtonHeight[] = {29, 33, 41, 34, 41};
-  const int mode = ui::MaterialDesignController::GetMode();
-  int height = kBrowserMaximizedCaptionButtonHeight[mode];
+  const bool touch = ui::MaterialDesignController::IsTouchOptimizedUiEnabled();
+  int height = touch ? 41 : 34;
   if (size == AshLayoutSize::kBrowserCaptionRestored)
     height += 8;  // Restored window titlebars are 8 DIP taller than maximized.
   return gfx::Size(kButtonWidth, height);

chrome/browser/extensions/extension_action_icon_factory_unittest.cc

@@ -25,9 +25,7 @@
 #include "extensions/common/image_util.h"
 #include "skia/ext/image_operations.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
-#include "ui/base/test/material_design_controller_test_api.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/skia_util.h"
@@ -82,7 +80,7 @@ gfx::Image LoadIcon(const std::string& filename) {
 }
 
 class ExtensionActionIconFactoryTest
-    : public testing::TestWithParam<ui::MaterialDesignController::Mode>,
+    : public testing::Test,
       public ExtensionActionIconFactory::Observer {
  public:
   ExtensionActionIconFactoryTest() : quit_in_icon_updated_(false) {}
@@ -134,12 +132,9 @@ class ExtensionActionIconFactoryTest
     extension_service_ = static_cast<extensions::TestExtensionSystem*>(
         extensions::ExtensionSystem::Get(profile_.get()))->
         CreateExtensionService(&command_line, base::FilePath(), false);
-    material_design_state_.reset(
-        new ui::test::MaterialDesignControllerTestAPI(GetParam()));
   }
 
   void TearDown() override {
-    material_design_state_.reset();
     profile_.reset();  // Get all DeleteSoon calls sent to ui_loop_.
     base::RunLoop().RunUntilIdle();
   }
@@ -166,8 +161,6 @@ class ExtensionActionIconFactoryTest
   bool quit_in_icon_updated_;
   std::unique_ptr<TestingProfile> profile_;
   ExtensionService* extension_service_;
-  std::unique_ptr<ui::test::MaterialDesignControllerTestAPI>
-      material_design_state_;
 
 #if defined OS_CHROMEOS
   chromeos::ScopedCrosSettingsTestHelper cros_settings_test_helper_;
@@ -177,15 +170,9 @@ class ExtensionActionIconFactoryTest
   DISALLOW_COPY_AND_ASSIGN(ExtensionActionIconFactoryTest);
 };
 
-INSTANTIATE_TEST_CASE_P(
-    ExtensionActionIconFactoryTest_MaterialDesign,
-    ExtensionActionIconFactoryTest,
-    testing::Values(ui::MaterialDesignController::MATERIAL_NORMAL,
-                    ui::MaterialDesignController::MATERIAL_HYBRID));
-
 // If there is no default icon, and the icon has not been set using |SetIcon|,
 // the factory should return the placeholder icon.
-TEST_P(ExtensionActionIconFactoryTest, NoIcons) {
+TEST_F(ExtensionActionIconFactoryTest, NoIcons) {
   // Load an extension that has browser action without default icon set in the
   // manifest and does not call |SetIcon| by default.
   scoped_refptr<Extension> extension(
@@ -209,7 +196,7 @@ TEST_P(ExtensionActionIconFactoryTest, NoIcons) {
 
 // If the explicitly-set icon is invisible, |ExtensionAction::GetIcon| should
 // return the placeholder icon.
-TEST_P(ExtensionActionIconFactoryTest, InvisibleIcon) {
+TEST_F(ExtensionActionIconFactoryTest, InvisibleIcon) {
   // Load an extension that has browser action with a default icon set in the
   // manifest, but that icon is not sufficiently visible.
   scoped_refptr<Extension> extension(
@@ -246,7 +233,7 @@ TEST_P(ExtensionActionIconFactoryTest, InvisibleIcon) {
 
 // If the icon has been set using |SetIcon|, the factory should return that
 // icon.
-TEST_P(ExtensionActionIconFactoryTest, AfterSetIcon) {
+TEST_F(ExtensionActionIconFactoryTest, AfterSetIcon) {
   // Load an extension that has browser action without default icon set in the
   // manifest and does not call |SetIcon| by default (but has an browser action
   // icon resource).
@@ -285,7 +272,7 @@ TEST_P(ExtensionActionIconFactoryTest, AfterSetIcon) {
 
 // If there is a default icon, and the icon has not been set using |SetIcon|,
 // the factory should return the default icon.
-TEST_P(ExtensionActionIconFactoryTest, DefaultIcon) {
+TEST_F(ExtensionActionIconFactoryTest, DefaultIcon) {
   // Load an extension that has browser action without default icon set in the
   // manifest and does not call |SetIcon| by default (but has an browser action
   // icon resource).

chrome/browser/flag_descriptions.cc

@@ -1836,12 +1836,10 @@ const char kTintGlCompositedContentDescription[] =
 
 const char kTopChromeMd[] = "UI Layout for the browser's top chrome";
 const char kTopChromeMdDescription[] =
-    "Toggles between 1) Material Design refresh, 2) Touchable Material Design "
-    "refresh and 3) Switching automatically between Touchable and "
-    "non-Touchable modes.";
-const char kTopChromeMdMaterialRefresh[] = "Refresh";
-const char kTopChromeMdMaterialRefreshTouchOptimized[] = "Touchable Refresh";
-const char kTopChromeMdMaterialRefreshDynamic[] = "Dynamic Refresh";
+    "Toggles between normal, touch-optimized, and dynamically switching UI.";
+const char kTopChromeMdMaterialRefresh[] = "Normal";
+const char kTopChromeMdMaterialRefreshTouchOptimized[] = "Touch-optimized";
+const char kTopChromeMdMaterialRefreshDynamic[] = "Dynamically switching";
 
 const char kThreadedScrollingName[] = "Threaded scrolling";
 const char kThreadedScrollingDescription[] =

chrome/browser/flag_descriptions.h

@@ -1113,10 +1113,6 @@ extern const char kTintGlCompositedContentDescription[];
 
 extern const char kTopChromeMd[];
 extern const char kTopChromeMdDescription[];
-extern const char kTopChromeMdMaterial[];
-extern const char kTopChromeMdMaterialAuto[];
-extern const char kTopChromeMdMaterialHybrid[];
-extern const char kTopChromeMdMaterialTouchOptimized[];
 extern const char kTopChromeMdMaterialRefresh[];
 extern const char kTopChromeMdMaterialRefreshTouchOptimized[];
 extern const char kTopChromeMdMaterialRefreshDynamic[];

chrome/browser/ui/extensions/extension_action_view_controller_unittest.cc

@@ -410,10 +410,7 @@ TEST_P(ExtensionActionViewControllerGrayscaleTest,
 INSTANTIATE_TEST_CASE_P(
     ,
     ExtensionActionViewControllerGrayscaleTest,
-    testing::Values(ui::MaterialDesignController::MATERIAL_NORMAL,
-                    ui::MaterialDesignController::MATERIAL_HYBRID,
-                    ui::MaterialDesignController::MATERIAL_TOUCH_OPTIMIZED,
-                    ui::MaterialDesignController::MATERIAL_REFRESH,
+    testing::Values(ui::MaterialDesignController::MATERIAL_REFRESH,
                     ui::MaterialDesignController::MATERIAL_TOUCH_REFRESH));
 
 TEST_P(ToolbarActionsBarUnitTest, RuntimeHostsTooltip) {

ui/base/material_design/material_design_controller.cc

@@ -36,15 +36,17 @@ namespace ui {
 namespace {
 
 #if defined(OS_WIN)
+bool IsTabletMode() {
+  return base::win::IsWindows10TabletMode(
+      gfx::SingletonHwnd::GetInstance()->hwnd());
+}
+
 void TabletModeWatcherWinProc(HWND hwnd,
                               UINT message,
                               WPARAM wparam,
                               LPARAM lparam) {
-  if (message == WM_SETTINGCHANGE) {
-    MaterialDesignController::OnTabletModeToggled(
-        base::win::IsWindows10TabletMode(
-            gfx::SingletonHwnd::GetInstance()->hwnd()));
-  }
+  if (message == WM_SETTINGCHANGE)
+    MaterialDesignController::OnTabletModeToggled(IsTabletMode());
 }
 #endif  // defined(OS_WIN)
 
@@ -53,34 +55,43 @@ void TabletModeWatcherWinProc(HWND hwnd,
 bool MaterialDesignController::is_mode_initialized_ = false;
 
 MaterialDesignController::Mode MaterialDesignController::mode_ =
-    MaterialDesignController::MATERIAL_NORMAL;
+    MaterialDesignController::MATERIAL_REFRESH;
 
 bool MaterialDesignController::is_refresh_dynamic_ui_ = false;
 
 // static
 void MaterialDesignController::Initialize() {
   TRACE_EVENT0("startup", "MaterialDesignController::InitializeMode");
-  CHECK(!is_mode_initialized_);
+  DCHECK(!is_mode_initialized_);
 
   base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
   const std::string switch_value =
       command_line->GetSwitchValueASCII(switches::kTopChromeMD);
+  bool touch =
+      switch_value == switches::kTopChromeMDMaterialRefreshTouchOptimized;
+  is_refresh_dynamic_ui_ = false;
 
-  if (switch_value == switches::kTopChromeMDMaterialRefresh) {
-    SetMode(MATERIAL_REFRESH);
-  } else if (switch_value ==
-             switches::kTopChromeMDMaterialRefreshTouchOptimized) {
-    SetMode(MATERIAL_TOUCH_REFRESH);
-  } else if (switch_value == switches::kTopChromeMDMaterialRefreshDynamic) {
-    MaybeSetDynamicRefreshMode();
-  } else {
-    if (!switch_value.empty()) {
-      LOG(ERROR) << "Invalid value='" << switch_value
-                 << "' for command line switch '" << switches::kTopChromeMD
-                 << "'.";
+  // When the mode is not explicitly forced, platforms vary as to the default
+  // behavior.
+  if (!touch && (switch_value != switches::kTopChromeMDMaterialRefresh)) {
+#if defined(OS_CHROMEOS)
+    // TabletModeClient's default state is in non-tablet mode.
+    is_refresh_dynamic_ui_ = true;
+#elif defined(OS_WIN)
+    if (base::win::GetVersion() >= base::win::VERSION_WIN10) {
+      // Win 10+ uses dynamic mode by default and checks the current tablet mode
+      // state to determine whether to start in touch mode.
+      is_refresh_dynamic_ui_ = true;
+      if (base::MessageLoopForUI::IsCurrent()) {
+        MaterialDesignController::GetInstance()->singleton_hwnd_observer_ =
+            std::make_unique<gfx::SingletonHwndObserver>(
+                base::BindRepeating(TabletModeWatcherWinProc));
+        touch = IsTabletMode();
+      }
     }
-    MaybeSetDynamicRefreshMode();
+#endif
   }
+  SetMode(touch ? MATERIAL_TOUCH_REFRESH : MATERIAL_REFRESH);
 
   // Ideally, there would be a more general, "initialize random stuff here"
   // function into which these things and a call to this function can be placed.
@@ -95,16 +106,10 @@ void MaterialDesignController::Initialize() {
   }
 }
 
-// static
-MaterialDesignController::Mode MaterialDesignController::GetMode() {
-  CHECK(is_mode_initialized_);
-  return mode_;
-}
-
 // static
 bool MaterialDesignController::IsTouchOptimizedUiEnabled() {
-  return GetMode() == MATERIAL_TOUCH_OPTIMIZED ||
-         GetMode() == MATERIAL_TOUCH_REFRESH;
+  DCHECK(is_mode_initialized_);
+  return mode_ == MATERIAL_TOUCH_REFRESH;
 }
 
 // static
@@ -146,30 +151,4 @@ void MaterialDesignController::SetMode(MaterialDesignController::Mode mode) {
   }
 }
 
-// static
-void MaterialDesignController::MaybeSetDynamicRefreshMode() {
-#if defined(OS_CHROMEOS)
-  // TabletModeClient's default state is in non-tablet mode.
-  SetMode(MATERIAL_REFRESH);
-  is_refresh_dynamic_ui_ = true;
-#elif defined(OS_WIN)
-  if (base::win::GetVersion() < base::win::VERSION_WIN10 ||
-      !base::MessageLoopForUI::IsCurrent()) {
-    SetMode(MATERIAL_REFRESH);
-    return;
-  }
-
-  MaterialDesignController::GetInstance()->singleton_hwnd_observer_ =
-      std::make_unique<gfx::SingletonHwndObserver>(
-          base::BindRepeating(TabletModeWatcherWinProc));
-  SetMode(base::win::IsWindows10TabletMode(
-              gfx::SingletonHwnd::GetInstance()->hwnd())
-              ? MATERIAL_TOUCH_REFRESH
-              : MATERIAL_REFRESH);
-  is_refresh_dynamic_ui_ = true;
-#else
-  SetMode(MATERIAL_REFRESH);
-#endif
-}
-
 }  // namespace ui

ui/base/material_design/material_design_controller.h

@@ -30,27 +30,17 @@ class MaterialDesignControllerTestAPI;
 // Central controller to handle material design modes.
 class UI_BASE_EXPORT MaterialDesignController {
  public:
-  // The different material design modes. The order cannot be changed without
-  // updating references as these are used as array indices.
+  // The different material design modes.
   enum Mode {
-    // Basic material design.
-    MATERIAL_NORMAL = 0,
-    // Material design targeted at mouse/touch hybrid devices.
-    MATERIAL_HYBRID = 1,
-    // Material design that is more optimized for touch devices.
-    MATERIAL_TOUCH_OPTIMIZED = 2,
     // Material Refresh design targeted at mouse devices.
-    MATERIAL_REFRESH = 3,
+    MATERIAL_REFRESH,
     // Material Refresh design optimized for touch devices.
-    MATERIAL_TOUCH_REFRESH = 4,
+    MATERIAL_TOUCH_REFRESH,
   };
 
   // Initializes |mode_|. Must be called before checking |mode_|.
   static void Initialize();
 
-  // Get the current Mode that should be used by the system.
-  static Mode GetMode();
-
   // Returns true if the touch-optimized UI material design mode is enabled.
   static bool IsTouchOptimizedUiEnabled();
 
@@ -70,7 +60,6 @@ class UI_BASE_EXPORT MaterialDesignController {
   friend class test::MaterialDesignControllerTestAPI;
 
   MaterialDesignController();
-
   ~MaterialDesignController() = delete;
 
   // Resets the initialization state to uninitialized. To be used by tests to
@@ -81,11 +70,6 @@ class UI_BASE_EXPORT MaterialDesignController {
   // used by tests to directly set the mode.
   static void SetMode(Mode mode);
 
-  // Sets |is_refresh_dynamic_ui_| to true and initializes the |mode_|. If the
-  // platform does not support tablet mode switching, |mode_| will be set to
-  // MATERIAL_REFRESH and |is_refresh_dynamic_ui_| will be left untouched.
-  static void MaybeSetDynamicRefreshMode();
-
   // Tracks whether |mode_| has been initialized. This is necessary to avoid
   // checking the |mode_| early in initialization before a call to Initialize().
   // Tests can use it to reset the state back to a clean state during tear down.

ui/base/material_design/material_design_controller_unittest.cc

@@ -14,12 +14,6 @@
 
 namespace ui {
 
-TEST(MaterialDesignControllerDeathTest, CrashesWithoutInitialization) {
-  ASSERT_FALSE(MaterialDesignController::is_mode_initialized());
-  EXPECT_DEATH_IF_SUPPORTED(
-      MaterialDesignController::IsTouchOptimizedUiEnabled(), "");
-}
-
 namespace {
 
 // Test fixture for the MaterialDesignController class.
@@ -139,9 +133,9 @@ TEST(MaterialDesignControllerObserver, InitializationOnMdModeChanged) {
 TEST(MaterialDesignControllerObserver, TabletOnMdModeChanged) {
   // Verifies that the MaterialDesignControllerObserver gets called back when
   // the tablet mode toggles.
+  MaterialDesignController::Initialize();
   test::MaterialDesignControllerTestAPI::SetDynamicRefreshUi(true);
 
-  MaterialDesignController::Initialize();
   TestObserver tablet_enabled_observer;
   MaterialDesignController::GetInstance()->AddObserver(
       &tablet_enabled_observer);
@@ -166,13 +160,13 @@ TEST(MaterialDesignControllerObserver, TabletOnMdModeChanged) {
 
   EXPECT_TRUE(tablet_disabled_observer.on_md_mode_changed_called());
 
-  test::MaterialDesignControllerTestAPI::Uninitialize();
   MaterialDesignController::GetInstance()->RemoveObserver(
       &tablet_disabled_observer);
   MaterialDesignController::GetInstance()->RemoveObserver(
       &tablet_enabled_observer);
 
   test::MaterialDesignControllerTestAPI::SetDynamicRefreshUi(false);
+  test::MaterialDesignControllerTestAPI::Uninitialize();
 }
 
 }  // namespace ui