Bug 1522572 - Move local tab specifics out of TargetMixin. r=jdescottes,yulia

Differential Revision: https://phabricator.services.mozilla.com/D41854

--HG--
extra : moz-landing-system : lando

devtools/client/framework/target.js

@@ -43,7 +43,6 @@ exports.TargetFactory = {
     target = await promise;
     // Then replace the promise with the target object
     targets.set(tab, target);
-    target.attachTab(tab);
     target.once("close", () => {
       targets.delete(tab);
     });
@@ -91,7 +90,7 @@ exports.TargetFactory = {
     // Connect the local client to the local server
     await client.connect();
 
-    // Fetch the FrameTargetActor's Front
+    // Fetch the FrameTargetActor's Front which is a BrowsingContextTargetFront
     return client.mainRoot.getTab({ tab });
   },
 

devtools/shared/fronts/root.js

@@ -29,6 +29,12 @@ loader.lazyRequireGetter(
   "devtools/shared/fronts/targets/content-process",
   true
 );
+loader.lazyRequireGetter(
+  this,
+  "LocalTabTargetFront",
+  "devtools/shared/fronts/targets/local-tab",
+  true
+);
 
 class RootFront extends FrontClassWithSpec(rootSpec) {
   constructor(client, form) {
@@ -320,7 +326,29 @@ class RootFront extends FrontClassWithSpec(rootSpec) {
       }
     }
 
-    return super.getTab(packet);
+    const form = await super.getTab(packet);
+    let front = this.actor(form.actor);
+    if (front) {
+      front.form(form);
+      return front;
+    }
+    // Instanciate a specialized class for a local tab as it needs some more
+    // client side integration with the Firefox frontend.
+    // But ignore the fake `tab` object we receive, where there is only a
+    // `linkedBrowser` attribute, but this isn't a real <tab> element.
+    // devtools/client/framework/test/browser_toolbox_target.js is passing such
+    // a fake tab.
+    if (filter && filter.tab && filter.tab.tagName == "tab") {
+      front = new LocalTabTargetFront(this._client, filter.tab);
+    } else {
+      front = new BrowsingContextTargetFront(this._client);
+    }
+    // As these fronts aren't instantiated by protocol.js, we have to set their actor ID
+    // manually like that:
+    front.actorID = form.actor;
+    front.form(form);
+    this.manage(front);
+    return front;
   }
 
   /**

devtools/shared/fronts/targets/local-tab.js

@@ -0,0 +1,138 @@
+/* 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";
+
+loader.lazyRequireGetter(
+  this,
+  "gDevTools",
+  "devtools/client/framework/devtools",
+  true
+);
+loader.lazyRequireGetter(
+  this,
+  "TargetFactory",
+  "devtools/client/framework/target",
+  true
+);
+loader.lazyRequireGetter(
+  this,
+  "BrowsingContextTargetFront",
+  "devtools/shared/fronts/targets/browsing-context",
+  true
+);
+
+// This represent a Target front for a local tab and includes specialized
+// implementation in order to:
+// * distinguish local tabs from remote (see target.isLocalTab)
+// * being able to hookup into Firefox UI (see Hosts and event listeners of
+//   this class)
+class LocalTabTargetFront extends BrowsingContextTargetFront {
+  constructor(client, tab) {
+    super(client);
+
+    this._teardownTabListeners = this._teardownTabListeners.bind(this);
+    this._handleTabEvent = this._handleTabEvent.bind(this);
+
+    this._tab = tab;
+    this._setupTabListeners();
+
+    this.once("close", this._teardownTabListeners);
+  }
+
+  get isLocalTab() {
+    return true;
+  }
+  get tab() {
+    return this._tab;
+  }
+  get contentPrincipal() {
+    return this.tab.linkedBrowser.contentPrincipal;
+  }
+  get csp() {
+    return this.tab.linkedBrowser.csp;
+  }
+  toString() {
+    return `Target:${this.tab}`;
+  }
+
+  /**
+   * Listen to the different events.
+   */
+  _setupTabListeners() {
+    this.tab.addEventListener("TabClose", this._handleTabEvent);
+    this.tab.ownerDocument.defaultView.addEventListener(
+      "unload",
+      this._handleTabEvent
+    );
+    this.tab.addEventListener("TabRemotenessChange", this._handleTabEvent);
+  }
+
+  /**
+   * Teardown event listeners.
+   */
+  _teardownTabListeners() {
+    if (this.tab.ownerDocument.defaultView) {
+      this.tab.ownerDocument.defaultView.removeEventListener(
+        "unload",
+        this._handleTabEvent
+      );
+    }
+    this.tab.removeEventListener("TabClose", this._handleTabEvent);
+    this.tab.removeEventListener("TabRemotenessChange", this._handleTabEvent);
+  }
+
+  /**
+   * Handle tabs events.
+   */
+  async _handleTabEvent(event) {
+    switch (event.type) {
+      case "TabClose":
+        // Always destroy the toolbox opened for this local tab target.
+        // Toolboxes are no longer destroyed on target destruction.
+        // When the toolbox is in a Window Host, it won't be removed from the
+        // DOM when the tab is closed.
+        const toolbox = gDevTools.getToolbox(this);
+        // A few tests are using TargetFactory.forTab, but aren't spawning any
+        // toolbox. In this case, the toobox won't destroy the target, so we
+        // do it from here. But ultimately, the target should destroy itself
+        // from the actor side anyway.
+        if (toolbox) {
+          // Toolbox.destroy will call target.destroy eventually.
+          await toolbox.destroy();
+        }
+        break;
+      case "unload":
+        this.destroy();
+        break;
+      case "TabRemotenessChange":
+        this._onRemotenessChange();
+        break;
+    }
+  }
+
+  /**
+   * Automatically respawn the toolbox when the tab changes between being
+   * loaded within the parent process and loaded from a content process.
+   * Process change can go in both ways.
+   */
+  async _onRemotenessChange() {
+    // Responsive design does a crazy dance around tabs and triggers
+    // remotenesschange events. But we should ignore them as at the end
+    // the content doesn't change its remoteness.
+    if (this.tab.isResponsiveDesignMode) {
+      return;
+    }
+
+    const toolbox = gDevTools.getToolbox(this);
+
+    // Force destroying the toolbox as the target will be destroyed,
+    // but not the toolbox.
+    await toolbox.destroy();
+
+    // Recreate a fresh target instance as the current one is now destroyed
+    const newTarget = await TargetFactory.forTab(this.tab);
+    gDevTools.showToolbox(newTarget);
+  }
+}
+exports.LocalTabTargetFront = LocalTabTargetFront;

