Bug 1621022 - Change the workflow of enabling the profiler menu button; r=canaltinova

This patch makes it so that the profiler shortcuts work based on the location of
the profiler menu button. It changes it so that if the menu button is in the navbar
or other menus, the shortcuts will work. Otherwise, the shortcuts will be a no-op.

This removes the Tools -> Web Developer - Enable Profiler Menu Button option. By
default on Nightly and Dev Edition the profiler menu button will be available.
On other channels, users must visit profiler.firefox.com.

Differential Revision: https://phabricator.services.mozilla.com/D66785

--HG--
extra : moz-landing-system : lando

devtools/client/framework/devtools-browser.js

@@ -57,12 +57,6 @@ loader.lazyRequireGetter(
   "ResponsiveUIManager",
   "devtools/client/responsive/manager"
 );
-loader.lazyRequireGetter(
-  this,
-  "AppConstants",
-  "resource://gre/modules/AppConstants.jsm",
-  true
-);
 loader.lazyImporter(
   this,
   "BrowserToolboxLauncher",
@@ -144,29 +138,6 @@ var gDevToolsBrowser = (exports.gDevToolsBrowser = {
       "menu_browserContentToolbox",
       remoteEnabled && win.gMultiProcessBrowser
     );
-
-    // The profiler's popup is experimental. The plan is to eventually turn it on
-    // everywhere, but while it's under active development we don't want everyone
-    // having it enabled. For now the default pref is to turn it on with Nightly,
-    // with the option to flip the pref in other releases. This feature flag will
-    // go away once it is fully shipped.
-    const isPopupFeatureFlagEnabled = Services.prefs.getBoolPref(
-      "devtools.performance.popup.feature-flag",
-      AppConstants.NIGHTLY_BUILD
-    );
-    // If the feature flag is disabled, hide the menu item.
-    toggleMenuItem("menu_toggleProfilerButtonMenu", isPopupFeatureFlagEnabled);
-
-    if (isPopupFeatureFlagEnabled) {
-      // Did the user enable the profiler button in the menu? If it is then update the
-      // initial UI to show the menu item as checked.
-      if (
-        Services.prefs.getBoolPref("devtools.performance.popup.enabled", false)
-      ) {
-        const cmd = doc.getElementById("menu_toggleProfilerButtonMenu");
-        cmd.setAttribute("checked", "true");
-      }
-    }
   },
 
   /**

devtools/client/locales/en-US/menus.properties

@@ -28,11 +28,6 @@ browserToolboxMenu.accesskey = e
 browserContentToolboxMenu.label = Browser Content Toolbox
 browserContentToolboxMenu.accesskey = x
 
-# LOCALIZATION NOTE (toggleProfilerButtonMenu.label): This is the label for the
-# application menu item that toggles the profiler button to record performance profiles.
-toggleProfilerButtonMenu.label = Enable Profiler Toolbar Icon
-toggleProfilerButtonMenu.accesskey = P
-
 devToolboxMenuItem.label = Toggle Tools
 devToolboxMenuItem.accesskey = T
 

devtools/client/menus.js

@@ -56,11 +56,6 @@ loader.lazyImporter(
   "BrowserToolboxLauncher",
   "resource://devtools/client/framework/browser-toolbox/Launcher.jsm"
 );
-loader.lazyImporter(
-  this,
-  "ProfilerMenuButton",
-  "resource://devtools/client/performance-new/popup/menu-button.jsm.js"
-);
 loader.lazyRequireGetter(
   this,
   "ResponsiveUIManager",
@@ -123,14 +118,6 @@ exports.menuitems = [
     },
     keyId: "browserConsole",
   },
-  {
-    id: "menu_toggleProfilerButtonMenu",
-    l10nKey: "toggleProfilerButtonMenu",
-    checkbox: true,
-    oncommand(event) {
-      ProfilerMenuButton.toggle(event.target.ownerDocument);
-    },
-  },
   {
     id: "menu_responsiveUI",
     l10nKey: "responsiveDesignMode",

devtools/client/performance-new/@types/perf.d.ts

@@ -376,13 +376,6 @@ export interface PerformancePref {
    * test the profile injection mechanism.
    */
   UIBaseUrlPathPref: "devtools.performance.recording.ui-base-url-path";
-  /**
-   * The profiler's menu button and its popup can be enabled/disabled by the user.
-   * This pref controls whether the user has turned it on or not. Note that this
-   * preference is also used outside of the type-checked files, so make sure
-   * and update it elsewhere.
-   */
-  PopupEnabled: "devtools.performance.popup.enabled";
   /**
    * The profiler popup has some introductory text explaining what it is the first
    * time that you open it. After that, it is not displayed by default.
@@ -390,8 +383,8 @@ export interface PerformancePref {
   PopupIntroDisplayed: "devtools.performance.popup.intro-displayed";
   /**
    * This preference is used outside of the performance-new type system
-   * (in DevToolsStartup). It toggles the availability of the menu item
-   * "Tools -> Web Developer -> Enable Profiler Toolbar Icon".
+   * (in DevToolsStartup). It toggles the availability of the profiler menu
+   * button in the customization palette.
    */
   PopupFeatureFlag: "devtools.performance.popup.feature-flag";
 }

