Bug 1330732 - Show install warnings in about:debugging r?jdescottes, r?aswan draft
authorMark Striemer <mstriemer@mozilla.com>
Thu, 29 Jun 2017 11:11:21 -0700
changeset 610006 ba0f74916cbf44a1f27cdd7c392f9835611a1087
parent 601484 2fb0d7773039db790e6fe359c5a5337b6f179e7c
child 637733 10e842a089b0f652e82cf49578c2ff0bbf1e3bd2
push id68751
push userbmo:mstriemer@mozilla.com
push dateMon, 17 Jul 2017 20:00:43 +0000
reviewersjdescottes, aswan
bugs1330732
milestone56.0a1
Bug 1330732 - Show install warnings in about:debugging r?jdescottes, r?aswan MozReview-Commit-ID: 8SFcYuln8w8
devtools/client/aboutdebugging/aboutdebugging.css
devtools/client/aboutdebugging/components/addons/panel.js
devtools/client/aboutdebugging/components/addons/target.js
devtools/client/aboutdebugging/test/addons/test-devtools-webextension-unknown-prop/manifest.json
devtools/client/aboutdebugging/test/browser.ini
devtools/client/aboutdebugging/test/browser_addons_debug_info.js
devtools/client/aboutdebugging/test/head.js
devtools/server/actors/webextension-parent.js
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionParent.jsm
--- a/devtools/client/aboutdebugging/aboutdebugging.css
+++ b/devtools/client/aboutdebugging/aboutdebugging.css
@@ -325,16 +325,35 @@ button {
 }
 
 .addon-target-button:first-of-type {
   /* Subtract the start padding so the button is still a bigger click target but
    * lines up with the icon. */
   margin-inline-start: -4px;
 }
 
+.addon-target-messages {
+  border-bottom: 1px solid rgba(0, 0, 0, 0.2);
+  margin-bottom: 16px;
+  padding: 0 0 12px 0;
+}
+
+.addon-target-message {
+  list-style-type: none;
+  padding: 4px 0;
+}
+
+.addon-target-message:first-of-type {
+  padding-top: 0;
+}
+
+.addon-target-warning-message {
+  color: #a47f00;
+}
+
 /* We want the ellipsis on the left-hand-side, so make the parent RTL
  * with an ellipsis and the child can be LTR. */
 .file-path {
   direction: rtl;
   max-width: 100%;
   overflow: hidden;
   text-overflow: ellipsis;
   white-space: nowrap;
--- a/devtools/client/aboutdebugging/components/addons/panel.js
+++ b/devtools/client/aboutdebugging/components/addons/panel.js
@@ -1,15 +1,16 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 const { AddonManager } = require("resource://gre/modules/AddonManager.jsm");
+const { Management } = require("resource://gre/modules/Extension.jsm");
 const { createFactory, createClass, DOM: dom, PropTypes } =
   require("devtools/client/shared/vendor/react");
 const Services = require("Services");
 
 const AddonsControls = createFactory(require("./controls"));
 const AddonTarget = createFactory(require("./target"));
 const PanelHeader = createFactory(require("../panel-header"));
 const TargetList = createFactory(require("../target-list"));
@@ -38,28 +39,33 @@ module.exports = createClass({
     return {
       extensions: [],
       debugDisabled: false,
     };
   },
 
   componentDidMount() {
     AddonManager.addAddonListener(this);
+    // Listen to startup since that's when errors and warnings
+    // get populated on the extension.
+    Management.on("startup", this.updateAddonsList);
 
     Services.prefs.addObserver(CHROME_ENABLED_PREF,
       this.updateDebugStatus);
     Services.prefs.addObserver(REMOTE_ENABLED_PREF,
       this.updateDebugStatus);
 
     this.updateDebugStatus();
     this.updateAddonsList();
   },
 
   componentWillUnmount() {
     AddonManager.removeAddonListener(this);
+    Management.off("startup", this.updateAddonsList);
+
     Services.prefs.removeObserver(CHROME_ENABLED_PREF,
       this.updateDebugStatus);
     Services.prefs.removeObserver(REMOTE_ENABLED_PREF,
       this.updateDebugStatus);
   },
 
   updateDebugStatus() {
     let debugDisabled =
@@ -76,16 +82,17 @@ module.exports = createClass({
           return {
             name: addon.name,
             icon: addon.iconURL || ExtensionIcon,
             addonID: addon.id,
             addonActor: addon.actor,
             temporarilyInstalled: addon.temporarilyInstalled,
             url: addon.url,
             manifestURL: addon.manifestURL,
+            warnings: addon.warnings,
           };
         });
 
         this.setState({ extensions });
       }, error => {
         throw new Error("Client error while listing addons: " + error);
       });
   },
