Bug 1361080 - display devtools onboarding screen if devtools.enabled if false;r=ochameau draft
authorJulian Descottes <jdescottes@mozilla.com>
Mon, 28 Aug 2017 15:12:03 +0200
changeset 678538 29b9149cecfc6c1087bce42b3afdd6b88c82e62e
parent 678476 808377154712b98c1bab76ee83ecbd72cb0ddc47
child 678539 b1dd8e4163fb6cbec236f8a1e3b64ab65b15577c
push id83952
push userjdescottes@mozilla.com
push dateWed, 11 Oct 2017 14:45:10 +0000
reviewersochameau
bugs1361080
milestone58.0a1
Bug 1361080 - display devtools onboarding screen if devtools.enabled if false;r=ochameau MozReview-Commit-ID: 6TtOAGc9Zzl
devtools/client/aboutdebugging/initializer.js
devtools/shim/DevToolsShim.jsm
devtools/shim/devtools-startup.js
devtools/shim/tests/unit/test_devtools_shim.js
--- a/devtools/client/aboutdebugging/initializer.js
+++ b/devtools/client/aboutdebugging/initializer.js
@@ -6,16 +6,18 @@
 /* globals DebuggerClient, DebuggerServer, Telemetry */
 
 "use strict";
 
 const { loader } = Components.utils.import(
   "resource://devtools/shared/Loader.jsm", {});
 const { BrowserLoader } = Components.utils.import(
   "resource://devtools/client/shared/browser-loader.js", {});
+const { Services } = Components.utils.import(
+  "resource://gre/modules/Services.jsm", {});
 
 loader.lazyRequireGetter(this, "DebuggerClient",
   "devtools/shared/client/debugger-client", true);
 loader.lazyRequireGetter(this, "DebuggerServer",
   "devtools/server/main", true);
 loader.lazyRequireGetter(this, "Telemetry",
   "devtools/client/shared/telemetry");
 
