Show alert failure for reloading unpacked extensions with bad manifest

chrome/browser/resources/extensions/extension_loader.js

 // Copyright 2014 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.
 
+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';
 
   /**
    * Construct an ExtensionLoadError around the given |div|.
    * @param {HTMLDivElement} div The HTML div for the extension load error.
    * @constructor
    */
   function ExtensionLoadError(div) {
     div.__proto__ = ExtensionLoadError.prototype;
     div.init();
     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,
 
     /**
      * Initialize the ExtensionLoadError div.
      */
     init: function() {
       /**
        * The element which displays the path of the extension.
        * @type {HTMLSpanElement}
        * @private
        */
       this.path_ = this.querySelector('#extension-load-error-path');
 
       /**
        * The element which displays the reason the extension failed to load.
        * @type {HTMLSpanElement}
        * @private
        */
       this.reason_ = this.querySelector('#extension-load-error-reason');
 
       /**
        * The element which displays the manifest code.
        * @type {ExtensionCode}
        * @private
        */
       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;
+    /**
+     * 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_();
+      }
+    },
+
+    /**
+     * 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
+     */
+    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(
-          manifest,
+          failure.manifest,
           loadTimeData.getString('extensionLoadCouldNotLoadManifest'));
       this.hidden = false;
       this.manifest_.scrollToError();
-    },
 
-    /**
-     * Hide the extension load error.
-     * @private
-     */
-    hide_: function() {
-      this.hidden = true;
+      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;
+      }
     }
   };
 
   /**
    * The ExtensionLoader is the class in charge of loading unpacked extensions.
    * @constructor
    */
   function ExtensionLoader() {
     /**
      * The ExtensionLoadError to show any errors from loading an unpacked
@@ skipping 10 matching lines @@
     /**
      * Begin the sequence of loading an unpacked extension. If an error is
      * encountered, this object will get notified via notifyFailed().
      */
     loadUnpacked: function() {
       chrome.send('extensionLoaderLoadUnpacked');
     },
 
     /**
      * 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,
      *     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.
      */
     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 {
     ExtensionLoader: ExtensionLoader
   };
 });