From 67039cecb047ee0da06ab2d78adfb61e9c279deb Mon Sep 17 00:00:00 2001 From: Mark Banner Date: Fri, 24 May 2024 09:10:05 +0000 Subject: [PATCH] Bug 1897508 - Separate out search engine icon updates into a separate notification subject. r=search-reviewers,settings-reviewers,mcheang This also avoids sending an engine-update Glean event for the default engine by not handling the engine-icon-update notification in the update tracking code. Differential Revision: https://phabricator.services.mozilla.com/D210838 --- .../browser/browser_ext_search_favicon.js | 2 +- browser/components/preferences/search.js | 1 + .../urlbar/UrlbarSearchUtils.sys.mjs | 4 ++++ .../search/AppProvidedSearchEngine.sys.mjs | 5 +++- .../components/search/SearchEngine.sys.mjs | 2 +- .../components/search/SearchSettings.sys.mjs | 11 +++++++++ toolkit/components/search/SearchUtils.sys.mjs | 1 + .../test_appProvided_icons_updates.js | 24 +++++++++---------- .../tests/xpcshell/test_opensearch_icon.js | 6 ++--- .../xpcshell/test_webextensions_install.js | 6 ++--- 10 files changed, 41 insertions(+), 21 deletions(-) diff --git a/browser/components/extensions/test/browser/browser_ext_search_favicon.js b/browser/components/extensions/test/browser/browser_ext_search_favicon.js index 474ab37e20c7..5494f5df13a1 100644 --- a/browser/components/extensions/test/browser/browser_ext_search_favicon.js +++ b/browser/components/extensions/test/browser/browser_ext_search_favicon.js @@ -30,7 +30,7 @@ async function promiseEngineIconLoaded(engineName) { "browser-search-engine-modified", (engine, verb) => { engine.QueryInterface(Ci.nsISearchEngine); - return verb == "engine-changed" && engine.name == engineName; + return verb == "engine-icon-changed" && engine.name == engineName; } ); Assert.ok( diff --git a/browser/components/preferences/search.js b/browser/components/preferences/search.js index a491d6c5ca6d..dcfb2c9a6d65 100644 --- a/browser/components/preferences/search.js +++ b/browser/components/preferences/search.js @@ -749,6 +749,7 @@ class EngineStore { this.notifyRowCountChanged(gEngineView.lastEngineIndex, 1); break; case "engine-changed": + case "engine-icon-changed": this.updateEngine(engine); this.notifyRebuildViews(); break; diff --git a/browser/components/urlbar/UrlbarSearchUtils.sys.mjs b/browser/components/urlbar/UrlbarSearchUtils.sys.mjs index 30cbdb73c424..76eb8b823f2c 100644 --- a/browser/components/urlbar/UrlbarSearchUtils.sys.mjs +++ b/browser/components/urlbar/UrlbarSearchUtils.sys.mjs @@ -131,6 +131,10 @@ class SearchUtils { /** * Gets the engine with a given alias. * + * Note: engines returned from this list may be updated at any time. If you + * are caching the icon or other fields for more than a single engagement of + * the urlbar, consider observing the SEARCH_ENGINE_TOPIC. + * * @param {string} alias * A search engine alias. The alias string comparison is case insensitive. * @param {string} [searchString] diff --git a/toolkit/components/search/AppProvidedSearchEngine.sys.mjs b/toolkit/components/search/AppProvidedSearchEngine.sys.mjs index ed815b96d1a1..1413de0ee417 100644 --- a/toolkit/components/search/AppProvidedSearchEngine.sys.mjs +++ b/toolkit/components/search/AppProvidedSearchEngine.sys.mjs @@ -453,7 +453,10 @@ export class AppProvidedSearchEngine extends SearchEngine { this.#blobURLPromise = null; } this.#blobURLPromise = Promise.resolve(blobURL); - lazy.SearchUtils.notifyAction(this, lazy.SearchUtils.MODIFIED_TYPE.CHANGED); + lazy.SearchUtils.notifyAction( + this, + lazy.SearchUtils.MODIFIED_TYPE.ICON_CHANGED + ); } /** diff --git a/toolkit/components/search/SearchEngine.sys.mjs b/toolkit/components/search/SearchEngine.sys.mjs index 7dfb4642f560..c09b35970b30 100644 --- a/toolkit/components/search/SearchEngine.sys.mjs +++ b/toolkit/components/search/SearchEngine.sys.mjs @@ -798,7 +798,7 @@ export class SearchEngine { if (this._engineAddedToStore) { lazy.SearchUtils.notifyAction( this, - lazy.SearchUtils.MODIFIED_TYPE.CHANGED + lazy.SearchUtils.MODIFIED_TYPE.ICON_CHANGED ); } this._hasPreferredIcon = isPreferred; diff --git a/toolkit/components/search/SearchSettings.sys.mjs b/toolkit/components/search/SearchSettings.sys.mjs index b4db403bb061..c4c3096f8e15 100644 --- a/toolkit/components/search/SearchSettings.sys.mjs +++ b/toolkit/components/search/SearchSettings.sys.mjs @@ -5,6 +5,8 @@ const lazy = {}; ChromeUtils.defineESModuleGetters(lazy, { + AppProvidedSearchEngine: + "resource://gre/modules/AppProvidedSearchEngine.sys.mjs", DeferredTask: "resource://gre/modules/DeferredTask.sys.mjs", ObjectUtils: "resource://gre/modules/ObjectUtils.sys.mjs", SearchUtils: "resource://gre/modules/SearchUtils.sys.mjs", @@ -501,6 +503,15 @@ export class SearchSettings { case lazy.SearchUtils.MODIFIED_TYPE.REMOVED: this._delayedWrite(); break; + case lazy.SearchUtils.MODIFIED_TYPE.ICON_CHANGED: + // Application Provided Search Engines have their icons stored in + // Remote Settings, so we don't need to update the saved settings. + if ( + !(engine?.wrappedJSObject instanceof lazy.AppProvidedSearchEngine) + ) { + this._delayedWrite(); + } + break; } break; case lazy.SearchUtils.TOPIC_SEARCH_SERVICE: diff --git a/toolkit/components/search/SearchUtils.sys.mjs b/toolkit/components/search/SearchUtils.sys.mjs index 024a3d2c1f41..4afcf4d204d9 100644 --- a/toolkit/components/search/SearchUtils.sys.mjs +++ b/toolkit/components/search/SearchUtils.sys.mjs @@ -186,6 +186,7 @@ export var SearchUtils = { TOPIC_ENGINE_MODIFIED: "browser-search-engine-modified", MODIFIED_TYPE: { CHANGED: "engine-changed", + ICON_CHANGED: "engine-icon-changed", REMOVED: "engine-removed", ADDED: "engine-added", DEFAULT: "engine-default", diff --git a/toolkit/components/search/tests/xpcshell/test_appProvided_icons_updates.js b/toolkit/components/search/tests/xpcshell/test_appProvided_icons_updates.js index 4159b9f0fbb9..5502e425603e 100644 --- a/toolkit/components/search/tests/xpcshell/test_appProvided_icons_updates.js +++ b/toolkit/components/search/tests/xpcshell/test_appProvided_icons_updates.js @@ -234,8 +234,8 @@ add_task(async function test_icon_added_existing_engine() { }); await client.db.update(mock.record, Date.now()); - let promiseEngineUpdated = SearchTestUtils.promiseSearchNotification( - SearchUtils.MODIFIED_TYPE.CHANGED, + let promiseIconChanged = SearchTestUtils.promiseSearchNotification( + SearchUtils.MODIFIED_TYPE.ICON_CHANGED, SearchUtils.TOPIC_ENGINE_MODIFIED ); @@ -250,7 +250,7 @@ add_task(async function test_icon_added_existing_engine() { SearchTestUtils.idleService._fireObservers("idle"); - await promiseEngineUpdated; + await promiseIconChanged; await assertEngineIcon("engine_no_initial_icon name", "bigIcon.ico"); }); @@ -270,8 +270,8 @@ add_task(async function test_icon_updated() { }); await client.db.update(mock.record, Date.now()); - let promiseEngineUpdated = SearchTestUtils.promiseSearchNotification( - SearchUtils.MODIFIED_TYPE.CHANGED, + let promiseIconChanged = SearchTestUtils.promiseSearchNotification( + SearchUtils.MODIFIED_TYPE.ICON_CHANGED, SearchUtils.TOPIC_ENGINE_MODIFIED ); @@ -285,7 +285,7 @@ add_task(async function test_icon_updated() { }); SearchTestUtils.idleService._fireObservers("idle"); - await promiseEngineUpdated; + await promiseIconChanged; await assertEngineIcon("engine_icon_updates name", "bigIcon.ico"); }); @@ -296,12 +296,12 @@ add_task(async function test_icon_not_local() { await assertEngineIcon("engine_icon_not_local name", null); // A download should have been queued, so fire idle to trigger it. - let promiseEngineUpdated = SearchTestUtils.promiseSearchNotification( - SearchUtils.MODIFIED_TYPE.CHANGED, + let promiseIconChanged = SearchTestUtils.promiseSearchNotification( + SearchUtils.MODIFIED_TYPE.ICON_CHANGED, SearchUtils.TOPIC_ENGINE_MODIFIED ); SearchTestUtils.idleService._fireObservers("idle"); - await promiseEngineUpdated; + await promiseIconChanged; await assertEngineIcon("engine_icon_not_local name", "bigIcon.ico"); }); @@ -313,12 +313,12 @@ add_task(async function test_icon_out_of_date() { await assertEngineIcon("engine_icon_out_of_date name", "remoteIcon.ico"); // A download should have been queued, so fire idle to trigger it. - let promiseEngineUpdated = SearchTestUtils.promiseSearchNotification( - SearchUtils.MODIFIED_TYPE.CHANGED, + let promiseIconChanged = SearchTestUtils.promiseSearchNotification( + SearchUtils.MODIFIED_TYPE.ICON_CHANGED, SearchUtils.TOPIC_ENGINE_MODIFIED ); SearchTestUtils.idleService._fireObservers("idle"); - await promiseEngineUpdated; + await promiseIconChanged; await assertEngineIcon("engine_icon_out_of_date name", "bigIcon.ico"); }); diff --git a/toolkit/components/search/tests/xpcshell/test_opensearch_icon.js b/toolkit/components/search/tests/xpcshell/test_opensearch_icon.js index 2a0ea9f52792..3910a0f14bb0 100644 --- a/toolkit/components/search/tests/xpcshell/test_opensearch_icon.js +++ b/toolkit/components/search/tests/xpcshell/test_opensearch_icon.js @@ -36,8 +36,8 @@ add_task(async function test_icon_types() { SearchUtils.MODIFIED_TYPE.ADDED, SearchUtils.TOPIC_ENGINE_MODIFIED ); - let promiseEngineChanged = SearchTestUtils.promiseSearchNotification( - SearchUtils.MODIFIED_TYPE.CHANGED, + let promiseIconChanged = SearchTestUtils.promiseSearchNotification( + SearchUtils.MODIFIED_TYPE.ICON_CHANGED, SearchUtils.TOPIC_ENGINE_MODIFIED ); const engineData = { @@ -54,7 +54,7 @@ add_task(async function test_icon_types() { let engine = await promiseEngineAdded; // Ensure this is a nsISearchEngine. engine.QueryInterface(Ci.nsISearchEngine); - await promiseEngineChanged; + await promiseIconChanged; Assert.ok(await engine.getIconURL(), `${test.name} engine has an icon`); Assert.ok( diff --git a/toolkit/components/search/tests/xpcshell/test_webextensions_install.js b/toolkit/components/search/tests/xpcshell/test_webextensions_install.js index a05218824afb..01043cb88901 100644 --- a/toolkit/components/search/tests/xpcshell/test_webextensions_install.js +++ b/toolkit/components/search/tests/xpcshell/test_webextensions_install.js @@ -201,8 +201,8 @@ add_task(async function test_load_favicon_invalid_redirect() { }); add_task(async function test_load_favicon_redirect() { - let promiseEngineChanged = SearchTestUtils.promiseSearchNotification( - SearchUtils.MODIFIED_TYPE.CHANGED, + let promiseIconChanged = SearchTestUtils.promiseSearchNotification( + SearchUtils.MODIFIED_TYPE.ICON_CHANGED, SearchUtils.TOPIC_ENGINE_MODIFIED ); @@ -216,7 +216,7 @@ add_task(async function test_load_favicon_redirect() { let engine = await Services.search.getEngineByName("Example"); - await promiseEngineChanged; + await promiseIconChanged; Assert.ok(await engine.getIconURL(), "Should have set an iconURI"); Assert.ok(