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..ee20f1a0695200a36fdecbe3a24e9b9a24aa9415 100644
--- a/chrome/browser/resources/extensions/extension_loader.js
+++ b/chrome/browser/resources/extensions/extension_loader.js
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+window.addEventListener('beforeunload', function() {

[Devlin, 2014/07/07 20:44:21]

Didn't we have a way of potentially getting rid of this?

[gpdavis, 2014/07/09 01:35:57]

I gave it a shot, and it didn't seem to work properly.  I don't remember exactly
what it was that went wrong.  This seems like the most direct and least messy
way to go about doing this.  Is this something that really should not be here? 
I can try and replace it with something else, but if I recall correctly, I
couldn't find a place for it that would work.

[Devlin, 2014/07/09 19:53:17]

I highly doubt that this is the best solution.  The settings page is incredibly
complex.  Which window does |window| refer to?  What if the frame is destroyed,
but not the window?  What happens to extensions?  Or the modifications to the
now-defunct DOM?

Can you find out what it was that didn't work with the other approach?

[gpdavis, 2014/07/09 21:16:25]

Well, I figured out what I attempted before, but this time it worked.  Must have
just made a mistake last time and wasn't sure what to do when it blew up. 
Problem solved now: I made the loader handler into a WebContentsObserver and had
it override the same function that the settings handler uses to trigger the
refresh.  Works just fine.

+  chrome.send('extensionLoaderSetDisplayLoading');
+});
+
 cr.define('extensions', function() {
   'use strict';
 
@@ -16,6 +20,19 @@ 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 The manifest of the failed extension.
+   * @constructor
+   */
+  function Failure(filePath, reason, manifest) {
+    this.path = filePath;
+    this.reason = reason;
+    this.manifest = manifest;
+  }
+
   ExtensionLoadError.prototype = {
     __proto__: HTMLDivElement.prototype,
 
@@ -45,42 +62,89 @@ 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}
+       * @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.
+     * Add a failure to failures_ array. If there is already a displayed
+     * failure, display the additional failures element.
      * @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.
      */
-    show: function(path, reason, manifest) {
-      this.path_.textContent = path;
-      this.reason_.textContent = reason;
+    add: function(path, reason, manifest) {

[Devlin, 2014/07/07 20:44:21]

Can this be private?

[gpdavis, 2014/07/09 01:35:56]

Done.

+      this.failures_.push(new Failure(path, reason, manifest));
+      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,

[Devlin, 2014/07/07 20:44:21]

nit: |failures_|
nit: 1 space after period.

[gpdavis, 2014/07/09 01:35:57]

Done.

+     * hide the error UI.  If this was the last additional failure, hide
+     * the additional failures UI.
+     * @private
+     */
+    remove_: function() {
+      this.failures_.pop();
+      if (this.failures_.length == 0) {

[Devlin, 2014/07/07 20:44:21]

nit: no brackets for one-line ifs.

[gpdavis, 2014/07/09 01:35:57]

Done.

+        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() {
+      var failure = this.failures_[this.failures_.length - 1];

[Devlin, 2014/07/07 20:44:21]

We should assert that there is at least one failure.

[gpdavis, 2014/07/09 01:35:56]

Done.

+      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) {
+        this.additional_.hidden = true;
+      } else if (this.failures_.length > 1) {

[Devlin, 2014/07/07 20:44:21]

We can know that there is >= 1 failure, so a simple "else" is enough.

[gpdavis, 2014/07/09 01:35:57]

Done.

+        this.additional_.hidden = false;
+        this.additional_.textContent =

[Devlin, 2014/07/07 20:44:21]

I think it would be cleaner to have this be a list element.  Helps with the
ARIA, among other things.

[gpdavis, 2014/07/09 01:35:56]

Done.

+            loadTimeData.getString('extensionLoadAdditionalFailures');
+        for (var i = 0; i < this.failures_.length - 1; ++i)
+          this.additional_.textContent += '\n' + this.failures_[i].path;
+      }
     }
   };
 
@@ -111,7 +175,7 @@ cr.define('extensions', function() {
 
     /**
      * Notify the ExtensionLoader that loading an unpacked extension failed.
-     * Show the ExtensionLoadError.
+     * Add the failure to failures_ and 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,
@@ -121,19 +185,21 @@ cr.define('extensions', function() {
      *     this portion. These may be empty.
      */
     notifyFailed: function(filePath, reason, manifest) {
-      this.loadError_.show(filePath, reason, manifest);
-    }
+      this.loadError_.add(filePath, reason, manifest);
+    },
   };
 
   /*
    * 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, and manifests.
    * @see ExtensionLoader.notifyFailed
    */
-  ExtensionLoader.notifyLoadFailed = function(filePath, reason, manifest) {
-    ExtensionLoader.getInstance().notifyFailed(filePath, reason, manifest);
+  ExtensionLoader.notifyLoadFailed = function(failures) {
+    for (var i = 0; i < failures.length; ++i)
+      ExtensionLoader.getInstance().notifyFailed(failures[i].path,

[Devlin, 2014/07/07 20:44:21]

We should add these in batches, because otherwise you could potentially see a
rapid flicker as they are all added (and it's less efficient).

[gpdavis, 2014/07/09 01:35:57]

Done.

+                                                 failures[i].error,
+                                                 failures[i].manifest);
   };
 
   return {