From c1fe9f539b9adf7f573892ad3e885d7a26b99ba7 Mon Sep 17 00:00:00 2001 From: Mike de Boer Date: Mon, 27 May 2013 17:21:47 +0200 Subject: [PATCH] Bug 860560: make sure that defaultEngine and currentEngine stay in sync, r=gavin --HG-- extra : rebase_source : 2a3169621b8cedf1382d738bd67072dae77cb4ce --- browser/components/nsBrowserGlue.js | 19 +++ toolkit/components/search/nsSearchService.js | 69 +++++++++- .../tests/xpcshell/test_engineselect.js | 17 +-- .../search/tests/xpcshell/test_prefSync.js | 129 ++++++++++++++++++ .../search/tests/xpcshell/xpcshell.ini | 1 + 5 files changed, 220 insertions(+), 15 deletions(-) create mode 100644 toolkit/components/search/tests/xpcshell/test_prefSync.js diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js index 01175557001c..a195e252076d 100644 --- a/browser/components/nsBrowserGlue.js +++ b/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() { diff --git a/toolkit/components/search/nsSearchService.js b/toolkit/components/search/nsSearchService.js index c6053a991dfb..aa5013c5dae9 100644 --- a/toolkit/components/search/nsSearchService.js +++ b/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) { diff --git a/toolkit/components/search/tests/xpcshell/test_engineselect.js b/toolkit/components/search/tests/xpcshell/test_engineselect.js index 6514d509f6af..86796ba393d9 100644 --- a/toolkit/components/search/tests/xpcshell/test_engineselect.js +++ b/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(); } diff --git a/toolkit/components/search/tests/xpcshell/test_prefSync.js b/toolkit/components/search/tests/xpcshell/test_prefSync.js new file mode 100644 index 000000000000..29b386ca6af7 --- /dev/null +++ b/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); +} diff --git a/toolkit/components/search/tests/xpcshell/xpcshell.ini b/toolkit/components/search/tests/xpcshell/xpcshell.ini index dc5d8b91dcf7..f27980ed8976 100644 --- a/toolkit/components/search/tests/xpcshell/xpcshell.ini +++ b/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]