[Extensions] Update Error Console UI

chrome/browser/resources/extensions/extension_error.js

 // Copyright 2013 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.
 
 cr.define('extensions', function() {
   'use strict';
 
   /**
    * Clone a template within the extension error template collection.
    * @param {string} templateName The class name of the template to clone.
@@ skipping 28 matching lines @@
     div.__proto__ = ExtensionError.prototype;
     div.decorate(error, boundary);
     return div;
   }
 
   ExtensionError.prototype = {
     __proto__: cr.ui.FocusRow.prototype,
 
     /** @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;
     },
 
     /**
      * @param {(RuntimeError|ManifestError)} error The error the element should
      *     represent
      * @param {Element} boundary The boundary for the FocusGrid.
      * @override
      */
     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) {
           case chrome.developerPrivate.ErrorLevel.LOG:
             this.classList.add('extension-error-severity-info');
             break;
           case chrome.developerPrivate.ErrorLevel.WARN:
             this.classList.add('extension-error-severity-warning');
             break;
           case chrome.developerPrivate.ErrorLevel.ERROR:
             this.classList.add('extension-error-severity-fatal');
             break;
           default:
             assertNotReached();
         }
       } else {
         // We classify manifest errors as "warnings".
         this.classList.add('extension-error-severity-warning');
       }
 
       var iconNode = document.createElement('img');
       iconNode.className = 'extension-error-icon';
       // TODO(hcarmona): Populate alt text with a proper description since this
       // icon conveys the severity of the error. (info, warning, fatal).
       iconNode.alt = '';
       this.insertBefore(iconNode, this.firstChild);
 
       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 }));
     },
   };
 
   /**
    * 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));
 
-      var numShowing = this.focusGrid_.rows.length;
-      if (numShowing > ExtensionErrorList.MAX_ERRORS_TO_SHOW_)
-        this.initShowMoreLink_();
+      /**
+       * 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);
     },
 
     /**
-     * @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);
     },
 
     /**
+     * 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);
+    },
+
+    /**
+     * 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);
+    },
+
+    /**
+     * Clears the list of all errors.
+     */
+    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 :))

+    },
+
+    /**
+     * 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 }));
+    },
+
+    /**
      * 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;
     },
 
     /**
      * 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');
-      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 {
     ExtensionErrorList: ExtensionErrorList
   };
 });