Sort ids uniformly when creating DisplayIdPair

1) Internal display comes first
2) If there is no internal displays, sorted by output index.

This was original intention, but isn't enforced.

This is speculative fix for the issue 514435.
This also sorts ID when loading prefs, so this should not break
existing saved data.

BUG=514435
TEST=DisplayUtilTest.CreateDisplayIdPair

Review URL: https://codereview.chromium.org/1264613003

Cr-Commit-Position: refs/heads/master@{#341230}

13 files changed, 81 insertions, 47 deletions

diff --git a/ash/display/display_change_observer_chromeos.cc b/ash/display/display_change_observer_chromeos.cc
index ac6b995..fd2a66d 100644
--- a/ash/display/display_change_observer_chromeos.cc
+++ b/ash/display/display_change_observer_chromeos.cc
@@ -60,6 +60,17 @@ const int kMinimumWidthFor4K = 3840;
 // available in extrenal large monitors.
 const float kAdditionalDeviceScaleFactorsFor4k[] = {1.25f, 2.0f};
 
+void UpdateInternalDisplayId(
+    const ui::DisplayConfigurator::DisplayStateList& display_states) {
+  for (auto* state : display_states) {
+    if (state->type() == ui::DISPLAY_CONNECTION_TYPE_INTERNAL) {
+      if (gfx::Display::HasInternalDisplay())
+        DCHECK_EQ(gfx::Display::InternalDisplayId(), state->display_id());
+      gfx::Display::SetInternalDisplayId(state->display_id());
+    }
+  }
+}
+
 }  // namespace
 
 // static