devtools/shared/fronts/targets/moz.build

@@ -8,6 +8,7 @@ DevToolsModules(
     'addon.js',
     'browsing-context.js',
     'content-process.js',
+    'local-tab.js',
     'target-mixin.js',
     'worker.js',
 )

devtools/shared/fronts/targets/target-mixin.js

@@ -4,22 +4,6 @@
 
 "use strict";
 
-// We are requiring a module from client whereas this module is from shared.
-// This shouldn't happen, but Fronts should rather be part of client anyway.
-// Otherwise gDevTools is only used for local tabs and should propably only
-// used by a subclass, specific to local tabs.
-loader.lazyRequireGetter(
-  this,
-  "gDevTools",
-  "devtools/client/framework/devtools",
-  true
-);
-loader.lazyRequireGetter(
-  this,
-  "TargetFactory",
-  "devtools/client/framework/target",
-  true
-);
 loader.lazyRequireGetter(
   this,
   "ThreadClient",
@@ -71,15 +55,6 @@ function TargetMixin(parentClass) {
       this._setupRemoteListeners();
     }
 
-    attachTab(tab) {
-      // When debugging local tabs, we also have a reference to the Firefox tab
-      // This is used to:
-      // * distinguish local tabs from remote (see target.isLocalTab)
-      // * being able to hookup into Firefox UI (see Hosts)
-      this._tab = tab;
-      this._setupListeners();
-    }
-
     /**
      * Returns a promise for the protocol description from the root actor. Used
      * internally with `target.actorHasMethod`. Takes advantage of caching if
@@ -178,8 +153,36 @@ function TargetMixin(parentClass) {
       return this.client.traits[traitName];
     }
 
+    /**
+     * The following getters: isLocalTab, tab, ... will be overriden for local
+     * tabs by some code in devtools/shared/fronts/targets/local-tab.js. They
+     * are all specific to local tabs, i.e. when you are debugging a tab of the
+     * current Firefox instance.
+     */
+    get isLocalTab() {
+      return false;
+    }
+
     get tab() {
-      return this._tab;
+      return null;
+    }
+
+    /**
+     * For local tabs, returns the tab's contentPrincipal, which can be used as a
+     * `triggeringPrincipal` when opening links.  However, this is a hack as it is not
+     * correct for subdocuments and it won't work for remote debugging.  Bug 1467945 hopes
+     * to devise a better approach.
+     */
+    get contentPrincipal() {
+      return null;
+    }
+
+    /**
+     * Similar to the above get contentPrincipal(), the get csp()
+     * returns the CSP which should be used for opening links.
+     */
+    get csp() {
+      return null;
     }
 
     // Get a promise of the RootActor's form
@@ -315,10 +318,6 @@ function TargetMixin(parentClass) {
       );
     }
 
-    get isLocalTab() {
-      return !!this._tab;
-    }
-
     get isMultiProcess() {
       return !this.window;
     }
@@ -350,30 +349,6 @@ function TargetMixin(parentClass) {
       }
     }
 
