Bug 860560: make sure that defaultEngine and currentEngine stay in sync, r=gavin

--HG--
extra : rebase_source : 2a3169621b8cedf1382d738bd67072dae77cb4ce

browser/components/nsBrowserGlue.js

@@ -317,6 +317,23 @@ BrowserGlue.prototype = {
         });
         break;
 #endif
+      case "browser-search-engine-modified":
+        if (data != "engine-default" && data != "engine-current") {
+          break;
+        }
+        // Enforce that the search service's defaultEngine is always equal to
+        // its currentEngine. The search service will notify us any time either
+        // of them are changed (either by directly setting the relevant prefs,
+        // i.e. if add-ons try to change this directly, or if the
+        // nsIBrowserSearchService setters are called).
+        let ss = Services.search;
+        if (ss.currentEngine.name == ss.defaultEngine.name)
+          return;
+        if (data == "engine-current")
+          ss.defaultEngine = ss.currentEngine;
+        else
+          ss.currentEngine = ss.defaultEngine;
+        break;
     }
   },
 
@@ -351,6 +368,7 @@ BrowserGlue.prototype = {
 #ifdef MOZ_SERVICES_HEALTHREPORT
     os.addObserver(this, "keyword-search", false);
 #endif
+    os.addObserver(this, "browser-search-engine-modified", false);
   },
 
   // cleanup (called on application shutdown)
@@ -384,6 +402,7 @@ BrowserGlue.prototype = {
 #ifdef MOZ_SERVICES_HEALTHREPORT
     os.removeObserver(this, "keyword-search");
 #endif
+    os.removeObserver(this, "browser-search-engine-modified");
   },
 
   _onAppDefaults: function BG__onAppDefaults() {

toolkit/components/search/nsSearchService.js

@@ -2670,6 +2670,15 @@ SearchService.prototype = {
     return this.__sortedEngines;
   },
 
+  // Get the original Engine object that belongs to the defaultenginename pref
+  // of the default branch.
+  get _originalDefaultEngine() {
+    let defaultPrefB = Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF);
+    let nsIPLS = Ci.nsIPrefLocalizedString;
+    let defaultEngine = defaultPrefB.getComplexValue("defaultenginename", nsIPLS).data;
+    return this.getEngineByName(defaultEngine);
+  },
+
   _buildCache: function SRCH_SVC__buildCache() {
     if (!getBoolPref(BROWSER_SEARCH_PREF + "cache.enabled", true))
       return;
@@ -3323,6 +3332,15 @@ SearchService.prototype = {
                                       });
   },
 
