Bug 1518545 - Remove the superfluous 'engine-current' Search Service observer topic in favour of 'engine-default'. r=daleharvey
authorMike de Boer <mdeboer@mozilla.com>
Wed, 17 Apr 2019 09:45:24 +0000
changeset 469796 aa8babb8b1ee
parent 469795 ee8e96e5d015
child 469797 ac38a6790d00
push id35882
push usercbrindusan@mozilla.com
push dateWed, 17 Apr 2019 15:54:01 +0000
treeherdermozilla-central@37185c0ae520 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdaleharvey
bugs1518545
milestone68.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 1518545 - Remove the superfluous 'engine-current' Search Service observer topic in favour of 'engine-default'. r=daleharvey Differential Revision: https://phabricator.services.mozilla.com/D27857
browser/base/content/browser.js
browser/base/content/test/general/browser_contentSearchUI.js
browser/components/newtab/lib/TopSitesFeed.jsm
browser/components/newtab/test/unit/lib/TopSitesFeed.test.js
browser/components/preferences/in-content/search.js
browser/components/search/test/browser/browser_426329.js
browser/components/search/test/browser/browser_addEngine.js
browser/components/search/test/browser/browser_healthreport.js
browser/components/search/test/browser/browser_oneOffContextMenu_setDefault.js
browser/components/uitour/test/browser_UITour.js
browser/modules/ContentSearch.jsm
browser/modules/test/browser/browser_ContentSearch.js
toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
toolkit/components/places/tests/unifiedcomplete/test_search_engine_current.js
toolkit/components/places/tests/unifiedcomplete/test_search_engine_default.js
toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
toolkit/components/search/SearchService.jsm
toolkit/components/search/nsISearchService.idl
toolkit/components/search/tests/xpcshell/test_notifications.js
toolkit/components/telemetry/app/TelemetryEnvironment.jsm
toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -4010,17 +4010,17 @@ const BrowserSearch = {
       this._addMaybeOfferedEngine(engineName);
       break;
     case "engine-added":
       // An engine was added to the search service.  If a page is offering the
       // engine, then the engine needs to be removed from the corresponding
       // browser's offered engines.
       this._removeMaybeOfferedEngine(engineName);
       break;
-    case "engine-current":
+    case "engine-default":
       if (this._searchInitComplete) {
         this._updateURLBarPlaceholder(engineName);
       }
       break;
     }
   },
 
   _addMaybeOfferedEngine(engineName) {
--- a/browser/base/content/test/general/browser_contentSearchUI.js
+++ b/browser/base/content/test/general/browser_contentSearchUI.js
@@ -387,17 +387,17 @@ add_task(async function formHistory() {
 
 add_task(async function cycleEngines() {
   await setUp();
   await msg("key", { key: "VK_DOWN", waitForSuggestions: true });
 
   let promiseEngineChange = function(newEngineName) {
     return new Promise(resolve => {
       Services.obs.addObserver(function resolver(subj, topic, data) {
-        if (data != "engine-current") {
+        if (data != "engine-default") {
           return;
         }
         subj.QueryInterface(Ci.nsISearchEngine);
         SimpleTest.is(subj.name, newEngineName, "Engine cycled correctly");
         Services.obs.removeObserver(resolver, "browser-search-engine-modified");
         resolve();
       }, "browser-search-engine-modified");
     });
--- a/browser/components/newtab/lib/TopSitesFeed.jsm
+++ b/browser/components/newtab/lib/TopSitesFeed.jsm
@@ -82,17 +82,17 @@ this.TopSitesFeed = class TopSitesFeed {
   uninit() {
     PageThumbs.removeExpirationFilter(this);
     Services.obs.removeObserver(this, "browser-search-engine-modified");
   }
 
   observe(subj, topic, data) {
     // We should update the current top sites if the search engine has been changed since
     // the search engine that gets filtered out of top sites has changed.
-    if (topic === "browser-search-engine-modified" && data === "engine-current" && this.store.getState().Prefs.values[NO_DEFAULT_SEARCH_TILE_EXP_PREF]) {
+    if (topic === "browser-search-engine-modified" && data === "engine-default" && this.store.getState().Prefs.values[NO_DEFAULT_SEARCH_TILE_EXP_PREF]) {
       delete this._currentSearchHostname;
       this._currentSearchHostname = getShortURLForCurrentSearch();
       this.refresh({broadcast: true});
     }
   }
 
   _dedupeKey(site) {
     return site && site.hostname;
--- a/browser/components/newtab/test/unit/lib/TopSitesFeed.test.js
+++ b/browser/components/newtab/test/unit/lib/TopSitesFeed.test.js
@@ -1159,17 +1159,17 @@ describe("Top Sites Feed", () => {
       links = [{url: "foo.com"}];
       fakeNewTabUtils.pinnedLinks.links = [{url: "google.com"}];
       const urlsReturned = (await feed.getLinksWithDefaults()).map(link => link.url);
       assert.include(urlsReturned, "google.com");
     });
     it("should call refresh and set ._currentSearchHostname to the new engine hostname when the the default search engine has been set", () => {
       sinon.stub(feed, "refresh");
       sandbox.stub(global.Services.search, "defaultEngine").value({identifier: "ddg", searchForm: "duckduckgo.com"});
-      feed.observe(null, "browser-search-engine-modified", "engine-current");
+      feed.observe(null, "browser-search-engine-modified", "engine-default");
       assert.equal(feed._currentSearchHostname, "duckduckgo");
       assert.calledOnce(feed.refresh);
     });
     it("should call refresh when the experiment pref has changed", () => {
       sinon.stub(feed, "refresh");
 
       feed.onAction({type: at.PREF_CHANGED, data: {name: NO_DEFAULT_SEARCH_TILE_PREF, value: true}});
       assert.calledOnce(feed.refresh);
--- a/browser/components/preferences/in-content/search.js
+++ b/browser/components/preferences/in-content/search.js
@@ -255,28 +255,25 @@ var gSearchPane = {
         break;
       case "engine-changed":
         gEngineView._engineStore.reloadIcons();
         gEngineView.invalidate();
         break;
       case "engine-removed":
         gSearchPane.remove(aEngine);
         break;
-      case "engine-current":
+      case "engine-default":
         // If the user is going through the drop down using up/down keys, the
-        // dropdown may still be open (eg. on Windows) when engine-current is
+        // dropdown may still be open (eg. on Windows) when engine-default is
         // fired, so rebuilding the list unconditionally would get in the way.
         let selectedEngine =
           document.getElementById("defaultEngine").selectedItem.engine;
         if (selectedEngine.name != aEngine.name)
           gSearchPane.buildDefaultEngineDropDown();
         break;
-      case "engine-default":
-        // Not relevant
-        break;
       }
     }
   },
 
   onInputBlur(aEvent) {
     let tree = document.getElementById("engineList");
     if (!tree.hasAttribute("editing"))
       return;
--- a/browser/components/search/test/browser/browser_426329.js
+++ b/browser/components/search/test/browser/browser_426329.js
@@ -62,17 +62,17 @@ function promiseSetEngine() {
 
     function observer(aSub, aTopic, aData) {
       switch (aData) {
         case "engine-added":
           let engine = ss.getEngineByName("Bug 426329");
           ok(engine, "Engine was added.");
           ss.defaultEngine = engine;
           break;
-        case "engine-current":
+        case "engine-default":
           ok(ss.defaultEngine.name == "Bug 426329", "defaultEngine set");
           searchBar = BrowserSearch.searchBar;
           searchButton = searchBar.querySelector(".search-go-button");
           ok(searchButton, "got search-go-button");
           searchBar.value = "test";
 
           Services.obs.removeObserver(observer, "browser-search-engine-modified");
           resolve();
--- a/browser/components/search/test/browser/browser_addEngine.js
+++ b/browser/components/search/test/browser/browser_addEngine.js
@@ -12,17 +12,17 @@ function observer(aSubject, aTopic, aDat
   let engine = aSubject.QueryInterface(Ci.nsISearchEngine);
   info("Observer: " + aData + " for " + engine.name);
   let method;
   switch (aData) {
     case "engine-added":
       if (gCurrentTest.added)
         method = "added";
       break;
-    case "engine-current":
+    case "engine-default":
       if (gCurrentTest.current)
         method = "current";
       break;
     case "engine-removed":
       if (gCurrentTest.removed)
         method = "removed";
       break;
   }
--- a/browser/components/search/test/browser/browser_healthreport.js
+++ b/browser/components/search/test/browser/browser_healthreport.js
@@ -54,17 +54,17 @@ function test() {
   function observer(subject, topic, data) {
     switch (data) {
       case "engine-added":
         let engine = Services.search.getEngineByName("Foo");
         ok(engine, "Engine was added.");
         Services.search.defaultEngine = engine;
         break;
 
-      case "engine-current":
+      case "engine-default":
         // We may be called again when resetting the engine at the end.
         if (!calledTestTelemetry) {
           is(Services.search.defaultEngine.name, "Foo", "Current engine is Foo");
           testTelemetry();
         }
         break;
 
       case "engine-removed":
--- a/browser/components/search/test/browser/browser_oneOffContextMenu_setDefault.js
+++ b/browser/components/search/test/browser/browser_oneOffContextMenu_setDefault.js
@@ -107,17 +107,17 @@ add_task(async function test_urlBarChang
  * Promises that an engine change has happened for the current engine, which
  * has resulted in the test engine now being the current engine.
  *
  * @returns {Promise} Resolved once the test engine is set as the current engine.
  */
 function promisedefaultEngineChanged() {
   return new Promise(resolve => {
     function observer(aSub, aTopic, aData) {
-      if (aData == "engine-current") {
+      if (aData == "engine-default") {
         Assert.equal(Services.search.defaultEngine.name, TEST_ENGINE_NAME, "defaultEngine set");
         Services.obs.removeObserver(observer, "browser-search-engine-modified");
         resolve();
       }
     }
 
     Services.obs.addObserver(observer, "browser-search-engine-modified");
   });
--- a/browser/components/uitour/test/browser_UITour.js
+++ b/browser/components/uitour/test/browser_UITour.js
@@ -372,17 +372,17 @@ var tests = [
        "the searchEngineIdentifier property should contain the defaultEngine's identifier");
 
     let someOtherEngineID = data.engines.filter(t => t != "searchEngine-" + defaultEngine.identifier)[0];
     someOtherEngineID = someOtherEngineID.replace(/^searchEngine-/, "");
 
     await new Promise(resolve => {
       let observe = function(subject, topic, verb) {
         info("browser-search-engine-modified: " + verb);
-        if (verb == "engine-current") {
+        if (verb == "engine-default") {
           is(Services.search.defaultEngine.identifier, someOtherEngineID, "correct engine was switched to");
           done();
         }
       };
       Services.obs.addObserver(observe, "browser-search-engine-modified");
       registerCleanupFunction(async () => {
         // Clean up
         Services.obs.removeObserver(observe, "browser-search-engine-modified");
--- a/browser/modules/ContentSearch.jsm
+++ b/browser/modules/ContentSearch.jsm
@@ -456,22 +456,20 @@ var ContentSearch = {
       engine.speculativeConnect({
         window: msg.target.contentWindow,
         originAttributes: msg.target.contentPrincipal.originAttributes,
       });
     }
   },
 
   async _onObserve(data) {
-    if (data === "engine-current") {
+    if (data === "engine-default") {
       let engine = await this._currentEngineObj();
       this._broadcast("CurrentEngine", engine);
-    } else if (data !== "engine-default") {
-      // engine-default is always sent with engine-current and isn't otherwise
-      // relevant to content searches.
+    } else {
       let state = await this.currentStateObj();
       this._broadcast("CurrentState", state);
     }
   },
 
   _suggestionDataForBrowser(browser, create = false) {
     let data = this._suggestionMap.get(browser);
     if (!data && create) {
--- a/browser/modules/test/browser/browser_ContentSearch.js
+++ b/browser/modules/test/browser/browser_ContentSearch.js
@@ -72,24 +72,24 @@ add_task(async function SetDefaultEngine
   }
   mm.sendAsyncMessage(TEST_MSG, {
     type: "SetCurrentEngine",
     data: newDefaultEngine.name,
   });
   let deferred = PromiseUtils.defer();
   Services.obs.addObserver(function obs(subj, topic, data) {
     info("Test observed " + data);
-    if (data == "engine-current") {
-      ok(true, "Test observed engine-current");
+    if (data == "engine-default") {
+      ok(true, "Test observed engine-default");
       Services.obs.removeObserver(obs, "browser-search-engine-modified");
       deferred.resolve();
     }
   }, "browser-search-engine-modified");
   let searchPromise = waitForTestMsg(mm, "CurrentEngine");
-  info("Waiting for test to observe engine-current...");
+  info("Waiting for test to observe engine-default...");
   await deferred.promise;
   let msg = await searchPromise;
   checkMsg(msg, {
     type: "CurrentEngine",
     data: await defaultEngineObj(newDefaultEngine),
   });
 
   await Services.search.setDefault(oldDefaultEngine);
--- a/toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
+++ b/toolkit/components/places/PlacesSearchAutocompleteProvider.jsm
@@ -55,17 +55,17 @@ const SearchAutocompleteProviderInternal
 
   initialized: false,
 
   observe(subject, topic, data) {
     switch (data) {
       case "engine-added":
       case "engine-changed":
       case "engine-removed":
-      case "engine-current":
+      case "engine-default":
         this._refresh();
     }
   },
 
   async _refresh() {
     this.enginesByDomain.clear();
     this.enginesByAlias.clear();
     this.tokenAliasEngines = [];
rename from toolkit/components/places/tests/unifiedcomplete/test_search_engine_current.js
rename to toolkit/components/places/tests/unifiedcomplete/test_search_engine_default.js
--- a/toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
+++ b/toolkit/components/places/tests/unifiedcomplete/xpcshell.ini
@@ -42,17 +42,17 @@ support-files =
 [test_multi_word_search.js]
 [test_PlacesSearchAutocompleteProvider.js]
 skip-if = appname == "thunderbird"
 [test_preloaded_sites.js]
 [test_query_url.js]
 [test_remote_tab_matches.js]
 skip-if = !sync
 [test_search_engine_alias.js]
-[test_search_engine_current.js]
+[test_search_engine_default.js]
 [test_search_engine_host.js]
 [test_search_engine_restyle.js]
 [test_search_suggestions.js]
 [test_special_search.js]
 [test_swap_protocol.js]
 [test_tab_matches.js]
 [test_trimming.js]
 [test_visit_url.js]
--- a/toolkit/components/search/SearchService.jsm
+++ b/toolkit/components/search/SearchService.jsm
@@ -60,17 +60,16 @@ const EXT_SIGNING_ADDRESS = "search.mozi
 const SEARCH_ENGINE_TOPIC        = "browser-search-engine-modified";
 const TOPIC_LOCALES_CHANGE       = "intl:app-locales-changed";
 const QUIT_APPLICATION_TOPIC     = "quit-application";
 
 const SEARCH_ENGINE_REMOVED      = "engine-removed";
 const SEARCH_ENGINE_ADDED        = "engine-added";
 const SEARCH_ENGINE_CHANGED      = "engine-changed";
 const SEARCH_ENGINE_LOADED       = "engine-loaded";
-const SEARCH_ENGINE_CURRENT      = "engine-current";
 const SEARCH_ENGINE_DEFAULT      = "engine-default";
 
 // The following constants are left undocumented in nsISearchService.idl
 // For the moment, they are meant for testing/debugging purposes only.
 
 /**
  * Topic used for events involving the service itself.
  */
@@ -2958,17 +2957,16 @@ SearchService.prototype = {
     await this._loadEngines(await this._readCacheFile(), true);
     // Make sure the current list of engines is persisted.
     await this._buildCache();
 
     // If the defaultEngine has changed between the previous load and this one,
     // dispatch the appropriate notifications.
     if (prevCurrentEngine && this.defaultEngine !== prevCurrentEngine) {
       notifyAction(this._currentEngine, SEARCH_ENGINE_DEFAULT);
-      notifyAction(this._currentEngine, SEARCH_ENGINE_CURRENT);
     }
     Services.obs.notifyObservers(null, SEARCH_SERVICE_TOPIC, "engines-reloaded");
   },
 
   _reInit(origin, skipRegionCheck = true) {
     LOG("_reInit");
     // Re-entrance guard, because we're using an async lambda below.
     if (gReinitializing) {
@@ -4164,17 +4162,16 @@ SearchService.prototype = {
     if (this._currentEngine == this.originalDefaultEngine) {
       newName = "";
     }
 
     this.setGlobalAttr("current", newName);
     this.setGlobalAttr("hash", getVerificationHash(newName));
 
     notifyAction(this._currentEngine, SEARCH_ENGINE_DEFAULT);
-    notifyAction(this._currentEngine, SEARCH_ENGINE_CURRENT);
   },
 
   async getDefault() {
     await this.init(true);
     return this.defaultEngine;
   },
 
   async setDefault(engine) {
--- a/toolkit/components/search/nsISearchService.idl
+++ b/toolkit/components/search/nsISearchService.idl
@@ -529,18 +529,13 @@ interface nsISearchService : nsISupports
  * file is completely loaded. This is used internally by the search service as
  * an indication of when the engine can be added to the internal store, and
  * therefore should not be used to detect engine availability. It is always
  * followed by an "added" notification.
  */
 #define SEARCH_ENGINE_LOADED       "engine-loaded"
 
 /**
- * Sent when the "current" engine is changed.
- */
-#define SEARCH_ENGINE_CURRENT      "engine-current";
-
-/**
  * Sent when the "default" engine is changed.
  */
 #define SEARCH_ENGINE_DEFAULT      "engine-default";
 
 %}
--- a/toolkit/components/search/tests/xpcshell/test_notifications.js
+++ b/toolkit/components/search/tests/xpcshell/test_notifications.js
@@ -5,27 +5,26 @@
 
 var gTestLog = [];
 
 /**
  * The order of notifications expected for this test is:
  *  - engine-changed (while we're installing the engine, we modify it, which notifies - bug 606886)
  *  - engine-added (engine was added to the store by the search service)
  *   -> our search observer is called, which sets:
- *    - .defaultEngine, triggering engine-default and engine-current (after bug
+ *    - .defaultEngine, triggering engine-default and engine-default (after bug
  *      493051 - for now the search service sets this after engine-added)
  *   ...and then schedules a removal
  *  - engine-loaded (the search service's observer is garanteed to fire first, which is what causes engine-added to fire)
  *  - engine-removed (due to the removal schedule above)
  */
 var expectedLog = [
   "engine-changed", // XXX bug 606886
   "engine-added",
   "engine-default",
-  "engine-current",
   "engine-loaded",
   "engine-removed",
 ];
 
 function create_search_observer(resolve) {
   return function search_observer(subject, topic, data) {
     let engine = subject.QueryInterface(Ci.nsISearchEngine);
     gTestLog.push(data + " for " + engine.name);
--- a/toolkit/components/telemetry/app/TelemetryEnvironment.jsm
+++ b/toolkit/components/telemetry/app/TelemetryEnvironment.jsm
@@ -1217,17 +1217,17 @@ EnvironmentCache.prototype = {
     Services.obs.removeObserver(this, SEARCH_SERVICE_TOPIC);
     Services.obs.removeObserver(this, AUTO_UPDATE_PREF_CHANGE_TOPIC);
   },
 
   observe(aSubject, aTopic, aData) {
     this._log.trace("observe - aTopic: " + aTopic + ", aData: " + aData);
     switch (aTopic) {
       case SEARCH_ENGINE_MODIFIED_TOPIC:
-        if (aData != "engine-current") {
+        if (aData != "engine-default") {
           return;
         }
         // Record the new default search choice and send the change notification.
         this._onSearchEngineChange();
         break;
       case SEARCH_SERVICE_TOPIC:
         if (aData != "init-complete") {
           return;
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ -1558,20 +1558,20 @@ add_task(async function test_defaultSear
     "submissionURL": "https://ar.wikipedia.org/wiki/%D8%AE%D8%A7%D8%B5:%D8%A8%D8%AD%D8%AB?search=&sourceId=Mozilla-search",
   };
   Assert.deepEqual(data.settings.defaultSearchEngineData, expectedSearchEngineData);
 
   // Remove all the search engines.
   for (let engine of await Services.search.getEngines()) {
     await Services.search.removeEngine(engine);
   }
-  // The search service does not notify "engine-current" when removing a default engine.
+  // The search service does not notify "engine-default" when removing a default engine.
   // Manually force the notification.
   // TODO: remove this when bug 1165341 is resolved.
-  Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-current");
+  Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-default");
   await promiseNextTick();
 
   // Then check that no default engine is reported if none is available.
   data = TelemetryEnvironment.currentEnvironment;
   checkEnvironmentData(data);
   Assert.equal(data.settings.defaultSearchEngine, "NONE");
   Assert.deepEqual(data.settings.defaultSearchEngineData, {name: "NONE"});
 
@@ -1630,17 +1630,17 @@ add_task(async function test_defaultSear
   Assert.deepEqual(data.settings.defaultSearchEngineData,
                    {"name": "engine-telemetry", "loadPath": "[other]/engine.xml", "origin": "verified"});
 
   // Now break this engine's load path hash.
   promise = new Promise(resolve => {
     TelemetryEnvironment.registerChangeListener("testWatch_SearchDefault", resolve);
   });
   engine.wrappedJSObject.setAttr("loadPathHash", "broken");
-  Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-current");
+  Services.obs.notifyObservers(null, "browser-search-engine-modified", "engine-default");
   await promise;
   TelemetryEnvironment.unregisterChangeListener("testWatch_SearchDefault");
   data = TelemetryEnvironment.currentEnvironment;
   Assert.equal(data.settings.defaultSearchEngineData.origin, "invalid");
   await Services.search.removeEngine(engine);
 
   // Define and reset the test preference.
   const PREF_TEST = "toolkit.telemetry.test.pref1";