Bug 1588785 - Send a notification when separate private default is toggled. r=mak

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

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

browser/base/content/browser.js

@@ -4403,7 +4403,7 @@ const BrowserSearch = {
     // Asynchronously initialize the search service if necessary, to get the
     // current engine for working out the placeholder.
     this._updateURLBarPlaceholderFromDefaultEngine(
-      this._usePrivateSettings,
+      PrivateBrowsingUtils.isWindowPrivate(window),
       // Delay the update for this until so that we don't change it while
       // the user is looking at it / isn't expecting it.
       true
@@ -4439,31 +4439,24 @@ const BrowserSearch = {
         this._removeMaybeOfferedEngine(engineName);
         break;
       case "engine-default":
-        if (this._searchInitComplete && !this._usePrivateSettings) {
+        if (
+          this._searchInitComplete &&
+          !PrivateBrowsingUtils.isWindowPrivate(window)
+        ) {
           this._updateURLBarPlaceholder(engineName, false);
         }
         break;
       case "engine-default-private":
-        if (this._searchInitComplete && this._usePrivateSettings) {
+        if (
+          this._searchInitComplete &&
+          PrivateBrowsingUtils.isWindowPrivate(window)
+        ) {
           this._updateURLBarPlaceholder(engineName, true);
         }
         break;
     }
   },
 
-  /**
-   * @returns True if we are using a separate default private engine, and
-   *          we are in a private window.
-   */
-  get _usePrivateSettings() {
-    return (
-      Services.prefs.getBoolPref(
-        "browser.search.separatePrivateDefault",
-        true
-      ) && PrivateBrowsingUtils.isWindowPrivate(window)
-    );
-  },
-
   _addMaybeOfferedEngine(engineName) {
     let selectedBrowserOffersEngine = false;
     for (let browser of gBrowser.browsers) {
@@ -4521,7 +4514,7 @@ const BrowserSearch = {
   initPlaceHolder() {
     const prefName =
       "browser.urlbar.placeholderName" +
-      (this._usePrivateSettings ? ".private" : "");
+      (PrivateBrowsingUtils.isWindowPrivate(window) ? ".private" : "");
     let engineName = Services.prefs.getStringPref(prefName, "");
     if (engineName) {
       // We can do this directly, since we know we're at DOMContentLoaded.

browser/components/preferences/in-content/tests/browser_searchDefaultEngine.js

@@ -228,9 +228,10 @@ async function setDefaultEngine(
     testPrivate ? "engine-default-private" : "engine-default",
     "browser-search-engine-modified"
   );
-  const popupHidden = BrowserTestUtils.waitForEvent(popup, "popuphidden");
+  // Waiting for popupHiding here seemed to cause a race condition, however
+  // as we're really just interested in the notification, we'll just use
+  // that here.
   EventUtils.synthesizeMouseAtCenter(engine2Item, {}, engine2Item.ownerGlobal);
-  await popupHidden;
   await defaultChanged;
 
   const newDefault = testPrivate

toolkit/components/search/SearchService.jsm

@@ -32,13 +32,6 @@ XPCOMUtils.defineLazyServiceGetters(this, {
   gEnvironment: ["@mozilla.org/process/environment;1", "nsIEnvironment"],
 });
 
-XPCOMUtils.defineLazyPreferenceGetter(
-  this,
-  "gSeparatePrivateDefault",
-  SearchUtils.BROWSER_SEARCH_PREF + "separatePrivateDefault",
-  false
-);
-
 XPCOMUtils.defineLazyPreferenceGetter(
   this,
   "gGeoSpecificDefaultsEnabled",
@@ -723,6 +716,14 @@ SearchService.prototype = {
   async _init(skipRegionCheck) {
     SearchUtils.log("_init start");
 
+    XPCOMUtils.defineLazyPreferenceGetter(
+      this,
+      "_separatePrivateDefault",
+      SearchUtils.BROWSER_SEARCH_PREF + "separatePrivateDefault",
+      false,
+      this._onSeparateDefaultPrefChanged.bind(this)
+    );
+
     try {
       // See if we have a cache file so we don't have to parse a bunch of XML.
       let cache = await this._readCacheFile();
@@ -969,7 +970,7 @@ SearchService.prototype = {
    *   browsing engine will be returned.
    */
   get originalPrivateDefaultEngine() {
-    return this._originalDefaultEngine(gSeparatePrivateDefault);
+    return this._originalDefaultEngine(this._separatePrivateDefault);
   },
 
   resetToOriginalDefaultEngine() {
@@ -1283,9 +1284,17 @@ SearchService.prototype = {
         this._currentEngine,
         SearchUtils.MODIFIED_TYPE.DEFAULT
       );
+      // If we've not got a separate private active, notify update of the
+      // private so that the UI updates correctly.
+      if (!this._separatePrivateDefault) {
+        SearchUtils.notifyAction(
+          this._currentEngine,
+          SearchUtils.MODIFIED_TYPE.DEFAULT_PRIVATE
+        );
+      }
     }
     if (
-      gSeparatePrivateDefault &&
+      this._separatePrivateDefault &&
       prevPrivateEngine &&
       this.defaultPrivateEngine !== prevPrivateEngine
     ) {
@@ -2622,7 +2631,7 @@ SearchService.prototype = {
     // tests. Really, removeEngine should always commit to updating any
     // changed defaults.
     if (
-      gSeparatePrivateDefault &&
+      this._separatePrivateDefault &&
       engineToRemove == this.defaultPrivateEngine
     ) {
       this._currentPrivateEngine = null;
@@ -2894,6 +2903,14 @@ SearchService.prototype = {
       this[currentEngine],
       SearchUtils.MODIFIED_TYPE[privateMode ? "DEFAULT_PRIVATE" : "DEFAULT"]
     );
+    // If we've not got a separate private active, notify update of the
+    // private so that the UI updates correctly.
+    if (!privateMode && !this._separatePrivateDefault) {
+      SearchUtils.notifyAction(
+        this[currentEngine],
+        SearchUtils.MODIFIED_TYPE.DEFAULT_PRIVATE
+      );
+    }
   },
 
   get defaultEngine() {
@@ -2905,11 +2922,11 @@ SearchService.prototype = {
   },
 
   get defaultPrivateEngine() {
-    return this._getEngineDefault(gSeparatePrivateDefault);
+    return this._getEngineDefault(this._separatePrivateDefault);
   },
 
   set defaultPrivateEngine(newEngine) {
-    this._setEngineDefault(gSeparatePrivateDefault, newEngine);
+    this._setEngineDefault(this._separatePrivateDefault, newEngine);
   },
 
   async getDefault() {
@@ -2932,6 +2949,19 @@ SearchService.prototype = {
     return (this.defaultPrivateEngine = engine);
   },
 
+  _onSeparateDefaultPrefChanged() {
+    // We should notify if the normal default, and the currently saved private
+    // default are different. Otherwise, save the energy.
+    if (this.defaultEngine != this._getEngineDefault(true)) {
+      SearchUtils.notifyAction(
+        // Always notify with the new private engine, the function checks
+        // the preference value for us.
+        this.defaultPrivateEngine,
+        SearchUtils.MODIFIED_TYPE.DEFAULT_PRIVATE
+      );
+    }
+  },
+
   async _getEngineInfo(engine) {
     if (!engine) {
       // The defaultEngine getter will throw if there's no engine at all,
@@ -3059,7 +3089,7 @@ SearchService.prototype = {
       defaultSearchEngineData,
     };
 
-    if (gSeparatePrivateDefault) {
+    if (this._separatePrivateDefault) {
       let [
         privateShortName,
         defaultPrivateSearchEngineData,

toolkit/components/search/tests/xpcshell/head_search.js

@@ -136,6 +136,15 @@ async function forceExpiration() {
   await promiseSaveGlobalMetadata(metadata);
 }
 
+function promiseDefaultNotification(type = "normal") {
+  return SearchTestUtils.promiseSearchNotification(
+    SearchUtils.MODIFIED_TYPE[
+      type == "private" ? "DEFAULT_PRIVATE" : "DEFAULT"
+    ],
+    SearchUtils.TOPIC_ENGINE_MODIFIED
+  );
+}
+
 /**
  * Clean the profile of any cache file left from a previous run.
  *

toolkit/components/search/tests/xpcshell/test_defaultEngine.js

@@ -13,6 +13,13 @@ add_task(async function setup() {
   await AddonTestUtils.promiseStartupManager();
 });
 
+function promiseDefaultNotification() {
+  return SearchTestUtils.promiseSearchNotification(
+    SearchUtils.MODIFIED_TYPE.DEFAULT,
+    SearchUtils.TOPIC_ENGINE_MODIFIED
+  );
+}
+
 add_task(async function test_defaultEngine() {
   let search = Services.search;
   await search.init();
@@ -24,11 +31,19 @@ add_task(async function test_defaultEngine() {
     { name: "A second test engine", xmlFileName: "engine2.xml" },
   ]);
 
+  let promise = promiseDefaultNotification();
   search.defaultEngine = engine1;
+  Assert.equal(await promise, engine1);
   Assert.equal(search.defaultEngine, engine1);
+
+  promise = promiseDefaultNotification();
   search.defaultEngine = engine2;
+  Assert.equal(await promise, engine2);
   Assert.equal(search.defaultEngine, engine2);
+
+  promise = promiseDefaultNotification();
   search.defaultEngine = engine1;
+  Assert.equal(await promise, engine1);
   Assert.equal(search.defaultEngine, engine1);
 
   // Test that hiding the currently-default engine affects the defaultEngine getter
@@ -44,6 +59,8 @@ add_task(async function test_defaultEngine() {
 
   // Test that setting defaultEngine to an already-hidden engine works, but
   // doesn't change the return value of the getter
+  promise = promiseDefaultNotification();
   search.defaultEngine = engine1;
+  Assert.equal(await promise, engine1);
   Assert.equal(search.defaultEngine, engine2);
 });

toolkit/components/search/tests/xpcshell/test_defaultPrivateEngine.js

@@ -45,7 +45,13 @@ add_task(async function test_defaultPrivateEngine() {
     "Should have the original default as the default engine"
   );
 
+  let promise = promiseDefaultNotification("private");
   Services.search.defaultPrivateEngine = engine1;
+  Assert.equal(
+    await promise,
+    engine1,
+    "Should have notified setting the private engine to the new one"
+  );
   Assert.equal(
     Services.search.defaultPrivateEngine,
     engine1,
@@ -56,7 +62,13 @@ add_task(async function test_defaultPrivateEngine() {
     originalDefault,
     "Should not have changed the original default engine"
   );
+  promise = promiseDefaultNotification("private");
   await Services.search.setDefaultPrivate(engine2);
+  Assert.equal(
+    await promise,
+    engine2,
+    "Should have notified setting the private engine to the new one using async api"
+  );
   Assert.equal(
     Services.search.defaultPrivateEngine,
     engine2,
@@ -76,7 +88,13 @@ add_task(async function test_defaultPrivateEngine() {
     "Should not have changed the original default engine"
   );
 
+  promise = promiseDefaultNotification("private");
   await Services.search.setDefaultPrivate(engine1);
+  Assert.equal(
+    await promise,
+    engine1,
+    "Should have notified reverting the private engine to the selected one using async api"
+  );
   Assert.equal(
     Services.search.defaultPrivateEngine,
     engine1,
@@ -107,12 +125,33 @@ add_task(async function test_defaultPrivateEngine() {
 });
 
 add_task(async function test_defaultPrivateEngine_turned_off() {
+  Services.search.defaultEngine = originalDefault;
+  Services.search.defaultPrivateEngine = engine1;
+
+  let promise = promiseDefaultNotification("private");
   Services.prefs.setBoolPref(
     SearchUtils.BROWSER_SEARCH_PREF + "separatePrivateDefault",
     false
   );
+  Assert.equal(
+    await promise,
+    originalDefault,
+    "Should have notified setting the first engine correctly."
+  );
 
+  promise = promiseDefaultNotification("normal");
+  let privatePromise = promiseDefaultNotification("private");
   Services.search.defaultPrivateEngine = engine1;
+  Assert.equal(
+    await promise,
+    engine1,
+    "Should have notified setting the first engine correctly."
+  );
+  Assert.equal(
+    await privatePromise,
+    engine1,
+    "Should have notified setting of the private engine as well."
+  );
   Assert.equal(
     Services.search.defaultPrivateEngine,
     engine1,
@@ -123,7 +162,13 @@ add_task(async function test_defaultPrivateEngine_turned_off() {
     engine1,
     "Should keep the default engine in sync with the pref off"
   );
+  promise = promiseDefaultNotification("normal");
   Services.search.defaultPrivateEngine = engine2;
+  Assert.equal(
+    await promise,
+    engine2,
+    "Should have notified setting the second engine correctly."
+  );
   Assert.equal(
     Services.search.defaultPrivateEngine,
     engine2,
@@ -134,7 +179,13 @@ add_task(async function test_defaultPrivateEngine_turned_off() {
     engine2,
     "Should keep the default engine in sync with the pref off"
   );
+  promise = promiseDefaultNotification("normal");
   Services.search.defaultPrivateEngine = engine1;
+  Assert.equal(
+    await promise,
+    engine1,
+    "Should have notified resetting to the first engine again"
+  );
   Assert.equal(
     Services.search.defaultPrivateEngine,
     engine1,