Bug 1266134 - address review 2 for: Pull host management out of toolbox.xul.fix pull out host. r=jryans draft
authorAlexandre Poirot <poirot.alex@gmail.com>
Wed, 28 Sep 2016 07:10:00 -0700
changeset 418493 fa5a6b55d5a34a56b30096387ea53f6cb2a7bed0
parent 417864 4a6ec59a319cdf794af2a9746fea0ad6c480214e
child 418494 34394e1ded5e9d5d0a66b2dbf843db878eafd3bc
push id30691
push userbmo:poirot.alex@gmail.com
push dateWed, 28 Sep 2016 15:53:00 +0000
reviewersjryans
bugs1266134
milestone52.0a1
Bug 1266134 - address review 2 for: Pull host management out of toolbox.xul.fix pull out host. r=jryans MozReview-Commit-ID: CmHctXBYpK0
devtools/client/framework/toolbox-browser-integration.js
devtools/client/framework/toolbox.js
--- a/devtools/client/framework/toolbox-browser-integration.js
+++ b/devtools/client/framework/toolbox-browser-integration.js
@@ -1,64 +1,76 @@
 const Services = require("Services");
-const { Ci } = require("chrome");
+const {Ci} = require("chrome");
 const {LocalizationHelper} = require("devtools/shared/l10n");
 const L10N = new LocalizationHelper("devtools/locale/toolbox.properties");
 const DevToolsUtils = require("devtools/shared/DevToolsUtils");
 const {Task} = require("devtools/shared/task");
 
 loader.lazyRequireGetter(this, "Toolbox", "devtools/client/framework/toolbox", true);
 loader.lazyRequireGetter(this, "Hosts", "devtools/client/framework/toolbox-hosts", true);
 
 /**
  * Implement a wrapper on the chrome side to setup a Toolbox within Firefox UI.
  *
  * This component handles iframe creation within Firefox, in which we are loading
  * the toolbox document. Then both the chrome and the toolbox document communicate
  * via "message" events.
  *
  * Messages sent by the toolbox to the chrome:
- * - switch-host: order to display the toolbox in another host (side, bottom
- *                or window)
- * - toggle-minimize-mode: when using the bottom host, the toolbox can be
- *                         miximized to only display the tool titles
- * - maximize-host: when using the bottom host in minimized mode, revert back
- *                  to regular mode in order to see tool titles and the tools
- * - raise-host: focus the tools
- * - set-host-title: when using the window host, update the window title
+ * - switch-host:
+ *   Order to display the toolbox in another host (side, bottom or window)
+ * - switch-to-previous-host:
+ *   Order to display the toolbox in the previously used host
+ * - toggle-minimize-mode:
+ *   When using the bottom host, the toolbox can be miximized to only display
+ *   the tool titles
+ * - maximize-host:
+ *   When using the bottom host in minimized mode, revert back to regular mode
+ *   in order to see tool titles and the tools
+ * - raise-host:
+ *   Focus the tools
+ * - set-host-title:
+ *   When using the window host, update the window title
  *
  * Messages sent by the chrome to the toolbox:
- * - host-minimized: the bottom host is done minimizing (after animation end)
- * - host-maximized: the bottom host is done switching back to regular mode
- *                   (after animation end)
- * - switched-host: the `switch-host` command sent by the toolbox is done
+ * - host-minimized:
+ *   The bottom host is done minimizing (after animation end)
+ * - host-maximized:
+ *   The bottom host is done switching back to regular mode (after animation
+ *   end)
+ * - switched-host:
+ *   The `switch-host` command sent by the toolbox is done
  */
 
 const LAST_HOST = "devtools.toolbox.host";