--- a/devtools/client/aboutdebugging/components/addons/target.js
+++ b/devtools/client/aboutdebugging/components/addons/target.js
@@ -65,46 +65,65 @@ function internalIDForTarget(target) {
           { href: target.manifestURL, target: "_blank", className: "manifest-url" },
           Strings.GetStringFromName("manifestURL"),
         ),
       )
     ),
   ];
 }
 
-function temporaryID(target) {
-  if (!isTemporaryID(target.addonID)) {
-    return [];
+function showMessages(target) {
+  const messages = [
+    ...warningMessages(target.warnings),
+    ...infoMessages(target),
+  ];
+  if (messages.length > 0) {
+    return dom.ul(
+      { className: "addon-target-messages" },
+      ...messages);
   }
+  return null;
+}
 
-  return [
-    dom.div({ className: "addons-tip" },
-      dom.span({ className: "addons-web-ext-tip" },
-        Strings.GetStringFromName("temporaryID")
-      ),
+function infoMessages(target) {
+  const messages = [];
+  if (isTemporaryID(target.addonID)) {
+    messages.push(dom.li(
+      { className: "addon-target-info-message addon-target-message" },
+      Strings.GetStringFromName("temporaryID"),
+      " ",
       dom.a({ href: TEMP_ID_URL, className: "temporary-id-url", target: "_blank" },
         Strings.GetStringFromName("temporaryID.learnMore")
-      )
-    )
-  ];
+      )));
+  }
+  return messages;
+}
+
+function warningMessages(warnings = []) {
+  return warnings.map((warning) => {
+    return dom.li(
+      { className: "addon-target-warning-message addon-target-message" },
+      warning);
+  });
 }
 
 module.exports = createClass({
   displayName: "AddonTarget",
 
   propTypes: {
     client: PropTypes.instanceOf(DebuggerClient).isRequired,
     debugDisabled: PropTypes.bool,
     target: PropTypes.shape({
       addonActor: PropTypes.string.isRequired,
       addonID: PropTypes.string.isRequired,
       icon: PropTypes.string,
       name: PropTypes.string.isRequired,
       temporarilyInstalled: PropTypes.bool,
       url: PropTypes.string,
+      warnings: PropTypes.array,
     }).isRequired
   },
 
   debug() {
     let { target } = this.props;
     debugAddon(target.addonID);
   },
 
@@ -134,17 +153,17 @@ module.exports = createClass({
       dom.div({ className: "target" },
         dom.img({
           className: "target-icon",
           role: "presentation",
           src: target.icon
         }),
         dom.span({ className: "target-name", title: target.name }, target.name)
       ),
-      ...temporaryID(target),
+      showMessages(target),
       dom.dl(
         { className: "addon-target-info" },
         ...filePathForTarget(target),
         ...internalIDForTarget(target),
       ),
       dom.div({className: "addon-target-actions"},
         dom.button({
           className: "debug-button addon-target-button",
new file mode 100644
--- /dev/null
+++ b/devtools/client/aboutdebugging/test/addons/test-devtools-webextension-unknown-prop/manifest.json
@@ -0,0 +1,14 @@
+{
+  "manifest_version": 2,
+  "name": "test-devtools-webextension-unknown-prop",
+  "version": "1.0",
+  "applications": {
+    "gecko": {
+      "id": "test-devtools-webextension-unknown-prop@mozilla.org"
+    }
+  },
+  "browser_actions": {
+    "default_title": "WebExtension Popup Debugging",
+    "default_popup": "popup.html"
+  }
+}
--- a/devtools/client/aboutdebugging/test/browser.ini
+++ b/devtools/client/aboutdebugging/test/browser.ini
@@ -5,16 +5,17 @@ support-files =
   head.js
   addons/unpacked/bootstrap.js
   addons/unpacked/install.rdf
   addons/bad/manifest.json
   addons/bug1273184.xpi
   addons/test-devtools-webextension/*
   addons/test-devtools-webextension-nobg/*
   addons/test-devtools-webextension-noid/*
+  addons/test-devtools-webextension-unknown-prop/*
   service-workers/delay-sw.html
   service-workers/delay-sw.js
   service-workers/empty-sw.html
   service-workers/empty-sw.js
   service-workers/fetch-sw.html
   service-workers/fetch-sw.js
   service-workers/push-sw.html
   service-workers/push-sw.js
--- a/devtools/client/aboutdebugging/test/browser_addons_debug_info.js
+++ b/devtools/client/aboutdebugging/test/browser_addons_debug_info.js
@@ -76,8 +76,37 @@ add_task(function* testTemporaryWebExten
 
   let temporaryID = container.querySelector(".temporary-id-url");
   ok(temporaryID, "Temporary ID message does appear");
 
   yield uninstallAddon({document, id: addonId, name: addonName});
 
   yield closeAboutDebugging(tab);
 });
+
+add_task(function* testUnknownManifestProperty() {
+  let addonId = "test-devtools-webextension-unknown-prop@mozilla.org";
+  let addonName = "test-devtools-webextension-unknown-prop";
+  let { tab, document } = yield openAboutDebugging("addons");
+
+  yield waitForInitialAddonList(document);
+  yield installAddon({
+    document,
+    path: "addons/test-devtools-webextension-unknown-prop/manifest.json",
+    name: addonName,
+    isWebExtension: true
+  });
+
+  let container = document.querySelector(`[data-addon-id="${addonId}"]`);
+
+  yield waitForInstallMessages(container);
+
+  let messages = container.querySelectorAll(".addon-target-message");
+  ok(messages.length === 1, "there is one message");
+  ok(messages[0].textContent.match(/Error processing browser_actions/),
+     "the message is helpful");
+  ok(messages[0].classList.contains("addon-target-warning-message"),
+     "the message is a warning");
+
+  yield uninstallAddon({document, id: addonId, name: addonName});
+
+  yield closeAboutDebugging(tab);
+});
--- a/devtools/client/aboutdebugging/test/head.js
+++ b/devtools/client/aboutdebugging/test/head.js
@@ -1,20 +1,13 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 /* eslint-env browser */
-/* exported openAboutDebugging, changeAboutDebuggingHash, closeAboutDebugging,
-   installAddon, uninstallAddon, waitForMutation, waitForContentMutation, assertHasTarget,
-   getServiceWorkerList, getTabList, openPanel, waitForInitialAddonList,
-   waitForServiceWorkerRegistered, unregisterServiceWorker,
-   waitForDelayedStartupFinished, setupTestAboutDebuggingWebExtension,
-   waitForServiceWorkerActivation, enableServiceWorkerDebugging,
-   getServiceWorkerContainer, promiseAddonEvent, installAddonWithManager, getAddonByID,
-   tearDownAddon */
+/* eslint no-unused-vars: [2, {"vars": "local"}] */
 /* import-globals-from ../../framework/test/shared-head.js */
 
 "use strict";
 
 // Load the shared-head file first.
 Services.scriptloader.loadSubScript(
   "chrome://mochitests/content/browser/devtools/client/framework/test/shared-head.js",
   this);
@@ -267,16 +260,33 @@ function waitForInitialAddonList(documen
     info("Actually, the add-ons have already loaded");
     result = Promise.resolve();
   } else {
     result = waitForMutation(addonListContainer, { childList: true });
   }
   return result;
 }
 
+function waitForInstallMessages(target) {
+  return new Promise(resolve => {
+    let observer = new MutationObserver((mutations) => {
+      const messageAdded = mutations.some((mutation) => {
+        return [...mutation.addedNodes].some((node) => {
+          return node.classList.contains("addon-target-messages");
+        });
+      });
+      if (messageAdded) {
+        observer.disconnect();
+        resolve();
+      }
+    });
+    observer.observe(target, { childList: true });
+  });
+}
+
 /**
  * Returns a promise that will resolve after receiving a mutation matching the
  * provided mutation options on the provided target.
  * @param {Node} target
  * @param {Object} mutationOptions
  * @return {Promise}
  */
 function waitForMutation(target, mutationOptions) {
--- a/devtools/server/actors/webextension-parent.js
+++ b/devtools/server/actors/webextension-parent.js
@@ -70,16 +70,17 @@ const WebExtensionParentActor = protocol
       id: this.id,
       name: this.addon.name,
       url: this.addon.sourceURI ? this.addon.sourceURI.spec : undefined,
       iconURL: this.addon.iconURL,
       debuggable: this.addon.isDebuggable,
       temporarilyInstalled: this.addon.temporarilyInstalled,
       isWebExtension: true,
       manifestURL: policy && policy.getURL("manifest.json"),
+      warnings: ExtensionParent.DebugUtils.getExtensionManifestWarnings(this.id),
     };
   },
 
   connect() {
     if (this._childFormPormise) {
       return this._childFormPromise;
     }
 
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -317,36 +317,58 @@ this.ExtensionData = class {
     this.localeData = null;
     this._promiseLocales = null;
 
     this.apiNames = new Set();
     this.dependencies = new Set();
     this.permissions = new Set();
 
     this.errors = [];
+    this.warnings = [];
   }
 
   get builtinMessages() {
     return null;
   }
 
   get logger() {
     let id = this.id || "<unknown>";
     return Log.repository.getLogger(LOGGER_ID_BASE + id);
   }
 
   // Report an error about the extension's manifest file.
   manifestError(message) {
     this.packagingError(`Reading manifest: ${message}`);
   }
 
+  manifestWarning(message) {
+    this.packagingWarning(`Reading manifest: ${message}`);
+  }
+
   // Report an error about the extension's general packaging.
   packagingError(message) {
     this.errors.push(message);
-    this.logger.error(`Loading extension '${this.id}': ${message}`);
+    this.logError(message);
+  }
+
+  packagingWarning(message) {
+    this.warnings.push(message);
+    this.logWarning(message);
+  }
+
+  logWarning(message) {
+    this._logMessage(message, "warn");
+  }
+
+  logError(message) {
+    this._logMessage(message, "error");
+  }
+
+  _logMessage(message, severity) {
+    this.logger[severity](`Loading extension '${this.id}': ${message}`);
   }
 
   /**
    * Returns the moz-extension: URL for the given path within this
    * extension.
    *
    * Must not be called unless either the `id` or `uuid` property has
    * already been set.
@@ -501,17 +523,17 @@ this.ExtensionData = class {
       }
     }).then(() => {
       let context = {
         url: this.baseURI && this.baseURI.spec,
 
         principal: this.principal,
 
         logError: error => {
-          this.logger.warn(`Loading extension '${this.id}': Reading manifest: ${error}`);
+          this.manifestWarning(error);
         },
 
         preprocessors: {},
       };
 
       if (this.manifest.theme) {
         let invalidProps = validateThemeManifest(Object.getOwnPropertyNames(this.manifest));
 
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -923,16 +923,24 @@ const DebugUtils = {
 
   unwatchExtensionUpdated() {
     if (this._extensionUpdatedWatcher) {
       apiManager.off("ready", this._extensionUpdatedWatcher);
       delete this._extensionUpdatedWatcher;
     }
   },
 
+  getExtensionManifestWarnings(id) {
+    const addon = GlobalManager.extensionMap.get(id);
+    if (addon) {
+      return addon.warnings;
+    }
+    return [];
+  },
+
 
   /**
    * Retrieve a XUL browser element which has been configured to be able to connect
    * the devtools actor with the process where the extension is running.
    *
    * @param {WebExtensionParentActor} webExtensionParentActor
    *        The devtools actor that is retrieving the browser element.
    *