Fix scroll regression when specifying an extension id.

chrome/browser/resources/extensions/extension_list.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.
 
 <include src="extension_error.js">
 
 ///////////////////////////////////////////////////////////////////////////////
 // ExtensionFocusRow:
 
 /**
@@ skipping 266 matching lines @@
       /** @private {Promise} */
       this.extensionsUpdated_ = new Promise(function(resolve, reject) {
         chrome.developerPrivate.getExtensionsInfo(
             {includeDisabled: true, includeTerminated: true},
             function(extensions) {
           // Sort in order of unpacked vs. packed, followed by name, followed by
           // id.
           extensions.sort(compareExtensions);
           this.extensions_ = extensions;
           this.showExtensionNodes_();
           resolve();

[Dan Beam, 2015/04/14 21:44:13]

nit: call this.whateverYouEndUpNamingIt_() here and make private?

[hcarmona, 2015/04/14 22:06:24]

I was looking at this place, but the list is still hidden here. What about
moving the hide logic into here instead of the extension-list-wrapper?

[Dan Beam, 2015/04/14 22:19:43]

talked about this IRL: I meant that after resolve() is called all the .then()
stuff should already be done (if all subscribers are resolved synchronously) and
we can do some private stuff after (and maybe assert(someNodeIsntHidden) to make
sure it doesn't break in the future)

         }.bind(this));
       }.bind(this));
       return this.extensionsUpdated_;
     },
 
+    /**
+     * Updates any element that needs to be visible to update properly.
+     * @param {boolean} visible Whether the extension list is visible.

[Devlin, 2015/04/14 22:23:55]

Drive-by: What param?

+     */
+    updateVisible: function() {

[not at google - send to devlin, 2015/04/14 21:14:50]

TBH "updateVisible" is still too vague for me. Are these visible elements?
nodes? extensions? Seems more like... well, things, but you can't say that.
Maybe something along the lines of "refresh" is more appropriate? Also: should
callers of updateFocusableElements be using this method instead? The public API
of ExtensionList is growing, and as a consumer of its API I don't know when to
call updateVisible and when to call updateFocusable. Or updateExtensionsData for
that matter.

[Dan Beam, 2015/04/14 21:44:13]

how about:
- updateAndHighlight()
- onExtensionsShown()
- afterUpdate()
- onUpdateFinished()
?

[hcarmona, 2015/04/14 22:06:24]

We can go with onUpdateFinished_() and make it private if there are no
objections to making the list show itself.

updateFocusableElements is different than this because this will potentially
scroll while the updateFocusableElements just updates the elements that can be
focused.

[not at google - send to devlin, 2015/04/14 22:39:25]

Gotcha. Maybe this doesn't make sense, but could there be a single method which
takes a parameter whether to scroll or not?

"no objections to making the list show itself" - in what way is the list showing
itself, and what might the objections be?

[hcarmona, 2015/04/14 23:06:43]

Scrolling should happen when the list is loaded, if we can keep it encapsulated
in the ExtensionList it'll make for a cleaner API
List currently becomes visible after loading via a promise. However, if we use
the same promise, it's not necessary to expose this method at all (see updated
code).

+      this.updateFocusableElements();
+
+      var idToHighlight = this.getIdQueryParam_();
+      if (idToHighlight && $(idToHighlight))
+        this.scrollToNode_(idToHighlight);
+
+      var idToOpenOptions = this.getOptionsQueryParam_();
+      if (idToOpenOptions && $(idToOpenOptions))
+        this.showEmbeddedExtensionOptions_(idToOpenOptions, true);
+    },
+
     /** @return {number} The number of extensions being displayed. */
     getNumExtensions: function() {
       return this.extensions_.length;
     },
 
     getIdQueryParam_: function() {
       return parseQueryParams(document.location)['id'];
     },
 
     getOptionsQueryParam_: function() {
@@ skipping 28 matching lines @@
               focusableNode.getEquivalentElement(
                   document.activeElement).focus();
             }
           }
 
           node.parentElement.removeChild(node);
           // Unregister the removed node from events.
           assertInstanceof(node, ExtensionFocusRow).destroy();
         }
       }
-
-      var idToHighlight = this.getIdQueryParam_();
-      if (idToHighlight && $(idToHighlight))
-        this.scrollToNode_(idToHighlight);
-
-      var idToOpenOptions = this.getOptionsQueryParam_();
-      if (idToOpenOptions && $(idToOpenOptions))
-        this.showEmbeddedExtensionOptions_(idToOpenOptions, true);
     },
 
     /** Updates each row's focusable elements without rebuilding the grid. */
     updateFocusableElements: function() {
       var rows = document.querySelectorAll('.extension-list-item-wrapper[id]');
       for (var i = 0; i < rows.length; ++i) {
         assertInstanceof(rows[i], ExtensionFocusRow).updateFocusableElements();
       }
     },
 
@@ skipping 698 matching lines @@
         var nextExt = this.extensions_[this.extensions_.indexOf(extension) + 1];
         this.createNode_(extension, nextExt ? $(nextExt.id) : null);
       }
     }
   };
 
   return {
     ExtensionList: ExtensionList
   };
 });