List only the native display mode for the same size

We used to allow listing a couple of display modes that have
the same size as that of the native mode, even though the display
configurator will always choose the native mode as the best mode
for that size, regardless of the user choice.
This lead to user confusion, when they attempt to set the resolution
of their external display with a mode with a higher refresh rate
(e.g. 75 Hz) while the native mode has the same size but with lower
refresh rate (e.g. 60 Hz). The native mode will always be the one
selected, not what the user wanted.

BUG=931799
TEST=Expanded tests

Change-Id: I6c2391a259a26f7d77facafeaf5b336906600ea4
Reviewed-on: https://chromium-review.googlesource.com/c/1487696Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635344}

ui/display/manager/display_change_observer.cc

@@ -137,10 +137,6 @@ DisplayChangeObserver::GetExternalManagedDisplayModeList(
     }
   }
 
-  ManagedDisplayInfo::ManagedDisplayModeList display_mode_list;
-  for (const auto& display_mode_pair : display_mode_map)
-    display_mode_list.push_back(std::move(display_mode_pair.second));
-
   if (output.native_mode()) {
     const gfx::Size size = native_mode.size();
 
@@ -148,11 +144,18 @@ DisplayChangeObserver::GetExternalManagedDisplayModeList(
     DCHECK(it != display_mode_map.end())
         << "Native mode must be part of the mode list.";
 
-    // If the native mode was replaced re-add it.
+    // If the native mode was replaced (e.g. by a mode with similar size but
+    // higher refresh rate), we overwrite that mode with the native mode. The
+    // native mode will always be chosen as the best mode for this size (see
+    // DisplayConfigurator::FindDisplayModeMatchingSize()).
     if (!it->second.native())
-      display_mode_list.push_back(native_mode);
+      it->second = native_mode;
   }
 
+  ManagedDisplayInfo::ManagedDisplayModeList display_mode_list;
+  for (const auto& display_mode_pair : display_mode_map)
+    display_mode_list.push_back(std::move(display_mode_pair.second));
+
   return display_mode_list;
 }
 

ui/display/manager/display_change_observer_unittest.cc

@@ -69,6 +69,8 @@ TEST_P(DisplayChangeObserverTest, GetExternalManagedDisplayModeList) {
       FakeDisplaySnapshot::Builder()
           .SetId(123)
           .SetNativeMode(MakeDisplayMode(1920, 1200, false, 60))
+          // Same size as native mode but with higher refresh rate.
+          .AddMode(MakeDisplayMode(1920, 1200, false, 75))
           // All non-interlaced (as would be seen with different refresh rates).
           .AddMode(MakeDisplayMode(1920, 1080, false, 80))
           .AddMode(MakeDisplayMode(1920, 1080, false, 70))
@@ -93,7 +95,7 @@ TEST_P(DisplayChangeObserverTest, GetExternalManagedDisplayModeList) {
 
   const bool listing_all_modes = GetParam();
   if (listing_all_modes) {
-    ASSERT_EQ(12u, display_modes.size());
+    ASSERT_EQ(13u, display_modes.size());
     EXPECT_EQ("640x480", display_modes[0].size().ToString());
     EXPECT_TRUE(display_modes[0].is_interlaced());
     EXPECT_EQ(display_modes[0].refresh_rate(), 60);
@@ -135,6 +137,10 @@ TEST_P(DisplayChangeObserverTest, GetExternalManagedDisplayModeList) {
     EXPECT_EQ("1920x1200", display_modes[11].size().ToString());
     EXPECT_FALSE(display_modes[11].is_interlaced());
     EXPECT_EQ(display_modes[11].refresh_rate(), 60);
+
+    EXPECT_EQ("1920x1200", display_modes[12].size().ToString());
+    EXPECT_FALSE(display_modes[12].is_interlaced());
+    EXPECT_EQ(display_modes[12].refresh_rate(), 75);
   } else {
     ASSERT_EQ(6u, display_modes.size());
     EXPECT_EQ("640x480", display_modes[0].size().ToString());
@@ -222,16 +228,28 @@ TEST_P(DisplayChangeObserverTest,
   ManagedDisplayInfo::ManagedDisplayModeList display_modes =
       DisplayChangeObserver::GetExternalManagedDisplayModeList(
           *display_snapshot);
-  ASSERT_EQ(2u, display_modes.size());
-  EXPECT_EQ("1920x1080", display_modes[0].size().ToString());
-  EXPECT_FALSE(display_modes[0].is_interlaced());
-  EXPECT_FALSE(display_modes[0].native());
-  EXPECT_EQ(display_modes[0].refresh_rate(), 60);
-
-  EXPECT_EQ("1920x1080", display_modes[1].size().ToString());
-  EXPECT_TRUE(display_modes[1].is_interlaced());
-  EXPECT_TRUE(display_modes[1].native());
-  EXPECT_EQ(display_modes[1].refresh_rate(), 60);
+
+  const bool listing_all_modes = GetParam();
+
+  if (listing_all_modes) {
+    ASSERT_EQ(2u, display_modes.size());
+    EXPECT_EQ("1920x1080", display_modes[0].size().ToString());
+    EXPECT_FALSE(display_modes[0].is_interlaced());
+    EXPECT_FALSE(display_modes[0].native());
+    EXPECT_EQ(display_modes[0].refresh_rate(), 60);
+
+    EXPECT_EQ("1920x1080", display_modes[1].size().ToString());
+    EXPECT_TRUE(display_modes[1].is_interlaced());
+    EXPECT_TRUE(display_modes[1].native());
+    EXPECT_EQ(display_modes[1].refresh_rate(), 60);
+  } else {
+    // Only the native mode will be listed.
+    ASSERT_EQ(1u, display_modes.size());
+    EXPECT_EQ("1920x1080", display_modes[0].size().ToString());
+    EXPECT_TRUE(display_modes[0].is_interlaced());
+    EXPECT_TRUE(display_modes[0].native());
+    EXPECT_EQ(display_modes[0].refresh_rate(), 60);
+  }
 }
 
 INSTANTIATE_TEST_CASE_P(,