Bug 1506546 - Refactor about:debuggings. r=yulia,jdescottes
authorAlexandre Poirot <poirot.alex@gmail.com>
Tue, 27 Nov 2018 19:17:11 +0000
changeset 507571 9abf7054a03ee08e6beb910e2b61448cc74934ad
parent 507570 abfeab4a1d650c8f275725b62a416d13b25e29dc
child 507572 24751811ee6e9ca54badacf0cca83e9dc266e1af
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersyulia, jdescottes
bugs1506546
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1506546 - Refactor about:debuggings. r=yulia,jdescottes I did a dedicated patch for converting both about:debuggings as it is slightly more complex as we have to tweak redux data, wrappers, mocks. This patch also convert a manual "reload" request being done by about:debugging and instead use protocol.js front to do it. Also, I moved isLegacyTemporaryExtension to the front as it depends on accessing the form and it should be better to keep form processing to the fronts, if possible. MozReview-Commit-ID: 16qZkuCBp9b Depends on D12576 Differential Revision: https://phabricator.services.mozilla.com/D12577
devtools/client/aboutdebugging-new/src/actions/debug-targets.js
devtools/client/aboutdebugging-new/src/components/debugtarget/TemporaryExtensionAction.js
devtools/client/aboutdebugging-new/src/modules/client-wrapper.js
devtools/client/aboutdebugging-new/src/modules/extensions-helper.js
devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_addons_usb_runtime.js
devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_system_addons.js
devtools/client/aboutdebugging-new/test/browser/mocks/head-client-wrapper-mock.js
devtools/client/aboutdebugging/components/addons/Panel.js
devtools/client/aboutdebugging/components/addons/Target.js
devtools/client/aboutdebugging/modules/addon.js
devtools/shared/fronts/targets/addon.js
devtools/shared/specs/targets/addon.js
--- a/devtools/client/aboutdebugging-new/src/actions/debug-targets.js
+++ b/devtools/client/aboutdebugging-new/src/actions/debug-targets.js
@@ -98,22 +98,23 @@ function pushServiceWorker(actor) {
     try {
       await clientWrapper.request({ to: actor, type: "push" });
     } catch (e) {
       console.error(e);
     }
   };
 }
 
