[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..68fba68bcfefbafa2c1d61a9b10633393b37ea1a 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;
+      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) {

[Dan Beam, 2015/04/22 21:02:38]

this code is out of date

[Devlin, 2015/04/22 23:17:31]

Resynced.

       cr.ui.FocusRow.prototype.decorate.call(this, boundary);
 
+      /**
+       * The backing error.
+       * @type {(ManifestError|RuntimeError)}
+       */
+      this.error = error;
+
       // 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}));
+      }.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.keyIdentifier == 'Enter')

[Dan Beam, 2015/04/22 21:02:38]

probably need target != deleteButton here as well

[Devlin, 2015/04/22 23:17:31]

Done.

+          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,137 +136,205 @@ 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);
         }
-      }, this);
+      }.bind(this));
+
+      /**
+       * The active error element in the list.
+       * @private {?}
+       */
+      this.activeError_ = null;
 
-      var numShowing = this.focusGrid_.rows.length;
-      if (numShowing > ExtensionErrorList.MAX_ERRORS_TO_SHOW_)
-        this.initShowMoreLink_();
+      this.setActiveError(0);
     },
 
     /**
-     * @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_|.
+     * Adds an error to the list.
+     * @param {(RuntimeError|ManifestError)} error The error to add.
+     * @private
      */
-    getToggleElement: function() {
-      return this.querySelector(
-          '.extension-error-list-show-more [is="action-link"]:not([hidden])');
-    },
-
-    /** @return {!Element} The element containing the list of errors. */
-    getErrorListElement: function() {
-      return this.gridBoundary_;
+    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);
     },
 
     /**
-     * The grid should not be focusable once it or an element inside it is
-     * focused. This is necessary to allow tabbing out of the grid in reverse.
+     * Removes an error from the list.
+     * @param {(RuntimeError|ManifestError)} error The error to remove.
      * @private
      */
-    onFocusin_: function() {
-      this.gridBoundary_.tabIndex = -1;
+    removeError_: function(error) {
+      var index = this.errors_.indexOf(error);

[Dan Beam, 2015/04/22 21:02:38]

using references as IDs is kinda dubious.  will errors never be copied or
regenerated?

[Devlin, 2015/04/22 23:17:31]

Hmm... in theory, no, but in a pass-by-reference language, I'm not willing to
bet on it.  Changed.

+      var errorList = this.querySelector('.extension-error-list-contents');
+
+      var wasActive = this.activeError_ && this.activeError_.error == error;

[Dan Beam, 2015/04/22 21:02:38]

same dubiousness with == across objects

[Devlin, 2015/04/22 23:17:31]

Done.

+
+      this.errors_.splice(index, 1);
+      var listElement = errorList.children[index];
+      listElement.parentNode.removeChild(listElement);
+
+      if (wasActive) {
+        index = Math.min(index, this.errors_.length - 1);
+        this.setActiveError(index);  // Gracefully handles the -1 case.
+      }
+
+      chrome.developerPrivate.deleteExtensionErrors({
+        extensionId: error.extensionId,
+        errorIds: [error.id]
+      });
     },
 
     /**
-     * Focus the first focusable row when tabbing into the grid for the
-     * first time.
+     * Updates the list of errors.
+     * @param {Array<(ManifestError|RuntimeError)>} newErrors The new list of
+     *     errors.
      * @private
      */
-    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_);
+    updateErrors_: function(newErrors) {
+      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 firstVisible = rows.length - ExtensionErrorList.MAX_ERRORS_TO_SHOW_;
-        if (rows.indexOf(activeRow) < firstVisible)
-          activeRow = rows[firstVisible];
-      } else if (!activeRow) {
-        activeRow = this.focusGrid_.rows[0];
+    /**
+     * 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;
+      if (index >= 0 && index < errorList.children.length) {

[Dan Beam, 2015/04/22 21:02:38]

nit:

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

[Devlin, 2015/04/22 23:17:31]

We also want to call setActiveErrorNode_ with null if there isn't one, so
adjusted slightly to
this.setActiveErrorNode_(
    item ? item.querySelector('.extension-error-metadata') : null);

+        node = errorList.children[index].querySelector(
+                   '.extension-error-metadata');

[Dan Beam, 2015/04/22 21:02:38]

nit:

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

[Devlin, 2015/04/22 23:17:31]

Moot with above, but noted. :)

       }
+      this.setActiveErrorNode_(node);
+    },
 
-      activeRow.getEquivalentElement(null).focus();
+    /**
+     * Clears the list of all errors.
+     */
+    clear: function() {
+      if (this.errors_.length == 0)
+        return;
+
+      var ids = this.errors_.map(function(error) { return error.id; });
+      chrome.developerPrivate.deleteExtensionErrors({
+        extensionId: this.extensionId_,
+        errorIds: ids
+      });
+
+      this.setActiveErrorNode_(null);
+      this.errors_.length = 0;
+      var errorList = this.querySelector('.extension-error-list-contents');

[Dan Beam, 2015/04/22 21:02:38]

nit:

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

?

[Devlin, 2015/04/22 23:17:31]

Where'd that comment go....
https://codereview.chromium.org/1016413004/diff/180001/chrome/browser/resourc...

But that definitely proves you are consistent. ;)

+      while (errorList.firstChild)
+        errorList.removeChild(errorList.firstChild);
     },
 
     /**
-     * Initialize the "Show More" link for the error list. If there are more
-     * than |MAX_ERRORS_TO_SHOW_| errors in the list.
+     * Sets the active error in the list.
+     * @param {?} node The error to make active.
      * @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');
-      });
+    setActiveErrorNode_: function(node) {
+      if (this.activeError_)
+        this.activeError_.classList.remove('extension-error-active');
 
-      link.addEventListener('click', function(e) {
-        // Needs to be enabled in case the focused row is now hidden.
-        this.gridBoundary_.tabIndex = 0;
+      if (node)
+        node.classList.add('extension-error-active');
 
-        link.isShowingAll = !link.isShowingAll;
+      this.activeError_ = node;
 
-        var message = link.isShowingAll ? 'extensionErrorsShowFewer' :
-                                          'extensionErrorsShowMore';
-        link.textContent = loadTimeData.getString(message);
+      this.dispatchEvent(
+          new CustomEvent('activeExtensionErrorChanged',
+                          {bubbles: true, detail: node ? node.error : null}));
+    },
 
-        // Disable scrolling while transitioning. If the element is active,
-        // scrolling is enabled when the transition ends.
-        listContents.classList.remove('scrollable');
+    /**
+     * The grid should not be focusable once it or an element inside it is
+     * focused. This is necessary to allow tabbing out of the grid in reverse.
+     * @private
+     */
+    onFocusin_: function() {
+      this.gridBoundary_.tabIndex = -1;
+    },
 
-        if (link.isShowingAll) {
-          listContents.classList.add('active');
-          listContents.classList.remove('deactivating');
-        } else {
-          listContents.classList.add('deactivating');
-        }
-      }.bind(this));
-    }
+    /**
+     * Focus the first focusable row when tabbing into the grid for the
+     * first time.
+     * @private
+     */
+    onFocus_: function() {
+      var activeRow = this.gridBoundary_.querySelector('.focus-row-active');
+      activeRow.getEquivalentElement(null).focus();
+    },
   };
 
   return {