[Extensions] Update Error Console UI

chrome/browser/resources/extensions/extension_error.js

Index: chrome/browser/resources/extensions/extension_error.js
diff --git a/chrome/browser/resources/extensions/extension_error.js b/chrome/browser/resources/extensions/extension_error.js
index 5d76630bd73ebd061649d3097e3cfb819547520c..e592b1eb26457c3c9977da170b95dccfa5811f4b 100644
--- a/chrome/browser/resources/extensions/extension_error.js
+++ b/chrome/browser/resources/extensions/extension_error.js
@@ -46,7 +46,14 @@ cr.define('extensions', function() {
 
     /** @override */
     getEquivalentElement: function(element) {
-      return assert(this.querySelector('.extension-error-view-details'));
+      if (element.classList.contains('extension-error-metadata')) {
+        return this;

[Dan Beam, 2015/03/31 00:33:49]

nit: no if/else after return, i.e.

  if (element.classList.contains('extension-error-metadata'))
    return this;

  if (element.classList.contains('error-delete-button'))
    return /** !HTMLElement */(this.querySelector('.error-delete-button'));

  assertNotReached();
  return element;

[Devlin, 2015/03/31 16:17:50]

Done.

+      } else if (element.classList.contains('error-delete-button')) {
+        return /** @type {!HTMLElement} */ (this.querySelector(
+            '.error-delete-button'));
+      }
+      assertNotReached();
+      return element;
     },
 
     /**
@@ -58,6 +65,12 @@ cr.define('extensions', function() {
     decorate: function(error, boundary) {
       cr.ui.FocusRow.prototype.decorate.call(this, boundary);
 
+      /**
+       * The backing error.
+       * @type {(ManifestError|RuntimeError)}
+       */
+      this.error = error;

[Dan Beam, 2015/03/31 00:33:50]

I couldn't really tell, but can this be @private?

[Devlin, 2015/03/31 16:17:51]

It's used by the ExtensionErrorList (in this patch set, line 316).  We could
make it private, and then expose a getter, but figured that might be overkill. 
WDYT?

+
       // Add an additional class for the severity level.
       if (error.type == chrome.developerPrivate.ErrorType.RUNTIME) {
         switch (error.severity) {
@@ -87,28 +100,35 @@ cr.define('extensions', function() {
 
       var messageSpan = this.querySelector('.extension-error-message');
       messageSpan.textContent = error.message;
-      messageSpan.title = error.message;
 
-      var extensionUrl = 'chrome-extension://' + error.extensionId + '/';
-      var viewDetailsLink = this.querySelector('.extension-error-view-details');
+      var deleteButton = this.querySelector('.error-delete-button');
+      deleteButton.addEventListener('click', function(e) {
+        this.dispatchEvent(
+            new CustomEvent('deleteExtensionError',
+                            { bubbles: true, detail: this.error }));

[Dan Beam, 2015/03/31 00:33:49]

{\s -> {
\s} -> }

(everywhere)

[Devlin, 2015/03/31 16:17:50]

Done.

+      }.bind(this));
 
-      // If we cannot open the file source and there are no external frames in
-      // the stack, then there are no details to display.
-      if (!extensions.ExtensionErrorOverlay.canShowOverlayForError(
-              error, extensionUrl)) {
-        viewDetailsLink.hidden = true;
-      } else {
-        var stringId = extensionUrl.toLowerCase() == 'manifest.json' ?
-            'extensionErrorViewManifest' : 'extensionErrorViewDetails';
-        viewDetailsLink.textContent = loadTimeData.getString(stringId);
+      this.addEventListener('click', function(e) {
+        if (e.target != deleteButton)
+          this.requestActive_();
+      }.bind(this));
+      this.addEventListener('keydown', function(e) {
+        if (e.keyCode == 13 /* enter */)

[Dan Beam, 2015/03/31 00:33:49]

e.keyIdentifier == 'Enter'

[Dan Beam, 2015/03/31 00:33:50]

isn't there already a click dispatched on Enter?

[Devlin, 2015/03/31 16:17:50]

Thought keyIdentifier was from an old draft...  Done.

[Devlin, 2015/03/31 16:17:50]

I would have thought so, but in testing, no.  I wonder if this is because it's
not typical button?  (It's a row in a focus grid)

+          this.requestActive_();
+      });
 
-        viewDetailsLink.addEventListener('click', function(e) {
-          extensions.ExtensionErrorOverlay.getInstance().setErrorAndShowOverlay(
-              error, extensionUrl);
-        });
+      this.addFocusableElement(this);
+      this.addFocusableElement(this.querySelector('.error-delete-button'));
+    },
 
-        this.addFocusableElement(viewDetailsLink);
-      }
+    /**
+     * Bubble up an event to request to become active.
+     * @private
+     */
+    requestActive_: function() {
+      this.dispatchEvent(
+          new CustomEvent('highlightExtensionError',
+                          { bubbles: true, detail: this.error }));
     },
   };
 
@@ -116,60 +136,184 @@ cr.define('extensions', function() {
    * A variable length list of runtime or manifest errors for a given extension.
    * @param {Array<(RuntimeError|ManifestError)>} errors The list of extension
    *     errors with which to populate the list.
+   * @param {string} extensionId The id of the extension.
    * @constructor
    * @extends {HTMLDivElement}
    */
-  function ExtensionErrorList(errors) {
+  function ExtensionErrorList(errors, extensionId) {
     var div = cloneTemplate('extension-error-list');
     div.__proto__ = ExtensionErrorList.prototype;
-    div.errors_ = errors;
-    div.decorate();
+    div.extensionId_ = extensionId;
+    div.decorate(errors);
     return div;
   }
 
-  /**
-   * @private
-   * @const
-   * @type {number}
-   */
-  ExtensionErrorList.MAX_ERRORS_TO_SHOW_ = 3;
-
   ExtensionErrorList.prototype = {
     __proto__: HTMLDivElement.prototype,
 
-    decorate: function() {
+    /**
+     * Initializes the extension error list.
+     * @param {Array<(RuntimeError|ManifestError)>} errors The list of errors.
+     */
+    decorate: function(errors) {
+      /**
+       * @private {Array<(ManifestError|RuntimeError)>}
+       */
+      this.errors_ = [];
+
       this.focusGrid_ = new cr.ui.FocusGrid();
       this.gridBoundary_ = this.querySelector('.extension-error-list-contents');
       this.gridBoundary_.addEventListener('focus', this.onFocus_.bind(this));
       this.gridBoundary_.addEventListener('focusin',
                                           this.onFocusin_.bind(this));
-      this.errors_.forEach(function(error) {
-        if (idIsValid(error.extensionId)) {
-          var focusRow = new ExtensionError(error, this.gridBoundary_);
-          this.gridBoundary_.appendChild(
-              document.createElement('li')).appendChild(focusRow);
-          this.focusGrid_.addRow(focusRow);
+      errors.forEach(this.addError_, this);
+
+      this.addEventListener('highlightExtensionError', function(e) {
+        this.setActiveErrorNode_(e.target);
+      });
+      this.addEventListener('deleteExtensionError', function(e) {
+        this.removeError_(e.detail);
+      });
+
+      this.querySelector('#extension-error-list-clear').addEventListener(
+          'click', function(e) {
+        this.clear();
+      }.bind(this));
+
+      chrome.developerPrivate.onItemStateChanged.addListener(
+          function(data) {
+        var type = chrome.developerPrivate.EventType;
+        if ((data.event_type == type.ERRORS_REMOVED ||
+             data.event_type == type.ERROR_ADDED) &&
+            data.info.id == this.extensionId_) {
+          var newErrors =
+              data.info.runtimeErrors.concat(data.info.manifestErrors);
+          this.updateErrors_(newErrors);
+        }
+      }.bind(this));
+
+      /**
+       * The active error element in the list.
+       * @private {?}

[Devlin, 2015/03/26 21:42:00]

Making this @private {ExtensionError} results in "ERROR - Bad type annotation.
Unknown type ExtensionError", even though the function on 37 is marked as a
constructor and the prototype is defined on line 44...any ideas?

[Dan Beam, 2015/03/31 00:33:50]

extensions.ExtensionError

[Devlin, 2015/03/31 16:17:50]

Thought of that and tried it, same error (well, "Unknown type
extensions.ExtensionError")

+       */
+      this.activeError_ = null;
+
+      this.setActiveError(0);
+    },
+
+    /**
+     * Adds an error to the list.
+     * @param {(RuntimeError|ManifestError)} error The error to add.
+     * @private
+     */
+    addError_: function(error) {
+      this.errors_.push(error);
+      var focusRow = new ExtensionError(error, this.gridBoundary_);
+      this.gridBoundary_.appendChild(document.createElement('li')).
+          appendChild(focusRow);
+      this.focusGrid_.addRow(focusRow);
+    },
+
+    /**
+     * Removes an error from the list.
+     * @param {(RuntimeError|ManifestError)} error The error to remove.
+     * @private
+     */
+    removeError_: function(error) {
+      var index = this.errors_.indexOf(error);
+      var errorList = this.querySelector('.extension-error-list-contents');
+
+      var wasActive = this.activeError_ && this.activeError_.error == error;
+
+      this.errors_.splice(index, 1);
+      var listElement = errorList.children[index];
+      listElement.parentNode.removeChild(listElement);
+
+      if (wasActive) {
+        index = Math.min(index, this.errors_.length - 1);

[Dan Beam, 2015/03/31 00:33:50]

what if this.errors_.length == 0?  does -1 work?

[Devlin, 2015/03/31 16:17:50]

Yep, line 274 (in this patch set) checks the index.

+        this.setActiveError(index);
+      }
+
+      chrome.developerPrivate.deleteExtensionErrors({
+        extensionId: error.extensionId,
+        errorIds: [error.id]
+      });
+    },
+
+    /**
+     * Updates the list of errors.
+     * @param {Array<(ManifestError|RuntimeError)>} newErrors The new list of
+     *     errors.
+     * @private
+     */
+    updateErrors_: function(newErrors) {

[Dan Beam, 2015/03/31 00:33:49]

how often is this called?  currently O(N) of M*N it seems :/ (M=list size,
N=newErrors size).  how big can M*N get?

[Devlin, 2015/03/31 16:17:50]

Yeah, right now it's M*N.  M and N are each capped at 100.  It can be called
each time an extension throws an error, or each time an error is deleted (the
latter is gated on user action, the former is not).

We could, technically, reduce this to O(N) = N trivially by deleting the current
error list and completely recreating it, but I think the cost of deleting 100 UI
elements and recreating 100 UI elements could be >100x more expensive than
comparing an id attribute.  Additionally, this way we preserve the existing DOM,
which I think is important for a11y.

Note also that in the "normal" case, this will not reach 100 or be called
terribly frequently - that would be a very broken extension (which we should
handle, but for it to be a little rough, I think, is reasonable).

+      var listHasError = function(list, id) {
+        for (var i = 0; i < list.length; ++i) {
+          if (list[i].id == id)
+            return true;
         }
+        return false;
+      };
+      this.errors_.forEach(function(error) {
+        if (!listHasError(newErrors, error.id))
+          this.removeError_(error);
+      }, this);
+      newErrors.forEach(function(error) {
+        if (!listHasError(this.errors_, error.id))
+          this.addError_(error);
       }, this);
+    },
 
-      var numShowing = this.focusGrid_.rows.length;
-      if (numShowing > ExtensionErrorList.MAX_ERRORS_TO_SHOW_)
-        this.initShowMoreLink_();
+    /**
+     * Sets the active error in the list.
+     * @param {number} index The index to set to be active.
+     */
+    setActiveError: function(index) {
+      var errorList = this.querySelector('.extension-error-list-contents');
+      var node = null;

[Dan Beam, 2015/03/31 00:33:49]

var node = errorList.children[index];
if (node)
  this.setActiveErrorNode_(node.querySelector('.extension-error-metadata));

[Devlin, 2015/03/31 16:17:50]

That's actually different.  setActiveErrorNode_() treats a null "active node" as
setting the active node to none, which is important (in the case when we need to
not have one).

+      if (index >= 0 && index < errorList.children.length) {
+        node = errorList.children[index].querySelector(
+                   '.extension-error-metadata');
+      }
+      this.setActiveErrorNode_(node);
     },
 
     /**
-     * @return {?Element} The element that toggles between show more and show
-     *     less, or null if it's hidden. Button will be hidden if there are less
-     *     errors than |MAX_ERRORS_TO_SHOW_|.
+     * Clears the list of all errors.
      */
-    getToggleElement: function() {
-      return this.querySelector(
-          '.extension-error-list-show-more [is="action-link"]:not([hidden])');
+    clear: function() {
+      if (this.errors_.length == 0)
+        return;
+
+      var ids = [];
+      this.errors_.forEach(function(error) { ids.push(error.id); });

[Dan Beam, 2015/03/31 00:33:50]

var ids = this.errors_.map(function(error) { return error.id; });

[Devlin, 2015/03/31 16:17:50]

Done.

+      chrome.developerPrivate.deleteExtensionErrors({
+        extensionId: this.extensionId_,
+        errorIds: ids
+      });
+
+      this.setActiveErrorNode_(null);
+      this.errors_.splice(0, this.errors_.length);

[Dan Beam, 2015/03/31 00:33:49]

this.errors_.length = 0;

[Devlin, 2015/03/31 16:17:51]

How have I never seen this?  Awesome.

+      var errorList = this.querySelector('.extension-error-list-contents');
+      while (errorList.firstChild)
+        errorList.removeChild(errorList.firstChild);

[Dan Beam, 2015/03/31 00:33:50]

this.querySelector('.extension-error-list-contents').innerHTML = '';

[Devlin, 2015/03/31 16:17:50]

Pushing back on this.  Two reasons:
- *** It seems to be so much faster.  There are dozens (insert meme here) if not
hundreds of perfs on this, but here's one:
http://jsperf.com/innerhtml-vs-removechild/290 (there are a lot in that same
series).  I have no idea what we're doing with innerHTML in blink, but it's 100x
slower.
- innerHTML is just kind of yucky in general, and I wouldn't be surprised if we
disallow it in chrome at some point out of principle (we already do this in
other projects, and I think there may even be a closure_compiler rule to enforce
it :))

     },
 
-    /** @return {!Element} The element containing the list of errors. */
-    getErrorListElement: function() {
-      return this.gridBoundary_;
+    /**
+     * Sets the active error in the list.
+     * @param {?} node The error to make active.
+     * @private
+     */
+    setActiveErrorNode_: function(node) {
+      if (this.activeError_)
+        this.activeError_.classList.remove('extension-error-active');

[Dan Beam, 2015/03/31 00:33:49]

nit: \n

[Devlin, 2015/03/31 16:17:50]

Done.

+      if (node)
+        node.classList.add('extension-error-active');

[Dan Beam, 2015/03/31 00:33:50]

nit: \n

[Devlin, 2015/03/31 16:17:50]

Done.

+      this.activeError_ = node;
+
+      this.dispatchEvent(
+          new CustomEvent('activeExtensionErrorChanged',
+                          { bubbles: true, detail: node ? node.error : null }));
     },
 
     /**
@@ -188,65 +332,8 @@ cr.define('extensions', function() {
      */
     onFocus_: function() {
       var activeRow = this.gridBoundary_.querySelector('.focus-row-active');
-      var toggleButton = this.getToggleElement();
-
-      if (toggleButton && !toggleButton.isShowingAll) {
-        var rows = this.focusGrid_.rows;
-        assert(rows.length > ExtensionErrorList.MAX_ERRORS_TO_SHOW_);
-
-        var firstVisible = rows.length - ExtensionErrorList.MAX_ERRORS_TO_SHOW_;
-        if (rows.indexOf(activeRow) < firstVisible)
-          activeRow = rows[firstVisible];
-      } else if (!activeRow) {
-        activeRow = this.focusGrid_.rows[0];
-      }
-
       activeRow.getEquivalentElement(null).focus();
     },
-
-    /**
-     * Initialize the "Show More" link for the error list. If there are more
-     * than |MAX_ERRORS_TO_SHOW_| errors in the list.
-     * @private
-     */
-    initShowMoreLink_: function() {
-      var link = this.querySelector(
-          '.extension-error-list-show-more [is="action-link"]');
-      link.hidden = false;
-      link.isShowingAll = false;
-
-      var listContents = this.querySelector('.extension-error-list-contents');
-
-      // TODO(dbeam/kalman): trade all this transition voodoo for .animate()?
-      listContents.addEventListener('webkitTransitionEnd', function(e) {
-        if (listContents.classList.contains('deactivating'))
-          listContents.classList.remove('deactivating', 'active');
-        else
-          listContents.classList.add('scrollable');
-      });
-
-      link.addEventListener('click', function(e) {
-        // Needs to be enabled in case the focused row is now hidden.
-        this.gridBoundary_.tabIndex = 0;
-
-        link.isShowingAll = !link.isShowingAll;
-
-        var message = link.isShowingAll ? 'extensionErrorsShowFewer' :
-                                          'extensionErrorsShowMore';
-        link.textContent = loadTimeData.getString(message);
-
-        // Disable scrolling while transitioning. If the element is active,
-        // scrolling is enabled when the transition ends.
-        listContents.classList.remove('scrollable');
-
-        if (link.isShowingAll) {
-          listContents.classList.add('active');
-          listContents.classList.remove('deactivating');
-        } else {
-          listContents.classList.add('deactivating');
-        }
-      }.bind(this));
-    }
   };
 
   return {