Bug 1506546 - Refactor about:debuggings. r=yulia,jdescottes
☠☠ backed out by c19baae766e9 ☠ ☠
authorAlexandre Poirot <poirot.alex@gmail.com>
Tue, 27 Nov 2018 11:23:24 +0000
changeset 504671 00fe26234b3da593bda143f9f7836b94c33f128e
parent 504670 7d8e650e25c2fc0070c7ca5e4a9d851b561d9734
child 504672 8972a2f1401568be3e4af284ac02af215f330209
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [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/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
@@ -28,31 +28,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/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,