+  _setEngineByPref: function SRCH_SVC_setEngineByPref(aEngineType, aPref) {
+    this._ensureInitialized();
+    let newEngine = this.getEngineByName(getLocalizedPref(aPref, ""));
+    if (!newEngine)
+      FAIL("Can't find engine in store!", Cr.NS_ERROR_UNEXPECTED);
+
+    this[aEngineType] = newEngine;
+  },
+
   // nsIBrowserSearchService
   init: function SRCH_SVC_init(observer) {
     LOG("SearchService.init");
@@ -3620,7 +3638,10 @@ SearchService.prototype = {
 
   set defaultEngine(val) {
     this._ensureInitialized();
-    if (!(val instanceof Ci.nsISearchEngine))
+    // 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))
       FAIL("Invalid argument passed to defaultEngine setter");
 
     let newDefaultEngine = this.getEngineByName(val.name);
@@ -3632,8 +3653,22 @@ SearchService.prototype = {
 
     this._defaultEngine = newDefaultEngine;
 
+    // Set a flag to keep track that this setter was called properly, not by
+    // setting the pref alone.
+    this._changingDefaultEngine = true;
     let defPref = BROWSER_SEARCH_PREF + "defaultenginename";
-    setLocalizedPref(defPref, this._defaultEngine.name);
+    // If we change the default engine in the future, that change should impact
+    // users who have switched away from and then back to the build's "default"
+    // engine. So clear the user pref when the defaultEngine is set to the
+    // build's default engine, so that the defaultEngine getter falls back to
+    // whatever the default is.
+    if (this._defaultEngine == this._originalDefaultEngine) {
+      Services.prefs.clearUserPref(defPref);
+    }
+    else {
+      setLocalizedPref(defPref, this._defaultEngine.name);
+    }
+    this._changingDefaultEngine = false;
 
     notifyAction(this._defaultEngine, SEARCH_ENGINE_DEFAULT);
   },
@@ -3653,7 +3688,10 @@ SearchService.prototype = {
 
   set currentEngine(val) {
     this._ensureInitialized();
-    if (!(val instanceof Ci.nsISearchEngine))
+    // 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))
       FAIL("Invalid argument passed to currentEngine setter");
 
     var newCurrentEngine = this.getEngineByName(val.name);
@@ -3667,12 +3705,21 @@ SearchService.prototype = {
 
     var currentEnginePref = BROWSER_SEARCH_PREF + "selectedEngine";
 
-    if (this._currentEngine == this.defaultEngine) {
+    // Set a flag to keep track that this setter was called properly, not by
+    // setting the pref alone.
+    this._changingCurrentEngine = true;
+    // If we change the default engine in the future, that change should impact
+    // users who have switched away from and then back to the build's "default"
+    // engine. So clear the user pref when the currentEngine is set to the
+    // build's default engine, so that the currentEngine getter falls back to
+    // whatever the default is.
+    if (this._currentEngine == this._originalDefaultEngine) {
       Services.prefs.clearUserPref(currentEnginePref);
     }
     else {
       setLocalizedPref(currentEnginePref, this._currentEngine.name);
     }
+    this._changingCurrentEngine = false;
 
     notifyAction(this._currentEngine, SEARCH_ENGINE_CURRENT);
   },
@@ -3709,6 +3756,16 @@ SearchService.prototype = {
         }
         engineMetadataService.flush();
         break;
+
+      case "nsPref:changed":
+        let currPref = BROWSER_SEARCH_PREF + "selectedEngine";
+        let defPref = BROWSER_SEARCH_PREF + "defaultenginename";
+        if (aVerb == currPref && !this._changingCurrentEngine) {
+          this._setEngineByPref("currentEngine", currPref);
+        } else if (aVerb == defPref && !this._changingDefaultEngine) {
+          this._setEngineByPref("defaultEngine", defPref);
+        }
+        break;
     }
   },
 
@@ -3754,11 +3811,15 @@ SearchService.prototype = {
   _addObservers: function SRCH_SVC_addObservers() {
     Services.obs.addObserver(this, SEARCH_ENGINE_TOPIC, false);
     Services.obs.addObserver(this, QUIT_APPLICATION_TOPIC, false);
+    Services.prefs.addObserver(BROWSER_SEARCH_PREF + "defaultenginename", this, false);
+    Services.prefs.addObserver(BROWSER_SEARCH_PREF + "selectedEngine", this, false);
   },
 
   _removeObservers: function SRCH_SVC_removeObservers() {
     Services.obs.removeObserver(this, SEARCH_ENGINE_TOPIC);
     Services.obs.removeObserver(this, QUIT_APPLICATION_TOPIC);
+    Services.prefs.removeObserver(BROWSER_SEARCH_PREF + "defaultenginename", this);
+    Services.prefs.removeObserver(BROWSER_SEARCH_PREF + "selectedEngine", this);
   },
 
   QueryInterface: function SRCH_SVC_QI(aIID) {

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

@@ -43,29 +43,24 @@ function search_observer(aSubject, aTopic, aData) {
   let engine2 = search.getEngineByName("A second test engine");
 
   search.defaultEngine = engine1;
-  do_check_eq(search.defaultEngine.name, "Test search engine");
-  do_check_eq(search.defaultEngine.searchForm, "http://www.google.com/");
+  do_check_eq(search.defaultEngine, engine1);
   
   // Tests search defaultEngine when it changes
   search.defaultEngine = engine2
-  do_check_eq(search.defaultEngine.name, "A second test engine");
-  do_check_eq(search.defaultEngine.searchForm, "https://duckduckgo.com");
+  do_check_eq(search.defaultEngine, engine2);
 
   // Test search defaultEngine again when we change back
   search.defaultEngine = engine1;
-  do_check_eq(search.defaultEngine.name, "Test search engine");
-  do_check_eq(search.defaultEngine.searchForm, "http://www.google.com/");
+  do_check_eq(search.defaultEngine, engine1);
 
   // Test search defaultEngine when the current default is hidden
-  search.moveEngine(engine2, 0)
+  search.moveEngine(engine2, 0);
   engine1.hidden = true;
-  do_check_eq(search.defaultEngine.name, "A second test engine");
-  do_check_eq(search.defaultEngine.searchForm, "https://duckduckgo.com");
+  do_check_eq(search.defaultEngine, engine2);
   
   // Test search defaultEngine when it is set to a hidden engine
   search.defaultEngine = engine1;
-  do_check_eq(search.defaultEngine.name, "A second test engine");
-  do_check_eq(search.defaultEngine.searchForm, "https://duckduckgo.com");
+  do_check_eq(search.defaultEngine, engine2);
 
   do_test_finished();
 }

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

@@ -0,0 +1,129 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+/*
+ * Test that currentEngine and defaultEngine properties are updated when the
+ * prefs are set independently.
+ */
+
+"use strict";
+
+const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
+
+Cu.import("resource://testing-common/httpd.js");
+
+let waitForEngines = {
+  "Test search engine": 1,
+  "A second test engine": 1
+};
+
+const PREF_BRANCH = "browser.search.";
+
+/**
+ * Wrapper for nsIPrefBranch::setComplexValue.
+ * @param aPrefName
+ *        The name of the pref to set.
+ */
+function setLocalizedPref(aPrefName, aValue) {
+  let nsIPLS = Ci.nsIPrefLocalizedString;
+  let branch = Services.prefs.getBranch(PREF_BRANCH);
+  try {
+    var pls = Cc["@mozilla.org/pref-localizedstring;1"].
+              createInstance(Ci.nsIPrefLocalizedString);
+    pls.data = aValue;
+    branch.setComplexValue(aPrefName, nsIPLS, pls);
+  } catch (ex) {}
+}
+
+function search_observer(aSubject, aTopic, aData) {
+  let engine = aSubject.QueryInterface(Ci.nsISearchEngine);
+  do_print("Observer: " + aData + " for " + engine.name);
+
+  if (aData != "engine-added") {
+    return;
+  }
+
+  // If the engine is defined in `waitForEngines`, remove it from the list
+  if (waitForEngines[engine.name]) {
+    delete waitForEngines[engine.name];
+  } else {
+    // This engine is not one we're waiting for, so bail out early.
+    return;
+  }
+
+  // Only continue when both engines have been loaded.
+  if (Object.keys(waitForEngines).length) {
+    return;
+  }
+
+  let search = Services.search;
+
+  let engine1Name = "Test search engine";
+  let engine2Name = "A second test engine";
+  let engine1 = search.getEngineByName(engine1Name);
+  let engine2 = search.getEngineByName(engine2Name);
+
+  // Initial sanity check:
+  search.defaultEngine = engine1;
+  do_check_eq(search.defaultEngine, engine1);
+  search.currentEngine = engine1;
+  do_check_eq(search.currentEngine, engine1);
+
+  setLocalizedPref("defaultenginename", engine2Name);
+  // Default engine should be synced with the pref
+  do_check_eq(search.defaultEngine, engine2);
+  // Current engine should've stayed the same
+  do_check_eq(search.currentEngine, engine1);
+  
+  setLocalizedPref("selectedEngine", engine2Name);
+  // Default engine should've stayed the same
+  do_check_eq(search.defaultEngine, engine2);
+  // Current engine should've been updated
+  do_check_eq(search.currentEngine, engine2);
+
+  // Test that setting the currentEngine to the original default engine clears
+  // the selectedEngine pref, rather than setting it. To do this we need to
+  // set the value of defaultenginename on the default branch.
+  let defaultBranch = Services.prefs.getDefaultBranch("");
+  let prefName = PREF_BRANCH + "defaultenginename";
+  let prefVal = "data:text/plain," + prefName + "=" + engine1Name;
+  defaultBranch.setCharPref(prefName, prefVal, true);
+  search.currentEngine = engine1;
+  // Current engine should've been updated
+  do_check_eq(search.currentEngine, engine1);
+  do_check_false(Services.prefs.prefHasUserValue("browser.search.selectedEngine"));
+
+  // Test that setting the defaultEngine to the original default engine clears
+  // the defaultenginename pref, rather than setting it.
+  do_check_true(Services.prefs.prefHasUserValue("browser.search.defaultenginename"));
+  search.defaultEngine = engine1;
+  do_check_eq(search.defaultEngine, engine1);
+  do_check_false(Services.prefs.prefHasUserValue("browser.search.defaultenginename"));
+
+  do_test_finished();
+}
+
+function run_test() {
+  removeMetadata();
+  updateAppInfo();
+
+  let httpServer = new HttpServer();
+  httpServer.start(4444);
+  httpServer.registerDirectory("/", do_get_cwd());
+
+  do_register_cleanup(function cleanup() {
+    httpServer.stop(function() {});
+    Services.obs.removeObserver(search_observer, "browser-search-engine-modified");
+  });
+
+  do_test_pending();
+
+  Services.obs.addObserver(search_observer, "browser-search-engine-modified", false);
+
+  Services.search.addEngine("http://localhost:4444/data/engine.xml",
+                   Ci.nsISearchEngine.DATA_XML,
+                   null, false);
+  Services.search.addEngine("http://localhost:4444/data/engine2.xml",
+                   Ci.nsISearchEngine.DATA_XML,
+                   null, false);
+}

toolkit/components/search/tests/xpcshell/xpcshell.ini

@@ -17,4 +17,5 @@ skip-if = debug && os == "linux"
 [test_save_sorted_engines.js]
 [test_purpose.js]
 [test_engineselect.js]
+[test_prefSync.js]