[Extensions Page] Fix focus when an extension is removed

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 263 matching lines @@
           case EventType.ERRORS_REMOVED:
           case EventType.PREFS_CHANGED:
             if (eventData.extensionInfo) {
               this.updateExtension_(eventData.extensionInfo);
               this.focusGrid_.ensureRowActive();
             }
             break;
           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();
         }
 
         if (eventData.event_type == EventType.UNLOADED)
           this.hideEmbeddedExtensionOptions_(eventData.item_id);
 
         if (eventData.event_type == EventType.INSTALLED ||
             eventData.event_type == EventType.UNINSTALLED) {
@@ skipping 100 matching lines @@
 
       // Iterate over the extension data and add each item to the list.
       this.extensions_.forEach(function(extension) {
         seenIds.push(extension.id);
         this.updateExtension_(extension);
       }, this);
       this.focusGrid_.ensureRowActive();
 
       // 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. */
     updateFocusableElements: function() {
       var rows = document.querySelectorAll('.extension-list-item-wrapper[id]');
       for (var i = 0; i < rows.length; ++i) {
         assertInstanceof(rows[i], ExtensionFocusRow).updateFocusableElements();
       }
     },
 
     /**
+     * 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
      */
     scrollToNode_: function(extensionId) {
       // Scroll offset should be calculated slightly higher than the actual
       // offset of the element being scrolled to, so that it ends up not all
       // the way at the top. That way it is clear that there are more elements
       // above the element being scrolled to.
       var scrollFudge = 1.2;
@@ skipping 716 matching lines @@
         this.createNode_(extension, nextExt ? $(nextExt.id) : null);
       }
     }
   };
 
   return {
     ExtensionList: ExtensionList,
     ExtensionListDelegate: ExtensionListDelegate
   };
 });