Introduce kDisplayChangeModal Flag

This change introduces the ash::kDisplayChangeModal feature and flag
to toggle the use of the display configuration change modal rather than
the notification. In addition, this change parametrizes the tests in
ResolutionNotificationControllerTest so that they are run with the flag
in both positions.

Change-Id: I9c52b618be412b8f3f41afe5bc3e256ac313932d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1979015
Auto-Submit: Bailey Berro <baileyberro@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Commit-Queue: Bailey Berro <baileyberro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732525}

ash/display/resolution_notification_controller_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "ash/display/resolution_notification_controller.h"
 
+#include "ash/public/cpp/ash_features.h"
 #include "ash/screen_util.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
@@ -13,6 +14,7 @@
 #include "base/bind.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/display/manager/display_manager.h"
 #include "ui/gfx/geometry/size.h"
@@ -22,7 +24,9 @@
 
 namespace ash {
 
-class ResolutionNotificationControllerTest : public AshTestBase {
+class ResolutionNotificationControllerTest
+    : public AshTestBase,
+      public ::testing::WithParamInterface<bool> {
  public:
   ResolutionNotificationControllerTest() : accept_count_(0) {}
 
@@ -49,6 +53,11 @@ class ResolutionNotificationControllerTest : public AshTestBase {
 
  protected:
   void SetUp() override {
+    if (GetParam())
+      scoped_feature_list_.InitAndEnableFeature(features::kDisplayChangeModal);
+    else
+      scoped_feature_list_.InitAndDisableFeature(features::kDisplayChangeModal);
+
     AshTestBase::SetUp();
     ResolutionNotificationController::SuppressTimerForTest();
   }
@@ -151,11 +160,13 @@ class ResolutionNotificationControllerTest : public AshTestBase {
 
   int accept_count_;
 
+  base::test::ScopedFeatureList scoped_feature_list_;
+
   DISALLOW_COPY_AND_ASSIGN(ResolutionNotificationControllerTest);
 };
 
 // Basic behaviors and verifies it doesn't cause crashes.
-TEST_F(ResolutionNotificationControllerTest, Basic) {
+TEST_P(ResolutionNotificationControllerTest, Basic) {
   UpdateDisplay("300x300#300x300%57|200x200%58,250x250#250x250%59|200x200%60");
   int64_t id2 = display_manager()->GetSecondaryDisplay().id();
   ASSERT_EQ(0, accept_count());
@@ -186,7 +197,7 @@ TEST_F(ResolutionNotificationControllerTest, Basic) {
 }
 
 // Check that notification is not shown when changes are forced by policy.
-TEST_F(ResolutionNotificationControllerTest, ForcedByPolicy) {
+TEST_P(ResolutionNotificationControllerTest, ForcedByPolicy) {
   UpdateDisplay("300x300#300x300%57|200x200%58,250x250#250x250%59|200x200%60");
   int64_t id2 = display_manager()->GetSecondaryDisplay().id();
   ASSERT_EQ(0, accept_count());
@@ -204,7 +215,7 @@ TEST_F(ResolutionNotificationControllerTest, ForcedByPolicy) {
   EXPECT_EQ(60.0, mode.refresh_rate());
 }
 
-TEST_F(ResolutionNotificationControllerTest, ClickMeansAccept) {
+TEST_P(ResolutionNotificationControllerTest, ClickMeansAccept) {
   UpdateDisplay("300x300#300x300%57|200x200%58,250x250#250x250%59|200x200%60");
   int64_t id2 = display_manager()->GetSecondaryDisplay().id();
   ASSERT_EQ(0, accept_count());
@@ -232,7 +243,7 @@ TEST_F(ResolutionNotificationControllerTest, ClickMeansAccept) {
   EXPECT_EQ(60.0, mode.refresh_rate());
 }
 
-TEST_F(ResolutionNotificationControllerTest, AcceptButton) {
+TEST_P(ResolutionNotificationControllerTest, AcceptButton) {
   UpdateDisplay("300x300#300x300%59|200x200%60");
   const display::Display& display =
       display::Screen::GetScreen()->GetPrimaryDisplay();
@@ -272,7 +283,7 @@ TEST_F(ResolutionNotificationControllerTest, AcceptButton) {
   EXPECT_EQ(59.0f, mode.refresh_rate());
 }
 
-TEST_F(ResolutionNotificationControllerTest, Close) {
+TEST_P(ResolutionNotificationControllerTest, Close) {
   UpdateDisplay("100x100,150x150#150x150%59|200x200%60");
   int64_t id2 = display_manager()->GetSecondaryDisplay().id();
   ASSERT_EQ(0, accept_count());
@@ -298,7 +309,7 @@ TEST_F(ResolutionNotificationControllerTest, Close) {
   EXPECT_EQ(1, accept_count());
 }
 
-TEST_F(ResolutionNotificationControllerTest, Timeout) {
+TEST_P(ResolutionNotificationControllerTest, Timeout) {
   UpdateDisplay("300x300#300x300%59|200x200%60");
   const display::Display& display =
       display::Screen::GetScreen()->GetPrimaryDisplay();
@@ -320,7 +331,7 @@ TEST_F(ResolutionNotificationControllerTest, Timeout) {
   EXPECT_EQ(59.0f, mode.refresh_rate());
 }
 
-TEST_F(ResolutionNotificationControllerTest, DisplayDisconnected) {
+TEST_P(ResolutionNotificationControllerTest, DisplayDisconnected) {
   UpdateDisplay(
       "300x300#300x300%56|200x200%57,"
       "200x200#250x250%58|200x200%59|100x100%60");
@@ -342,7 +353,7 @@ TEST_F(ResolutionNotificationControllerTest, DisplayDisconnected) {
 }
 
 // See http://crbug.com/869401 for details.
-TEST_F(ResolutionNotificationControllerTest, MultipleResolutionChange) {
+TEST_P(ResolutionNotificationControllerTest, MultipleResolutionChange) {
   UpdateDisplay(
       "300x300#300x300%56|200x200%57,"
       "250x250#250x250%58|200x200%59");
@@ -380,7 +391,7 @@ TEST_F(ResolutionNotificationControllerTest, MultipleResolutionChange) {
   EXPECT_EQ(58.0f, mode.refresh_rate());
 }
 
-TEST_F(ResolutionNotificationControllerTest, Fallback) {
+TEST_P(ResolutionNotificationControllerTest, Fallback) {
   UpdateDisplay(
       "300x300#300x300%56|200x200%57,"
       "250x250#250x250%58|220x220%59|200x200%60");
@@ -415,7 +426,7 @@ TEST_F(ResolutionNotificationControllerTest, Fallback) {
   EXPECT_EQ(58.0f, mode.refresh_rate());
 }
 
-TEST_F(ResolutionNotificationControllerTest, NoTimeoutInKioskMode) {
+TEST_P(ResolutionNotificationControllerTest, NoTimeoutInKioskMode) {
   // Login in as kiosk app.
   UserSession session;
   session.session_id = 1u;
@@ -434,4 +445,10 @@ TEST_F(ResolutionNotificationControllerTest, NoTimeoutInKioskMode) {
   EXPECT_FALSE(controller()->DoesNotificationTimeout());
 }
 
+// Parametrizes all tests to run with features::kDisplayChangeModal enabled and
+// disabled.
+INSTANTIATE_TEST_SUITE_P(All,
+                         ResolutionNotificationControllerTest,
+                         ::testing::Bool());
+
 }  // namespace ash

ash/public/cpp/ash_features.cc

@@ -18,6 +18,9 @@ const base::Feature kAllowAmbientEQ{"AllowAmbientEQ",
 const base::Feature kAutoNightLight{"AutoNightLight",
                                     base::FEATURE_DISABLED_BY_DEFAULT};
 
+const base::Feature kDisplayChangeModal{"DisplayChangeModal",
+                                        base::FEATURE_DISABLED_BY_DEFAULT};
+
 const base::Feature kDockedMagnifier{"DockedMagnifier",
                                      base::FEATURE_ENABLED_BY_DEFAULT};
 
@@ -224,5 +227,9 @@ bool IsHideShelfControlsInTabletModeEnabled() {
          IsDragFromShelfToHomeOrOverviewEnabled();
 }
 
+bool IsDisplayChangeModalEnabled() {
+  return base::FeatureList::IsEnabled(kDisplayChangeModal);
+}
+
 }  // namespace features
 }  // namespace ash

ash/public/cpp/ash_features.h

@@ -21,6 +21,9 @@ ASH_PUBLIC_EXPORT extern const base::Feature kAllowAmbientEQ;
 // certain devices.
 ASH_PUBLIC_EXPORT extern const base::Feature kAutoNightLight;
 
+// Enables a modal dialog when resolution or refresh rate change.
+ASH_PUBLIC_EXPORT extern const base::Feature kDisplayChangeModal;
+
 // Enables the docked (a.k.a. picture-in-picture) magnifier.
 // TODO(afakhry): Remove this after the feature is fully launched.
 // https://crbug.com/709824.
@@ -186,6 +189,8 @@ ASH_PUBLIC_EXPORT bool IsReduceDisplayNotificationsEnabled();
 
 ASH_PUBLIC_EXPORT bool IsHideShelfControlsInTabletModeEnabled();
 
+ASH_PUBLIC_EXPORT bool IsDisplayChangeModalEnabled();
+
 }  // namespace features
 }  // namespace ash
 

chrome/browser/about_flags.cc

@@ -4209,6 +4209,10 @@ const FeatureEntry kFeatureEntries[] = {
     {"enable-print-server-ui", flag_descriptions::kPrintServerUiName,
      flag_descriptions::kPrintServerUiDescription, kOsCrOS,
      FEATURE_VALUE_TYPE(features::kPrintServerUi)},
+
+    {"display-change-modal", flag_descriptions::kDisplayChangeModalName,
+     flag_descriptions::kDisplayChangeModalDescription, kOsCrOS,
+     FEATURE_VALUE_TYPE(ash::features::kDisplayChangeModal)},
 #endif  // OS_CHROMEOS
 
     {"autofill-off-no-server-data",

chrome/browser/flag-metadata.json

@@ -831,6 +831,11 @@
     "owners": [ "alemate" ],
     "expiry_milestone": 80
   },
+  {
+    "name": "display-change-modal",
+    "owners": [ "baileyberro", "zentaro" ],
+    "expiry_milestone": 85
+  },
   {
     "name": "dns-over-https",
     "owners": [ "dalyk", "doh-core@google.com" ],

chrome/browser/flag_descriptions.cc

@@ -3422,6 +3422,11 @@ const char kDisableExplicitDmaFencesDescription[] =
     "Always rely on implicit syncrhonization between GPU and display "
     "controller instead of using dma-fences explcitily when available.";
 
+const char kDisplayChangeModalName[] = "Enable display change modal";
+const char kDisplayChangeModalDescription[] =
+    "If enabled, a modal dialog will be shown when resolution or refresh rate "
+    "is changed rather than a notification.";
+
 const char kEnableUseHDRTransferFunctionName[] =
     "Enable using HDR transfer function";
 const char kEnableUseHDRTransferFunctionDescription[] =

chrome/browser/flag_descriptions.h

@@ -2034,6 +2034,9 @@ extern const char kDisableCryptAuthV1DeviceSyncDescription[];
 extern const char kDisableExplicitDmaFencesName[];
 extern const char kDisableExplicitDmaFencesDescription[];
 
+extern const char kDisplayChangeModalName[];
+extern const char kDisplayChangeModalDescription[];
+
 extern const char kEnableUseHDRTransferFunctionName[];
 extern const char kEnableUseHDRTransferFunctionDescription[];
 

tools/metrics/histograms/enums.xml

@@ -37126,6 +37126,7 @@ from previous Chrome versions.
   <int value="-1143007275" label="EnableNewStyleLauncher:disabled"/>
   <int value="-1142933895" label="ReduceDisplayNotifications:disabled"/>
   <int value="-1142034548" label="BuiltInModuleKvStorage:enabled"/>
+  <int value="-1141780291" label="DisplayChangeModal:disabled"/>
   <int value="-1137696948" label="enable-chromeos-account-manager"/>
   <int value="-1137442543" label="enable-slimming-paint"/>
   <int value="-1136627751" label="ignore-autocomplete-off-autofill"/>
@@ -39230,6 +39231,7 @@ from previous Chrome versions.
   <int value="1517863401" label="history-entry-requires-user-gesture"/>
   <int value="1526718531"
       label="DarkenWebsitesCheckboxInThemesSetting:enabled"/>
+  <int value="1527292264" label="DisplayChangeModal:enabled"/>
   <int value="1529979182" label="EnablePasswordSelection:enabled"/>
   <int value="1530113113" label="disable-pushstate-throttle"/>
   <int value="1530177325" label="LanguagesPreference:disabled"/>