Bug 1374590 - Refactoring ParentDevToolsPanel class to provide create/destroyBrowserElement helper methods. r=aswan,ochameau
authorLuca Greco <lgreco@mozilla.com>
Mon, 11 Sep 2017 15:49:36 +0200
changeset 430276 d1dc7082f9e3c5a96f4cce794c48f9e7575f558c
parent 430275 337bae3d28e485077d8518331171eec33de84b5f
child 430277 97b428deeefae7e859ff336ea7f21a45663925a4
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaswan, ochameau
bugs1374590
milestone57.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 1374590 - Refactoring ParentDevToolsPanel class to provide create/destroyBrowserElement helper methods. r=aswan,ochameau MozReview-Commit-ID: FsGlCU0H9VG
browser/components/extensions/ext-devtools-panels.js
devtools/client/framework/toolbox.js
--- a/browser/components/extensions/ext-devtools-panels.js
+++ b/browser/components/extensions/ext-devtools-panels.js
@@ -57,22 +57,23 @@ class ParentDevToolsPanel {
     this.panelOptions = panelOptions;
 
     this.context.callOnClose(this);
 
     this.id = this.panelOptions.id;
 
     this.onToolboxPanelSelect = this.onToolboxPanelSelect.bind(this);
 
-    this.panelAdded = false;
-    this.addPanel();
-
+    this.unwatchExtensionProxyContextLoad = null;
     this.waitTopLevelContext = new Promise(resolve => {
       this._resolveTopLevelContext = resolve;
     });
+
+    this.panelAdded = false;
+    this.addPanel();
   }
 
   addPanel() {
     const {icon, title} = this.panelOptions;
     const extensionName = this.context.extension.name;
 
     this.toolbox.addAdditionalTool({
       id: this.id,
@@ -83,26 +84,85 @@ class ParentDevToolsPanel {
       invertIconForLightTheme: false,
       visibilityswitch:  `devtools.webext-${this.id}.enabled`,
       isTargetSupported: target => target.isLocalTab,
       build: (window, toolbox) => {
         if (toolbox !== this.toolbox) {
           throw new Error("Unexpected toolbox received on addAdditionalTool build property");
         }
 
-        const destroy = this.buildPanel(window, toolbox);
+        const destroy = this.buildPanel(window);
 
         return {toolbox, destroy};
       },
     });
 
     this.panelAdded = true;
   }
 
-  buildPanel(window, toolbox) {
+  buildPanel(window) {
+    const {toolbox} = this;
+
+    this.createBrowserElement(window);
+
+    toolbox.on("select", this.onToolboxPanelSelect);
+
+    // Return a cleanup method that is when the panel is destroyed, e.g.
+    // - when addon devtool panel has been disabled by the user from the toolbox preferences,
+    //   its ParentDevToolsPanel instance is still valid, but the built devtools panel is removed from
+    //   the toolbox (and re-built again if the user re-enables it from the toolbox preferences panel)
+    // - when the creator context has been destroyed, the ParentDevToolsPanel close method is called,
+    //   it removes the tool definition from the toolbox, which will call this destroy method.
+    return () => {
+      this.destroyBrowserElement();
+      toolbox.off("select", this.onToolboxPanelSelect);
+    };
+  }
+
+  async onToolboxPanelSelect(what, id) {
+    if (!this.waitTopLevelContext || !this.panelAdded) {
+      return;
+    }
+
+    // Wait that the panel is fully loaded and emit show.
+    await this.waitTopLevelContext;
+
+    if (!this.visible && id === this.id) {
+      this.visible = true;
+    } else if (this.visible && id !== this.id) {
+      this.visible = false;
+    }
+
+    const extensionMessage = `Extension:DevToolsPanel${this.visible ? "Shown" : "Hidden"}`;
+    this.context.parentMessageManager.sendAsyncMessage(extensionMessage, {
+      toolboxPanelId: this.id,
+    });
+  }
+
+  close() {
+    const {toolbox} = this;
+
+    if (!toolbox) {
+      throw new Error("Unable to destroy a closed devtools panel");
+    }
+
+    // Explicitly remove the panel if it is registered and the toolbox is not
+    // closing itself.
+    if (this.panelAdded && toolbox.isToolRegistered(this.id)) {
+      toolbox.removeAdditionalTool(this.id);
+    }
+
+    this.context = null;
+    this.toolbox = null;
+    this.waitTopLevelContext = null;
+    this._resolveTopLevelContext = null;
+  }
+
+  createBrowserElement(window) {
+    const {toolbox} = this;
     const {url} = this.panelOptions;
     const {document} = window;
 
     const browser = document.createElementNS(XUL_NS, "browser");
     browser.setAttribute("type", "content");
     browser.setAttribute("disableglobalhistory", "true");
     browser.setAttribute("style", "width: 100%; height: 100%;");
     browser.setAttribute("transparent", "true");
@@ -125,17 +185,17 @@ class ParentDevToolsPanel {
       // because of a swapFrameLoader exception (see bug 1075490).
       browser.setAttribute("type", "chrome");
       browser.setAttribute("forcemessagemanager", true);
     }
 
     let hasTopLevelContext = false;
 
     // Listening to new proxy contexts.
-    const unwatchExtensionProxyContextLoad = watchExtensionProxyContextLoad(this, context => {
+    this.unwatchExtensionProxyContextLoad = watchExtensionProxyContextLoad(this, context => {
       // Keep track of the toolbox and target associated to the context, which is
       // needed by the API methods implementation.
       context.devToolsToolbox = toolbox;
 
       if (!hasTopLevelContext) {
         hasTopLevelContext = true;
 
         // Resolve the promise when the root devtools_panel context has been created.
@@ -149,75 +209,36 @@ class ParentDevToolsPanel {
     extensions.emit("extension-browser-inserted", browser, {
       devtoolsToolboxInfo: {
         toolboxPanelId: this.id,
         inspectedWindowTabId: getTargetTabIdForToolbox(toolbox),
       },
     });
 
     browser.loadURI(url);
-
-    toolbox.on("select", this.onToolboxPanelSelect);
-
-    // Return a cleanup method that is when the panel is destroyed, e.g.
-    // - when addon devtool panel has been disabled by the user from the toolbox preferences,
-    //   its ParentDevToolsPanel instance is still valid, but the built devtools panel is removed from
-    //   the toolbox (and re-built again if the user re-enable it from the toolbox preferences panel)
-    // - when the creator context has been destroyed, the ParentDevToolsPanel close method is called,
-    //   it remove the tool definition from the toolbox, which will call this destroy method.
-    return () => {
-      unwatchExtensionProxyContextLoad();
-      browser.remove();
-      toolbox.off("select", this.onToolboxPanelSelect);
-
-      // If the panel has been disabled from the toolbox preferences,
-      // we need to re-initialize the waitTopLevelContext Promise.
-      this.waitTopLevelContext = new Promise(resolve => {
-        this._resolveTopLevelContext = resolve;
-      });
-    };
   }
 
-  onToolboxPanelSelect(what, id) {
-    if (!this.waitTopLevelContext || !this.panelAdded) {
-      return;
-    }
-    if (!this.visible && id === this.id) {
-      // Wait that the panel is fully loaded and emit show.
-      this.waitTopLevelContext.then(() => {
-        this.visible = true;
-        this.context.parentMessageManager.sendAsyncMessage("Extension:DevToolsPanelShown", {
-          toolboxPanelId: this.id,
-        });
-      });
-    } else if (this.visible && id !== this.id) {
-      this.visible = false;
-      this.context.parentMessageManager.sendAsyncMessage("Extension:DevToolsPanelHidden", {
-        toolboxPanelId: this.id,
-      });
-    }
-  }
-
-  close() {
-    const {toolbox} = this;
-
-    if (!toolbox) {
-      throw new Error("Unable to destroy a closed devtools panel");
+  destroyBrowserElement() {
+    const {browser, unwatchExtensionProxyContextLoad} = this;
+    if (unwatchExtensionProxyContextLoad) {
+      this.unwatchExtensionProxyContextLoad = null;
+      unwatchExtensionProxyContextLoad();
     }
 
-    // Explicitly remove the panel if it is registered and the toolbox is not
-    // closing itself.
-    if (this.panelAdded && toolbox.isToolRegistered(this.id) && !toolbox._destroyer) {
-      toolbox.removeAdditionalTool(this.id);
+    if (browser) {
+      browser.remove();
+      this.browser = null;
     }
 
-    this.context = null;
-    this.toolbox = null;
-    this.waitTopLevelContext = null;
-    this._resolveTopLevelContext = null;
+    // If the panel has been removed or disabled (e.g. from the toolbox preferences
+    // or during the toolbox switching between docked and undocked),
+    // we need to re-initialize the waitTopLevelContext Promise.
+    this.waitTopLevelContext = new Promise(resolve => {
+      this._resolveTopLevelContext = resolve;
+    });
   }
 }
 
 class DevToolsSelectionObserver extends EventEmitter {
   constructor(context) {
     if (!context.devToolsToolbox) {
       // This should never happen when this constructor is called with a valid
       // devtools extension context.
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -1542,16 +1542,21 @@ Toolbox.prototype = {
 
   /**
    * Unregister and unload an additional tool from this particular toolbox.
    *
    * @param {string} toolId
    *        the id of the additional tool to unregister and remove.
    */
   removeAdditionalTool(toolId) {
+    // Early exit if the toolbox is already destroying itself.
+    if (this._destroyer) {
+      return;
+    }
+
     if (!this.hasAdditionalTool(toolId)) {
       throw new Error("Tool definition not registered to this toolbox: " +
                       toolId);
     }
 
     this.additionalToolDefinitions.delete(toolId);
     this.visibleAdditionalTools = this.visibleAdditionalTools
                                       .filter(id => id !== toolId);