@@ -26,16 +28,22 @@ const { require } = BrowserLoader({
 
 const { createFactory } = require("devtools/client/shared/vendor/react");
 const { render, unmountComponentAtNode } = require("devtools/client/shared/vendor/react-dom");
 
 const AboutDebuggingApp = createFactory(require("./components/aboutdebugging"));
 
 var AboutDebugging = {
   init() {
+    if (!Services.prefs.getBoolPref("devtools.enabled", true)) {
+      // If DevTools are disabled, navigate to about:devtools.
+      window.location = "about:devtools?reason=AboutDebugging";
+      return;
+    }
+
     if (!DebuggerServer.initialized) {
       DebuggerServer.init();
     }
     DebuggerServer.allowChromeProcess = true;
     // We want a full featured server for about:debugging. Especially the
     // "browser actors" like addons.
     DebuggerServer.registerActors({ root: true, browser: true, tab: true });
 
@@ -48,18 +56,20 @@ var AboutDebugging = {
       render(AboutDebuggingApp({ client, telemetry }),
         document.querySelector("#body"));
     });
   },
 
   destroy() {
     unmountComponentAtNode(document.querySelector("#body"));
 
-    this.client.close();
-    this.client = null;
+    if (this.client) {
+      this.client.close();
+      this.client = null;
+    }
   },
 };
 
 window.addEventListener("DOMContentLoaded", function () {
   AboutDebugging.init();
 }, {once: true});
 
 window.addEventListener("unload", function () {
--- a/devtools/shim/DevToolsShim.jsm
+++ b/devtools/shim/DevToolsShim.jsm
@@ -39,34 +39,16 @@ function removeItem(array, callback) {
  * DevToolsShim.isInstalled() can also be used to know if DevTools are currently
  * installed.
  */
 this.DevToolsShim = {
   _gDevTools: null,
   listeners: [],
 
   /**
-   * Lazy getter for the `gDevTools` instance. Should only be called when users interacts
-   * with DevTools as it will force loading them.
-   *
-   * @return {DevTools} a devtools instance (from client/framework/devtools)
-   */
-  get gDevTools() {
-    if (!this.isInstalled()) {
-      throw new Error(`Trying to interact with DevTools, but they are not installed`);
-    }
-
-    if (!this.isInitialized()) {
-      this._initDevTools();
-    }
-
-    return this._gDevTools;
-  },
-
-  /**
    * Check if DevTools are currently installed (but not necessarily initialized).
    *
    * @return {Boolean} true if DevTools are installed.
    */
   isInstalled: function () {
     return Services.io.getProtocolHandler("resource")
              .QueryInterface(Ci.nsIResProtocolHandler)
              .hasSubstitution("devtools");
@@ -142,77 +124,87 @@ this.DevToolsShim = {
    *
    * @param {Object} state
    *                 A SessionStore state object that gets modified by reference
    */
   saveDevToolsSession: function (state) {
     if (!this.isInitialized()) {
       return;
     }
+
     this._gDevTools.saveDevToolsSession(state);
   },
 
   /**
    * Called from SessionStore.jsm in mozilla-central when restoring a state that contained
    * opened scratchpad windows and browser console.
    */
   restoreDevToolsSession: function (session) {
-    if (!this.isInstalled()) {
+    let devtoolsReady = this._maybeInitializeDevTools();
+    if (!devtoolsReady) {
       return;
     }
 
-    this.gDevTools.restoreDevToolsSession(session);
+    this._gDevTools.restoreDevToolsSession(session);
   },
 
   /**
    * Called from nsContextMenu.js in mozilla-central when using the Inspect Element
    * context menu item.
    *
    * @param {XULTab} tab
    *        The browser tab on which inspect node was used.
    * @param {Array} selectors
    *        An array of CSS selectors to find the target node. Several selectors can be
    *        needed if the element is nested in frames and not directly in the root
    *        document.
    * @return {Promise} a promise that resolves when the node is selected in the inspector
    *         markup view or that resolves immediately if DevTools are not installed.
    */
   inspectNode: function (tab, selectors) {
-    if (!this.isInstalled()) {
+    let devtoolsReady = this._maybeInitializeDevTools("ContextMenu");
+    if (!devtoolsReady) {
       return Promise.resolve();
     }
 
-    // Initialize DevTools explicitly to pass the "ContextMenu" reason to telemetry.
-    if (!this.isInitialized()) {
-      this._initDevTools("ContextMenu");
-    }
-
-    return this.gDevTools.inspectNode(tab, selectors);
-  },
-
-  /**
-   * Initialize DevTools via the devtools-startup command line handler component.
-   * Overridden in tests.
-   *
-   * @param {String} reason
-   *        optional, if provided should be a valid entry point for DEVTOOLS_ENTRY_POINT
-   *        in toolkit/components/telemetry/Histograms.json
-   */
-  _initDevTools: function (reason) {
-    DevtoolsStartup.initDevTools(reason);
+    return this._gDevTools.inspectNode(tab, selectors);
   },
 
   _onDevToolsRegistered: function () {
     // Register all pending event listeners on the real gDevTools object.
     for (let [event, listener] of this.listeners) {
       this._gDevTools.on(event, listener);
     }
 
     this.listeners = [];
   },
+
+  /**
+   * Should be called if a shim method attempts to initialize devtools.
+   * - if DevTools are already initialized, returns true.
+   * - if DevTools are not initialized, call initDevTools from devtools-startup:
+   *   - if devtools.enabled is true, DevTools will synchronously initialize and the
+   *     method will return true.
+   *   - if devtools.enabled is false, DevTools installation flow will start and the
+   *     method will return false
+   *
+   * @param {String} reason
+   *        optional, if provided should be a valid entry point for DEVTOOLS_ENTRY_POINT
+   *        in toolkit/components/telemetry/Histograms.json
+   */
+  _maybeInitializeDevTools: function (reason) {
+    // Attempt to initialize DevTools, which should be synchronous.
+    if (!this.isInitialized()) {
+      DevtoolsStartup.initDevTools(reason);
+    }
+
+    // The initialization process can lead to show the user installation screen, in this
+    // case this.isInitialized() will still be false after calling initDevTools().
+    return this.isInitialized();
+  }
 };
 
 /**
  * Compatibility layer for webextensions.
  *
  * Those methods are called only after a DevTools webextension was loaded in DevTools,
  * therefore DevTools should always be available when they are called.
  */
@@ -221,11 +213,16 @@ let webExtensionsMethods = [
   "createWebExtensionInspectedWindowFront",
   "getTargetForTab",
   "getTheme",
   "openBrowserConsole",
 ];
 
 for (let method of webExtensionsMethods) {
   this.DevToolsShim[method] = function () {
-    return this.gDevTools[method].apply(this.gDevTools, arguments);
+    let devtoolsReady = this._maybeInitializeDevTools();
+    if (!devtoolsReady) {
+      throw new Error("Could not call a DevToolsShim webextension method ('" + method +
+        "'): DevTools are not initialized.");
+    }
+    return this._gDevTools[method].apply(this._gDevTools, arguments);
   };
 }
--- a/devtools/shim/devtools-startup.js
+++ b/devtools/shim/devtools-startup.js
@@ -26,16 +26,18 @@ const kDebuggerPrefs = [
   "devtools.debugger.remote-enabled",
   "devtools.chrome.enabled"
 ];
 
 // If devtools.toolbar.visible is set to true, the developer toolbar should appear on
 // startup.
 const TOOLBAR_VISIBLE_PREF = "devtools.toolbar.visible";
 
+const DEVTOOLS_ENABLED_PREF = "devtools.enabled";
+
 const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
                                   "resource://gre/modules/AppConstants.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
                                   "resource:///modules/CustomizableUI.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "CustomizableWidgets",
@@ -183,16 +185,19 @@ DevToolsStartup.prototype = {
    */
   developerToggleCreated: false,
 
   handle: function (cmdLine) {
     let consoleFlag = cmdLine.handleFlag("jsconsole", false);
     let debuggerFlag = cmdLine.handleFlag("jsdebugger", false);
     let devtoolsFlag = cmdLine.handleFlag("devtools", false);
 
+    let hasDevToolsFlag = consoleFlag || devtoolsFlag || debuggerFlag;
+    this.setupEnabledPref(hasDevToolsFlag);
+
     if (consoleFlag) {
       this.handleConsoleFlag(cmdLine);
     }
     if (debuggerFlag) {
       this.handleDebuggerFlag(cmdLine);
     }
     let debuggerServerFlag;
     try {
@@ -336,16 +341,35 @@ DevToolsStartup.prototype = {
    */
   hookWebDeveloperMenu(window) {
     let menu = window.document.getElementById("webDeveloperMenu");
     menu.addEventListener("popupshowing", () => {
       this.initDevTools("SystemMenu");
     }, { once: true });
   },
 
+  /**
+   * Depending on some runtime parameters (command line arguments as well as existing
+   * preferences), the DEVTOOLS_ENABLED_PREF might be forced to true.
+   *
+   * @param {Boolean} hasDevToolsFlag
+   *        true if any DevTools command line argument was passed when starting Firefox.
+   */
+  setupEnabledPref(hasDevToolsFlag) {
+    if (Services.prefs.getBoolPref(DEVTOOLS_ENABLED_PREF)) {
+      // Nothing to do if DevTools are already enabled.
+      return;
+    }
+
+    let hasToolbarPref = Services.prefs.getBoolPref(TOOLBAR_VISIBLE_PREF, false);
+    if (hasDevToolsFlag || hasToolbarPref) {
+      Services.prefs.setBoolPref(DEVTOOLS_ENABLED_PREF, true);
+    }
+  },
+
   hookKeyShortcuts(window) {
     let doc = window.document;
     let keyset = doc.createElement("keyset");
     keyset.setAttribute("id", "devtoolsKeyset");
 
     for (let key of KeyShortcuts) {
       let xulKey = this.createKey(doc, key, () => this.onKey(window, key));
       keyset.appendChild(xulKey);
@@ -355,18 +379,21 @@ DevToolsStartup.prototype = {
     // to be detached and reattached to make sure the <key> is taken into
     // account (see bug 832984).
     let mainKeyset = doc.getElementById("mainKeyset");
     mainKeyset.parentNode.insertBefore(keyset, mainKeyset);
   },
 
   onKey(window, key) {
     let require = this.initDevTools("KeyShortcut");
-    let { gDevToolsBrowser } = require("devtools/client/framework/devtools-browser");
-    gDevToolsBrowser.onKeyShortcut(window, key);
+    if (require) {
+      // require might be null if initDevTools was called while DevTools are disabled.
+      let { gDevToolsBrowser } = require("devtools/client/framework/devtools-browser");
+      gDevToolsBrowser.onKeyShortcut(window, key);
+    }
   },
 
   // Create a <xul:key> DOM Element
   createKey(doc, { id, toolId, shortcut, modifiers: mod }, oncommand) {
     let k = doc.createElement("key");
     k.id = "key_" + (id || toolId);
 
     if (shortcut.startsWith("VK_")) {
@@ -382,45 +409,56 @@ DevToolsStartup.prototype = {
     // Bug 371900: command event is fired only if "oncommand" attribute is set.
     k.setAttribute("oncommand", ";");
     k.addEventListener("command", oncommand);
 
     return k;
   },
 
   initDevTools: function (reason) {
+    // If an entry point is fired and tools are not enabled open the installation page
+    if (!Services.prefs.getBoolPref(DEVTOOLS_ENABLED_PREF)) {
+      this.openInstallPage(reason);
+      return null;
+    }
+
     if (reason && !this.recorded) {
       // Only save the first call for each firefox run as next call
       // won't necessarely start the tool. For example key shortcuts may
       // only change the currently selected tool.
       try {
         Services.telemetry.getHistogramById("DEVTOOLS_ENTRY_POINT")
                           .add(reason);
       } catch (e) {
         dump("DevTools telemetry entry point failed: " + e + "\n");
       }
       this.recorded = true;
     }
-    if (!this.initialized) {
-      Services.prefs.setBoolPref("devtools.enabled", true);
-    }
+
     this.initialized = true;
     let { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
     // Ensure loading main devtools module that hooks up into browser UI
     // and initialize all devtools machinery.
     require("devtools/client/framework/devtools-browser");
     return require;
   },
 
+  openInstallPage: function (reason) {
+    let { gBrowser } = Services.wm.getMostRecentWindow("navigator:browser");
+    let url = "about:devtools";
+    if (reason) {
+      url += "?reason=" + encodeURIComponent(reason);
+    }
+    gBrowser.selectedTab = gBrowser.addTab(url);
+  },
+
   handleConsoleFlag: function (cmdLine) {
     let window = Services.wm.getMostRecentWindow("devtools:webconsole");
     if (!window) {
-      this.initDevTools("CommandLine");
-
-      let { require } = Cu.import("resource://devtools/shared/Loader.jsm", {});
+      let require = this.initDevTools("CommandLine");
       let { HUDService } = require("devtools/client/webconsole/hudservice");
       let { console } = Cu.import("resource://gre/modules/Console.jsm", {});
       HUDService.toggleBrowserConsole().catch(console.error);
     } else {
       // the Browser Console was already open
       window.focus();
     }
 
--- a/devtools/shim/tests/unit/test_devtools_shim.js
+++ b/devtools/shim/tests/unit/test_devtools_shim.js
@@ -32,25 +32,16 @@ function createMockDevTools() {
       mock.callLog[method].push(args);
     };
     mock.callLog[method] = [];
   }
 
   return mock;
 }
 
-let isInstalledMethodBackup = DevToolsShim.isInstalled;
-function mockDevToolsInstalled(isInstalled) {
-  DevToolsShim.isInstalled = () => isInstalled;
-}
-
-function restoreDevToolsInstalled() {
-  DevToolsShim.isInstalled = isInstalledMethodBackup;
-}
-
 /**
  * Check if a given method was called an expected number of times, and finally check the
  * arguments provided to the last call, if appropriate.
  */
 function checkCalls(mock, method, length, lastArgs) {
   ok(mock.callLog[method].length === length,
       "Devtools.on was called the expected number of times");
 
@@ -62,147 +53,121 @@ function checkCalls(mock, method, length
   for (let i = 0; i < lastArgs.length; i++) {
     let expectedArg = lastArgs[i];
     ok(mock.callLog[method][length - 1][i] === expectedArg,
         `Devtools.${method} was called with the expected argument (index ${i})`);
   }
 }
 
 function test_register_unregister() {
-  mockDevToolsInstalled(true);
-
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
 
   DevToolsShim.register(createMockDevTools());
   ok(DevToolsShim.isInitialized(), "DevTools are installed");
 
   DevToolsShim.unregister();
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
-
-  restoreDevToolsInstalled();
 }
 
 function test_on_is_forwarded_to_devtools() {
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
 
   function cb1() {}
   function cb2() {}
   let mock = createMockDevTools();
 
   DevToolsShim.on("test_event", cb1);
   DevToolsShim.register(mock);
   checkCalls(mock, "on", 1, ["test_event", cb1]);
 
   DevToolsShim.on("other_event", cb2);
   checkCalls(mock, "on", 2, ["other_event", cb2]);
-
-  restoreDevToolsInstalled();
 }
 
 function test_off_called_before_registering_devtools() {
-  mockDevToolsInstalled(true);
-
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
 
   function cb1() {}
   let mock = createMockDevTools();
 
   DevToolsShim.on("test_event", cb1);
   DevToolsShim.off("test_event", cb1);
 
   DevToolsShim.register(mock);
   checkCalls(mock, "on", 0);
-
-  restoreDevToolsInstalled();
 }
 
 function test_off_called_before_with_bad_callback() {
-  mockDevToolsInstalled(true);
-
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
 
   function cb1() {}
   function cb2() {}
   let mock = createMockDevTools();
 
   DevToolsShim.on("test_event", cb1);
   DevToolsShim.off("test_event", cb2);
 
   DevToolsShim.register(mock);
   // on should still be called
   checkCalls(mock, "on", 1, ["test_event", cb1]);
   // Calls to off should not be held and forwarded.
   checkCalls(mock, "off", 0);
-
-  restoreDevToolsInstalled();
 }
 
 function test_events() {
-  mockDevToolsInstalled(true);
-
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
 
   let mock = createMockDevTools();
   // Check emit was not called.
   checkCalls(mock, "emit", 0);
 
   // Check emit is called once with the devtools-registered event.
   DevToolsShim.register(mock);
   checkCalls(mock, "emit", 1, ["devtools-registered"]);
 
   // Check emit is called once with the devtools-unregistered event.
   DevToolsShim.unregister();
   checkCalls(mock, "emit", 2, ["devtools-unregistered"]);
-
-  restoreDevToolsInstalled();
 }
 
 function test_scratchpad_apis() {
-  mockDevToolsInstalled(false);
+  let backupMaybeInitializeDevTools = DevToolsShim._maybeInitializeDevTools;
+  DevToolsShim._maybeInitializeDevTools = () => {
+    return false;
+  };
 
-  ok(!DevToolsShim.isInstalled(), "DevTools are not installed");
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
 
-  // Ensure that saveDevToolsSession doesn't initialize the tools
+  // Ensure that save & restore DevToolsSession don't initialize the tools and don't crash
   DevToolsShim.saveDevToolsSession({});
-
-  ok(!DevToolsShim.isInstalled(), "DevTools are not installed");
-  ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
-
-  // Check that restoreDevToolsSession doesn't crash.
   DevToolsShim.restoreDevToolsSession({
     scratchpads: [{}],
     browserConsole: true,
   });
 
-  // Check that saveDevToolsSession doesn't crash.
-  DevToolsShim.saveDevToolsSession({});
-
-  mockDevToolsInstalled(true);
-
-  ok(DevToolsShim.isInstalled(), "DevTools are installed");
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
 
   let mock = createMockDevTools();
-  DevToolsShim._initDevTools = () => {
+  DevToolsShim._maybeInitializeDevTools = () => {
     // Next call to restoreDevToolsSession is expected to initialize DevTools, which we
     // simulate here by registering our mock.
     DevToolsShim.register(mock);
+    return true;
   };
 
   let scratchpadSessions = [{}];
   DevToolsShim.restoreDevToolsSession(scratchpadSessions);
   checkCalls(mock, "restoreDevToolsSession", 1, [scratchpadSessions]);
 
   ok(DevToolsShim.isInitialized(), "DevTools are initialized");
 
   DevToolsShim.saveDevToolsSession({});
   checkCalls(mock, "saveDevToolsSession", 1, []);
 
-  restoreDevToolsInstalled();
+  DevToolsShim._maybeInitializeDevTools = backupMaybeInitializeDevTools;
 }
 
 function run_test() {
   test_register_unregister();
   DevToolsShim.unregister();
 
   test_on_is_forwarded_to_devtools();
   DevToolsShim.unregister();