Fix bug when transitioning from an empty to a non-empty password list

chrome/browser/resources/options/password_manager.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 cr.define('options', function() {
   /** @const */ var Page = cr.ui.pageManager.Page;
   /** @const */ var PageManager = cr.ui.pageManager.PageManager;
   /** @const */ var ArrayDataModel = cr.ui.ArrayDataModel;
 
   /////////////////////////////////////////////////////////////////////////////
@@ skipping 142 matching lines @@
       if (this.lastQuery_ != filter) {
         this.lastQuery_ = filter;
         // Searching for passwords has the side effect of requerying the
         // underlying password store. This is done intentionally, as on OS X and
         // Linux they can change from outside and we won't be notified of it.
         chrome.send('updatePasswordLists');
       }
     },
 
     /**
-     * Updates the visibility of the list and empty list placeholder.
+     * Updates the list with the given entries and updates the visibility of the
+     * list and empty list placeholder.
      * @param {!cr.ui.List} list The list to toggle visilibility for.
+     * @param {!Array} entries The list of entries.
      */
-    updateListVisibility_: function(list) {
-      var empty = list.dataModel.length == 0;
+    updateListAndVisibility_: function(list, entries) {
+      // Setting the dataModel results in a redraw of the viewport, which is why
+      // the visibility needs to be updated first. Otherwise, redraw will not
+      // render the updated entries when transitioning from a previously empty
+      // list to a non-empty one. The attribute list.hidden would be true in
+      // this case, resulting in |redraw()| not adding the new elements to the
+      // viewport and thus showing a empty list to the user
+      // (http://crbug.com/672869).
+      var empty = entries.length == 0;
       var listPlaceHolderID = list.id + '-empty-placeholder';
       list.hidden = empty;
       $(listPlaceHolderID).hidden = !empty;
+      list.dataModel = new ArrayDataModel(entries);
     },
 
     /**
      * Updates eliding of origins. If there is no enough space to show the full
      * origin, the origin is elided from the left with ellipsis.
      * @param {!cr.ui.List} list The list to update eliding.
      */
     updateOriginsEliding_: function(list) {
       var entries = list.getElementsByClassName('deletable-item');
       if (entries.length == 0)
@@ skipping 57 matching lines @@
               username.toLowerCase().indexOf(query.toLowerCase()) >= 0) {
             // Keep the original index so we can delete correctly. See also
             // deleteItemAtIndex() in password_manager_list.js that uses this.
             entry[options.passwordManager.ORIGINAL_INDEX_FIELD] = index;
             return true;
           }
           return false;
         };
         entries = entries.filter(filter);
       }
-      this.savedPasswordsList_.dataModel = new ArrayDataModel(entries);
-      this.updateListVisibility_(this.savedPasswordsList_);
+      assert(this.savedPasswordsList_ !== null);
+      this.updateListAndVisibility_(this.savedPasswordsList_, entries);

[Dan Beam, 2016/12/20 22:25:12]

can you just wrap with assert(), as in:

this.updateListAndVisibility_(assert(this.savedPasswordsList_), entries);

[jdoerrie, 2016/12/21 07:07:41]

Done.

       // updateOriginsEliding_ should be called after updateListVisibility_,
       // otherwise updateOrigins... might be not able to read width of elements.
       this.updateOriginsEliding_(this.savedPasswordsList_);
     },
 
     /**
      * Updates the data model for the password exceptions list with the values
      * from |entries|.
      * @param {!Array} entries The list of password exception data.
      */
     setPasswordExceptionsList_: function(entries) {
-      this.passwordExceptionsList_.dataModel = new ArrayDataModel(entries);
-      this.updateListVisibility_(this.passwordExceptionsList_);
+      assert(this.passwordExceptionsList_ !== null);
+      this.updateListAndVisibility_(this.passwordExceptionsList_, entries);
       // updateOriginsEliding_ should be called after updateListVisibility_,
       // otherwise updateOrigins... might be not able to read width of elements.
       this.updateOriginsEliding_(this.passwordExceptionsList_);
     },
 
     /**
      * Reveals the password for a saved password entry. This is called by the
      * backend after it has authenticated the user.
      * @param {number} index The original index of the entry in the model.
      * @param {string} password The saved password.
@@ skipping 61 matching lines @@
     'showImportExportButton',
     'showPassword',
   ]);
 
   // Export
   return {
     PasswordManager: PasswordManager
   };
 
 });