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.
 
+document.addEventListener('DOMContentLoaded', function() {
+  chrome.send('extensionLoaderDisplayFailures');
+});
+
+window.addEventListener('beforeunload', function() {

[Devlin, 2014/06/30 21:06:42]

Actually, I'm wondering if this wouldn't be better on the C++ side.  We don't
typically use the low-level DOM functions like these (partly because we have
complicated settings pages - frames in frames, etc).

Could we put the ready signal inside the constructor for ExtensionLoader, and
move the "not ready" to the C++ when learn the page is navigating?  (This will
be the same time that we reload all extensions for the chrome:extensions page).

[gpdavis, 2014/06/30 21:41:19]

Will the ExtensionLoader be constructed after the DOM is ready?  The problem I
was running into was messages being sent before the page was ready to receive
them (var extension didn't exist when the message was received).

I wasn't sure how to determine if the page was loading from the C++ side.  How
would I go about doing that?

[Devlin, 2014/06/30 22:32:36]

Well, since extensions are loaded on page load, we can listen to that same event
for when the page is probably loading, right? :)  See the other comment for when
that happens.

I think that any messages sent from the constructor of ExtensionLoader will be
sent after the extensions object is created, because ExtensionLoader is entirely
within extensions.  Give it a shot and see if it works.  If not, we'll revisit
it.

[gpdavis, 2014/06/30 23:35:11]

Wait, are you talking about the Extension Loader Handler constructor?  Because
that object is constructed on a page load, but isn't destroyed and reconstructed
on a refresh.

Also, I should point out that extensions are not reloaded on chrome://extensions
page loads, only reloads.  So if the manifest is broken between starting up
chrome and loading the extensions page, it won't show up until a page refresh of
chrome://extensions, or an explicit reload of the extensions.

[Devlin, 2014/07/01 17:02:55]

I was talking about the ExtensionLoader JS class.
That should be fine - for now, let's worry about matching current behavior with
the new UI.

[gpdavis, 2014/07/01 20:58:55]

I replaced the DOMContentLoaded listener with a message sent at the end of init,
and it still works.  I'm not sure what to do about the beforeunload listener,
though.  Perhaps a GetInstanceOf method in the Extension Loader Handler would
make this a simple matter of getting the instance and making a function call
that will signal that the display isn't ready?

[Devlin, 2014/07/01 22:14:17]

Can we not just unset it when the page is reloading, and reset it when it's
recreated?

[gpdavis, 2014/07/01 23:50:59]

Well, that is the goal.  I figured adding a listener here would be the most
direct way to ensure that it's set to false when the page begins reloading.  Not
sure where else to put it, unless it's put in the C++ where all the extensions
are reloaded, which would require an instance getter, as I suggested.

Unless I'm missing something fundamental, here, which is most certainly
possible.

[Devlin, 2014/07/02 17:46:49]

Can we not just listen for the same signal that ExtensionSettingsHandler does
for its reload?

+  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;
@@ skipping 16 matching lines @@
       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 information about additional load failures.
+       * @type {HTMLPreElement}
+       * @private
+       */
+      this.additional_ =
+          this.querySelector('#extension-load-error-additional');
+
+      /**
        * The element which displays the manifest code.
        * @type {ExtensionCode}
        * @private
        */
       this.manifest_ = new extensions.ExtensionCode(
           this.querySelector('#extension-load-error-manifest'));
 
       this.querySelector('#extension-load-error-retry-button').addEventListener(
           'click', function(e) {
         chrome.send('extensionLoaderRetry');
@@ skipping 18 matching lines @@
 
       manifest.message = reason;
       this.manifest_.populate(
           manifest,
           loadTimeData.getString('extensionLoadCouldNotLoadManifest'));
       this.hidden = false;
       this.manifest_.scrollToError();
     },
 
     /**
+     * Display additional load failures to the user.

[Devlin, 2014/06/30 21:06:41]

We should be able to clean this up when we move all of it into one message to
send to the frontend.

+     * @param {string} failures The error message listing paths of additional
+     *     extensions that failed to load.
+     */
+    showAdditional: function(failures) {
+      this.additional_.textContent = failures;
+    },
+
+    /**
      * Hide the extension load error.
      * @private
      */
     hide_: function() {
       this.hidden = true;
     }
   };
 
   /**
    * The ExtensionLoader is the class in charge of loading unpacked extensions.
@@ skipping 26 matching lines @@
      * @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);
+    },
+
+    /**
+     * Notify the ExtensionLoader of additional failures.
+     * @param {string} filePath The path to the unpacked extension.
+     * @param {string} failures The error message listing paths of additional
+     *     extensions that failed to load.
+     */
+    notifyAdditionalFailures: function(failures) {
+      this.loadError_.showAdditional(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.
    * @see ExtensionLoader.notifyFailed
    */
   ExtensionLoader.notifyLoadFailed = function(filePath, reason, manifest) {
     ExtensionLoader.getInstance().notifyFailed(filePath, reason, manifest);
   };
 
+  /*
+   * A static forwarding function for ExtensionLoader.notifyFailed.
+   * @param {string} failures The error message listing paths of additional
+   *     extensions that failed to load.
+   * @see ExtensionLoader.notifyAdditionalFailures
+   */
+  ExtensionLoader.notifyAdditional = function(failures) {
+    ExtensionLoader.getInstance().notifyAdditionalFailures(failures);
+  };
+
   return {
     ExtensionLoader: ExtensionLoader
   };
 });