Bug 1599745 - Register all performance panel recording preferences and move their default values there. r=julienw

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

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

browser/app/profile/firefox.js

@@ -2077,12 +2077,32 @@ pref("devtools.performance.ui.enable-memory-flame", false);
   pref("devtools.performance.ui.experimental", false);
 #endif
 
-// Preferences for the new performance panel. This pref configures the base URL
-// for the profiler.firefox.com instance to 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.
+// Preferences for the new performance panel.
+// This pref configures the base URL for the profiler.firefox.com instance to
+// 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");
 
+// Profiler buffer size. It is the maximum number of 8-bytes entries in the
+// profiler's buffer. 10000000 is ~80mb.
+pref("devtools.performance.recording.entries", 10000000);
+// Profiler interval in microseconds. 1000µs is 1ms
+pref("devtools.performance.recording.interval", 1000);
+// Profiler duration of entries in the profiler's buffer in seconds.
+// `0` means no time limit for the markers, they roll off naturally from the
+// circular buffer.
+pref("devtools.performance.recording.duration", 0);
+// Profiler feature set. See tools/profiler/core/platform.cpp for features and
+// explanations.
+#if defined(__ANDROID__)
+  // If it's android, add "java" feature as well. Other features must be same
+  // with else branch. Please update both of them.
+  pref("devtools.performance.recording.features", "[\"js\",\"leaf\",\"responsiveness\",\"stackwalk\",\"java\"]");
+#else
+  // Please update the if branch as well if you upadate this.
+  pref("devtools.performance.recording.features", "[\"js\",\"leaf\",\"responsiveness\",\"stackwalk\"]");
+#endif
+pref("devtools.performance.recording.threads", "[\"GeckoMain\",\"Compositor\"]");
 // A JSON array of strings, where each string is a file path to an objdir on
 // the host machine. This is used in order to look up symbol information from
 // build artifacts of local builds.

devtools/client/performance-new/browser.js

