Bug 1412050 - avoid devtools onboarding flow when restoring session;r=ochameau draft
authorJulian Descottes <jdescottes@mozilla.com>
Thu, 26 Oct 2017 20:16:05 +0200
changeset 687443 533d576fc094290e89feb7a4feb77b83054d458c
parent 686866 ffc9aefc91c94e65f3354c0ee46b76b733334162
child 687444 137ffca2e8ebdee6056e6746f54afa3e0c4539aa
push id86510
push userjdescottes@mozilla.com
push dateFri, 27 Oct 2017 08:48:05 +0000
reviewersochameau
bugs1412050
milestone58.0a1
Bug 1412050 - avoid devtools onboarding flow when restoring session;r=ochameau MozReview-Commit-ID: 8fJOPaOnkQe
devtools/shim/DevToolsShim.jsm
devtools/shim/tests/unit/test_devtools_shim.js
--- a/devtools/shim/DevToolsShim.jsm
+++ b/devtools/shim/DevToolsShim.jsm
@@ -9,16 +9,18 @@ const { Services } = Cu.import("resource
 
 const { XPCOMUtils } = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
 XPCOMUtils.defineLazyGetter(this, "DevtoolsStartup", () => {
   return Cc["@mozilla.org/devtools/startup-clh;1"]
             .getService(Ci.nsICommandLineHandler)
             .wrappedJSObject;
 });
 
+const DEVTOOLS_ENABLED_PREF = "devtools.enabled";
+
 this.EXPORTED_SYMBOLS = [
   "DevToolsShim",
 ];
 
 function removeItem(array, callback) {
   let index = array.findIndex(callback);
   if (index >= 0) {
     array.splice(index, 1);
@@ -50,16 +52,25 @@ this.DevToolsShim = {
    */
   isInstalled: function () {
     return Services.io.getProtocolHandler("resource")
              .QueryInterface(Ci.nsIResProtocolHandler)
              .hasSubstitution("devtools");
   },
 
   /**
+   * Returns true if DevTools are enabled for the current profile. If devtools are not
+   * enabled, initializing DevTools will open the onboarding page. Some entry points
+   * should no-op in this case.
+   */
+  isEnabled: function () {
+    return Services.prefs.getBoolPref(DEVTOOLS_ENABLED_PREF);
+  },
+
+  /**
    * Check if DevTools have already been initialized.
    *
    * @return {Boolean} true if DevTools are initialized.
    */
   isInitialized: function () {
     return !!this._gDevTools;
   },
 
@@ -129,25 +140,25 @@ this.DevToolsShim = {
     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.
+   * Called from SessionStore.jsm in mozilla-central when restoring a previous session.
+   * Will always be called, even if the session does not contain DevTools related items.
    */
   restoreDevToolsSession: function (session) {
-    let devtoolsReady = this._maybeInitializeDevTools();
-    if (!devtoolsReady) {
+    if (!this.isEnabled()) {
       return;
     }
 
+    this.initDevTools();
     this._gDevTools.restoreDevToolsSession(session);
   },
 
   /**
    * Called from nsContextMenu.js in mozilla-central when using the Inspect Element
    * context menu item.
    *
    * @param {XULTab} tab
@@ -155,61 +166,58 @@ this.DevToolsShim = {
    * @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.isEnabled()) {
+      DevtoolsStartup.openInstallPage("ContextMenu");
+      return Promise.resolve();
+    }
+
     // Record the timing at which this event started in order to compute later in
     // gDevTools.showToolbox, the complete time it takes to open the toolbox.
     // i.e. especially take `DevtoolsStartup.initDevTools` into account.
     let { performance } = Services.appShell.hiddenDOMWindow;
     let startTime = performance.now();
 
-    let devtoolsReady = this._maybeInitializeDevTools("ContextMenu");
-    if (!devtoolsReady) {
-      return Promise.resolve();
-    }
+    this.initDevTools("ContextMenu");
 
     return this._gDevTools.inspectNode(tab, selectors, startTime);
   },
 
   _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
+   * Initialize DevTools via DevToolsStartup if needed. This method throws if DevTools are
+   * not enabled.. If the entry point is supposed to trigger the onboarding, call it
+   * explicitly via DevtoolsStartup.openInstallPage().
    *
    * @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.
+  initDevTools: function (reason) {
+    if (!this.isEnabled()) {
+      throw new Error("DevTools are not enabled and can not be initialized.");
+    }
+
     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.
@@ -219,16 +227,17 @@ let webExtensionsMethods = [
   "createWebExtensionInspectedWindowFront",
   "getTargetForTab",
   "getTheme",
   "openBrowserConsole",
 ];
 
 for (let method of webExtensionsMethods) {
   this.DevToolsShim[method] = function () {
-    let devtoolsReady = this._maybeInitializeDevTools();
-    if (!devtoolsReady) {
+    if (!this.isEnabled()) {
       throw new Error("Could not call a DevToolsShim webextension method ('" + method +
         "'): DevTools are not initialized.");
     }
+
+    this.initDevTools();
     return this._gDevTools[method].apply(this._gDevTools, arguments);
   };
 }
--- a/devtools/shim/tests/unit/test_devtools_shim.js
+++ b/devtools/shim/tests/unit/test_devtools_shim.js
@@ -1,16 +1,17 @@
 /* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 "use strict";
 
 const { DevToolsShim } =
     Components.utils.import("chrome://devtools-shim/content/DevToolsShim.jsm", {});
+const { Services } = Components.utils.import("resource://gre/modules/Services.jsm", {});
 
 // Test the DevToolsShim
 
 /**
  * Create a mocked version of DevTools that records all calls made to methods expected
  * to be called by DevToolsShim.
  */
 function createMockDevTools() {
@@ -123,63 +124,67 @@ function test_events() {
   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"]);
 }
 
-function test_scratchpad_apis() {
-  let backupMaybeInitializeDevTools = DevToolsShim._maybeInitializeDevTools;
-  DevToolsShim._maybeInitializeDevTools = () => {
-    return false;
-  };
+function test_restore_session_apis() {
+  // Backup method and preferences that will be updated for the test.
+  let initDevToolsBackup = DevToolsShim.initDevTools;
+  let devtoolsEnabledValue = Services.prefs.getBoolPref("devtools.enabled");
 
+  Services.prefs.setBoolPref("devtools.enabled", false);
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
+  ok(!DevToolsShim.isEnabled(), "DevTools are not enabled");
 
   // Ensure that save & restore DevToolsSession don't initialize the tools and don't crash
   DevToolsShim.saveDevToolsSession({});
   DevToolsShim.restoreDevToolsSession({
     scratchpads: [{}],
     browserConsole: true,
   });
 
+  Services.prefs.setBoolPref("devtools.enabled", true);
+  ok(DevToolsShim.isEnabled(), "DevTools are enabled");
   ok(!DevToolsShim.isInitialized(), "DevTools are not initialized");
 
   let mock = createMockDevTools();
-  DevToolsShim._maybeInitializeDevTools = () => {
+  DevToolsShim.initDevTools = () => {
     // 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, []);
 
-  DevToolsShim._maybeInitializeDevTools = backupMaybeInitializeDevTools;
+  // Restore initial backups.
+  DevToolsShim.initDevTools = initDevToolsBackup;
+  Services.prefs.setBoolPref("devtools.enabled", devtoolsEnabledValue);
 }
 
 function run_test() {
   test_register_unregister();
   DevToolsShim.unregister();
 
   test_on_is_forwarded_to_devtools();
   DevToolsShim.unregister();
 
   test_off_called_before_registering_devtools();
   DevToolsShim.unregister();
 
   test_off_called_before_with_bad_callback();
   DevToolsShim.unregister();
 
-  test_scratchpad_apis();
+  test_restore_session_apis();
   DevToolsShim.unregister();
 
   test_events();
 }