@@ -141,9 +152,12 @@ DisplayChangeObserver::~DisplayChangeObserver() {
 }
 
 ui::MultipleDisplayState DisplayChangeObserver::GetStateForDisplayIds(
-    const std::vector<int64>& display_ids) const {
-  CHECK_EQ(2U, display_ids.size());
-  DisplayIdPair pair = std::make_pair(display_ids[0], display_ids[1]);
+    const ui::DisplayConfigurator::DisplayStateList& display_states) const {
+  UpdateInternalDisplayId(display_states);
+  if (display_states.size() != 2)
+    return ui::MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED;
+  DisplayIdPair pair = CreateDisplayIdPair(display_states[0]->display_id(),
+                                           display_states[1]->display_id());
   DisplayLayout layout = Shell::GetInstance()->display_manager()->
       layout_store()->GetRegisteredDisplayLayout(pair);
   return layout.mirrored ? ui::MULTIPLE_DISPLAY_STATE_DUAL_MIRROR :
@@ -163,22 +177,11 @@ bool DisplayChangeObserver::GetResolutionForDisplayId(int64 display_id,
 
 void DisplayChangeObserver::OnDisplayModeChanged(
     const ui::DisplayConfigurator::DisplayStateList& display_states) {
+  UpdateInternalDisplayId(display_states);
+
   std::vector<DisplayInfo> displays;
   std::set<int64> ids;
   for (const ui::DisplaySnapshot* state : display_states) {
-    if (state->type() == ui::DISPLAY_CONNECTION_TYPE_INTERNAL) {
-      if (!gfx::Display::HasInternalDisplay()) {
-        gfx::Display::SetInternalDisplayId(state->display_id());
-      } else {
-#if defined(USE_OZONE)
-        // TODO(dnicoara) Remove when Ozone can properly perform the initial
-        // display configuration.
-        gfx::Display::SetInternalDisplayId(state->display_id());
-#endif
-        DCHECK_EQ(gfx::Display::InternalDisplayId(), state->display_id());
-      }
-    }
-
     const ui::DisplayMode* mode_info = state->current_mode();
     if (!mode_info)
       continue;
diff --git a/ash/display/display_change_observer_chromeos.h b/ash/display/display_change_observer_chromeos.h
index d275aa1..7b9dd12 100644
--- a/ash/display/display_change_observer_chromeos.h
+++ b/ash/display/display_change_observer_chromeos.h
@@ -40,7 +40,7 @@ class DisplayChangeObserver : public ui::DisplayConfigurator::StateController,
 
   // ui::DisplayConfigurator::StateController overrides:
   ui::MultipleDisplayState GetStateForDisplayIds(
-      const std::vector<int64>& outputs) const override;
+      const ui::DisplayConfigurator::DisplayStateList& outputs) const override;
   bool GetResolutionForDisplayId(int64 display_id,
                                  gfx::Size* size) const override;
 
diff --git a/ash/display/display_layout_store.cc b/ash/display/display_layout_store.cc
index 3ef9134..2f5dc2b 100644
--- a/ash/display/display_layout_store.cc
+++ b/ash/display/display_layout_store.cc
@@ -7,6 +7,7 @@
 #include "ash/ash_switches.h"
 #include "ash/display/display_layout_store.h"
 #include "ash/display/display_manager.h"
+#include "ash/display/display_util.h"
 #include "ash/shell.h"
 #include "base/command_line.h"
 #include "base/logging.h"
@@ -48,7 +49,7 @@ void DisplayLayoutStore::RegisterLayoutForDisplayIdPair(
     int64 id1,
     int64 id2,
     const DisplayLayout& layout) {
-  auto key = std::make_pair(id1, id2);
+  auto key = CreateDisplayIdPair(id1, id2);
   paired_layouts_[key] = layout;
 #if defined(OS_CHROMEOS)
   // Force disabling unified desktop if the flag is not set.
diff --git a/ash/display/display_manager.cc b/ash/display/display_manager.cc
index c85d2de..88987f0 100644
--- a/ash/display/display_manager.cc
+++ b/ash/display/display_manager.cc
@@ -230,8 +230,8 @@ DisplayLayout DisplayManager::GetCurrentDisplayLayout() {
 
 DisplayIdPair DisplayManager::GetCurrentDisplayIdPair() const {
   if (IsInUnifiedMode()) {
-    return std::make_pair(software_mirroring_display_list_[0].id(),
-                          software_mirroring_display_list_[1].id());
+    return CreateDisplayIdPair(software_mirroring_display_list_[0].id(),
+                               software_mirroring_display_list_[1].id());
   } else if (IsInMirrorMode()) {
     if (software_mirroring_enabled()) {
       CHECK_EQ(2u, num_connected_displays());
@@ -239,16 +239,12 @@ DisplayIdPair DisplayManager::GetCurrentDisplayIdPair() const {
       // between two checks.
       CHECK_EQ(1u, active_display_list_.size());
     }
-    return std::make_pair(active_display_list_[0].id(), mirroring_display_id_);
+    return CreateDisplayIdPair(active_display_list_[0].id(),
+                               mirroring_display_id_);
   } else {
     CHECK_LE(2u, active_display_list_.size());
-    int64 id_at_zero = active_display_list_[0].id();
-    if (gfx::Display::IsInternalDisplayId(id_at_zero) ||
-        id_at_zero == first_display_id()) {
-      return std::make_pair(id_at_zero, active_display_list_[1].id());
-    } else {
-      return std::make_pair(active_display_list_[1].id(), id_at_zero);
-    }
+    return CreateDisplayIdPair(active_display_list_[0].id(),
+                               active_display_list_[1].id());
   }
 }
 
@@ -628,8 +624,8 @@ void DisplayManager::OnNativeDisplaysChanged(
   if (new_display_info_list.size() > 1) {
     std::sort(new_display_info_list.begin(), new_display_info_list.end(),
               DisplayInfoSortFunctor());
-    DisplayIdPair pair = std::make_pair(new_display_info_list[0].id(),
-                                        new_display_info_list[1].id());
+    DisplayIdPair pair = CreateDisplayIdPair(new_display_info_list[0].id(),
+                                             new_display_info_list[1].id());
     DisplayLayout layout = layout_store_->GetRegisteredDisplayLayout(pair);
     default_multi_display_mode_ =
         (layout.default_unified && switches::UnifiedDesktopEnabled())
@@ -1308,13 +1304,8 @@ bool DisplayManager::UpdateNonPrimaryDisplayBoundsForLayout(
     return true;
   }
 
-  int64 id_at_zero = displays->at(0).id();
-  DisplayIdPair pair = (id_at_zero == first_display_id_ ||
-                        gfx::Display::IsInternalDisplayId(id_at_zero))
-                           ? std::make_pair(id_at_zero, displays->at(1).id())
-                           : std::make_pair(displays->at(1).id(), id_at_zero);
-  DisplayLayout layout =
-      layout_store_->ComputeDisplayLayoutForDisplayIdPair(pair);
+  DisplayLayout layout = layout_store_->ComputeDisplayLayoutForDisplayIdPair(
+      CreateDisplayIdPair(displays->at(0).id(), displays->at(1).id()));
 
   // Ignore if a user has a old format (should be extremely rare)
   // and this will be replaced with DCHECK.
diff --git a/ash/display/display_manager_unittest.cc b/ash/display/display_manager_unittest.cc
index ca7be60..e3c05e4 100644
--- a/ash/display/display_manager_unittest.cc
+++ b/ash/display/display_manager_unittest.cc
@@ -668,9 +668,10 @@ TEST_F(DisplayManagerTest, DisplayAddRemoveAtTheSameTime) {
   display_info_list.push_back(secondary_info);
   display_manager()->OnNativeDisplaysChanged(display_info_list);
 
-  EXPECT_EQ(third_id, WindowTreeHostManager::GetPrimaryDisplayId());
+  // Secondary seconary_id becomes the primary as it has smaller output index.
+  EXPECT_EQ(secondary_id, WindowTreeHostManager::GetPrimaryDisplayId());
+  EXPECT_EQ(third_id, ScreenUtil::GetSecondaryDisplay().id());
   EXPECT_EQ("600x600", GetDisplayForId(third_id).size().ToString());
-  EXPECT_EQ(secondary_id, ScreenUtil::GetSecondaryDisplay().id());
 }
 
 #if defined(OS_WIN)
diff --git a/ash/display/display_util.cc b/ash/display/display_util.cc
index e6c002e..5219dad 100644
--- a/ash/display/display_util.cc
+++ b/ash/display/display_util.cc
@@ -338,4 +338,16 @@ int FindDisplayIndexContainingPoint(const std::vector<gfx::Display>& displays,
   return iter == displays.end() ? -1 : (iter - displays.begin());
 }
 
+DisplayIdPair CreateDisplayIdPair(int64 id1, int64 id2) {
+  DCHECK_NE(id1, id2);
+  // Output index is stored in the first 8 bits. See GetDisplayIdFromEDID
+  // in edid_parser.cc.
+  int index_1 = id1 & 0xFF;
+  int index_2 = id2 & 0xFF;
+  if (gfx::Display::IsInternalDisplayId(id2) || index_2 < index_1)
+    return std::make_pair(id2, id1);
+  else
+    return std::make_pair(id1, id2);
+}
+
 }  // namespace ash
diff --git a/ash/display/display_util.h b/ash/display/display_util.h
index 9cd3208..dd0784b 100644
--- a/ash/display/display_util.h
+++ b/ash/display/display_util.h
@@ -87,6 +87,11 @@ ASH_EXPORT int FindDisplayIndexContainingPoint(
     const std::vector<gfx::Display>& displays,
     const gfx::Point& point_in_screen);
 
+// Creates the DisplayIdPair where ids are sorted in the following manner.
+// 1) ID for internal display comes first.
+// 2) If none of the ids are internal, sorted by the output index.
+ASH_EXPORT DisplayIdPair CreateDisplayIdPair(int64 id1, int64 id2);
+
 }  // namespace ash
 
 #endif  // ASH_DISPLAY_DISPLAY_UTIL_H_
diff --git a/ash/display/display_util_unittest.cc b/ash/display/display_util_unittest.cc
index a104fe0..1bd3d8c 100644
--- a/ash/display/display_util_unittest.cc
+++ b/ash/display/display_util_unittest.cc
@@ -7,6 +7,7 @@
 #include "ash/root_window_controller.h"
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
+#include "ash/test/display_manager_test_api.h"
 
 namespace ash {
 
@@ -90,4 +91,18 @@ TEST_F(DisplayUtilTest, RotatedDisplay) {
   }
 }
 
+TEST_F(DisplayUtilTest, CreateDisplayIdPair) {
+  DisplayIdPair pair = CreateDisplayIdPair(10, 1);
+  EXPECT_EQ(1, pair.first);
+  EXPECT_EQ(10, pair.second);
+  pair = CreateDisplayIdPair(10, 100);
+  EXPECT_EQ(10, pair.first);
+  EXPECT_EQ(100, pair.second);
+
+  test::ScopedSetInternalDisplayId set_internal(100);
+  pair = CreateDisplayIdPair(10, 100);
+  EXPECT_EQ(100, pair.first);
+  EXPECT_EQ(10, pair.second);
+}
+
 }  // namespace
diff --git a/ash/display/window_tree_host_manager_unittest.cc b/ash/display/window_tree_host_manager_unittest.cc
index b04d72e..675484f 100644
--- a/ash/display/window_tree_host_manager_unittest.cc
+++ b/ash/display/window_tree_host_manager_unittest.cc
@@ -675,6 +675,15 @@ TEST_F(WindowTreeHostManagerTest, BoundsUpdated) {
   // UI scale is eanbled only on internal display.
   int64 secondary_id = GetSecondaryDisplay().id();
   test::ScopedSetInternalDisplayId set_internal(secondary_id);
+  // Changing internal ID display changes the DisplayIdPair (it comes
+  // first), which also changes the primary display candidate.  Update
+  // the primary display manually to update the primary display to
+  // avoid getting the OnDisplayConfigurationChanged() call twice in
+  // SetDisplayUIScale. Note that this scenario will never happen on
+  // real devices.
+  Shell::GetInstance()->window_tree_host_manager()->SetPrimaryDisplayId(
+      secondary_id);
+  EXPECT_EQ(1, observer.CountAndReset());
 
   SetDisplayUIScale(secondary_id, 1.125f);
   EXPECT_EQ(1, observer.CountAndReset());
diff --git a/chrome/browser/chromeos/display/display_preferences.cc b/chrome/browser/chromeos/display/display_preferences.cc
index ede3e38..45b242c 100644
--- a/chrome/browser/chromeos/display/display_preferences.cc
+++ b/chrome/browser/chromeos/display/display_preferences.cc
@@ -7,6 +7,7 @@
 #include "ash/display/display_layout_store.h"
 #include "ash/display/display_manager.h"
 #include "ash/display/display_pref_util.h"
+#include "ash/display/display_util.h"
 #include "ash/shell.h"
 #include "base/prefs/pref_registry_simple.h"
 #include "base/prefs/pref_service.h"
@@ -394,7 +395,7 @@ void LoadDisplayPreferences(bool first_run_after_boot) {
 void StoreDisplayLayoutPrefForTest(int64 id1,
                                    int64 id2,
                                    const ash::DisplayLayout& layout) {
-  StoreDisplayLayoutPref(std::make_pair(id1, id2), layout);
+  StoreDisplayLayoutPref(ash::CreateDisplayIdPair(id1, id2), layout);
 }
 
 // Stores the given |power_state|.
diff --git a/ui/display/chromeos/display_configurator.h b/ui/display/chromeos/display_configurator.h
index 5564aa3..eab3d90 100644
--- a/ui/display/chromeos/display_configurator.h
+++ b/ui/display/chromeos/display_configurator.h
@@ -98,7 +98,7 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
 
     // Called when displays are detected.
     virtual MultipleDisplayState GetStateForDisplayIds(
-        const std::vector<int64_t>& display_ids) const = 0;
+        const ui::DisplayConfigurator::DisplayStateList& outputs) const = 0;
 
     // Queries the resolution (|size|) in pixels to select display mode for the
     // given display id.
diff --git a/ui/display/chromeos/display_configurator_unittest.cc b/ui/display/chromeos/display_configurator_unittest.cc
index d46c41d..bd5f7e9 100644
--- a/ui/display/chromeos/display_configurator_unittest.cc
+++ b/ui/display/chromeos/display_configurator_unittest.cc
@@ -78,7 +78,7 @@ class TestStateController : public DisplayConfigurator::StateController {
 
   // DisplayConfigurator::StateController overrides:
   MultipleDisplayState GetStateForDisplayIds(
-      const std::vector<int64_t>& outputs) const override {
+      const DisplayConfigurator::DisplayStateList& outputs) const override {
     return state_;
   }
   bool GetResolutionForDisplayId(int64_t display_id,
diff --git a/ui/display/chromeos/update_display_configuration_task.cc b/ui/display/chromeos/update_display_configuration_task.cc
index 33ac1fe..335c661 100644
--- a/ui/display/chromeos/update_display_configuration_task.cc
+++ b/ui/display/chromeos/update_display_configuration_task.cc
@@ -196,12 +196,8 @@ MultipleDisplayState UpdateDisplayConfigurationTask::ChooseDisplayState()
           return MULTIPLE_DISPLAY_STATE_DUAL_EXTENDED;
         // With either both displays on or both displays off, use one of the
         // dual modes.
-        std::vector<int64_t> display_ids;
-        for (size_t i = 0; i < cached_displays_.size(); ++i)
-          display_ids.push_back(cached_displays_[i]->display_id());
-
         return layout_manager_->GetStateController()->GetStateForDisplayIds(
-            display_ids);
+            cached_displays_);
       }
       NOTREACHED();
     }