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}

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;

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;
 

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.

ash/display/display_manager.cc

@@ -230,25 +230,21 @@ 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());
       // This comment is to make it easy to distinguish the crash
       // 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.

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)

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

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_

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

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());

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|.

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.

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,

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();
     }