Don't remember best resolution.

 It's safe not to rely on the state to use best resolution.
Reject unsupported resolution.

BUG=none
TEST=covered by test

Review URL: https://chromiumcodereview.appspot.com/22662005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@217218 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 136 insertions, 43 deletions

diff --git a/ash/display/display_info.cc b/ash/display/display_info.cc
index 8f71e6d..5e89a0d 100644
--- a/ash/display/display_info.cc
+++ b/ash/display/display_info.cc
@@ -96,6 +96,20 @@ DisplayInfo DisplayInfo::CreateFromSpecWithID(const std::string& spec,
              &device_scale_factor) >= 4) {
     bounds_in_pixel.SetRect(x, y, width, height);
   }
+
+  std::vector<Resolution> resolutions;
+  if (Tokenize(main_spec, "#", &parts) == 2) {
+    main_spec = parts[0];
+    std::string resolution_list = parts[1];
+    count = Tokenize(resolution_list, "|", &parts);
+    for (size_t i = 0; i < count; ++i) {
+      std::string resolution = parts[i];
+      int width, height;
+      if (sscanf(resolution.c_str(), "%dx%d", &width, &height) == 2)
+        resolutions.push_back(Resolution(gfx::Size(width, height), false));
+    }
+  }
+
   if (id == gfx::Display::kInvalidDisplayID)
     id = synthesized_display_id++;
   DisplayInfo display_info(
@@ -104,6 +118,7 @@ DisplayInfo DisplayInfo::CreateFromSpecWithID(const std::string& spec,
   display_info.set_rotation(rotation);
   display_info.set_ui_scale(ui_scale);
   display_info.SetBounds(bounds_in_pixel);
+  display_info.set_resolutions(resolutions);
 
   // To test the overscan, it creates the default 5% overscan.
   if (has_overscan) {
diff --git a/ash/display/display_info.h b/ash/display/display_info.h
index 899dd15..17ba5f6 100644
--- a/ash/display/display_info.h
+++ b/ash/display/display_info.h
@@ -36,7 +36,8 @@ class ASH_EXPORT DisplayInfo {
   // whose size is 1440x800 at the location (100, 200) in host coordinates.
   // The format is
   //
-  // [origin-]widthxheight[*device_scale_factor][/<properties>][@ui-scale]
+  // [origin-]widthxheight[*device_scale_factor][#resolutions list]
+  //     [/<properties>][@ui-scale]
   //
   // where [] are optional:
   // - |origin| is given in x+y- format.
@@ -46,6 +47,8 @@ class ASH_EXPORT DisplayInfo {
   //   (to the 'r'ight) 'u' is 180 degrees ('u'pside-down) and 'l' is
   //   270 degrees (to the 'l'eft).
   // - ui-scale is floating value, e.g. @1.5 or @1.25.
+  // - |resolution list| is the list of size that is given in
+  //   |width x height| separated by '|'.
   //
   // A couple of examples:
   // "100x100"
@@ -61,6 +64,9 @@ class ASH_EXPORT DisplayInfo {
   // "10+20-300x200/u@1.5"
   //      300x200 window at 10,20 origin. 1x device scale factor.
   //      no overscan. flipped upside-down (180 degree) and 1.5 ui scale.
+  // "200x100#300x200|200x100|100x100"
+  //      200x100 window at 0,0 origin, with 3 possible resolutions,
+  //      300x200, 200x100 and 100x100.
   static DisplayInfo CreateFromSpec(const std::string& spec);
 
   // Creates a DisplayInfo from string spec using given |id|.
diff --git a/ash/display/display_info_unittest.cc b/ash/display/display_info_unittest.cc
index d425046..4e9768d 100644
--- a/ash/display/display_info_unittest.cc
+++ b/ash/display/display_info_unittest.cc
@@ -43,6 +43,14 @@ TEST_F(DisplayInfoTest, CreateFromSpec) {
   EXPECT_EQ("10,20 300x400", info.bounds_in_pixel().ToString());
   EXPECT_EQ(gfx::Display::ROTATE_270, info.rotation());
   EXPECT_EQ(1.5f, info.ui_scale());
+
+  info = DisplayInfo::CreateFromSpecWithID(
+      "200x200#300x200|200x200|100x100", 10);
+  EXPECT_EQ("0,0 200x200", info.bounds_in_pixel().ToString());
+  EXPECT_EQ(3u, info.resolutions().size());
+  EXPECT_EQ("300x200", info.resolutions()[0].size.ToString());
+  EXPECT_EQ("200x200", info.resolutions()[1].size.ToString());
+  EXPECT_EQ("100x100", info.resolutions()[2].size.ToString());
 }
 
 }  // namespace internal
diff --git a/ash/display/display_manager.cc b/ash/display/display_manager.cc
index 5943f99..b87fed0 100644
--- a/ash/display/display_manager.cc
+++ b/ash/display/display_manager.cc
@@ -72,6 +72,14 @@ struct DisplayInfoSortFunctor {
   }
 };
 
+struct ResolutionMatcher {
+  ResolutionMatcher(const gfx::Size& size) : size(size) {}
+  bool operator()(const Resolution& resolution) {
+    return resolution.size == size;
+  }
+  gfx::Size size;
+};
+
 struct ScaleComparator {
   ScaleComparator(float s) : scale(s) {}
 
@@ -326,7 +334,23 @@ void DisplayManager::SetDisplayResolution(int64 display_id,
   DCHECK_NE(gfx::Display::InternalDisplayId(), display_id);
   if (gfx::Display::InternalDisplayId() == display_id)
     return;
-  resolutions_[display_id] = resolution;
+  const DisplayInfo& display_info = GetDisplayInfo(display_id);
+  const std::vector<Resolution>& resolutions = display_info.resolutions();
+  DCHECK_NE(0u, resolutions.size());
+  std::vector<Resolution>::const_iterator iter =
+      std::find_if(resolutions.begin(),
+                   resolutions.end(),
+                   ResolutionMatcher(resolution));
+  if (iter == resolutions.end()) {
+    LOG(WARNING) << "Unsupported resolution was requested:"
+                 << resolution.ToString();
+    return;
+  } else if (iter == resolutions.begin()) {
+    // The best resolution was set, so forget it.
+    resolutions_.erase(display_id);
+  } else {
+    resolutions_[display_id] = resolution;
+  }
 #if defined(OS_CHROMEOS) && defined(USE_X11)
   if (base::chromeos::IsRunningOnChromeOS())
     Shell::GetInstance()->output_configurator()->ScheduleConfigureOutputs();
@@ -733,7 +757,7 @@ void DisplayManager::SetMirrorMode(bool mirrored) {
 void DisplayManager::AddRemoveDisplay() {
   DCHECK(!displays_.empty());
   std::vector<DisplayInfo> new_display_info_list;
-  DisplayInfo first_display = GetDisplayInfo(displays_[0].id());
+  const DisplayInfo& first_display = GetDisplayInfo(displays_[0].id());
   new_display_info_list.push_back(first_display);
   // Add if there is only one display connected.
   if (num_connected_displays() == 1) {
diff --git a/ash/display/display_manager_unittest.cc b/ash/display/display_manager_unittest.cc
index 33ca06e..a188f9b 100644
--- a/ash/display/display_manager_unittest.cc
+++ b/ash/display/display_manager_unittest.cc
@@ -717,6 +717,42 @@ TEST_F(DisplayManagerTest, NativeDisplaysChangedAfterPrimaryChange) {
   EXPECT_EQ("0,0 100x100", GetDisplayForId(10).bounds().ToString());
 }
 
+TEST_F(DisplayManagerTest, DontRememberBestResolution) {
+  int display_id = 1000;
+  DisplayInfo native_display_info =
+      CreateDisplayInfo(display_id, gfx::Rect(0, 0, 1000, 500));
+  std::vector<Resolution> resolutions;
+  resolutions.push_back(Resolution(gfx::Size(1000, 500), false));
+  resolutions.push_back(Resolution(gfx::Size(800, 300), false));
+  resolutions.push_back(Resolution(gfx::Size(400, 500), false));
+
+  native_display_info.set_resolutions(resolutions);
+
+  std::vector<DisplayInfo> display_info_list;
+  display_info_list.push_back(native_display_info);
+  display_manager()->OnNativeDisplaysChanged(display_info_list);
+
+  gfx::Size selected;
+  EXPECT_FALSE(display_manager()->GetSelectedResolutionForDisplayId(
+      display_id, &selected));
+
+  // Unsupported resolution.
+  display_manager()->SetDisplayResolution(display_id, gfx::Size(800, 4000));
+  EXPECT_FALSE(display_manager()->GetSelectedResolutionForDisplayId(
+      display_id, &selected));
+
+  // Supported resolution.
+  display_manager()->SetDisplayResolution(display_id, gfx::Size(800, 300));
+  EXPECT_TRUE(display_manager()->GetSelectedResolutionForDisplayId(
+      display_id, &selected));
+  EXPECT_EQ("800x300", selected.ToString());
+
+  // Best resolution.
+  display_manager()->SetDisplayResolution(display_id, gfx::Size(1000, 500));
+  EXPECT_FALSE(display_manager()->GetSelectedResolutionForDisplayId(
+      display_id, &selected));
+}
+
 TEST_F(DisplayManagerTest, Rotate) {
   if (!SupportsMultipleDisplays())
     return;
diff --git a/ash/display/resolution_notification_controller_unittest.cc b/ash/display/resolution_notification_controller_unittest.cc
index bdb6f12..1ae1750 100644
--- a/ash/display/resolution_notification_controller_unittest.cc
+++ b/ash/display/resolution_notification_controller_unittest.cc
@@ -43,16 +43,18 @@ class ResolutionNotificationControllerTest : public ash::test::AshTestBase {
 
     // OnConfigurationChanged event won't be emitted in the test environment,
     // so invoke UpdateDisplay() to emit that event explicitly.
-    std::string display_spec;
+    std::vector<DisplayInfo> info_list;
     for (size_t i = 0; i < display_manager->GetNumDisplays(); ++i) {
-      if (i > 0)
-        display_spec.append(",");
       int64 id = display_manager->GetDisplayAt(i).id();
-      gfx::Size size = (display.id() == id) ?
-          new_resolution : display_manager->GetDisplayInfo(id).size_in_pixel();
-      display_spec.append(size.ToString());
+      DisplayInfo info = display_manager->GetDisplayInfo(id);
+      if (display.id() == id) {
+        gfx::Rect bounds = info.bounds_in_pixel();
+        bounds.set_size(new_resolution);
+        info.SetBounds(bounds);
+      }
+      info_list.push_back(info);
     }
-    UpdateDisplay(display_spec);
+    display_manager->OnNativeDisplaysChanged(info_list);
     RunAllPendingInMessageLoop();
   }
 
@@ -99,7 +101,7 @@ TEST_F(ResolutionNotificationControllerTest, Basic) {
   if (!SupportsMultipleDisplays())
     return;
 
-  UpdateDisplay("100x100,150x150");
+  UpdateDisplay("300x300#300x300|200x200,250x250#250x250|200x200");
   int64 id2 = ash::ScreenAsh::GetSecondaryDisplay().id();
   ash::internal::DisplayManager* display_manager =
       ash::Shell::GetInstance()->display_manager();
@@ -116,21 +118,20 @@ TEST_F(ResolutionNotificationControllerTest, Basic) {
       display_manager->GetSelectedResolutionForDisplayId(id2, &resolution));
   EXPECT_EQ("200x200", resolution.ToString());
 
-  // Click the revert button, which reverts the resolution.
+  // Click the revert button, which reverts to the best resolution.
   ClickOnNotificationButton(0);
   RunAllPendingInMessageLoop();
   EXPECT_FALSE(IsNotificationVisible());
   EXPECT_EQ(0, accept_count());
-  EXPECT_TRUE(
+  EXPECT_FALSE(
       display_manager->GetSelectedResolutionForDisplayId(id2, &resolution));
-  EXPECT_EQ("150x150", resolution.ToString());
 }
 
 TEST_F(ResolutionNotificationControllerTest, ClickMeansAccept) {
   if (!SupportsMultipleDisplays())
     return;
 
-  UpdateDisplay("100x100,150x150");
+  UpdateDisplay("300x300#300x300|200x200,250x250#250x250|200x200");
   int64 id2 = ash::ScreenAsh::GetSecondaryDisplay().id();
   ash::internal::DisplayManager* display_manager =
       ash::Shell::GetInstance()->display_manager();
@@ -164,7 +165,7 @@ TEST_F(ResolutionNotificationControllerTest, AcceptButton) {
   ash::internal::DisplayManager* display_manager =
       ash::Shell::GetInstance()->display_manager();
 
-  UpdateDisplay("100x100");
+  UpdateDisplay("300x300#300x300|200x200");
   const gfx::Display& display = ash::Shell::GetScreen()->GetPrimaryDisplay();
   SetDisplayResolutionAndNotify(display, gfx::Size(200, 200));
   EXPECT_TRUE(IsNotificationVisible());
@@ -181,7 +182,7 @@ TEST_F(ResolutionNotificationControllerTest, AcceptButton) {
   EXPECT_EQ("200x200", resolution.ToString());
 
   // In that case the second button is revert.
-  UpdateDisplay("100x100");
+  UpdateDisplay("300x300#300x300|200x200");
   SetDisplayResolutionAndNotify(display, gfx::Size(200, 200));
   EXPECT_TRUE(IsNotificationVisible());
 
@@ -189,16 +190,15 @@ TEST_F(ResolutionNotificationControllerTest, AcceptButton) {
   ClickOnNotificationButton(1);
   EXPECT_FALSE(IsNotificationVisible());
   EXPECT_EQ(1, accept_count());
-  EXPECT_TRUE(display_manager->GetSelectedResolutionForDisplayId(
+  EXPECT_FALSE(display_manager->GetSelectedResolutionForDisplayId(
       display.id(), &resolution));
-  EXPECT_EQ("100x100", resolution.ToString());
 }
 
 TEST_F(ResolutionNotificationControllerTest, Timeout) {
   if (!SupportsMultipleDisplays())
     return;
 
-  UpdateDisplay("100x100");
+  UpdateDisplay("300x300#300x300|200x200");
   const gfx::Display& display = ash::Shell::GetScreen()->GetPrimaryDisplay();
   SetDisplayResolutionAndNotify(display, gfx::Size(200, 200));
 
@@ -213,39 +213,38 @@ TEST_F(ResolutionNotificationControllerTest, Timeout) {
   gfx::Size resolution;
   ash::internal::DisplayManager* display_manager =
       ash::Shell::GetInstance()->display_manager();
-  EXPECT_TRUE(display_manager->GetSelectedResolutionForDisplayId(
+  EXPECT_FALSE(display_manager->GetSelectedResolutionForDisplayId(
       display.id(), &resolution));
-  EXPECT_EQ("100x100", resolution.ToString());
 }
 
 TEST_F(ResolutionNotificationControllerTest, DisplayDisconnected) {
   if (!SupportsMultipleDisplays())
     return;
 
-  UpdateDisplay("100x100,150x150");
+  UpdateDisplay("300x300#300x300|200x200,200x200#250x250|200x200|100x100");
   int64 id2 = ash::ScreenAsh::GetSecondaryDisplay().id();
   ash::internal::DisplayManager* display_manager =
       ash::Shell::GetInstance()->display_manager();
   SetDisplayResolutionAndNotify(
-      ScreenAsh::GetSecondaryDisplay(), gfx::Size(200, 200));
+      ScreenAsh::GetSecondaryDisplay(), gfx::Size(100, 100));
   ASSERT_TRUE(IsNotificationVisible());
 
   // Disconnects the secondary display and verifies it doesn't cause crashes.
-  UpdateDisplay("100x100");
+  UpdateDisplay("300x300#300x300|200x200");
   RunAllPendingInMessageLoop();
   EXPECT_FALSE(IsNotificationVisible());
   EXPECT_EQ(0, accept_count());
   gfx::Size resolution;
   EXPECT_TRUE(
       display_manager->GetSelectedResolutionForDisplayId(id2, &resolution));
-  EXPECT_EQ("150x150", resolution.ToString());
+  EXPECT_EQ("200x200", resolution.ToString());
 }
 
 TEST_F(ResolutionNotificationControllerTest, MultipleResolutionChange) {
   if (!SupportsMultipleDisplays())
     return;
 
-  UpdateDisplay("100x100,150x150");
+  UpdateDisplay("300x300#300x300|200x200,250x250#250x250|200x200");
   int64 id2 = ash::ScreenAsh::GetSecondaryDisplay().id();
   ash::internal::DisplayManager* display_manager =
       ash::Shell::GetInstance()->display_manager();
@@ -263,9 +262,8 @@ TEST_F(ResolutionNotificationControllerTest, MultipleResolutionChange) {
   // visible.
   SetDisplayResolutionAndNotify(
       ScreenAsh::GetSecondaryDisplay(), gfx::Size(250, 250));
-  EXPECT_TRUE(
+  EXPECT_FALSE(
       display_manager->GetSelectedResolutionForDisplayId(id2, &resolution));
-  EXPECT_EQ("250x250", resolution.ToString());
 
   // Then, click the revert button. Although |old_resolution| for the second
   // SetDisplayResolutionAndNotify is 200x200, it should revert to the original
@@ -274,9 +272,8 @@ TEST_F(ResolutionNotificationControllerTest, MultipleResolutionChange) {
   RunAllPendingInMessageLoop();
   EXPECT_FALSE(IsNotificationVisible());
   EXPECT_EQ(0, accept_count());
-  EXPECT_TRUE(
+  EXPECT_FALSE(
       display_manager->GetSelectedResolutionForDisplayId(id2, &resolution));
-  EXPECT_EQ("150x150", resolution.ToString());
 }
 
 }  // namespace internal
diff --git a/chrome/browser/chromeos/display/display_preferences_unittest.cc b/chrome/browser/chromeos/display/display_preferences_unittest.cc
index 8299957..6d0614d 100644
--- a/chrome/browser/chromeos/display/display_preferences_unittest.cc
+++ b/chrome/browser/chromeos/display/display_preferences_unittest.cc
@@ -190,7 +190,7 @@ TEST_F(DisplayPreferencesTest, BasicStores) {
   ash::internal::DisplayManager* display_manager =
       ash::Shell::GetInstance()->display_manager();
 
-  UpdateDisplay("200x200*2,400x300");
+  UpdateDisplay("200x200*2, 400x300#400x400|300x200");
   int64 id1 = gfx::Screen::GetNativeScreen()->GetPrimaryDisplay().id();
   gfx::Display::SetInternalDisplayId(id1);
   int64 id2 = ash::ScreenAsh::GetSecondaryDisplay().id();
@@ -271,7 +271,7 @@ TEST_F(DisplayPreferencesTest, BasicStores) {
   EXPECT_FALSE(property->GetInteger("width", &width));
   EXPECT_FALSE(property->GetInteger("height", &height));
 
-  display_manager->SetDisplayResolution(id2, gfx::Size(400, 300));
+  display_manager->SetDisplayResolution(id2, gfx::Size(300, 200));
 
   display_controller->SetPrimaryDisplayId(id2);
 
@@ -282,12 +282,13 @@ TEST_F(DisplayPreferencesTest, BasicStores) {
   EXPECT_FALSE(property->GetInteger("width", &width));
   EXPECT_FALSE(property->GetInteger("height", &height));
 
-  // External dispaly's resolution must be stored this time.
+  // External dispaly's resolution must be stored this time because
+  // it's not best.
   EXPECT_TRUE(properties->GetDictionary(base::Int64ToString(id2), &property));
   EXPECT_TRUE(property->GetInteger("width", &width));
   EXPECT_TRUE(property->GetInteger("height", &height));
-  EXPECT_EQ(400, width);
-  EXPECT_EQ(300, height);
+  EXPECT_EQ(300, width);
+  EXPECT_EQ(200, height);
 
   // The layout remains the same.
   EXPECT_TRUE(displays->GetDictionary(key, &layout_value));
@@ -331,12 +332,18 @@ TEST_F(DisplayPreferencesTest, BasicStores) {
   EXPECT_TRUE(properties->GetDictionary(base::Int64ToString(id2), &property));
   EXPECT_TRUE(property->GetInteger("width", &width));
   EXPECT_TRUE(property->GetInteger("height", &height));
-  EXPECT_EQ(400, width);
-  EXPECT_EQ(300, height);
+  EXPECT_EQ(300, width);
+  EXPECT_EQ(200, height);
 
   // Set new display's selected resolution.
-  display_manager->SetDisplayResolution(id2 + 1, gfx::Size(500, 400));
-  UpdateDisplay("200x200*2,500x400");
+  display_manager->RegisterDisplayProperty(id2 + 1,
+                                           gfx::Display::ROTATE_0,
+                                           1.0f,
+                                           NULL,
+                                           gfx::Size(500, 400));
+
+  UpdateDisplay("200x200*2, 600x500#600x500|500x400");
+
   // Update key as the 2nd display gets new id.
   id2 = ash::ScreenAsh::GetSecondaryDisplay().id();
   key = base::Int64ToString(id1) + "," + base::Int64ToString(id2);
@@ -362,14 +369,14 @@ TEST_F(DisplayPreferencesTest, BasicStores) {
 TEST_F(DisplayPreferencesTest, PreventStore) {
   ResolutionNotificationController::SuppressTimerForTest();
   LoggedInAsUser();
-  UpdateDisplay("400x300");
+  UpdateDisplay("400x300#500x400|400x300|300x200");
   int64 id = ash::Shell::GetScreen()->GetPrimaryDisplay().id();
   // Set display's resolution in single display. It creates the notification and
   // display preferences should not stored meanwhile.
   ash::Shell::GetInstance()->resolution_notification_controller()->
       SetDisplayResolutionAndNotify(
           id, gfx::Size(400, 300), gfx::Size(500, 400), base::Closure());
-  UpdateDisplay("500x400");
+  UpdateDisplay("500x400#500x400|400x300|300x200");
 
   const base::DictionaryValue* properties =
       local_state()->GetDictionary(prefs::kDisplayProperties);
@@ -390,7 +397,7 @@ TEST_F(DisplayPreferencesTest, PreventStore) {
   // by SetDisplayResolution.
   ash::Shell::GetInstance()->display_manager()->SetDisplayResolution(
       id, gfx::Size(300, 200));
-  UpdateDisplay("300x200");
+  UpdateDisplay("300x200#500x400|400x300|300x200");
 
   property = NULL;
   EXPECT_TRUE(properties->GetDictionary(base::Int64ToString(id), &property));