devtools/client/performance-new/README.md

@@ -32,9 +32,9 @@ This is the remote view of the about:profiling page. It is embedded in the about
 
 ### Profiler Popup
 
-The popup can be enabled by going to `Tools -> Web Developer -> Enable Profiler Toolbar Icon", or by visiting [profiler.firefox.com] and clicking `Enable Profiler Menu Button`. This UI is not a React Redux app, but has a vanilla browser chrome implementation. This was done to make the popup as fast as possible, with a trade-off of some complexity with dealing with the non-standard (i.e. not a normal webpage) browser chrome environment. The popup is designed to be as low overhead as possible in order to get the cleanest performance profiles. Special care must be taken to not impact browser startup times when working with this implementation, as it also turns on the global profiler shortcuts.
+The popup is enabled by default on Nightly and Dev Edition, but it's not added to the navbar. Once the profiler menu button is added to the navbar, or other places in the UI, the shortcuts for the profiler will work. In any release channel the popup can be enabled by visiting [profiler.firefox.com] and clicking `Enable Profiler Menu Button`. This flips the pref `"devtools.performance.popup.feature-flag"` and the profiler button will always be available in the list of buttons for the Firefox UI.
 
-At the time of this writing, the popup feature is not enabled by default. This pref is `"devtools.performance.popup.feature-flag"`.
+The popup UI is not a React Redux app, but has a vanilla browser chrome implementation. This was done to make the popup as fast as possible, with a trade-off of some complexity with dealing with the non-standard (i.e. not a normal webpage) browser chrome environment. The popup is designed to be as low overhead as possible in order to get the cleanest performance profiles. Special care must be taken to not impact browser startup times when working with this implementation, as it also turns on the global profiler shortcuts.
 
 ## Injecting profiles into [profiler.firefox.com]
 

devtools/client/performance-new/popup/background.jsm.js

@@ -127,6 +127,11 @@ const lazyProfilerMenuButton = requireLazy(() =>
   ))
 );
 
+const lazyCustomizableUI = requireLazy(() =>
+  /** @type {import("resource:///modules/CustomizableUI.jsm")} */
+  (ChromeUtils.import("resource:///modules/CustomizableUI.jsm"))
+);
+
 /** @type {Presets} */
 const presets = {
   "web-developer": {
@@ -513,7 +518,7 @@ function handleWebChannelMessage(channel, id, message, target) {
       channel.send(
         {
           type: "STATUS_RESPONSE",
-          menuButtonIsEnabled: ProfilerMenuButton.isEnabled(),
+          menuButtonIsEnabled: ProfilerMenuButton.isInNavbar(),
           requestId,
         },
         target
@@ -521,22 +526,27 @@ function handleWebChannelMessage(channel, id, message, target) {
       break;
     }
     case "ENABLE_MENU_BUTTON": {
+      const { ownerDocument } = target.browser;
+      if (!ownerDocument) {
+        throw new Error(
+          "Could not find the owner document for the current browser while enabling " +
+            "the profiler menu button"
+        );
+      }
+      // Ensure the widget is enabled.
+      Services.prefs.setBoolPref(POPUP_FEATURE_FLAG_PREF, true);
+
       // Enable the profiler menu button.
       const { ProfilerMenuButton } = lazyProfilerMenuButton();
-      if (!ProfilerMenuButton.isEnabled()) {
-        const { ownerDocument } = target.browser;
-        if (!ownerDocument) {
-          throw new Error(
-            "Could not find the owner document for the current browser while enabling " +
-              "the profiler menu button"
-          );
-        }
-        // The menu button toggle is only enabled on Nightly by default. Once the profiler
-        // is turned on once, make sure that the menu button is also available.
-        Services.prefs.setBoolPref(POPUP_FEATURE_FLAG_PREF, true);
+      ProfilerMenuButton.addToNavbar(ownerDocument);
 
-        ProfilerMenuButton.toggle(ownerDocument);
-      }
+      // Dispatch the change event manually, so that the shortcuts will also be
+      // added.
+      const { CustomizableUI } = lazyCustomizableUI();
+      CustomizableUI.dispatchToolboxEvent("customizationchange");
+
+      // Open the popup with a message.
+      ProfilerMenuButton.openPopup(ownerDocument);
 
       // Respond back that we've done it.
       channel.send(

devtools/client/performance-new/popup/menu-button.jsm.js

@@ -55,56 +55,54 @@ const lazyPopupPanel = requireLazy(() =>
   ))
 );
 
-/** @type {PerformancePref["PopupEnabled"]} */
-const BUTTON_ENABLED_PREF = "devtools.performance.popup.enabled";
 const WIDGET_ID = "profiler-button";
 
 /**
+ * Add the profiler button to the navbar.
+ *
+ * @param {ChromeDocument} document  The browser's document.
+ * @return {void}
+ */
+function addToNavbar(document) {
+  const { CustomizableUI } = lazyCustomizableUI();
+
+  CustomizableUI.addWidgetToArea(WIDGET_ID, CustomizableUI.AREA_NAVBAR);
+}
+
+/**
+ * Remove the widget and place it in the customization palette. This will also
+ * disable the shortcuts.
+ *
+ * @return {void}
+ */
+function remove() {
+  const { CustomizableUI } = lazyCustomizableUI();
+  CustomizableUI.removeWidgetFromArea(WIDGET_ID);
+}
+
+/**
+ * See if the profiler menu button is in the navbar, or other active areas. The
+ * placement is null when it's inactive in the customization palette.
+ *
  * @return {boolean}
  */
-function isEnabled() {
-  const { Services } = lazyServices();
-  return Services.prefs.getBoolPref(BUTTON_ENABLED_PREF, false);
-}
-
-/**
- * @param {HTMLDocument} document
- * @param {boolean} isChecked
- * @return {void}
- */
-function setMenuItemChecked(document, isChecked) {
-  const menuItem = document.querySelector("#menu_toggleProfilerButtonMenu");
-  if (!menuItem) {
-    return;
-  }
-  menuItem.setAttribute("checked", isChecked.toString());
-}
-
-/**
- * Toggle the menu button, and initialize the widget if needed.
- *
- * @param {ChromeDocument} document - The browser's document.
- * @return {void}
- */
-function toggle(document) {
+function isInNavbar() {
   const { CustomizableUI } = lazyCustomizableUI();
-  const { Services } = lazyServices();
+  return Boolean(CustomizableUI.getPlacementOfWidget("profiler-button"));
+}
 
-  const toggledValue = !isEnabled();
-  Services.prefs.setBoolPref(BUTTON_ENABLED_PREF, toggledValue);
-
-  if (toggledValue) {
-    initialize();
-    CustomizableUI.addWidgetToArea(WIDGET_ID, CustomizableUI.AREA_NAVBAR);
-  } else {
-    setMenuItemChecked(document, false);
-    CustomizableUI.destroyWidget(WIDGET_ID);
-
-    // The widgets are not being properly destroyed. This is a workaround
-    // until Bug 1552565 lands.
-    const element = document.getElementById("PanelUI-profiler");
-    delete (/** @type {any} */ (element._addedEventListeners));
+/**
+ * Opens the popup for the profiler.
+ * @param {Document} document
+ */
+function openPopup(document) {
+  // First find the button.
+  /** @type {HTMLButtonElement | null} */
+  const button = document.querySelector("#profiler-button");
+  if (!button) {
+    throw new Error("Could not find the profiler button.");
   }
+  button.click();
 }
 
 /**
@@ -127,9 +125,10 @@ function updateButtonColorForElement(buttonElement) {
 /**
  * This function creates the widget definition for the CustomizableUI. It should
  * only be run if the profiler button is enabled.
+ * @param {(isEnabled: boolean) => void} toggleProfilerKeyShortcuts
  * @return {void}
  */
-function initialize() {
+function initialize(toggleProfilerKeyShortcuts) {
   const { CustomizableUI } = lazyCustomizableUI();
   const { CustomizableWidgets } = lazyCustomizableWidgets();
   const { Services } = lazyServices();
@@ -154,6 +153,30 @@ function initialize() {
     isInfoCollapsed: true,
   };
 
+  /**
+   * Handle when the customization changes for the button. This event is not
+   * very specific, and fires for any CustomizableUI widget. This event is
+   * pretty rare to fire, and only affects users of the profiler button,
+   * so it shouldn't have much overhead even if it runs a lot.
+   */
+  function handleCustomizationChange() {
+    const isEnabled = isInNavbar();
+    toggleProfilerKeyShortcuts(isEnabled);
+
+    if (!isEnabled) {
+      // The profiler menu button is no longer in the navbar, make sure that the
+      // "intro-displayed" preference is reset.
+      /** @type {PerformancePref["PopupIntroDisplayed"]} */
+      const popupIntroDisplayedPref =
+        "devtools.performance.popup.intro-displayed";
+      Services.prefs.setBoolPref(popupIntroDisplayedPref, false);
+
+      if (Services.profiler.IsActive()) {
+        Services.profiler.StopProfiler();
+      }
+    }
+  }
+
   const item = {
     id: WIDGET_ID,
     type: "view",
@@ -219,7 +242,15 @@ function initialize() {
         Services.prefs.setBoolPref(popupIntroDisplayedPref, true);
       }
 
-      setMenuItemChecked(document, true);
+      // Handle customization event changes. If the profiler is no longer in the
+      // navbar, then reset the popup intro preference.
+      const window = document.defaultView;
+      if (window) {
+        /** @type {any} */ (window).gNavToolbox.addEventListener(
+          "customizationchange",
+          handleCustomizationChange
+        );
+      }
     },
 
     /** @type {(document: HTMLElement) => void} */
@@ -231,14 +262,24 @@ function initialize() {
       // Also run the observer right away to update the color if the profiler is
       // already running at startup.
       observer();
+
+      toggleProfilerKeyShortcuts(isInNavbar());
     },
 
-    onDestroyed: () => {
+    /** @type {(document: HTMLDocument) => void} */
+    onDestroyed: document => {
       if (observer) {
         Services.obs.removeObserver(observer, "profiler-started");
         Services.obs.removeObserver(observer, "profiler-stopped");
         observer = null;
       }
+      const window = document.defaultView;
+      if (window) {
+        /** @type {any} */ (window).gNavToolbox.removeEventListener(
+          "customizationchange",
+          handleCustomizationChange
+        );
+      }
     },
   };
 
@@ -246,7 +287,13 @@ function initialize() {
   CustomizableWidgets.push(item);
 }
 
-const ProfilerMenuButton = { toggle, initialize, isEnabled };
+const ProfilerMenuButton = {
+  initialize,
+  addToNavbar,
+  isInNavbar,
+  openPopup,
+  remove,
+};
 
 exports.ProfilerMenuButton = ProfilerMenuButton;
 

devtools/client/performance-new/test/browser/head.js

@@ -163,9 +163,9 @@ async function makeSureProfilerPopupIsEnabled() {
     "resource://devtools/client/performance-new/popup/menu-button.jsm.js"
   );
 
-  if (!ProfilerMenuButton.isEnabled()) {
-    info("> The menu button is not enabled, turn it on.");
-    ProfilerMenuButton.toggle(document);
+  if (!ProfilerMenuButton.isInNavbar()) {
+    info("> The menu button is not in the nav bar, add it.");
+    ProfilerMenuButton.addToNavbar(document);
 
     await waitUntil(
       () => gBrowser.ownerDocument.getElementById("profiler-button"),
@@ -178,12 +178,12 @@ async function makeSureProfilerPopupIsEnabled() {
       info(
         "Clean up after the test by disabling the profiler popup menu button."
       );
-      if (!ProfilerMenuButton.isEnabled()) {
+      if (!ProfilerMenuButton.isInNavbar()) {
         throw new Error(
-          "Expected the profiler popup to still be enabled during the test cleanup."
+          "Expected the profiler popup to still be in the navbar during the test cleanup."
         );
       }
-      ProfilerMenuButton.toggle(document);
+      ProfilerMenuButton.remove();
     });
   } else {
     info("> The menu button was already enabled.");
@@ -545,19 +545,19 @@ async function makeSureProfilerPopupIsDisabled() {
     "resource://devtools/client/performance-new/popup/menu-button.jsm.js"
   );
 
-  const originallyIsEnabled = ProfilerMenuButton.isEnabled();
+  const isOriginallyInNavBar = ProfilerMenuButton.isInNavbar();
 
-  if (originallyIsEnabled) {
-    info("> The menu button is enabled, turn it off for this test.");
-    ProfilerMenuButton.toggle(document);
+  if (isOriginallyInNavBar) {
+    info("> The menu button is in the navbar, remove it for this test.");
+    ProfilerMenuButton.remove();
   } else {
-    info("> The menu button was already disabled.");
+    info("> The menu button was not in the navbar yet.");
   }
 
   registerCleanupFunction(() => {
-    info("Revert the profiler menu button to its original enabled state.");
-    if (originallyIsEnabled !== ProfilerMenuButton.isEnabled()) {
-      ProfilerMenuButton.toggle(document);
+    info("Revert the profiler menu button to be back in its original place");
+    if (isOriginallyInNavBar !== ProfilerMenuButton.isInNavbar()) {
+      ProfilerMenuButton.remove();
     }
   });
 }

devtools/startup/DevToolsStartup.jsm

@@ -30,7 +30,6 @@ const kDebuggerPrefs = [
 const DEVTOOLS_ENABLED_PREF = "devtools.enabled";
 
 const DEVTOOLS_POLICY_DISABLED_PREF = "devtools.policy.disabled";
-const PROFILER_POPUP_ENABLED_PREF = "devtools.performance.popup.enabled";
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
@@ -234,7 +233,7 @@ XPCOMUtils.defineLazyGetter(this, "KeyShortcuts", function() {
     });
   }
 
-  if (isProfilerButtonEnabled()) {
+  if (ProfilerMenuButton.isInNavbar()) {
     shortcuts.push(...getProfilerKeyShortcuts());
   }
 
@@ -258,14 +257,6 @@ function getProfilerKeyShortcuts() {
   ];
 }
 
-/**
- * Instead of loading the ProfilerMenuButton.jsm file, provide an independent check
- * to see if it is turned on.
- */
-function isProfilerButtonEnabled() {
-  return Services.prefs.getBoolPref(PROFILER_POPUP_ENABLED_PREF, false);
-}
-
 /**
  * Validate the URL that will be used for the WebChannel for the profiler.
  *
@@ -390,13 +381,6 @@ DevToolsStartup.prototype = {
         DEVTOOLS_ENABLED_PREF,
         this.onEnabledPrefChanged
       );
-
-      // Watch for the profiler to enable or disable the profiler popup, then toggle
-      // the keyboard shortcuts on and off.
-      Services.prefs.addObserver(
-        PROFILER_POPUP_ENABLED_PREF,
-        this.toggleProfilerKeyShortcuts
-      );
       /* eslint-enable mozilla/balanced-observers */
 
       if (!this.isDisabledByPolicy()) {
@@ -624,38 +608,36 @@ DevToolsStartup.prototype = {
   },
 
   /**
-   * Dynamically register a profiler recording button in the
-   * customization menu. You can use this button by right clicking
-   * on Firefox toolbar and dragging it from the customization panel
-   * to the toolbar. (i.e. this isn't displayed by default to users.)
+   * Register the profiler recording button. This button will be available
+   * in the customization palette for the Firefox toolbar. In addition, it can be
+   * enabled from profiler.firefox.com.
    */
   hookProfilerRecordingButton() {
     if (this.profilerRecordingButtonCreated) {
       return;
     }
-    this.profilerRecordingButtonCreated = true;
-
+    const featureFlagPref = "devtools.performance.popup.feature-flag";
     const isPopupFeatureFlagEnabled = Services.prefs.getBoolPref(
-      "devtools.performance.popup.feature-flag",
-      AppConstants.NIGHTLY_BUILD
+      featureFlagPref
     );
+    this.profilerRecordingButtonCreated = true;
 
     // Listen for messages from the front-end. This needs to happen even if the
     // button isn't enabled yet. This will allow the front-end to turn on the
     // popup for our users, regardless of if the feature is enabled by default.
     this.initializeProfilerWebChannel();
 
-    if (!isPopupFeatureFlagEnabled) {
-      // The profiler's popup is experimental. The plan is to eventually turn it on
-      // everywhere, but while it's under active development we don't want everyone
-      // having it enabled. For now the default pref is to turn it on with Nightly,
-      // with the option to flip the pref in other releases. This feature flag will
-      // go away once it is fully shipped.
-      return;
-    }
-
-    if (isProfilerButtonEnabled()) {
-      ProfilerMenuButton.initialize();
+    if (isPopupFeatureFlagEnabled) {
+      // Initialize the CustomizableUI widget.
+      ProfilerMenuButton.initialize(this.toggleProfilerKeyShortcuts);
+    } else {
+      // The feature flag is not enabled, but watch for it to be enabled. If it is,
+      // initialize everything.
+      const enable = () => {
+        ProfilerMenuButton.initialize(this.toggleProfilerKeyShortcuts);
+        Services.prefs.removeObserver(featureFlagPref, enable);
+      };
+      Services.prefs.addObserver(featureFlagPref, enable);
     }
   },
 
@@ -832,9 +814,9 @@ DevToolsStartup.prototype = {
   /**
    * We only want to have the keyboard shortcuts active when the menu button is on.
    * This function either adds or removes the elements.
+   * @param {boolean} isEnabled
    */
-  toggleProfilerKeyShortcuts() {
-    const isEnabled = isProfilerButtonEnabled();
+  toggleProfilerKeyShortcuts(isEnabled) {
     const profilerKeyShortcuts = getProfilerKeyShortcuts();
     for (const { document } of Services.wm.getEnumerator(null)) {
       const devtoolsKeyset = document.getElementById("devtoolsKeyset");
@@ -845,6 +827,13 @@ DevToolsStartup.prototype = {
         continue;
       }
 
+      const areProfilerKeysPresent = !!document.getElementById(
+        "key_profilerStartStop"
+      );
+      if (isEnabled === areProfilerKeysPresent) {
+        // Don't double add or double remove the shortcuts.
+        continue;
+      }
       if (isEnabled) {
         this.attachKeys(document, profilerKeyShortcuts);
       } else {
@@ -855,11 +844,6 @@ DevToolsStartup.prototype = {
       // account (see bug 832984).
       mainKeyset.parentNode.insertBefore(devtoolsKeyset, mainKeyset);
     }
-
-    if (!isEnabled) {
-      // Ensure the profiler isn't left profiling in the background.
-      ProfilerPopupBackground.stopProfiler();
-    }
   },
 
   async onKey(window, key) {

modules/libpref/init/all.js

@@ -852,7 +852,15 @@ pref("toolkit.dump.emit", false);
 // use. This is useful so that a developer can change it while working on
 // profiler.firefox.com, or in tests. This isn't exposed directly to the user.
 pref("devtools.performance.recording.ui-base-url", "https://profiler.firefox.com");
-
+// The popup is only enabled by default on Nightly, Dev Edition, and debug buildsd since
+// it's a developer focused item. It can still be enabled by going to profiler.firefox.com,
+// but by default it is off on Release and Beta. Note that this only adds it to the
+// the customization palette, not to the navbar.
+#if defined(NIGHTLY_BUILD) || defined(MOZ_DEV_EDITION) || defined(DEBUG)
+  pref("devtools.performance.popup.feature-flag", true);
+#else
+  pref("devtools.performance.popup.feature-flag", false);
+#endif
 // The preset to use for the recording settings. If set to "custom" then the pref
 // values below will be used.
 #if defined(NIGHTLY_BUILD) || !defined(MOZILLA_OFFICIAL)