Fix resolution menu not appearing for HDMI monitors

Duplicate modes were causing lookups in modeToParentModeMap_ to fail,
resulting in undefined pref values and errors at the display menu level.

This change ensures that all modes have an entry in
modeToParentModeMap_.

It also introduces a preference for native or selected modes when
de-duplicating equivalent modes.

Bug: 1127398
Change-Id: I7be8f1e34b31e1a1e7f3b106519c5ce529f568c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2406716
Commit-Queue: Nick Czajka <czajka@google.com>
Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Bailey Berro <baileyberro@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809097}

chrome/browser/resources/settings/chromeos/device_page/display.js

@@ -573,6 +573,15 @@ cr.define('settings.display', function() {
           modes.get(mode.width).set(mode.height, new Map());
         }
 
+        // Prefer the first native mode we find, for consistency.
+        if (modes.get(mode.width).get(mode.height).has(mode.refreshRate)) {
+          const existingModeIndex =
+              modes.get(mode.width).get(mode.height).get(mode.refreshRate);
+          const existingMode = selectedDisplay.modes[existingModeIndex];
+          if (existingMode.isNative || !mode.isNative) {
+            continue;
+          }
+        }
         modes.get(mode.width).get(mode.height).set(mode.refreshRate, i);
       }
       return modes;
@@ -612,8 +621,8 @@ cr.define('settings.display', function() {
           // (resolution) is the default and therefore the "parent" mode.
           const height = heightArr[j];
           const refreshRates = heightsMap.get(height);
-
-          this.addResolution_(width, height, refreshRates);
+          const parentModeIndex = this.getParentModeIndex_(refreshRates);
+          this.addResolution_(parentModeIndex, width, height);
 
           // For each of the refresh rates at a given resolution, add an entry
           // to |parentModeToRefreshRateMap_|. This allows us to retrieve a
@@ -621,7 +630,6 @@ cr.define('settings.display', function() {
           // parentModeIndex.
           const refreshRatesArr = Array.from(refreshRates.keys());
           for (let k = 0; k < refreshRatesArr.length; k++) {
-            const parentModeIndex = Array.from(refreshRates.values())[0];
             const rate = refreshRatesArr[k];
             const modeIndex = refreshRates.get(rate);
             const isInterlaced = selectedDisplay.modes[modeIndex].isInterlaced;
@@ -632,25 +640,44 @@ cr.define('settings.display', function() {
         }
       }
 
+      // Construct mode->parentMode map so we can get parent modes later.
+      for (let i = 0; i < selectedDisplay.modes.length; i++) {
+        const mode = selectedDisplay.modes[i];
+        const parentModeIndex =
+            this.getParentModeIndex_(modes.get(mode.width).get(mode.height));
+        this.modeToParentModeMap_.set(i, parentModeIndex);
+      }
+      assert(this.modeToParentModeMap_.size == selectedDisplay.modes.length);
+
       // Use the new sort order.
       this.sortResolutionList_();
     },
 
+    /**
+     * Picks the appropriate parent mode from a refresh rate -> mode index map.
+     * Currently this chooses the mode with the highest refresh rate.
+     * @param {Map<number,number>} refreshRates each possible refresh rate
+     *   mapped to the corresponding mode index.
+     * @private
+     */
+    getParentModeIndex_(refreshRates) {
+      const maxRefreshRate = Math.max(...refreshRates.keys());
+      return refreshRates.get(maxRefreshRate);
+    },
+
     /**
      * Adds a an entry in |displayModeList_| for the resolution represented by
      * |width| and |height| and possible |refreshRates|.
+     * @param {number} parentModeIndex
      * @param {number} width
      * @param {number} height
-     * @param {Map<number,number>} refreshRates each possible refresh rate for
-     *   the resolution mapped to the corresponding modeIndex.
      * @private
      */
-    addResolution_(width, height, refreshRates) {
+    addResolution_(parentModeIndex, width, height) {
       assert(this.listAllDisplayModes_);
-      const parentModeIndex = Array.from(refreshRates.values())[0];
 
       // Add an entry in the outer map for |parentModeIndex|. The inner
-      // array (the value at |parentModeIndex) will be populated with all
+      // array (the value at |parentModeIndex|) will be populated with all
       // possible refresh rates for the given resolution.
       this.parentModeToRefreshRateMap_.set(parentModeIndex, new Array());
 
@@ -676,9 +703,6 @@ cr.define('settings.display', function() {
      */
     addRefreshRate_(parentModeIndex, modeIndex, rate, isInterlaced) {
       assert(this.listAllDisplayModes_);
-      // Maintain a mapping from a given |modeIndex| back to the
-      // corresponding |parentModeIndex|.
-      this.modeToParentModeMap_.set(modeIndex, parentModeIndex);
 
       // Truncate at two decimal places for display. If the refresh rate
       // is a whole number, remove the mantissa.

chrome/test/data/webui/settings/chromeos/device_page_tests.js

@@ -494,6 +494,25 @@ cr.define('device_page_tests', function() {
             height: 2000,
             refreshRate: 75,
           },
+          // Include 3 copies of 3000x2000 mode to emulate duplicated modes
+          // reported by some monitors.  Only one is marked 'isNative'.
+          {
+            deviceScaleFactor: 1.0,
+            widthInNativePixels: 3000,
+            heightInNativePixels: 2000,
+            width: 3000,
+            height: 2000,
+            refreshRate: 100,
+          },
+          {
+            isNative: true,
+            deviceScaleFactor: 1.0,
+            widthInNativePixels: 3000,
+            heightInNativePixels: 2000,
+            width: 3000,
+            height: 2000,
+            refreshRate: 100,
+          },
           {
             deviceScaleFactor: 1.0,
             widthInNativePixels: 3000,
@@ -954,12 +973,14 @@ cr.define('device_page_tests', function() {
               // Verify the display modes are parsed correctly.
 
               // 5 total modes, 2 parent modes.
-              expectEquals(5, displayPage.modeToParentModeMap_.size);
+              expectEquals(7, displayPage.modeToParentModeMap_.size);
               expectEquals(0, displayPage.modeToParentModeMap_.get(0));
               expectEquals(0, displayPage.modeToParentModeMap_.get(1));
-              expectEquals(2, displayPage.modeToParentModeMap_.get(2));
-              expectEquals(2, displayPage.modeToParentModeMap_.get(3));
-              expectEquals(2, displayPage.modeToParentModeMap_.get(4));
+              expectEquals(5, displayPage.modeToParentModeMap_.get(2));
+              expectEquals(5, displayPage.modeToParentModeMap_.get(3));
+              expectEquals(5, displayPage.modeToParentModeMap_.get(4));
+              expectEquals(5, displayPage.modeToParentModeMap_.get(5));
+              expectEquals(5, displayPage.modeToParentModeMap_.get(6));
 
               // Two resolution options, one for each parent mode.
               expectEquals(2, displayPage.refreshRateList_.length);
@@ -969,7 +990,7 @@ cr.define('device_page_tests', function() {
               expectEquals(
                   2, displayPage.parentModeToRefreshRateMap_.get(0).length);
               expectEquals(
-                  3, displayPage.parentModeToRefreshRateMap_.get(2).length);
+                  3, displayPage.parentModeToRefreshRateMap_.get(5).length);
 
               // Ambient EQ never shown on non-internal display regardless of
               // whether it is enabled.