From 20319dd55d79af02c9336b6c85b7cb686010f817 Mon Sep 17 00:00:00 2001 From: Andrei Oprea Date: Mon, 4 Feb 2019 16:21:55 +0100 Subject: [PATCH] Port bug 1524593 - nsISearchService (aka nsIBrowserSearchService, previously) refactor to be mostly an asynchronous, in preparation of WebExtension engines --- lib/ASRouterTargeting.jsm | 21 +++---- lib/SearchShortcuts.jsm | 4 +- lib/SnippetsFeed.jsm | 22 +++---- lib/TopSitesFeed.jsm | 72 +++++++++++----------- test/browser/browser_asrouter_targeting.js | 8 +-- test/unit/lib/TopSitesFeed.test.js | 8 +-- 6 files changed, 61 insertions(+), 74 deletions(-) diff --git a/lib/ASRouterTargeting.jsm b/lib/ASRouterTargeting.jsm index 7d8f6b19c..46964a416 100644 --- a/lib/ASRouterTargeting.jsm +++ b/lib/ASRouterTargeting.jsm @@ -238,19 +238,14 @@ const TargetingGetters = { get searchEngines() { return new Promise(resolve => { // Note: calling init ensures this code is only executed after Search has been initialized - Services.search.init(rv => { - if (Components.isSuccessCode(rv)) { - let engines = Services.search.getVisibleEngines(); - resolve({ - current: Services.search.defaultEngine.identifier, - installed: engines - .map(engine => engine.identifier) - .filter(engine => engine), - }); - } else { - resolve({installed: [], current: ""}); - } - }); + Services.search.getVisibleEngines().then(engines => { + resolve({ + current: Services.search.defaultEngine.identifier, + installed: engines + .map(engine => engine.identifier) + .filter(engine => engine), + }); + }).catch(() => resolve({installed: [], current: ""})); }); }, get isDefaultBrowser() { diff --git a/lib/SearchShortcuts.jsm b/lib/SearchShortcuts.jsm index 92dadc51a..a82b0d338 100644 --- a/lib/SearchShortcuts.jsm +++ b/lib/SearchShortcuts.jsm @@ -37,8 +37,8 @@ this.getSearchProvider = getSearchProvider; // Check topsite against predefined list of valid search engines // https://searchfox.org/mozilla-central/rev/ca869724246f4230b272ed1c8b9944596e80d920/toolkit/components/search/nsSearchService.js#939 -function checkHasSearchEngine(keyword) { - return Services.search.getDefaultEngines() +async function checkHasSearchEngine(keyword) { + return (await Services.search.getDefaultEngines()) .find(e => e.wrappedJSObject._internalAliases.includes(keyword)); } this.checkHasSearchEngine = checkHasSearchEngine; diff --git a/lib/SnippetsFeed.jsm b/lib/SnippetsFeed.jsm index 522110945..cfae4116d 100644 --- a/lib/SnippetsFeed.jsm +++ b/lib/SnippetsFeed.jsm @@ -71,20 +71,14 @@ this.SnippetsFeed = class SnippetsFeed { getSelectedSearchEngine() { return new Promise(resolve => { // Note: calling init ensures this code is only executed after Search has been initialized - Services.search.init(rv => { - // istanbul ignore else - if (Components.isSuccessCode(rv)) { - let engines = Services.search.getVisibleEngines(); - resolve({ - searchEngineIdentifier: Services.search.defaultEngine.identifier, - engines: engines - .filter(engine => engine.identifier) - .map(engine => `${TARGET_SEARCHENGINE_PREFIX}${engine.identifier}`), - }); - } else { - resolve({engines: [], searchEngineIdentifier: ""}); - } - }); + Services.search.getVisibleEngines().then(engines => { + resolve({ + searchEngineIdentifier: Services.search.defaultEngine.identifier, + engines: engines + .filter(engine => engine.identifier) + .map(engine => `${TARGET_SEARCHENGINE_PREFIX}${engine.identifier}`), + }); + }).catch(() => resolve({engines: [], searchEngineIdentifier: ""})); }); } diff --git a/lib/TopSitesFeed.jsm b/lib/TopSitesFeed.jsm index f0ce30e78..c6add82a0 100644 --- a/lib/TopSitesFeed.jsm +++ b/lib/TopSitesFeed.jsm @@ -180,9 +180,7 @@ this.TopSitesFeed = class TopSitesFeed { Array(emptySlots).fill(null) ); - await new Promise(resolve => Services.search.init(resolve)); - - const tryToInsertSearchShortcut = shortcut => { + const tryToInsertSearchShortcut = async shortcut => { const nextAvailable = pinnedSites.indexOf(null); // Only add a search shortcut if the site isn't already pinned, we // haven't previously inserted it, there's space to pin it, and the @@ -191,16 +189,18 @@ this.TopSitesFeed = class TopSitesFeed { !pinnedSites.find(s => s && s.hostname === shortcut.shortURL) && !prevInsertedShortcuts.includes(shortcut.shortURL) && nextAvailable > -1 && - checkHasSearchEngine(shortcut.keyword) + await checkHasSearchEngine(shortcut.keyword) ) { - const site = this.topSiteToSearchTopSite({url: shortcut.url}); + const site = await this.topSiteToSearchTopSite({url: shortcut.url}); this._pinSiteAt(site, nextAvailable); pinnedSites[nextAvailable] = site; newInsertedShortcuts.push(shortcut.shortURL); } }; - shouldPin.forEach(shortcut => tryToInsertSearchShortcut(shortcut)); + for (let shortcut of shouldPin) { + await tryToInsertSearchShortcut(shortcut); + } if (newInsertedShortcuts.length) { this.store.dispatch(ac.SetPref(SEARCH_SHORTCUTS_HAVE_PINNED_PREF, prevInsertedShortcuts.concat(newInsertedShortcuts).join(","))); @@ -216,43 +216,42 @@ this.TopSitesFeed = class TopSitesFeed { const searchShortcutsExperiment = this.store.getState().Prefs.values[SEARCH_SHORTCUTS_EXPERIMENT]; // We must wait for search services to initialize in order to access default // search engine properties without triggering a synchronous initialization - await new Promise(resolve => Services.search.init(resolve)); + await Services.search.init(); - // Get all frecent sites from history - const frecent = (await this.frecentCache.request({ + // Get all frecent sites from history. + let frecent = []; + const cache = await this.frecentCache.request({ // We need to overquery due to the top 5 alexa search + default search possibly being removed numItems: numItems + SEARCH_FILTERS.length + 1, topsiteFrecency: FRECENCY_THRESHOLD, - })) - .reduce((validLinks, link) => { + }); + for (let link of cache) { const hostname = shortURL(link); if (!this.isExperimentOnAndLinkFilteredSearch(hostname)) { - validLinks.push({ - ...(searchShortcutsExperiment ? this.topSiteToSearchTopSite(link) : link), + frecent.push({ + ...(searchShortcutsExperiment ? await this.topSiteToSearchTopSite(link) : link), hostname, }); } - return validLinks; - }, []); + } - // Remove any defaults that have been blocked - const notBlockedDefaultSites = DEFAULT_TOP_SITES - .reduce((topsites, link) => { - const searchProvider = getSearchProvider(shortURL(link)); - if (NewTabUtils.blockedLinks.isBlocked({url: link.url})) { - return topsites; - } else if (this.isExperimentOnAndLinkFilteredSearch(link.hostname)) { - return topsites; - // If we've previously blocked a search shortcut, remove the default top site - // that matches the hostname - } else if (searchProvider && NewTabUtils.blockedLinks.isBlocked({url: searchProvider.url})) { - return topsites; - } - return [ - ...topsites, - searchShortcutsExperiment ? this.topSiteToSearchTopSite(link) : link, - ]; - }, []); + // Remove any defaults that have been blocked. + let notBlockedDefaultSites = []; + for (let link of DEFAULT_TOP_SITES) { + const searchProvider = getSearchProvider(shortURL(link)); + if (NewTabUtils.blockedLinks.isBlocked({url: link.url})) { + continue; + } else if (this.isExperimentOnAndLinkFilteredSearch(link.hostname)) { + continue; + // If we've previously blocked a search shortcut, remove the default top site + // that matches the hostname + } else if (searchProvider && NewTabUtils.blockedLinks.isBlocked({url: searchProvider.url})) { + continue; + } + notBlockedDefaultSites.push( + searchShortcutsExperiment ? await this.topSiteToSearchTopSite(link) : link, + ); + } // Get pinned links augmented with desired properties let plainPinned = await this.pinnedCache.request(); @@ -379,8 +378,7 @@ this.TopSitesFeed = class TopSitesFeed { } // Populate the state with available search shortcuts - await new Promise(resolve => Services.search.init(resolve)); - const searchShortcuts = Services.search.getDefaultEngines().reduce((result, engine) => { + const searchShortcuts = (await Services.search.getDefaultEngines()).reduce((result, engine) => { const shortcut = CUSTOM_SEARCH_SHORTCUTS.find(s => engine.wrappedJSObject._internalAliases.includes(s.keyword)); if (shortcut) { result.push(this._tippyTopProvider.processSite({...shortcut})); @@ -393,9 +391,9 @@ this.TopSitesFeed = class TopSitesFeed { })); } - topSiteToSearchTopSite(site) { + async topSiteToSearchTopSite(site) { const searchProvider = getSearchProvider(shortURL(site)); - if (!searchProvider || !checkHasSearchEngine(searchProvider.keyword)) { + if (!searchProvider || !await checkHasSearchEngine(searchProvider.keyword)) { return site; } return { diff --git a/test/browser/browser_asrouter_targeting.js b/test/browser/browser_asrouter_targeting.js index 912c593d7..bec03b44c 100644 --- a/test/browser/browser_asrouter_targeting.js +++ b/test/browser/browser_asrouter_targeting.js @@ -188,7 +188,7 @@ add_task(async function check_needsUpdate() { add_task(async function checksearchEngines() { const result = await ASRouterTargeting.Environment.searchEngines; - const expectedInstalled = Services.search.getVisibleEngines() + const expectedInstalled = (await Services.search.getVisibleEngines()) .map(engine => engine.identifier) .sort() .join(","); @@ -198,14 +198,14 @@ add_task(async function checksearchEngines() { "searchEngines.installed should be an array of visible search engines"); ok(result.current && typeof result.current === "string", "searchEngines.current should be a truthy string"); - is(result.current, Services.search.defaultEngine.identifier, + is(result.current, (await Services.search.getDefault()).identifier, "searchEngines.current should be the current engine name"); - const message = {id: "foo", targeting: `searchEngines[.current == ${Services.search.defaultEngine.identifier}]`}; + const message = {id: "foo", targeting: `searchEngines[.current == ${(await Services.search.getDefault()).identifier}]`}; is(await ASRouterTargeting.findMatchingMessage({messages: [message]}), message, "should select correct item by searchEngines.current"); - const message2 = {id: "foo", targeting: `searchEngines[${Services.search.getVisibleEngines()[0].identifier} in .installed]`}; + const message2 = {id: "foo", targeting: `searchEngines[${(await Services.search.getVisibleEngines())[0].identifier} in .installed]`}; is(await ASRouterTargeting.findMatchingMessage({messages: [message2]}), message2, "should select correct item by searchEngines.installed"); }); diff --git a/test/unit/lib/TopSitesFeed.test.js b/test/unit/lib/TopSitesFeed.test.js index d48bf5623..14b6fa606 100644 --- a/test/unit/lib/TopSitesFeed.test.js +++ b/test/unit/lib/TopSitesFeed.test.js @@ -1124,7 +1124,7 @@ describe("Top Sites Feed", () => { describe("improvesearch.noDefaultSearchTile experiment", () => { const NO_DEFAULT_SEARCH_TILE_PREF = "improvesearch.noDefaultSearchTile"; beforeEach(() => { - sandbox.stub(global.Services.search, "defaultEngine").value({identifier: "google", searchForm: "google.com"}); + global.Services.search.getDefault = async () => {identifier: "google", searchForm: "google.com"}; feed.store.state.Prefs.values[NO_DEFAULT_SEARCH_TILE_PREF] = true; }); it("should filter out alexa top 5 search from the default sites", async () => { @@ -1162,7 +1162,7 @@ describe("Top Sites Feed", () => { }); 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"}); + global.Services.search.getDefault = async () => {identifier: "ddg", searchForm: "duckduckgo.com"}; feed.observe(null, "browser-search-engine-modified", "engine-current"); assert.equal(feed._currentSearchHostname, "duckduckgo"); assert.calledOnce(feed.refresh); @@ -1187,7 +1187,7 @@ describe("Top Sites Feed", () => { {wrappedJSObject: {_internalAliases: ["@google"]}}, {wrappedJSObject: {_internalAliases: ["@amazon"]}}, ]; - global.Services.search.getDefaultEngines = () => searchEngines; + global.Services.search.getDefaultEngines = async () => searchEngines; fakeNewTabUtils.pinnedLinks.pin = sinon.stub().callsFake((site, index) => { fakeNewTabUtils.pinnedLinks.links[index] = site; }); @@ -1367,7 +1367,7 @@ describe("Top Sites Feed", () => { it("should not pin a shortcut if the corresponding search engine is not available", async () => { // Make Amazon search engine unavailable - global.Services.search.getDefaultEngines = () => [{wrappedJSObject: {_internalAliases: ["@google"]}}]; + global.Services.search.getDefaultEngines = async () => [{wrappedJSObject: {_internalAliases: ["@google"]}}]; fakeNewTabUtils.pinnedLinks.links.fill(null); await feed._maybeInsertSearchShortcuts(fakeNewTabUtils.pinnedLinks.links); assert.notOk(fakeNewTabUtils.pinnedLinks.links.find(s => s && s.url === "https://amazon.com"));