[Extensions Page] Fix focus when an extension is removed

chrome/browser/resources/extensions/extension_list.js

Index: chrome/browser/resources/extensions/extension_list.js
diff --git a/chrome/browser/resources/extensions/extension_list.js b/chrome/browser/resources/extensions/extension_list.js
index cbac155401277d806f165d977c049bed80ea769c..874bb81465318c3c068b2a15a0ccbe5281523abc 100644
--- a/chrome/browser/resources/extensions/extension_list.js
+++ b/chrome/browser/resources/extensions/extension_list.js
@@ -281,11 +281,9 @@ cr.define('extensions', function() {
           case EventType.UNINSTALLED:
             var index = this.getIndexOfExtension_(eventData.item_id);
             this.extensions_.splice(index, 1);
-            var childNode = $(eventData.item_id);
-            childNode.parentNode.removeChild(childNode);
-            this.focusGrid_.removeRow(assertInstanceof(childNode,
-                                                       ExtensionFocusRow));
-            this.focusGrid_.ensureRowActive();
+            var node = $(eventData.item_id);
+            assert(node != null);
+            this.removeNode_(node);

[Devlin, 2015/05/15 23:29:43]

I thought there was something like assertNotNull so that this could be
this.removeNode_(assertNotNull($(eventData.item_id)));
but I couldn't find it.  Did I dream that up?

[Dan Beam, 2015/05/15 23:47:45]

  this.removeNode_(getRequiredElement(eventData.item_id))

or

  assert(node);  // top line of removeNode_

or just let removeNode_ explode

[Devlin, 2015/05/19 16:29:50]

I like getRequiredElement.  Gives us compile errors. :)

[Dan Beam, 2015/05/19 21:09:33]

compile-time type filtering + runtime errors.  assert() also does this magic,
too, btw:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...

[Devlin, 2015/05/19 21:36:39]

(For my own edification)

Hmm... If I make the param in removeNode an HTMLElement (i.e., remove the '!')
and add an assert(node) as the first line, I don't get compile-time warnings for
not using getRequiredElement() here, even though it could (for all the compiler
knows) be null (that's why I preferred the getRequiredElement approach).  Does
compile-time type filtering include nullness with assert()?

[Dan Beam, 2015/05/19 21:44:22]

this.removeNode_(assert(node));

             break;
           default:
             assertNotReached();
@@ -406,22 +404,10 @@ cr.define('extensions', function() {
 
       // Remove extensions that are no longer installed.
       var nodes = document.querySelectorAll('.extension-list-item-wrapper[id]');
-      for (var i = 0; i < nodes.length; ++i) {
-        var node = nodes[i];
-        if (seenIds.indexOf(node.id) < 0) {
-          if (node.contains(document.activeElement)) {
-            var focusableNode = nodes[i + 1] || nodes[i - 1];
-            if (focusableNode) {
-              focusableNode.getEquivalentElement(
-                  document.activeElement).focus();
-            }
-          }
-
-          node.parentElement.removeChild(node);
-          // Unregister the removed node from events.
-          assertInstanceof(node, ExtensionFocusRow).destroy();
-        }
-      }
+      Array.prototype.forEach.call(nodes, function(node) {
+        if (seenIds.indexOf(node.id) < 0)
+          this.removeNode_(node);
+      }, this);
     },
 
     /** Updates each row's focusable elements without rebuilding the grid. */
@@ -433,6 +419,30 @@ cr.define('extensions', function() {
     },
 
     /**
+     * Removes the node from the DOM, and updates the focused element if needed.
+     * @param {!HTMLElement} node
+     * @private
+     */
+    removeNode_: function(node) {
+      if (node.contains(document.activeElement)) {
+        var nodes =
+            document.querySelectorAll('.extension-list-item-wrapper[id]');
+        var index = Array.prototype.indexOf.call(nodes, node);
+        assert(index != -1);
+        var focusableNode = nodes[index + 1] || nodes[index - 1];
+        if (focusableNode)
+          focusableNode.getEquivalentElement(document.activeElement).focus();
+      }
+      node.parentNode.removeChild(node);
+      this.focusGrid_.removeRow(assertInstanceof(node, ExtensionFocusRow));
+
+      // Unregister the removed node from events.
+      assertInstanceof(node, ExtensionFocusRow).destroy();
+
+      this.focusGrid_.ensureRowActive();
+    },
+
+    /**
      * Scrolls the page down to the extension node with the given id.
      * @param {string} extensionId The id of the extension to scroll to.
      * @private