+const PREVIOUS_HOST = "devtools.toolbox.previousHost";
 let ID_COUNTER = 1;
 
 function ToolboxBrowserIntegration(target, hostType, hostOptions) {
   this.target = target;
 
   this.frameId = ID_COUNTER++;
 
   if (!hostType) {
     hostType = Services.prefs.getCharPref(LAST_HOST);
   }
   this.onHostMinimized = this.onHostMinimized.bind(this);
   this.onHostMaximized = this.onHostMaximized.bind(this);
   this.host = this.createHost(hostType, hostOptions);
+  this.hostType = hostType;
 }
 
 ToolboxBrowserIntegration.prototype = {
   create: Task.async(function* (toolId) {
     yield this.host.create();
 
     this.host.frame.setAttribute("aria-label", L10N.getStr("toolbox.label"));
     this.host.frame.ownerDocument.defaultView.addEventListener("message", this);
+    // We have to listen on capture as no event fires on bubble
     this.host.frame.addEventListener("unload", this, true);
 
     let toolbox = new Toolbox(this.target, toolId, this.host.type, this.host.frame.contentWindow, this.frameId);
 
     // Prevent reloading the toolbox when loading the tools in a tab (e.g. from about:debugging)
     if (!this.host.frame.contentWindow.location.href.startsWith("about:devtools-toolbox")) {
       this.host.frame.setAttribute("src", "about:devtools-toolbox");
     }
@@ -76,17 +88,17 @@ ToolboxBrowserIntegration.prototype = {
         // we can only compare against locations. Here we filter two very
         // different cases: preliminary about:blank document as well as iframes
         // like tool iframes.
         if (!event.target.location.href.startsWith("about:devtools-toolbox")) {
           break;
         }
         // Don't destroy the host during unload event (esp., don't remove the
         // iframe from DOM!). Otherwise the unload event for the toolbox
-        // document doesn't fire within the toolbox *document*! (here this is
+        // document doesn't fire within the toolbox *document*! This is
         // the unload event that fires on the toolbox *iframe*.
         DevToolsUtils.executeSoon(() => {
           this.destroy();
         });
         break;
     }
   },
 
@@ -98,16 +110,19 @@ ToolboxBrowserIntegration.prototype = {
     // origin via event.source as it is null. So use a custom id.
     if (event.data.frameId != this.frameId) {
       return;
     }
     switch (event.data.name) {
       case "switch-host":
         this.switchHost(event.data.hostType);
         break;
+      case "switch-to-previous-host":
+        this.switchToPreviousHost();
+        break;
       case "maximize-host":
         this.host.maximize();
         break;
       case "raise-host":
         this.host.raise();
         break;
       case "toggle-minimize-mode":
         this.host.toggleMinimizeMode(event.data.toolbarHeight);
@@ -121,16 +136,17 @@ ToolboxBrowserIntegration.prototype = {
   postMessage(data) {
     let window = this.host.frame.contentWindow;
     window.postMessage(data, "*");
   },
 
   destroy() {
     this.destroyHost();
     this.host = null;
+    this.hostType = null;
     this.target = null;
   },
 
   /**
    * Create a host object based on the given host type.
    *
    * Warning: bottom and sidebar hosts require that the toolbox target provides
    * a reference to the attached tab. Not all Targets have a tab property -
@@ -161,31 +177,56 @@ ToolboxBrowserIntegration.prototype = {
   },
 
   onHostMaximized() {
     this.postMessage({
       name: "host-maximized"
     });
   },
 
+  /**
+   * Switch to the last used host for the toolbox UI.
+   * This is determined by the devtools.toolbox.previousHost pref.
+   */
+  switchToPreviousHost() {
+    let hostType = Services.prefs.getCharPref(PREVIOUS_HOST);
+
+    // Handle the case where the previous host happens to match the current
+    // host. If so, switch to bottom if it's not already used, and side if not.
+    if (hostType === this.hostType) {
+      if (hostType === Toolbox.HostType.BOTTOM) {
+        hostType = Toolbox.HostType.SIDE;
+      } else {
+        hostType = Toolbox.HostType.BOTTOM;
+      }
+    }
+
+    return this.switchHost(hostType);
+  },
+
   switchHost: Task.async(function* (hostType) {
     let iframe = this.host.frame;
     let newHost = this.createHost(hostType);
     let newIframe = yield newHost.create();
     // change toolbox document's parent to the new host
     newIframe.swapFrameLoaders(iframe);
 
     // See bug 1022726, most probably because of swapFrameLoaders we need to
     // first focus the window here, and then once again further from
     // toolbox.js to make sure focus actually happens.
     iframe.contentWindow.focus();
 
     this.destroyHost();
 
+    if (this.hostType != Toolbox.HostType.CUSTOM) {
+      Services.prefs.setCharPref(PREVIOUS_HOST, this.hostType);
+    }
+
     this.host = newHost;
+    this.hostType = hostType;
     this.host.setTitle(this.host.frame.contentWindow.document.title);
     this.host.frame.ownerDocument.defaultView.addEventListener("message", this);
     this.host.frame.addEventListener("unload", this, true);
 
     if (hostType != Toolbox.HostType.CUSTOM) {
       Services.prefs.setCharPref(LAST_HOST, hostType);
     }
 
--- a/devtools/client/framework/toolbox.js
+++ b/devtools/client/framework/toolbox.js
@@ -99,17 +99,17 @@ const ToolboxButtons = exports.ToolboxBu
  *        The object the toolbox is debugging.
  * @param {string} selectedTool
  *        Tool to select initially
  * @param {Toolbox.HostType} hostType
  *        Type of host that will host the toolbox (e.g. sidebar, window)
  * @param {DOMWindow} contentWindow
  *        The window object of the toolbox document
  * @param {string} frameId
- *        A unique identifier to differenciate toolbox documents from the
+ *        A unique identifier to differentiate toolbox documents from the
  *        chrome codebase when passing DOM messages
  */
 function Toolbox(target, selectedTool, hostType, contentWindow, frameId) {
   this._target = target;
   this._win = contentWindow;
   this.frameId = frameId;
 
   this._toolPanels = new Map();
@@ -187,17 +187,16 @@ Toolbox.HostType = {
 };
 
 Toolbox.prototype = {
   _URL: "about:devtools-toolbox",
 
   _prefs: {
     LAST_TOOL: "devtools.toolbox.selectedTool",
     SIDE_ENABLED: "devtools.toolbox.sideEnabled",
-    PREVIOUS_HOST: "devtools.toolbox.previousHost"
   },
 
   currentToolId: null,
   lastUsedToolId: null,
 
   /**
    * Returns a *copy* of the _toolPanels collection.
    *
@@ -606,26 +605,26 @@ Toolbox.prototype = {
                  (name, event) => {
                    this.switchToPreviousHost();
                    event.preventDefault();
                  });
 
     this.doc.addEventListener("keypress", this._splitConsoleOnKeypress, false);
     this.doc.addEventListener("focus", this._onFocus, true);
     this.win.addEventListener("unload", this.destroy);
-    this.win.addEventListener("message", this._onBrowserMessage);
+    this.win.addEventListener("message", this._onBrowserMessage, true);
   },
 
   _removeHostListeners: function () {
     // The host iframe's contentDocument may already be gone.
     if (this.doc) {
       this.doc.removeEventListener("keypress", this._splitConsoleOnKeypress, false);
       this.doc.removeEventListener("focus", this._onFocus, true);
       this.win.removeEventListener("unload", this.destroy);
-      this.win.removeEventListener("message", this._onBrowserMessage);
+      this.win.removeEventListener("message", this._onBrowserMessage, true);
     }
   },
 
   // Called whenever the chrome send a message
   _onBrowserMessage: function (event) {
     if (!event.data) {
       return;
     }
@@ -1801,32 +1800,22 @@ Toolbox.prototype = {
     // marked as 'checked' indicating that a child frame is active.
     if (!topFrameSelected && this.selectedFrameId) {
       button.setAttribute("checked", "true");
     }
   },
 
   /**
    * Switch to the last used host for the toolbox UI.
-   * This is determined by the devtools.toolbox.previousHost pref.
    */
   switchToPreviousHost: function () {
-    let hostType = Services.prefs.getCharPref(this._prefs.PREVIOUS_HOST);
-
-    // Handle the case where the previous host happens to match the current
-    // host. If so, switch to bottom if it's not already used, and side if not.
-    if (hostType === this.hostType) {
-      if (hostType === Toolbox.HostType.BOTTOM) {
-        hostType = Toolbox.HostType.SIDE;
-      } else {
-        hostType = Toolbox.HostType.BOTTOM;
-      }
-    }
-
-    return this.switchHost(hostType);
+    this.postMessage({
+      name: "switch-to-previous-host"
+    });
+    return this.once("host-changed");
   },
 
   /**
    * Switch to a new host for the toolbox UI. E.g. bottom, sidebar, window,
    * and focus the window when done.
    *
    * @param {string} hostType
    *        The host type of the new host object
@@ -1850,20 +1839,16 @@ Toolbox.prototype = {
       name: "switch-host",
       hostType
     });
 
     return this.once("host-changed");
   },
 
   _onSwitchedHost: function ({ hostType }) {
-    if (hostType != Toolbox.HostType.CUSTOM) {
-      Services.prefs.setCharPref(this._prefs.PREVIOUS_HOST, this.hostType);
-    }
-
     this._hostType = hostType;
 
     this._buildDockButtons();
     this._addKeysToWindow();
 
     // Focus the tool to make sure keyboard shortcuts work straight away.
     this.focusTool(this.currentToolId, true);