-function reloadTemporaryExtension(actor) {
+function reloadTemporaryExtension(id) {
   return async (_, getState) => {
     const clientWrapper = getCurrentClient(getState().runtimes);
 
     try {
-      await clientWrapper.request({ to: actor, type: "reload" });
+      const addonTargetFront = await clientWrapper.getAddon({ id });
+      await addonTargetFront.reload();
     } catch (e) {
       console.error(e);
     }
   };
 }
 
 function removeTemporaryExtension(id) {
   return async () => {
@@ -144,17 +145,17 @@ function requestTabs() {
 function requestExtensions() {
   return async (dispatch, getState) => {
     dispatch({ type: REQUEST_EXTENSIONS_START });
 
     const runtime = getCurrentRuntime(getState().runtimes);
     const clientWrapper = getCurrentClient(getState().runtimes);
 
     try {
-      const { addons } = await clientWrapper.listAddons();
+      const addons = await clientWrapper.listAddons();
       let extensions = addons.filter(a => a.debuggable);
 
       // Filter out system addons unless the dedicated preference is set to true.
       if (!getState().ui.showSystemAddons) {
         extensions = extensions.filter(e => !e.isSystem);
       }
 
       if (runtime.type !== RUNTIMES.THIS_FIREFOX) {
--- a/devtools/client/aboutdebugging-new/src/components/debugtarget/TemporaryExtensionAction.js
+++ b/devtools/client/aboutdebugging-new/src/components/debugtarget/TemporaryExtensionAction.js
@@ -24,17 +24,17 @@ class TemporaryExtensionAction extends P
     return {
       dispatch: PropTypes.func.isRequired,
       target: Types.debugTarget.isRequired,
     };
   }
 
   reload() {
     const { dispatch, target } = this.props;
-    dispatch(Actions.reloadTemporaryExtension(target.details.actor));
+    dispatch(Actions.reloadTemporaryExtension(target.id));
   }
 
   remove() {
     const { dispatch, target } = this.props;
     dispatch(Actions.removeTemporaryExtension(target.id));
   }
 
   render() {
--- a/devtools/client/aboutdebugging-new/src/modules/client-wrapper.js
+++ b/devtools/client/aboutdebugging-new/src/modules/client-wrapper.js
@@ -92,17 +92,21 @@ class ClientWrapper {
     }
   }
 
   async listTabs(options) {
     return this.client.listTabs(options);
   }
 
   async listAddons() {
-    return this.client.listAddons();
+    return this.client.mainRoot.listAddons();
+  }
+
+  async getAddon({ id }) {
+    return this.client.mainRoot.getAddon({ id });
   }
 
   async listWorkers() {
     const { other, service, shared } = await this.client.mainRoot.listAllWorkers();
 
     return {
       otherWorkers: other,
       serviceWorkers: service,
--- a/devtools/client/aboutdebugging-new/src/modules/extensions-helper.js
+++ b/devtools/client/aboutdebugging-new/src/modules/extensions-helper.js
@@ -48,24 +48,23 @@ exports.debugLocalAddon = async function
  * Start debugging an addon in a remote instance of Firefox.
  *
  * @param {String} id
  *        The addon id to debug.
  * @param {DebuggerClient} client
  *        Required for remote debugging.
  */
 exports.debugRemoteAddon = async function(id, client) {
-  const { addons } = await client.listAddons();
-  const addonForm = addons.find(addon => addon.id === id);
+  const addonTargetFront = await client.mainRoot.getAddon({ id });
 
   // Close previous addon debugging toolbox.
   closeToolbox();
 
   const options = {
-    form: addonForm,
+    activeTab: addonTargetFront,
     chrome: true,
     client,
   };
 
   const target = await TargetFactory.forRemoteTab(options);
 
   const hostType = Toolbox.HostType.WINDOW;
   remoteAddonToolbox = await gDevTools.showToolbox(target, null, hostType);
--- a/devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_addons_usb_runtime.js
+++ b/devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_addons_usb_runtime.js
@@ -29,31 +29,31 @@ add_task(async function() {
 
   const extensionPane = getDebugTargetPane("Extensions", document);
   info("Check an empty target pane message is displayed");
   ok(extensionPane.querySelector(".js-debug-target-list-empty"),
     "Extensions list is empty");
 
   info("Add an extension to the remote client");
   const addon = { name: "Test extension name", debuggable: true };
-  usbClient.listAddons = () => ({ addons: [addon] });
+  usbClient.listAddons = () => [addon];
   usbClient._eventEmitter.emit("addonListChanged");
 
   info("Wait until the extension appears");
   await waitUntil(() => !extensionPane.querySelector(".js-debug-target-list-empty"));
 
   const extensionTarget = findDebugTargetByText("Test extension name", document);
   ok(extensionTarget, "Extension target appeared for the USB runtime");
 
   // The goal here is to check that USB runtimes addons are only updated when the USB
   // runtime is sending addonListChanged events. The reason for this test is because the
   // previous implementation was updating the USB runtime extensions list when the _local_
   // AddonManager was updated.
   info("Remove the extension from the remote client WITHOUT sending an event");
-  usbClient.listAddons = () => ({ addons: [] });
+  usbClient.listAddons = () => [];
 
   info("Simulate an addon update on the ThisFirefox client");
   usbMocks.thisFirefoxClient._eventEmitter.emit("addonListChanged");
 
   // To avoid wait for a set period of time we trigger another async update, adding a new
   // tab. We assume that if the addon update mechanism had started, it would also be done
   // when the new tab was processed.
   info("Wait until the tab target for 'http://some.random/url.com' appears");
--- a/devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_system_addons.js
+++ b/devtools/client/aboutdebugging-new/test/browser/browser_aboutdebugging_system_addons.js
@@ -15,34 +15,34 @@ Services.scriptloader.loadSubScript(
 
 const SYSTEM_ADDON =
   createAddonData({ id: "system", name: "System Addon", isSystem: true });
 const INSTALLED_ADDON =
   createAddonData({ id: "installed", name: "Installed Addon", isSystem: false });
 
 add_task(async function testShowSystemAddonsFalse() {
   const thisFirefoxClient = setupThisFirefoxMock();
-  thisFirefoxClient.listAddons = () => ({ addons: [SYSTEM_ADDON, INSTALLED_ADDON] });
+  thisFirefoxClient.listAddons = () => ([SYSTEM_ADDON, INSTALLED_ADDON]);
 
   info("Hide system addons in aboutdebugging via preference");
   await pushPref("devtools.aboutdebugging.showSystemAddons", false);
 
   const { document, tab } = await openAboutDebugging();
 
   const hasSystemAddon = !!findDebugTargetByText("System Addon", document);
   const hasInstalledAddon = !!findDebugTargetByText("Installed Addon", document);
   ok(!hasSystemAddon, "System addon is hidden when system addon pref is false");
   ok(hasInstalledAddon, "Installed addon is displayed when system addon pref is false");
 
   await removeTab(tab);
 });
 
 add_task(async function testShowSystemAddonsTrue() {
   const thisFirefoxClient = setupThisFirefoxMock();
-  thisFirefoxClient.listAddons = () => ({ addons: [SYSTEM_ADDON, INSTALLED_ADDON] });
+  thisFirefoxClient.listAddons = () => ([SYSTEM_ADDON, INSTALLED_ADDON]);
 
   info("Show system addons in aboutdebugging via preference");
   await pushPref("devtools.aboutdebugging.showSystemAddons", true);
 
   const { document, tab } = await openAboutDebugging();
   const hasSystemAddon = !!findDebugTargetByText("System Addon", document);
   const hasInstalledAddon = !!findDebugTargetByText("Installed Addon", document);
   ok(hasSystemAddon, "System addon is displayed when system addon pref is true");
--- a/devtools/client/aboutdebugging-new/test/browser/mocks/head-client-wrapper-mock.js
+++ b/devtools/client/aboutdebugging-new/test/browser/mocks/head-client-wrapper-mock.js
@@ -57,17 +57,17 @@ function createClientMock() {
     // Return default preference value or null if no match.
     getPreference: (prefName) => {
       if (prefName in DEFAULT_PREFERENCES) {
         return DEFAULT_PREFERENCES[prefName];
       }
       return null;
     },
     // Empty array of addons
-    listAddons: () => ({ addons: [] }),
+    listAddons: () => [],
     // Empty array of tabs
     listTabs: () => ({ tabs: []}),
     // Empty arrays of workers
     listWorkers: () => ({
       otherWorkers: [],
       serviceWorkers: [],
       sharedWorkers: [],
     }),
--- a/devtools/client/aboutdebugging/components/addons/Panel.js
+++ b/devtools/client/aboutdebugging/components/addons/Panel.js
@@ -95,24 +95,22 @@ class AddonsPanel extends Component {
   }
 
   updateShowSystemStatus() {
     const showSystemAddons = Services.prefs.getBoolPref(SYSTEM_ENABLED_PREF, false);
     this.setState({ showSystemAddons });
   }
 
   updateAddonsList() {
-    this.props.client.listAddons()
-      .then(({addons}) => {
+    this.props.client.mainRoot.listAddons()
+      .then(addons => {
         const extensions = addons.filter(addon => addon.debuggable).map(addon => {
           return {
-            addonTargetActor: addon.actor,
+            addonTargetFront: addon,
             addonID: addon.id,
-            // Forward the whole addon actor form for potential remote debugging.
-            form: addon,
             icon: addon.iconURL || ExtensionIcon,
             isSystem: addon.isSystem,
             manifestURL: addon.manifestURL,
             name: addon.name,
             temporarilyInstalled: addon.temporarilyInstalled,
             url: addon.url,
             warnings: addon.warnings,
           };
--- a/devtools/client/aboutdebugging/components/addons/Target.js
+++ b/devtools/client/aboutdebugging/components/addons/Target.js
@@ -8,17 +8,16 @@
 
 const { Component } = require("devtools/client/shared/vendor/react");
 const PropTypes = require("devtools/client/shared/vendor/react-prop-types");
 const dom = require("devtools/client/shared/vendor/react-dom-factories");
 const {
   debugLocalAddon,
   debugRemoteAddon,
   getExtensionUuid,
-  isLegacyTemporaryExtension,
   isTemporaryID,
   parseFileUri,
   uninstallAddon,
 } = require("../../modules/addon");
 const Services = require("Services");
 
 loader.lazyRequireGetter(this, "DebuggerClient",
   "devtools/shared/client/debugger-client", true);
@@ -119,17 +118,17 @@ function infoMessages(target) {
   }
 
   return messages;
 }
 
 function warningMessages(target) {
   let messages = [];
 
-  if (isLegacyTemporaryExtension(target.form)) {
+  if (target.addonTargetFront.isLegacyTemporaryExtension()) {
     messages.push(dom.li(
       {
         className: "addon-target-warning-message addon-target-message",
       },
       Strings.GetStringFromName("legacyExtensionWarning"),
       " ",
       dom.a(
         {
@@ -152,19 +151,18 @@ function warningMessages(target) {
 
 class AddonTarget extends Component {
   static get propTypes() {
     return {
       client: PropTypes.instanceOf(DebuggerClient).isRequired,
       connect: PropTypes.object,
       debugDisabled: PropTypes.bool,
       target: PropTypes.shape({
-        addonTargetActor: PropTypes.string.isRequired,
+        addonTargetFront: PropTypes.object.isRequired,
         addonID: PropTypes.string.isRequired,
-        form: PropTypes.object.isRequired,
         icon: PropTypes.string,
         name: PropTypes.string.isRequired,
         temporarilyInstalled: PropTypes.bool,
         url: PropTypes.string,
         warnings: PropTypes.array,
       }).isRequired,
     };
   }
@@ -187,23 +185,20 @@ class AddonTarget extends Component {
   }
 
   uninstall() {
     const { target } = this.props;
     uninstallAddon(target.addonID);
   }
 
   async reload() {
-    const { client, target } = this.props;
+    const { target } = this.props;
     const { AboutDebugging } = window;
     try {
-      await client.request({
-        to: target.addonTargetActor,
-        type: "reload",
-      });
+      await target.addonTargetFront.reload();
       AboutDebugging.emit("addon-reload");
     } catch (e) {
       throw new Error("Error reloading addon " + target.addonID + ": " + e.message);
     }
   }
 
   render() {
     const { target, debugDisabled } = this.props;
--- a/devtools/client/aboutdebugging/modules/addon.js
+++ b/devtools/client/aboutdebugging/modules/addon.js
@@ -20,29 +20,16 @@ const {
  * devtools/client/aboutdebugging-new/src/modules/extensions-helper.js
  * The only methods implemented here are the ones used in the old aboutdebugging only.
  */
 
 exports.isTemporaryID = function(addonID) {
   return AddonManagerPrivate.isTemporaryInstallID(addonID);
 };
 
-exports.isLegacyTemporaryExtension = function(addonForm) {
-  if (!addonForm.type) {
-    // If about:debugging is connected to an older then 59 remote Firefox, and type is
-    // not available on the addon/webextension actors, return false to avoid showing
-    // irrelevant warning messages.
-    return false;
-  }
-  return addonForm.type == "extension" &&
-         addonForm.temporarilyInstalled &&
-         !addonForm.isWebExtension &&
-         !addonForm.isAPIExtension;
-};
-
 /**
  * See JSDoc in devtools/client/aboutdebugging-new/src/modules/extensions-helper for all
  * the methods exposed below.
  */
 
 exports.debugLocalAddon = debugLocalAddon;
 exports.debugRemoteAddon = debugRemoteAddon;
 exports.getExtensionUuid = getExtensionUuid;
--- a/devtools/shared/fronts/targets/addon.js
+++ b/devtools/shared/fronts/targets/addon.js
@@ -28,16 +28,29 @@ const AddonTargetFront = protocol.FrontC
     for (const name in json) {
       if (name == "actor") {
         continue;
       }
       this[name] = json[name];
     }
   },
 
+  isLegacyTemporaryExtension() {
+    if (!this.type) {
+      // If about:debugging is connected to an older then 59 remote Firefox, and type is
+      // not available on the addon/webextension actors, return false to avoid showing
+      // irrelevant warning messages.
+      return false;
+    }
+    return this.type == "extension" &&
+           this.temporarilyInstalled &&
+           !this.isWebExtension &&
+           !this.isAPIExtension;
+  },
+
   attach: custom(async function() {
     const response = await this._attach();
 
     this.threadActor = response.threadActor;
 
     return response;
   }, {
     impl: "_attach",
--- a/devtools/shared/specs/targets/addon.js
+++ b/devtools/shared/specs/targets/addon.js
@@ -18,16 +18,20 @@ const addonTargetSpec = generateActorSpe
       response: RetVal("json"),
     },
     connect: {
       request: {
         options: Arg(0, "json"),
       },
       response: RetVal("json"),
     },
+    reload: {
+      request: {},
+      response: {},
+    },
     push: {
       request: {},
       response: RetVal("json"),
     },
   },
 
   events: {
     // newSource is being sent by ThreadActor in the name of its parent,