Bug 1227045 - Save the new current engine when it has been set as a fallback after the previous current engine was removed, r=mak.
authorFlorian Quèze <florian@queze.net>
Wed, 02 Dec 2015 16:51:15 +0100
changeset 309342 3294bc6d07c44017e6b7a307f6f97052dda42d9c
parent 309341 87204339949047f3c630cb57f37fb6e818d41f5f
child 309343 2d33fa3346b2e4bfd7720a9f1f19e4a59b84e143
push id5513
push userraliiev@mozilla.com
push dateMon, 25 Jan 2016 13:55:34 +0000
treeherdermozilla-beta@5ee97dd05b5c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1227045
milestone45.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1227045 - Save the new current engine when it has been set as a fallback after the previous current engine was removed, r=mak.
browser/components/search/test/browser_addEngine.js
toolkit/components/search/nsSearchService.js
toolkit/components/search/tests/xpcshell/test_selectedEngine.js
--- a/browser/components/search/test/browser_addEngine.js
+++ b/browser/components/search/test/browser_addEngine.js
@@ -42,16 +42,18 @@ var gTests = [
     name: "opensearch install",
     engine: {
       name: "Foo",
       alias: null,
       description: "Foo Search",
       searchForm: "http://mochi.test:8888/browser/browser/components/search/test/"
     },
     run: function () {
+      Services.obs.addObserver(observer, "browser-search-engine-modified", false);
+
       gSS.addEngine("http://mochi.test:8888/browser/browser/components/search/test/testEngine.xml",
                     null, "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAIAAACQkWg2AAABGklEQVQoz2NgGB6AnZ1dUlJSXl4eSDIyMhLW4Ovr%2B%2Fr168uXL69Zs4YoG%2BLi4i5dusTExMTGxsbNzd3f37937976%2BnpmZmagbHR09J49e5YvX66kpATVEBYW9ubNm2nTphkbG7e2tp44cQLIuHfvXm5urpaWFlDKysqqu7v73LlzECMYIiIiHj58mJCQoKKicvXq1bS0NKBgW1vbjh074uPjgeqAXE1NzSdPnvDz84M0AEUvXLgAsW379u1z5swBen3jxo2zZ892cHB4%2BvQp0KlAfwI1cHJyghQFBwfv2rULokFXV%2FfixYu7d%2B8GGqGgoMDKyrpu3br9%2B%2FcDuXl5eVA%2FAEWBfoWHAdAYoNuAYQ0XAeoUERFhGDYAAPoUaT2dfWJuAAAAAElFTkSuQmCC",
                     false);
     },
     added: function (engine) {
       ok(engine, "engine was added.");
 
       checkEngine(this.engine, engine);
@@ -67,16 +69,22 @@ var gTests = [
     current: function (engine) {
       let currentEngine = gSS.currentEngine;
       is(engine, currentEngine, "engine is current");
       is(engine.name, this.engine.name, "current engine was changed successfully");
 
       gSS.removeEngine(engine);
     },
     removed: function (engine) {
+      // Remove the observer before calling the currentEngine getter,
+      // as that getter will set the currentEngine to the original default
+      // which will trigger a notification causing the test to loop over all
+      // engines.
+      Services.obs.removeObserver(observer, "browser-search-engine-modified");
+
       let currentEngine = gSS.currentEngine;
       ok(currentEngine, "An engine is present.");
       isnot(currentEngine.name, this.engine.name, "Current engine reset after removal");
 
       nextTest();
     }
   }
 ];
@@ -88,17 +96,10 @@ function nextTest() {
     info("Running " + gCurrentTest.name);
     gCurrentTest.run();
   } else
     executeSoon(finish);
 }
 
 function test() {
   waitForExplicitFinish();
-  Services.obs.addObserver(observer, "browser-search-engine-modified", false);
-  registerCleanupFunction(cleanup);
-
   nextTest();
 }
