Show alert failure for reloading unpacked extensions with bad manifest

chrome/browser/resources/extensions/extension_loader.js

Index: chrome/browser/resources/extensions/extension_loader.js
diff --git a/chrome/browser/resources/extensions/extension_loader.js b/chrome/browser/resources/extensions/extension_loader.js
index fb57c18b139075cba35b00c733df28187052c4c9..5fb0d5e01c52c688c1181dad417d84372ff0bd42 100644
--- a/chrome/browser/resources/extensions/extension_loader.js
+++ b/chrome/browser/resources/extensions/extension_loader.js
@@ -16,6 +16,26 @@ cr.define('extensions', function() {
     return div;
   }
 
+  /**
+   * Construct a Failure.
+   * @param {string} filePath The path to the unpacked extension.
+   * @param {string} reason The reason the extension failed to load.
+   * @param {Object} manifest An object with three strings: beforeHighlight,
+   *     afterHighlight, and highlight. These represent three portions of the
+   *     file's content to display - the portion which is most relevant and
+   *     should be emphasized (highlight), and the parts both before and after
+   *     this portion. These may be empty.
+   * @param {Element} listElement The HTML element used for displaying the

[Devlin, 2014/07/09 23:03:32]

HTMLLIElement

[gpdavis, 2014/07/10 00:59:25]

Done.

+   *     failure path for the additional failures UI.
+   * @constructor
+   */
+  function Failure(filePath, reason, manifest, listElement) {
+    this.path = filePath;
+    this.reason = reason;
+    this.manifest = manifest;
+    this.listElement = listElement;
+  }
+
   ExtensionLoadError.prototype = {
     __proto__: HTMLDivElement.prototype,
 
@@ -45,42 +65,99 @@ cr.define('extensions', function() {
       this.manifest_ = new extensions.ExtensionCode(
           this.querySelector('#extension-load-error-manifest'));
 
+      /**
+       * The element which displays information about additional errors.
+       * @type {HTMLPreElement}

[Devlin, 2014/07/09 23:03:32]

Update.

[gpdavis, 2014/07/10 00:59:25]

Done.

+       * @private
+       */
+      this.additional_ = this.querySelector('#extension-load-error-additional');
+
+      /**
+       * An array of Failures for keeping track of multiple active failures.
+       * @type {Array.<Failure>}
+       * @private
+       */
+      this.failures_ = [];
+
       this.querySelector('#extension-load-error-retry-button').addEventListener(
           'click', function(e) {
         chrome.send('extensionLoaderRetry');
-        this.hide_();
+        this.remove_();
       }.bind(this));
 
       this.querySelector('#extension-load-error-give-up-button').
           addEventListener('click', function(e) {
-        this.hide_();
+        chrome.send('extensionLoaderIgnoreFailure');
+        this.remove_();
       }.bind(this));
+
+      chrome.send('extensionLoaderDisplayFailures');
     },
 
     /**
-     * Display the load error to the user.
-     * @param {string} path The path from which the extension was loaded.
-     * @param {string} reason The reason the extension failed to load.
-     * @param {string} manifest The manifest object, with highlighted regions.
+     * Add a failure to failures_ array. If there is already a displayed
+     * failure, display the additional failures element.
+     * @param {Array.<Object>} failures Array of failures containing paths,
+     *     errors, manifests, and listElements.
+     * @private
      */
-    show: function(path, reason, manifest) {
-      this.path_.textContent = path;
-      this.reason_.textContent = reason;
+    add_: function(failures) {
+      // If a failure is already being displayed, unhide the last item.
+      if (this.failures_.length > 0)
+        this.failures_[this.failures_.length - 1].listElement.hidden = false;
+      for (var i = 0; i < failures.length; ++i) {
+        var listItem = document.createElement('li');
+        listItem.textContent = failures[i].path;
+        this.additional_.appendChild(listItem);
+        failures[i].listElement = listItem;
+        this.failures_.push(failures[i]);
+      }
+      // Hide the last item because the UI is displaying its information.
+      failures[failures.length - 1].listElement.hidden = true;

[Devlin, 2014/07/09 23:03:32]

I'd prefer you use |failures_| for this (even though they will be the same, it
makes it clearer, as you want to mark it for the last element of failures_,
which just happens to also be the last element of failures).

[gpdavis, 2014/07/10 00:59:26]

Fair enough.  I chose to use failures just because it was less cluttered because
it doesn't need this.

+      this.show_();
+    },
 
-      manifest.message = reason;
-      this.manifest_.populate(
-          manifest,
-          loadTimeData.getString('extensionLoadCouldNotLoadManifest'));
-      this.hidden = false;
-      this.manifest_.scrollToError();
+    /**
+     * Remove a failure from |failures_| array. If this was the last failure,
+     * hide the error UI.  If this was the last additional failure, hide
+     * the additional failures UI.
+     * @private
+     */
+    remove_: function() {
+      this.additional_.removeChild(
+          this.failures_[this.failures_.length - 1].listElement);
+      this.failures_.pop();
+      if (this.failures_.length > 0)

[Devlin, 2014/07/09 23:03:32]

Why three ifs?
if (this.failures_.length > 0) {
  this.failures_[this.failures_.length - 1].listElement.hidden = true;
  this.show_();
} else {
  this.hidden = true;
}

[gpdavis, 2014/07/10 00:59:26]

Good point... this is the result of adding conditionals in the middle of a train
of thought.  Messy logic.  Fixed.

+        this.failures_[this.failures_.length - 1].listElement.hidden = true;
+      if (this.failures_.length == 0)
+        this.hidden = true;
+      else
+        this.show_();
     },
 
     /**
-     * Hide the extension load error.
+     * Display the load error to the user. The last failure gets its manifest
+     * and error displayed, while additional failures have their path names
+     * displayed in the additional failures element.
      * @private
      */
-    hide_: function() {
-      this.hidden = true;
+    show_: function() {
+      assert(this.failures_.length >= 1);
+      var failure = this.failures_[this.failures_.length - 1];
+      this.path_.textContent = failure.path;
+      this.reason_.textContent = failure.reason;
+
+      failure.manifest.message = failure.reason;
+      this.manifest_.populate(
+          failure.manifest,
+          loadTimeData.getString('extensionLoadCouldNotLoadManifest'));
+      this.hidden = false;
+      this.manifest_.scrollToError();
+
+      if (this.failures_.length == 1)

[Devlin, 2014/07/09 23:03:32]

this.additional_.hidden = this.failures_.length == 1;

[gpdavis, 2014/07/10 00:59:25]

Nice.

+        this.additional_.hidden = true;
+      else
+        this.additional_.hidden = false;
     }
   };
 
@@ -111,29 +188,23 @@ cr.define('extensions', function() {
 
     /**
      * Notify the ExtensionLoader that loading an unpacked extension failed.
-     * Show the ExtensionLoadError.
-     * @param {string} filePath The path to the unpacked extension.
-     * @param {string} reason The reason the extension failed to load.
-     * @param {Object} manifest An object with three strings: beforeHighlight,
-     *     afterHighlight, and highlight. These represent three portions of the
-     *     file's content to display - the portion which is most relevant and
-     *     should be emphasized (highlight), and the parts both before and after
-     *     this portion. These may be empty.
+     * Add the failure to failures_ and show the ExtensionLoadError.
+     * @param {Array.<Object>} failures Array of failures containing paths,
+     *     errors, manifests, and listElements.

[Devlin, 2014/07/09 23:03:32]

It doesn't contain listElements, though, right?  (Same for the other places you
mention this.)

[gpdavis, 2014/07/10 00:59:26]

Oops.  You're right; these objects aren't yet Failures.

      */
-    notifyFailed: function(filePath, reason, manifest) {
-      this.loadError_.show(filePath, reason, manifest);
-    }
+    notifyFailed: function(failures) {
+      this.loadError_.add_(failures);
+    },
   };
 
   /*
    * A static forwarding function for ExtensionLoader.notifyFailed.
-   * @param {string} filePath The path to the unpacked extension.
-   * @param {string} reason The reason the extension failed to load.
-   * @param {Object} manifest The manifest of the failed extension.
+   * @param {Array.<Object>} failures Array of failures containing paths,
+   *     errors, manifests, and listElements.
    * @see ExtensionLoader.notifyFailed
    */
-  ExtensionLoader.notifyLoadFailed = function(filePath, reason, manifest) {
-    ExtensionLoader.getInstance().notifyFailed(filePath, reason, manifest);
+  ExtensionLoader.notifyLoadFailed = function(failures) {
+    ExtensionLoader.getInstance().notifyFailed(failures);
   };
 
   return {