Bug 1954705 - Confirm deletion of user and open search engines. r=search-reviewers,fluent-reviewers,urlbar-reviewers,bolsson,Standard8

Differential Revision: https://phabricator.services.mozilla.com/D245412

browser/components/preferences/search.js

@@ -8,6 +8,7 @@
 const lazy = {};
 
 ChromeUtils.defineESModuleGetters(lazy, {
+  AddonSearchEngine: "resource://gre/modules/AddonSearchEngine.sys.mjs",
   CustomizableUI: "resource:///modules/CustomizableUI.sys.mjs",
   SearchUIUtils: "moz-src:///browser/components/search/SearchUIUtils.sys.mjs",
   SearchUtils: "resource://gre/modules/SearchUtils.sys.mjs",
@@ -608,9 +609,11 @@ class EngineStore {
     var clonedObj = {
       iconURL: null,
     };
-    for (let i of ["id", "name", "alias", "hidden"]) {
+    for (let i of ["id", "name", "alias", "hidden", "isAppProvided"]) {
       clonedObj[i] = aEngine[i];
     }
+    clonedObj.isAddonEngine =
+      aEngine.wrappedJSObject instanceof lazy.AddonSearchEngine;
     clonedObj.isUserEngine =
       aEngine.wrappedJSObject instanceof lazy.UserSearchEngine;
     clonedObj.originalEngine = aEngine;
@@ -676,8 +679,8 @@ class EngineStore {
       throw new Error("Cannot remove last engine!");
     }
 
-    let engineName = aEngine.name;
-    let index = this.engines.findIndex(element => element.name == engineName);
+    let engineId = aEngine.id;
+    let index = this.engines.findIndex(element => element.id == engineId);
 
     if (index == -1) {
       throw new Error("invalid engine?");
@@ -785,10 +788,13 @@ class EngineStore {
  * Manages the view of the Search Shortcuts tree on the search pane of preferences.
  */
 class EngineView {
-  _engineStore = null;
+  _engineStore;
   _engineList = null;
   tree = null;
 
+  /**
+   * @param {EngineStore} aEngineStore
+   */
   constructor(aEngineStore) {
     this._engineStore = aEngineStore;
     this._engineList = document.getElementById("engineList");
@@ -939,6 +945,55 @@ class EngineView {
     );
   }
 
+  /**
+   * Removes a search engine from the search service.
+   *
+   * Application provided engines are removed without confirmation since they
+   * can easily be restored. Addon engines are not removed (see comment).
+   * For other engine types, the user is prompted for confirmation.
+   *
+   * @param {object} engine
+   *   The search engine object from EngineStore to remove.
+   */
+  async promptAndRemoveEngine(engine) {
+    if (engine.isAppProvided) {
+      Services.search.removeEngine(this.selectedEngine.originalEngine);
+      return;
+    }
+
+    if (engine.isAddonEngine) {
+      // Addon engines will re-appear after restarting, see Bug 1546652.
+      // This should ideally prompt the user if they want to remove the addon.
+      let msg = await document.l10n.formatValue("remove-addon-engine-alert");
+      alert(msg);
+      return;
+    }
+
+    let [body, removeLabel] = await document.l10n.formatValues([
+      "remove-engine-confirmation",
+      "remove-engine-remove",
+    ]);
+
+    let button = Services.prompt.confirmExBC(
+      window.browsingContext,
+      Services.prompt.MODAL_TYPE_CONTENT,
+      null,
+      body,
+      (Services.prompt.BUTTON_TITLE_IS_STRING * Services.prompt.BUTTON_POS_0) |
+        (Services.prompt.BUTTON_TITLE_CANCEL * Services.prompt.BUTTON_POS_1),
+      removeLabel,
+      null,
+      null,
+      null,
+      {}
+    );
+
+    // Button 0 is the remove button.
+    if (button == 0) {
+      Services.search.removeEngine(this.selectedEngine.originalEngine);
+    }
+  }
+
   /**
    * Returns the local shortcut corresponding to a tree row, or null if the row
    * is not a local shortcut.
@@ -1010,7 +1065,9 @@ class EngineView {
             this.#onRestoreDefaults();
             break;
           case "removeEngineButton":
-            Services.search.removeEngine(this.selectedEngine.originalEngine);
+            if (this.isEngineSelectedAndRemovable()) {
+              this.promptAndRemoveEngine(this.selectedEngine);
+            }
             break;
           case "editEngineButton":
             gSubDialog.open(
@@ -1110,11 +1167,12 @@ class EngineView {
         aEvent.keyCode == KeyEvent.DOM_VK_DELETE ||
         (isMac &&
           aEvent.shiftKey &&
-          aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE &&
-          this.isEngineSelectedAndRemovable())
+          aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE)
       ) {
         // Delete and Shift+Backspace (Mac) removes selected engine.
-        Services.search.removeEngine(this.selectedEngine.originalEngine);
+        if (this.isEngineSelectedAndRemovable()) {
+          this.promptAndRemoveEngine(this.selectedEngine);
+        }
       }
     }
   }

browser/components/preferences/tests/browser_search_engineList.js

@@ -1,3 +1,6 @@
+const { PromptTestUtils } = ChromeUtils.importESModule(
+  "resource://testing-common/PromptTestUtils.sys.mjs"
+);
 const { SearchTestUtils } = ChromeUtils.importESModule(
   "resource://testing-common/SearchTestUtils.sys.mjs"
 );
@@ -45,6 +48,24 @@ async function engine_list_test(fn) {
   Object.defineProperty(task, "name", { value: fn.name });
   add_task(task);
 }
+async function selectEngine(tree, index) {
+  let rect = tree.getCoordsForCellItem(
+    index,
+    tree.columns.getNamedColumn("engineName"),
+    "text"
+  );
+  let x = rect.x + rect.width / 2;
+  let y = rect.y + rect.height / 2;
+  let promise = BrowserTestUtils.waitForEvent(tree, "click");
+  EventUtils.synthesizeMouse(
+    tree.body,
+    x,
+    y,
+    { clickCount: 1 },
+    tree.ownerGlobal
+  );
+  return promise;
+}
 
 add_setup(async function () {
   installedEngines = await Services.search.getAppProvidedEngines();
@@ -64,11 +85,7 @@ add_setup(async function () {
     alias: "u",
   });
   installedEngines.push(userEngine);
-
-  registerCleanupFunction(async () => {
-    await Services.search.removeEngine(userEngine);
-    // Extension engine is cleaned up by SearchTestUtils.
-  });
+  // The added engines are removed in the last test.
 });
 
 engine_list_test(async function test_engine_list(tree) {
@@ -201,25 +218,13 @@ engine_list_test(async function test_rename_engines(tree) {
 
 engine_list_test(async function test_remove_button_disabled_state(tree, doc) {
   let appProvidedEngines = await Services.search.getAppProvidedEngines();
-  let win = tree.ownerGlobal;
   for (let i = 0; i < appProvidedEngines.length; i++) {
     let engine = appProvidedEngines[i];
     let isDefaultSearchEngine =
       engine.id == Services.search.defaultEngine.id ||
       engine.id == Services.search.defaultPrivateEngine.id;
 
-    let rect = tree.getCoordsForCellItem(
-      i,
-      tree.columns.getNamedColumn("engineName"),
-      "text"
-    );
-    let x = rect.x + rect.width / 2;
-    let y = rect.y + rect.height / 2;
-
-    let promise = BrowserTestUtils.waitForEvent(tree, "click");
-    EventUtils.synthesizeMouse(tree.body, x, y, { clickCount: 1 }, win);
-    await promise;
-
+    await selectEngine(tree, i);
     Assert.equal(
       doc.querySelector("#removeEngineButton").disabled,
       isDefaultSearchEngine,
@@ -227,3 +232,69 @@ engine_list_test(async function test_remove_button_disabled_state(tree, doc) {
     );
   }
 });
+
+engine_list_test(async function test_remove_button(tree, doc) {
+  let win = tree.ownerGlobal;
+  let alertSpy = sinon.stub(win, "alert");
+
+  info("Removing user engine.");
+  let userEngineIndex = installedEngines.findIndex(e => e.id == userEngine.id);
+  await selectEngine(tree, userEngineIndex);
+
+  let promptPromise = PromptTestUtils.handleNextPrompt(
+    gBrowser.selectedBrowser,
+    { modalType: Services.prompt.MODAL_TYPE_CONTENT },
+    { buttonNumClick: 0 } // 0 = cancel, 1 = remove
+  );
+  let removedPromise = SearchTestUtils.promiseSearchNotification(
+    SearchUtils.MODIFIED_TYPE.REMOVED,
+    SearchUtils.TOPIC_ENGINE_MODIFIED
+  );
+
+  doc.querySelector("#removeEngineButton").click();
+  await promptPromise;
+  let removedEngine = await removedPromise;
+  Assert.equal(
+    removedEngine.id,
+    userEngine.id,
+    "User engine was removed after a prompt."
+  );
+
+  // Re-fetch the engines since removing the user engine changed it.
+  installedEngines = await Services.search.getEngines();
+
+  info("Removing extension engine.");
+  let extensionEngineIndex = installedEngines.findIndex(
+    e => e.id == extensionEngine.id
+  );
+  await selectEngine(tree, extensionEngineIndex);
+
+  doc.querySelector("#removeEngineButton").click();
+  await TestUtils.waitForCondition(() => alertSpy.calledOnce);
+  Assert.ok(true, "Alert is shown when attempting to remove extension engine.");
+
+  info("Removing (last) app provided engine.");
+  let appProvidedEngines = await Services.search.getAppProvidedEngines();
+  let lastAppEngine = appProvidedEngines[appProvidedEngines.length - 1];
+  let lastAppEngineIndex = installedEngines.findIndex(
+    e => e.id == lastAppEngine.id
+  );
+  await selectEngine(tree, lastAppEngineIndex);
+
+  doc.querySelector("#removeEngineButton").click();
+  removedEngine = await SearchTestUtils.promiseSearchNotification(
+    SearchUtils.MODIFIED_TYPE.REMOVED,
+    SearchUtils.TOPIC_ENGINE_MODIFIED
+  );
+  Assert.equal(
+    removedEngine.id,
+    lastAppEngine.id,
+    "Last app provided engine was removed without a prompt."
+  );
+
+  // Cleanup.
+  alertSpy.restore();
+  Services.search.resetToAppDefaultEngine();
+  // The user engine is purposefully not re-added.
+  // The extension engine is removed automatically on cleanup.
+});

browser/locales/en-US/browser/preferences/preferences.ftl

@@ -820,6 +820,10 @@ search-keyword-warning-title = Duplicate Keyword
 search-keyword-warning-engine = You have chosen a keyword that is currently in use by “{ $name }”. Please select another.
 search-keyword-warning-bookmark = You have chosen a keyword that is currently in use by a bookmark. Please select another.
 
+remove-engine-confirmation = Are you sure you want to remove this search engine?
+remove-engine-remove = Remove
+remove-addon-engine-alert = To remove this search engine, remove the associated add-on.
+
 ## Containers Section
 
 containers-back-button2 =