Bug 1657572 - Store form history source from search access points. r=adw

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

browser/actors/ContentSearchParent.jsm

@@ -304,7 +304,7 @@ let ContentSearch = {
     return result;
   },
 
-  async addFormHistoryEntry(browser, entry = "") {
+  async addFormHistoryEntry(browser, entry = null) {
     let isPrivate = false;
     try {
       // isBrowserPrivate assumes that the passed-in browser has all the normal
@@ -314,7 +314,7 @@ let ContentSearch = {
     } catch (err) {
       return false;
     }
-    if (isPrivate || entry === "") {
+    if (isPrivate || !entry) {
       return false;
     }
     let browserData = this._suggestionDataForBrowser(browser, true);
@@ -322,7 +322,8 @@ let ContentSearch = {
       {
         op: "bump",
         fieldname: browserData.controller.formHistoryParam,
-        value: entry,
+        value: entry.value,
+        source: entry.engineName,
       },
       {
         handleCompletion: () => {},

browser/base/content/contentSearchUI.js

@@ -234,7 +234,12 @@ this.ContentSearchUIController = (function() {
     },
 
     addInputValueToFormHistory() {
-      this._sendMsg("AddFormHistoryEntry", this.input.value);
+      let entry = {
+        value: this.input.value,
+        engineName: this.selectedEngineName,
+      };
+      this._sendMsg("AddFormHistoryEntry", entry);
+      return entry;
     },
 
     handleEvent(event) {

browser/base/content/test/general/browser_contentSearchUI.js

@@ -9,9 +9,10 @@ const TEST_ENGINE_2_BASENAME = "searchSuggestionEngine2.xml";
 
 const TEST_MSG = "ContentSearchUIControllerTest";
 
-let { SearchTestUtils } = ChromeUtils.import(
-  "resource://testing-common/SearchTestUtils.jsm"
-);
+XPCOMUtils.defineLazyModuleGetters(this, {
+  FormHistoryTestUtils: "resource://testing-common/FormHistoryTestUtils.jsm",
+  SearchTestUtils: "resource://testing-common/SearchTestUtils.jsm",
+});
 
 SearchTestUtils.init(Assert, registerCleanupFunction);
 
@@ -541,10 +542,17 @@ add_task(async function formHistory() {
     }, "satchel-storage-changed");
   });
 
-  await SpecialPowers.spawn(gBrowser.selectedBrowser, [], () => {
-    content.gController.addInputValueToFormHistory();
+  await FormHistoryTestUtils.clear("searchbar-history");
+  let entry = await SpecialPowers.spawn(gBrowser.selectedBrowser, [], () => {
+    return content.gController.addInputValueToFormHistory();
   });
   await observePromise;
+  Assert.greater(
+    await FormHistoryTestUtils.count("searchbar-history", {
+      source: entry.source,
+    }),
+    0
+  );
 
   // Reset the input.
   state = await msg("reset");

browser/components/search/content/searchbar.js

@@ -385,6 +385,7 @@
 
     doSearch(aData, aWhere, aEngine, aParams, aOneOff) {
       let textBox = this._textbox;
+      let engine = aEngine || this.currentEngine;
 
       // Save the current value in the form history
       if (
@@ -397,6 +398,7 @@
             op: "bump",
             fieldname: textBox.getAttribute("autocompletesearchparam"),
             value: aData,
+            source: engine.name,
           },
           {
             handleError(aError) {
@@ -408,7 +410,6 @@
         );
       }
 
-      let engine = aEngine || this.currentEngine;
       let submission = engine.getSubmission(aData, null, "searchbar");
       let telemetrySearchDetails = this.telemetrySearchDetails;
       this.telemetrySearchDetails = null;

browser/components/search/test/browser/browser_426329.js

@@ -1,9 +1,7 @@
 /* eslint-disable mozilla/no-arbitrary-setTimeout */
-ChromeUtils.defineModuleGetter(
-  this,
-  "FormHistory",
-  "resource://gre/modules/FormHistory.jsm"
-);
+XPCOMUtils.defineLazyModuleGetters(this, {
+  FormHistoryTestUtils: "resource://testing-common/FormHistoryTestUtils.jsm",
+});
 
 function expectedURL(aSearchTerms) {
   const ENGINE_HTML_BASE =
@@ -60,26 +58,6 @@ function getMenuEntries() {
   );
 }
 
-function countEntries(name, value) {
-  return new Promise(resolve => {
-    let count = 0;
-    let obj = name && value ? { fieldname: name, value } : {};
-    FormHistory.count(obj, {
-      handleResult(result) {
-        count = result;
-      },
-      handleError(error) {
-        throw error;
-      },
-      handleCompletion(reason) {
-        if (!reason) {
-          resolve(count);
-        }
-      },
-    });
-  });
-}
-
 var searchBar;
 var searchButton;
 var searchEntries = ["test"];
@@ -287,9 +265,9 @@ add_task(async function testRightClick() {
 add_task(async function testSearchHistory() {
   let textbox = searchBar._textbox;
   for (let i = 0; i < searchEntries.length; i++) {
-    let count = await countEntries(
+    let count = await FormHistoryTestUtils.count(
       textbox.getAttribute("autocompletesearchparam"),
-      searchEntries[i]
+      { value: searchEntries[i], source: "Bug 426329" }
     );
     ok(count > 0, "form history entry '" + searchEntries[i] + "' should exist");
   }
@@ -327,7 +305,9 @@ add_task(async function testClearHistory() {
   let historyCleared = promiseObserver("satchel-storage-changed");
   menuitem.click();
   await historyCleared;
-  let count = await countEntries();
+  let count = await FormHistoryTestUtils.count(
+    textbox.getAttribute("autocompletesearchparam")
+  );
   ok(count == 0, "History cleared");
 });
 

browser/components/urlbar/UrlbarInput.jsm

@@ -463,7 +463,11 @@ class UrlbarInput {
       );
       this._recordSearch(selectedOneOff.engine, event);
 
-      UrlbarUtils.addToFormHistory(this, searchString).catch(Cu.reportError);
+      UrlbarUtils.addToFormHistory(
+        this,
+        searchString,
+        selectedOneOff.engine.name
+      ).catch(Cu.reportError);
     } else {
       // Use the current value if we don't have a UrlbarResult e.g. because the
       // view is closed.
@@ -759,7 +763,8 @@ class UrlbarInput {
         if (!result.payload.inPrivateWindow) {
           UrlbarUtils.addToFormHistory(
             this,
-            result.payload.suggestion || result.payload.query
+            result.payload.suggestion || result.payload.query,
+            engine.name
           ).catch(Cu.reportError);
         }
         break;

browser/components/urlbar/UrlbarUtils.jsm

@@ -795,9 +795,11 @@ var UrlbarUtils = {
    * history for the search.
    * @param {UrlbarInput} input The UrlbarInput object requesting the addition.
    * @param {string} value The value to add.
+   * @param {string} [source] The source of the addition, usually
+   *        the name of the engine the search was made with.
    * @returns {Promise} resolved once the operation is complete
    */
-  addToFormHistory(input, value) {
+  addToFormHistory(input, value, source) {
     // If the user types a search engine alias without a search string,
     // we have an empty search string and we can't bump it.
     // We also don't want to add history in private browsing mode.
@@ -810,6 +812,7 @@ var UrlbarUtils = {
           op: "bump",
           fieldname: input.formHistoryName,
           value,
+          source,
         },
         {
           handleError: reject,

browser/components/urlbar/tests/UrlbarTestUtils.jsm

@@ -15,7 +15,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   BrowserTestUtils: "resource://testing-common/BrowserTestUtils.jsm",
   BrowserUtils: "resource://gre/modules/BrowserUtils.jsm",
   BrowserWindowTracker: "resource:///modules/BrowserWindowTracker.jsm",
-  FormHistory: "resource://gre/modules/FormHistory.jsm",
+  FormHistoryTestUtils: "resource://testing-common/FormHistoryTestUtils.jsm",
   setTimeout: "resource://gre/modules/Timer.jsm",
   TestUtils: "resource://testing-common/TestUtils.jsm",
   UrlbarController: "resource:///modules/UrlbarController.jsm",
@@ -509,58 +509,18 @@ var UrlbarTestUtils = {
 };
 
 UrlbarTestUtils.formHistory = {
-  /**
-   * Performs an operation on the urlbar's form history.
-   *
-   * @param {object} updateObject
-   *   An object describing the form history operation.  See FormHistory.jsm.
-   * @param {object} window
-   *   The window containing the urlbar.
-   */
-  async update(
-    updateObject = {},
-    window = BrowserWindowTracker.getTopWindow()
-  ) {
-    await new Promise((resolve, reject) => {
-      FormHistory.update(
-        Object.assign(
-          {
-            fieldname: this.getFormHistoryName(window),
-          },
-          updateObject
-        ),
-        {
-          handleError(error) {
-            reject(error);
-          },
-          handleCompletion(errored) {
-            if (!errored) {
-              resolve();
-            }
-          },
-        }
-      );
-    });
-  },
-
   /**
    * Adds values to the urlbar's form history.
    *
    * @param {array} values
-   *   The form history string values to remove.
+   *   The form history entries to remove.
    * @param {object} window
    *   The window containing the urlbar.
+   * @returns {Promise} resolved once the operation is complete.
    */
-  async add(values = [], window = BrowserWindowTracker.getTopWindow()) {
-    for (let value of values) {
-      await this.update(
-        {
-          value,
-          op: "bump",
-        },
-        window
-      );
-    }
+  add(values = [], window = BrowserWindowTracker.getTopWindow()) {
+    let fieldname = this.getFormHistoryName(window);
+    return FormHistoryTestUtils.add(fieldname, values);
   },
 
   /**
@@ -568,20 +528,14 @@ UrlbarTestUtils.formHistory = {
    * history, use clearFormHistory.
    *
    * @param {array} values
-   *   The form history string values to remove.
+   *   The form history entries to remove.
    * @param {object} window
    *   The window containing the urlbar.
+   * @returns {Promise} resolved once the operation is complete.
    */
-  async remove(values = [], window = BrowserWindowTracker.getTopWindow()) {
-    for (let value of values) {
-      await this.update(
-        {
-          value,
-          op: "remove",
-        },
-        window
-      );
-    }
+  remove(values = [], window = BrowserWindowTracker.getTopWindow()) {
+    let fieldname = this.getFormHistoryName(window);
+    return FormHistoryTestUtils.remove(fieldname, values);
   },
 
   /**
@@ -590,9 +544,11 @@ UrlbarTestUtils.formHistory = {
    *
    * @param {object} window
    *   The window containing the urlbar.
+   * @returns {Promise} resolved once the operation is complete.
    */
-  async clear(window = BrowserWindowTracker.getTopWindow()) {
-    await this.update({ op: "remove" }, window);
+  clear(window = BrowserWindowTracker.getTopWindow()) {
+    let fieldname = this.getFormHistoryName(window);
+    return FormHistoryTestUtils.clear(fieldname);
   },
 
   /**
@@ -606,31 +562,8 @@ UrlbarTestUtils.formHistory = {
    *   A promise resolved with an array of found form history entries.
    */
   search(criteria = {}, window = BrowserWindowTracker.getTopWindow()) {
-    return new Promise((resolve, reject) => {
-      let results = [];
-      FormHistory.search(
-        null,
-        Object.assign(
-          {
-            fieldname: this.getFormHistoryName(window),
-          },
-          criteria
-        ),
-        {
-          handleResult(result) {
-            results.push(result);
-          },
-          handleError(error) {
-            reject(error);
-          },
-          handleCompletion(errored) {
-            if (!errored) {
-              resolve(results);
-            }
-          },
-        }
-      );
-    });
+    let fieldname = this.getFormHistoryName(window);
+    return FormHistoryTestUtils.search(fieldname, criteria);
   },
 
   /**

browser/components/urlbar/tests/browser/browser_oneOffs_searchSuggestions.js

@@ -16,6 +16,9 @@ const serverInfo = {
   port: 20709, // Must be identical to what is in searchSuggestionEngine2.xml
 };
 
+var gEngine;
+var gEngine2;
+
 add_task(async function init() {
   await PlacesUtils.history.clear();
   await UrlbarTestUtils.formHistory.clear();
@@ -25,16 +28,16 @@ add_task(async function init() {
       ["browser.urlbar.maxHistoricalSearchSuggestions", 2],
     ],
   });
-  let engine = await SearchTestUtils.promiseNewSearchEngine(
+  gEngine = await SearchTestUtils.promiseNewSearchEngine(
     getRootDirectory(gTestPath) + TEST_ENGINE_BASENAME
   );
-  let engine2 = await SearchTestUtils.promiseNewSearchEngine(
+  gEngine2 = await SearchTestUtils.promiseNewSearchEngine(
     getRootDirectory(gTestPath) + TEST_ENGINE2_BASENAME
   );
   let oldDefaultEngine = await Services.search.getDefault();
-  await Services.search.moveEngine(engine2, 0);
-  await Services.search.moveEngine(engine, 0);
-  await Services.search.setDefault(engine);
+  await Services.search.moveEngine(gEngine2, 0);
+  await Services.search.moveEngine(gEngine, 0);
+  await Services.search.setDefault(gEngine);
   registerCleanupFunction(async function() {
     await Services.search.setDefault(oldDefaultEngine);
 
@@ -136,6 +139,7 @@ add_task(async function test_returnAfterSuggestion() {
     let entries = (
       await UrlbarTestUtils.formHistory.search({
         value: "foobar",
+        source: gEngine.name,
       })
     ).map(entry => entry.value);
     Assert.ok(entries.includes("foobar"));

browser/components/urlbar/tests/browser/browser_searchSuggestions.js

@@ -58,9 +58,9 @@ add_task(async function clickSuggestion() {
   EventUtils.synthesizeMouseAtCenter(element, {}, window);
   await loadPromise;
 
-  let formHistory = (await UrlbarTestUtils.formHistory.search()).map(
-    entry => entry.value
-  );
+  let formHistory = (
+    await UrlbarTestUtils.formHistory.search({ source: engineName })
+  ).map(entry => entry.value);
   Assert.deepEqual(
     formHistory,
     ["foofoo"],
@@ -113,9 +113,9 @@ async function testPressEnterOnSuggestion(
 
   if (!hasExpectedUrl) {
     await promiseFormHistory;
-    let formHistory = (await UrlbarTestUtils.formHistory.search()).map(
-      entry => entry.value
-    );
+    let formHistory = (
+      await UrlbarTestUtils.formHistory.search({ source: engineName })
+    ).map(entry => entry.value);
     Assert.deepEqual(
       formHistory,
       ["foofoo"],
@@ -284,9 +284,11 @@ add_task(async function heuristicAddsFormHistory() {
   await loadPromise;
 
   await formHistoryPromise;
-  formHistory = (await UrlbarTestUtils.formHistory.search()).map(
-    entry => entry.value
-  );
+  formHistory = (
+    await UrlbarTestUtils.formHistory.search({
+      source: result.searchParams.engine,
+    })
+  ).map(entry => entry.value);
   Assert.deepEqual(
     formHistory,
     ["foo"],

browser/modules/test/browser/browser_ContentSearch.js

@@ -250,7 +250,10 @@ add_task(
     // Add a form history suggestion and wait for Satchel to notify about it.
     sendEventToContent(browser, {
       type: "AddFormHistoryEntry",
-      data: searchStr + "form",
+      data: {
+        value: searchStr + "form",
+        engineName: engine.name,
+      },
     });
     await new Promise(resolve => {
       Services.obs.addObserver(function onAdd(subj, topic, data) {

toolkit/components/satchel/test/FormHistoryTestUtils.jsm

@@ -43,23 +43,23 @@ var FormHistoryTestUtils = {
    * Adds values to form history.
    *
    * @param {string} fieldname The field name.
-   * @param {array} additions Array of objects describing the values to add,
-   *   each object must have a value property and can have a source property.
+   * @param {array} additions Array of entries describing the values to add.
+   *   Each entry can either be a string, or an object with the shape
+   *   { value, source}.
    * @returns {Promise} Resolved once the operation is complete.
    */
-  add(fieldname, additions = []) {
-    let promises = [];
+  async add(fieldname, additions = []) {
+    // Additions are made one by one, so multiple identical entries are properly
+    // applied.
+    additions = additions.map(v => (typeof v == "string" ? { value: v } : v));
     for (let { value, source } of additions) {
-      promises.push(
-        new Promise((resolve, reject) => {
-          FormHistory.update(
-            Object.assign({ fieldname }, { op: "bump", value, source }),
-            this.makeListener(resolve, reject)
-          );
-        })
-      );
+      await new Promise((resolve, reject) => {
+        FormHistory.update(
+          Object.assign({ fieldname }, { op: "bump", value, source }),
+          this.makeListener(resolve, reject)
+        );
+      });
     }
-    return Promise.all(promises);
   },
 
   /**
@@ -84,26 +84,21 @@ var FormHistoryTestUtils = {
    * If you want to remove all history, use clear() instead.
    *
    * @param {string} fieldname The field name.
-   * @param {array} removals Array of objects describing the values to remove,
-   *   each object must have a value property and can have a source property.
-   *   If source is specified, only the source relation will be removed, while
-   *   the global form history value persists.
+   * @param {array} removals Array of entries describing the values to add.
+   *   Each entry can either be a string, or an object with the shape
+   *   { value, source}. If source is specified, only the source relation will
+   *   be removed, while the global form history value persists.
    * @param {object} window The window containing the urlbar.
    * @returns {Promise} Resolved once the operation is complete.
    */
   remove(fieldname, removals) {
-    let promises = [];
-    for (let { value, source } of removals) {
-      promises.push(
-        new Promise((resolve, reject) => {
-          FormHistory.update(
-            Object.assign({ fieldname }, { op: "remove", value, source }),
-            this.makeListener(resolve, reject)
-          );
-        })
-      );
-    }
-    return Promise.all(promises);
+    let changes = removals.map(v => {
+      let criteria = typeof v == "string" ? { value: v } : v;
+      return Object.assign({ fieldname, op: "remove" }, criteria);
+    });
+    return new Promise((resolve, reject) => {
+      FormHistory.update(changes, this.makeListener(resolve, reject));
+    });
   },
 
   /**