-
-function cleanup() {
-  Services.obs.removeObserver(observer, "browser-search-engine-modified");
-}
--- a/toolkit/components/search/nsSearchService.js
+++ b/toolkit/components/search/nsSearchService.js
@@ -4097,41 +4097,51 @@ SearchService.prototype = {
       setLocalizedPref(defPref, this._defaultEngine.name);
     }
 
     notifyAction(this._defaultEngine, SEARCH_ENGINE_DEFAULT);
   },
 
   get currentEngine() {
     this._ensureInitialized();
-    if (!this._currentEngine) {
+    let currentEngine = this._currentEngine;
+    if (!currentEngine) {
       let name = this.getGlobalAttr("current");
       let engine = this.getEngineByName(name);
       if (engine && (this.getGlobalAttr("hash") == getVerificationHash(name) ||
                      engine._isDefault)) {
         // If the current engine is a default one, we can relax the
         // verification hash check to reduce the annoyance for users who
         // backup/sync their profile in custom ways.
-        this._currentEngine = engine;
+        currentEngine = engine;
       }
     }
 
-    if (!this._currentEngine || this._currentEngine.hidden)
-      this._currentEngine = this._originalDefaultEngine;
-    if (!this._currentEngine || this._currentEngine.hidden)
-      this._currentEngine = this._getSortedEngines(false)[0];
-
-    if (!this._currentEngine) {
+    if (!currentEngine || currentEngine.hidden)
+      currentEngine = this._originalDefaultEngine;
+    if (!currentEngine || currentEngine.hidden)
+      currentEngine = this._getSortedEngines(false)[0];
+
+    if (!currentEngine) {
       // Last resort fallback: unhide the original default engine.
-      this._currentEngine = this._originalDefaultEngine;
-      if (this._currentEngine)
-        this._currentEngine.hidden = false;
+      currentEngine = this._originalDefaultEngine;
+      if (currentEngine)
+        currentEngine.hidden = false;
     }
 
-    return this._currentEngine;
+    if (currentEngine) {
+      // If the current engine wasn't set or was hidden, we used a fallback
+      // to pick a new current engine. As soon as we return it, this new
+      // current engine will become user-visible, so we should persist it.
+      // Calling the setter achieves this, and is a no-op when we haven't
+      // actually changed the current engine.
+      this.currentEngine = currentEngine;
+    }
+
+    return currentEngine;
   },
 
   set currentEngine(val) {
     this._ensureInitialized();
     // Sometimes we get wrapped nsISearchEngine objects (external XPCOM callers),
     // and sometimes we get raw Engine JS objects (callers in this file), so
     // handle both.
     if (!(val instanceof Ci.nsISearchEngine) && !(val instanceof Engine))
--- a/toolkit/components/search/tests/xpcshell/test_selectedEngine.js
+++ b/toolkit/components/search/tests/xpcshell/test_selectedEngine.js
@@ -88,16 +88,67 @@ add_task(function* test_settingToDefault
     Services.search.getEngineByName(getDefaultEngineName());
   yield promiseAfterCache();
 
   // Check that the current engine is no longer saved in the JSON file.
   metadata = yield promiseGlobalMetadata();
   do_check_eq(metadata.current, "");
 });
 
+add_task(function* test_resetToOriginalDefaultEngine() {
+  let defaultName = getDefaultEngineName();
+  do_check_eq(Services.search.currentEngine.name, defaultName);
+
+  Services.search.currentEngine =
+    Services.search.getEngineByName(kTestEngineName);
+  do_check_eq(Services.search.currentEngine.name, kTestEngineName);
+  yield promiseAfterCache();
+
+  Services.search.resetToOriginalDefaultEngine();
+  do_check_eq(Services.search.currentEngine.name, defaultName);
+  yield promiseAfterCache();
+});
+
+add_task(function* test_fallback_kept_after_restart() {
+  // Set current engine to a default engine that isn't the original default.
+  let builtInEngines = Services.search.getDefaultEngines();
+  let defaultName = getDefaultEngineName();
+  let nonDefaultBuiltInEngine;
+  for (let engine of builtInEngines) {
+    if (engine.name != defaultName) {
+      nonDefaultBuiltInEngine = engine;
+      break;
+    }
+  }
+  Services.search.currentEngine = nonDefaultBuiltInEngine;
+  do_check_eq(Services.search.currentEngine.name, nonDefaultBuiltInEngine.name);
+  yield promiseAfterCache();
+
+  // Remove that engine...
+  Services.search.removeEngine(nonDefaultBuiltInEngine);
+  // The engine being a default (built-in) one, it should be hidden
+  // rather than actually removed.
+  do_check_true(nonDefaultBuiltInEngine.hidden);
+
+  // Using the currentEngine getter should force a fallback to the
+  // original default engine.
+  do_check_eq(Services.search.currentEngine.name, defaultName);
+
+  // Restoring the default engines should unhide our built-in test
+  // engine, but not change the value of currentEngine.
+  Services.search.restoreDefaultEngines();
+  do_check_false(nonDefaultBuiltInEngine.hidden);
+  do_check_eq(Services.search.currentEngine.name, defaultName);
+  yield promiseAfterCache();
+
+  // After a restart, the currentEngine value should still be unchanged.
+  yield asyncReInit();
+  do_check_eq(Services.search.currentEngine.name, defaultName);
+});
+
 
 function run_test() {
   removeMetadata();
   removeCacheFile();
 
   do_check_false(Services.search.isInitialized);
 
   let engineDummyFile = gProfD.clone();