From 7c790c6c819765fe29d3cf192e2e5c4386994d8c Mon Sep 17 00:00:00 2001 From: Alexandre Poirot Date: Wed, 28 Aug 2019 16:06:22 +0000 Subject: [PATCH] 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 | 3 +- devtools/shared/fronts/root.js | 30 +++- devtools/shared/fronts/targets/local-tab.js | 138 ++++++++++++++ devtools/shared/fronts/targets/moz.build | 1 + .../shared/fronts/targets/target-mixin.js | 169 ++++-------------- devtools/shared/specs/root.js | 2 +- 6 files changed, 200 insertions(+), 143 deletions(-) create mode 100644 devtools/shared/fronts/targets/local-tab.js diff --git a/devtools/client/framework/target.js b/devtools/client/framework/target.js index 55ffa9eb8e03..888e019657f3 100644 --- a/devtools/client/framework/target.js +++ b/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 }); }, diff --git a/devtools/shared/fronts/root.js b/devtools/shared/fronts/root.js index 6c6ebb3df144..396bb9ce6926 100644 --- a/devtools/shared/fronts/root.js +++ b/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 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; } /** diff --git a/devtools/shared/fronts/targets/local-tab.js b/devtools/shared/fronts/targets/local-tab.js new file mode 100644 index 000000000000..b015cb28d81a --- /dev/null +++ b/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; diff --git a/devtools/shared/fronts/targets/moz.build b/devtools/shared/fronts/targets/moz.build index a4c4347e91e0..80a7bb6da197 100644 --- a/devtools/shared/fronts/targets/moz.build +++ b/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', ) diff --git a/devtools/shared/fronts/targets/target-mixin.js b/devtools/shared/fronts/targets/target-mixin.js index e8ea205ec6ba..841e1b11cd4c 100644 --- a/devtools/shared/fronts/targets/target-mixin.js +++ b/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}`; } diff --git a/devtools/shared/specs/root.js b/devtools/shared/specs/root.js index 0c8f2139f7a2..06a5286322d3 100644 --- a/devtools/shared/specs/root.js +++ b/devtools/shared/specs/root.js @@ -47,7 +47,7 @@ const rootSpecPrototype = { tabId: Option(0, "number"), }, response: { - tab: RetVal("browsingContextTarget"), + tab: RetVal("json"), }, },