-    /**
-     * For local tabs, returns the tab's contentPrincipal, which can be used as a
-     * `triggeringPrincipal` when opening links.  However, this is a hack as it is not
-     * correct for subdocuments and it won't work for remote debugging.  Bug 1467945 hopes
-     * to devise a better approach.
-     */
-    get contentPrincipal() {
-      if (!this.isLocalTab) {
-        return null;
-      }
-      return this.tab.linkedBrowser.contentPrincipal;
-    }
-
-    /**
-     * Similar to the above get contentPrincipal(), the get csp()
-     * returns the CSP which should be used for opening links.
-     */
-    get csp() {
-      if (!this.isLocalTab) {
-        return null;
-      }
-      return this.tab.linkedBrowser.csp;
-    }
-
     // Attach the console actor
     async attachConsole() {
       this.activeConsole = await this.getFront("console");
@@ -421,26 +396,6 @@ function TargetMixin(parentClass) {
       this.emit("source-updated", packet);
     }
 
-    /**
-     * Listen to the different events.
-     */
-    _setupListeners() {
-      this.tab.addEventListener("TabClose", this);
-      this.tab.ownerDocument.defaultView.addEventListener("unload", this);
-      this.tab.addEventListener("TabRemotenessChange", this);
-    }
-
-    /**
-     * Teardown event listeners.
-     */
-    _teardownListeners() {
-      if (this._tab.ownerDocument.defaultView) {
-        this._tab.ownerDocument.defaultView.removeEventListener("unload", this);
-      }
-      this._tab.removeEventListener("TabClose", this);
-      this._tab.removeEventListener("TabRemotenessChange", this);
-    }
-
     /**
      * Setup listeners for remote debugging, updating existing ones as necessary.
      */
@@ -471,63 +426,6 @@ function TargetMixin(parentClass) {
       }
     }
 
-    /**
-     * Handle tabs events.
-     */
-    async handleEvent(event) {
-      switch (event.type) {
-        case "TabClose":
-          // Always destroy the toolbox opened for this local tab target.
-          // Toolboxes are no longer destroyed on target destruction.
-          // When the toolbox is in a Window Host, it won't be removed from the
-          // DOM when the tab is closed.
-          const toolbox = gDevTools.getToolbox(this);
-          // A few tests are using TargetFactory.forTab, but aren't spawning any
-          // toolbox. In this case, the toobox won't destroy the target, so we
-          // do it from here. But ultimately, the target should destroy itself
-          // from the actor side anyway.
-          if (toolbox) {
-            // Toolbox.destroy will call target.destroy eventually.
-            await toolbox.destroy();
-          } else {
-            this.destroy();
-          }
-          break;
-        case "unload":
-          this.destroy();
-          break;
-        case "TabRemotenessChange":
-          this.onRemotenessChange();
-          break;
-      }
-    }
-
-    /**
-     * Automatically respawn the toolbox when the tab changes between being
-     * loaded within the parent process and loaded from a content process.
-     * Process change can go in both ways.
-     */
-    async onRemotenessChange() {
-      // Responsive design do a crazy dance around tabs and triggers
-      // remotenesschange events. But we should ignore them as at the end
-      // the content doesn't change its remoteness.
-      if (this._tab.isResponsiveDesignMode) {
-        return;
-      }
-
-      // Save a reference to the tab as it will be nullified on destroy
-      const tab = this._tab;
-      const toolbox = gDevTools.getToolbox(this);
-
-      // Force destroying the toolbox as the target will be destroyed,
-      // but not the toolbox.
-      await toolbox.destroy();
-
-      // Recreate a fresh target instance as the current one is now destroyed
-      const newTarget = await TargetFactory.forTab(tab);
-      gDevTools.showToolbox(newTarget);
-    }
-
     /**
      * Target is not alive anymore.
      */
@@ -547,10 +445,6 @@ function TargetMixin(parentClass) {
           await front.destroy();
         }
 
-        if (this._tab) {
-          this._teardownListeners();
-        }
-
         this._teardownRemoteListeners();
 
         this.threadFront = null;
@@ -595,7 +489,6 @@ function TargetMixin(parentClass) {
       this.activeConsole = null;
       this.threadFront = null;
       this._client = null;
-      this._tab = null;
 
       // All target front subclasses set this variable in their `attach` method.
       // None of them overload destroy, so clean this up from here.
@@ -606,9 +499,7 @@ function TargetMixin(parentClass) {
     }
 
     toString() {
-      const id = this._tab
-        ? this._tab
-        : this.targetForm && this.targetForm.actor;
+      const id = this.targetForm ? this.targetForm.actor : null;
       return `Target:${id}`;
     }
 

devtools/shared/specs/root.js

@@ -47,7 +47,7 @@ const rootSpecPrototype = {
         tabId: Option(0, "number"),
       },
       response: {
-        tab: RetVal("browsingContextTarget"),
+        tab: RetVal("json"),
       },
     },