@@ -159,25 +159,10 @@ function receiveProfile(profile, getSymbolTableCallback) {
  * function always returns a valid array of strings.
  * @param {PreferenceFront} preferenceFront
  * @param {string} prefName
- * @param {string[]} defaultValue
  */
-async function _getArrayOfStringsPref(preferenceFront, prefName, defaultValue) {
-  let array;
-  try {
-    const text = await preferenceFront.getCharPref(prefName);
-    array = JSON.parse(text);
-  } catch (error) {
-    return defaultValue;
-  }
-
-  if (
-    Array.isArray(array) &&
-    array.every(feature => typeof feature === "string")
-  ) {
-    return array;
-  }
-
-  return defaultValue;
+async function _getArrayOfStringsPref(preferenceFront, prefName) {
+  const text = await preferenceFront.getCharPref(prefName);
+  return JSON.parse(text);
 }
 
 /**
@@ -187,29 +172,12 @@ async function _getArrayOfStringsPref(preferenceFront, prefName, defaultValue) {
  * even exists. Gracefully handle malformed data or missing data. Ensure that this
  * function always returns a valid array of strings.
  * @param {string} prefName
- * @param {string[]} defaultValue
  */
-async function _getArrayOfStringsHostPref(prefName, defaultValue) {
+async function _getArrayOfStringsHostPref(prefName) {
   const { Services } = lazyServices();
-  let array;
-  try {
-    const text = Services.prefs.getStringPref(
-      prefName,
-      JSON.stringify(defaultValue)
-    );
-    array = JSON.parse(text);
-  } catch (error) {
-    return defaultValue;
-  }
+  const text = Services.prefs.getStringPref(prefName);
 
-  if (
-    Array.isArray(array) &&
-    array.every(feature => typeof feature === "string")
-  ) {
-    return array;
-  }
-
-  return defaultValue;
+  return JSON.parse(text);
 }
 
 /**
@@ -217,14 +185,9 @@ async function _getArrayOfStringsHostPref(prefName, defaultValue) {
  *
  * @param {PreferenceFront} preferenceFront
  * @param {string} prefName
- * @param {number} defaultValue
  */
-async function _getIntPref(preferenceFront, prefName, defaultValue) {
-  try {
-    return await preferenceFront.getIntPref(prefName);
-  } catch (error) {
-    return defaultValue;
-  }
+async function _getIntPref(preferenceFront, prefName) {
+  return preferenceFront.getIntPref(prefName);
 }
 
 /**
@@ -234,34 +197,20 @@ async function _getIntPref(preferenceFront, prefName, defaultValue) {
  * different features or configurations.
  *
  * @param {PreferenceFront} preferenceFront
- * @param {RecordingStateFromPreferences} defaultPrefs
  */
-async function getRecordingPreferencesFromDebuggee(
-  preferenceFront,
-  defaultPrefs
-) {
+async function getRecordingPreferencesFromDebuggee(preferenceFront) {
   const [entries, interval, features, threads, objdirs] = await Promise.all([
-    _getIntPref(
+    _getIntPref(preferenceFront, `devtools.performance.recording.entries`),
+    _getIntPref(preferenceFront, `devtools.performance.recording.interval`),
+    _getArrayOfStringsPref(
       preferenceFront,
-      `devtools.performance.recording.entries`,
-      defaultPrefs.entries
-    ),
-    _getIntPref(
-      preferenceFront,
-      `devtools.performance.recording.interval`,
-      defaultPrefs.interval
+      `devtools.performance.recording.features`
     ),
     _getArrayOfStringsPref(
       preferenceFront,
-      `devtools.performance.recording.features`,
-      defaultPrefs.features
+      `devtools.performance.recording.threads`
     ),
-    _getArrayOfStringsPref(
-      preferenceFront,
-      `devtools.performance.recording.threads`,
-      defaultPrefs.threads
-    ),
-    _getArrayOfStringsHostPref(OBJDIRS_PREF, defaultPrefs.objdirs),
+    _getArrayOfStringsHostPref(OBJDIRS_PREF),
   ]);
 
   return { entries, interval, features, threads, objdirs };

devtools/client/performance-new/initializer.js

@@ -48,10 +48,6 @@ const {
   createMultiModalGetSymbolTableFn,
 } = require("devtools/client/performance-new/browser");
 
-const { getDefaultRecordingPreferences } = ChromeUtils.import(
-  "resource://devtools/client/performance-new/popup/background.jsm.js"
-);
-
 /**
  * This file initializes the DevTools Panel UI. It is in charge of initializing
  * the DevTools specific environment, and then passing those requirements into
@@ -73,10 +69,7 @@ async function gInit(perfFront, preferenceFront) {
     // according to what's in the target's preferences. This way the preferences are
     // stored on the target. This could be useful for something like Android where you
     // might want to tweak the settings.
-    getRecordingPreferencesFromDebuggee(
-      preferenceFront,
-      getDefaultRecordingPreferences()
-    ),
+    getRecordingPreferencesFromDebuggee(preferenceFront),
     // Get the supported features from the debuggee. If the debuggee is before
     // Firefox 72, then return null, as the actor does not support that feature.
     // We can't use `target.actorHasMethod`, because the target is not provided

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

@@ -231,108 +231,34 @@ function restartProfiler() {
 
 /**
  * @param {string} prefName
- * @param {string[]} defaultValue
  * @return {string[]}
  */
-function _getArrayOfStringsPref(prefName, defaultValue) {
-  let array;
-  try {
-    const text = Services.prefs.getCharPref(prefName);
-    array = JSON.parse(text);
-  } catch (error) {
-    return defaultValue;
-  }
-
-  if (
-    Array.isArray(array) &&
-    array.every(feature => typeof feature === "string")
-  ) {
-    return array;
-  }
-
-  return defaultValue;
+function _getArrayOfStringsPref(prefName) {
+  const text = Services.prefs.getCharPref(prefName);
+  return JSON.parse(text);
 }
 
 /**
  * @param {string} prefName
- * @param {string[]} defaultValue
  * @return {string[]}
  */
-function _getArrayOfStringsHostPref(prefName, defaultValue) {
-  let array;
-  try {
-    const text = Services.prefs.getStringPref(
-      prefName,
-      JSON.stringify(defaultValue)
-    );
-    array = JSON.parse(text);
-  } catch (error) {
-    return defaultValue;
-  }
-
-  if (
-    Array.isArray(array) &&
-    array.every(feature => typeof feature === "string")
-  ) {
-    return array;
-  }
-
-  return defaultValue;
-}
-
-/**
- * A simple cache for the default recording preferences.
- * @type {RecordingStateFromPreferences}
- */
-let _defaultPrefs;
-
-/**
- * This function contains the canonical defaults for the data store in the
- * preferences in the user profile. They represent the default values for both
- * the popup and panel's recording settings.
- */
-function getDefaultRecordingPreferences() {
-  if (!_defaultPrefs) {
-    _defaultPrefs = {
-      entries: 10000000, // ~80mb,
-      // Do not expire markers, let them roll off naturally from the circular buffer.
-      duration: 0,
-      interval: 1000, // 1000µs = 1ms
-      features: ["js", "leaf", "responsiveness", "stackwalk"],
-      threads: ["GeckoMain", "Compositor"],
-      objdirs: [],
-    };
-
-    if (AppConstants.platform === "android") {
-      // Java profiling is only meaningful on android.
-      _defaultPrefs.features.push("java");
-    }
-  }
-
-  return _defaultPrefs;
+function _getArrayOfStringsHostPref(prefName) {
+  const text = Services.prefs.getStringPref(prefName);
+  return JSON.parse(text);
 }
 
 /**
  * @returns {RecordingStateFromPreferences}
  */
 function getRecordingPreferencesFromBrowser() {
-  const defaultPrefs = getDefaultRecordingPreferences();
-
-  const entries = Services.prefs.getIntPref(ENTRIES_PREF, defaultPrefs.entries);
-  const interval = Services.prefs.getIntPref(
-    INTERVAL_PREF,
-    defaultPrefs.interval
-  );
-  const features = _getArrayOfStringsPref(FEATURES_PREF, defaultPrefs.features);
-  const threads = _getArrayOfStringsPref(THREADS_PREF, defaultPrefs.threads);
-  const objdirs = _getArrayOfStringsHostPref(
-    OBJDIRS_PREF,
-    defaultPrefs.objdirs
-  );
-  const duration = Services.prefs.getIntPref(
-    DURATION_PREF,
-    defaultPrefs.duration
-  );
+  // If you add a new preference here, please do not forget to update
+  // `revertRecordingPreferences` as well.
+  const entries = Services.prefs.getIntPref(ENTRIES_PREF);
+  const interval = Services.prefs.getIntPref(INTERVAL_PREF);
+  const features = _getArrayOfStringsPref(FEATURES_PREF);
+  const threads = _getArrayOfStringsPref(THREADS_PREF);
+  const objdirs = _getArrayOfStringsHostPref(OBJDIRS_PREF);
+  const duration = Services.prefs.getIntPref(DURATION_PREF);
 
   const supportedFeatures = new Set(Services.profiler.GetFeatures());
 
@@ -365,7 +291,12 @@ const platform = AppConstants.platform;
  * @type {() => void}
  */
 function revertRecordingPreferences() {
-  setRecordingPreferencesOnBrowser(getDefaultRecordingPreferences());
+  Services.prefs.clearUserPref(ENTRIES_PREF);
+  Services.prefs.clearUserPref(INTERVAL_PREF);
+  Services.prefs.clearUserPref(FEATURES_PREF);
+  Services.prefs.clearUserPref(THREADS_PREF);
+  Services.prefs.clearUserPref(OBJDIRS_PREF);
+  Services.prefs.clearUserPref(DURATION_PREF);
 }
 
 var EXPORTED_SYMBOLS = [
@@ -376,7 +307,6 @@ var EXPORTED_SYMBOLS = [
   "toggleProfiler",
   "platform",
   "getSymbolsFromThisBrowser",
-  "getDefaultRecordingPreferences",
   "getRecordingPreferencesFromBrowser",
   "setRecordingPreferencesOnBrowser",
   "revertRecordingPreferences",

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

@@ -191,7 +191,7 @@ function createPerfComponent() {
   const reducers = require("devtools/client/performance-new/store/reducers");
   const actions = require("devtools/client/performance-new/store/actions");
   const selectors = require("devtools/client/performance-new/store/selectors");
-  const { getDefaultRecordingPreferences } = ChromeUtils.import(
+  const { getRecordingPreferencesFromBrowser } = ChromeUtils.import(
     "resource://devtools/client/performance-new/popup/background.jsm.js"
   );
 
@@ -216,7 +216,7 @@ function createPerfComponent() {
       actions.initializeStore({
         perfFront: perfFrontMock,
         receiveProfile: receiveProfileMock,
-        recordingPreferences: getDefaultRecordingPreferences(),
+        recordingPreferences: getRecordingPreferencesFromBrowser(),
         setRecordingPreferences: recordingPreferencesMock,
         getSymbolTableGetter: () => noop,
         isPopup: false,

devtools/client/performance-new/test/xpcshell/test_popup_initial_state.js

@@ -14,29 +14,49 @@ function setupBackgroundJsm() {
 }
 
 add_task(function test() {
-  info("Test that we get the default values from state.");
-  const {
-    getRecordingPreferencesFromBrowser,
-    revertRecordingPreferences,
-    getDefaultRecordingPreferences,
-  } = setupBackgroundJsm();
+  info("Test that we get the default preference values from the browser.");
+  const { getRecordingPreferencesFromBrowser } = setupBackgroundJsm();
 
-  Assert.equal(
+  Assert.notEqual(
     getRecordingPreferencesFromBrowser().entries,
-    getDefaultRecordingPreferences().entries,
+    undefined,
     "The initial state has the default entries."
   );
-  Assert.equal(
+  Assert.notEqual(
     getRecordingPreferencesFromBrowser().interval,
-    getDefaultRecordingPreferences().interval,
+    undefined,
     "The initial state has the default interval."
   );
+  Assert.notEqual(
+    getRecordingPreferencesFromBrowser().features,
+    undefined,
+    "The initial state has the default features."
+  );
   Assert.equal(
     getRecordingPreferencesFromBrowser().features.includes("js"),
-    getDefaultRecordingPreferences().features.includes("js"),
+    true,
     "The js feature is initialized to the default."
   );
-  revertRecordingPreferences();
+  Assert.notEqual(
+    getRecordingPreferencesFromBrowser().threads,
+    undefined,
+    "The initial state has the default threads."
+  );
+  Assert.equal(
+    getRecordingPreferencesFromBrowser().threads.includes("GeckoMain"),
+    true,
+    "The GeckoMain thread is initialized to the default."
+  );
+  Assert.notEqual(
+    getRecordingPreferencesFromBrowser().objdirs,
+    undefined,
+    "The initial state has the default objdirs."
+  );
+  Assert.notEqual(
+    getRecordingPreferencesFromBrowser().duration,
+    undefined,
+    "The duration is initialized to the duration."
+  );
 });
 
 add_task(function test() {