Bug 1493636 - Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold. r=mak
authorDrew Willcoxon <adw@mozilla.com>
Sat, 06 Oct 2018 00:22:04 +0000
changeset 495604 237c55ddb11256f42d744146f8b7f3dd11b7491b
parent 495603 75b958f179a8ba69ef1a3bc909ef741d0e8ec146
child 495605 f936a4baa698ac02c3ae215f61dee51c595cb37b
push id9984
push userffxbld-merge
push dateMon, 15 Oct 2018 21:07:35 +0000
treeherdermozilla-beta@183d27ea8570 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1493636
milestone64.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 1493636 - Always autofill bookmarks, regardless of their frecency and the autofill frecency threshold. r=mak Differential Revision: https://phabricator.services.mozilla.com/D7673
testing/firefox-ui/tests/functional/locationbar/test_escape_autocomplete.py
toolkit/components/places/UnifiedComplete.js
toolkit/components/places/tests/unifiedcomplete/autofill_tasks.js
toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
toolkit/components/places/tests/unifiedcomplete/test_autofill_origins.js
--- a/testing/firefox-ui/tests/functional/locationbar/test_escape_autocomplete.py
+++ b/testing/firefox-ui/tests/functional/locationbar/test_escape_autocomplete.py
@@ -17,17 +17,17 @@ class TestEscapeAutocomplete(PuppeteerMi
 
         self.test_urls = [
             'layout/mozilla.html',
             'layout/mozilla_community.html',
         ]
         self.test_urls = [self.marionette.absolute_url(t)
                           for t in self.test_urls]
 
-        self.test_string = 'mozilla'
+        self.test_string = 'mozilla.org/'
 
         self.locationbar = self.browser.navbar.locationbar
         self.autocomplete_results = self.locationbar.autocomplete_results
 
     def tearDown(self):
         self.autocomplete_results.close(force=True)
 
         super(TestEscapeAutocomplete, self).tearDown()
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -221,55 +221,61 @@ function originQuery(conditions = "", bo
                  IFNULL(:prefix, prefix) || moz_origins.host || '/',
                  frecency,
                  bookmarked,
                  id
           FROM (
             SELECT host,
                    host AS fixed_up_host,
                    TOTAL(frecency) AS host_frecency,
-                   ${bookmarkedFragment} AS bookmarked
+                   (
+                     SELECT TOTAL(foreign_count) > 0
+                     FROM moz_places
+                     WHERE moz_places.origin_id = moz_origins.id
+                   ) AS bookmarked
             FROM moz_origins
             WHERE host BETWEEN :searchString AND :searchString || X'FFFF'
                   ${conditions}
             GROUP BY host
             HAVING host_frecency >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD}
+                   OR bookmarked
             UNION ALL
             SELECT host,
                    fixup_url(host) AS fixed_up_host,
                    TOTAL(frecency) AS host_frecency,
-                   ${bookmarkedFragment} AS bookmarked
+                   (
+                     SELECT TOTAL(foreign_count) > 0
+                     FROM moz_places
+                     WHERE moz_places.origin_id = moz_origins.id
+                   ) AS bookmarked
             FROM moz_origins
             WHERE host BETWEEN 'www.' || :searchString AND 'www.' || :searchString || X'FFFF'
                   ${conditions}
             GROUP BY host
             HAVING host_frecency >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD}
+                   OR bookmarked
           ) AS grouped_hosts
           JOIN moz_origins ON moz_origins.host = grouped_hosts.host
           ORDER BY frecency DESC, id DESC
           LIMIT 1 `;
 }
 
 const SQL_ORIGIN_QUERY = originQuery();
 
 const SQL_ORIGIN_PREFIX_QUERY = originQuery(
   `AND prefix BETWEEN :prefix AND :prefix || X'FFFF'`
 );
 
 const SQL_ORIGIN_BOOKMARKED_QUERY = originQuery(
-  `AND bookmarked`,
-  `(SELECT TOTAL(foreign_count) > 0 FROM moz_places
-    WHERE moz_places.origin_id = moz_origins.id)`
+  `AND bookmarked`
 );
 
 const SQL_ORIGIN_PREFIX_BOOKMARKED_QUERY = originQuery(
   `AND bookmarked
    AND prefix BETWEEN :prefix AND :prefix || X'FFFF'`,
-  `(SELECT TOTAL(foreign_count) > 0 FROM moz_places
-    WHERE moz_places.origin_id = moz_origins.id)`
 );
 
 // Result row indexes for urlQuery()
 const QUERYINDEX_URL_URL = 1;
 const QUERYINDEX_URL_STRIPPED_URL = 2;
 const QUERYINDEX_URL_FRECENCY = 3;
 
 function urlQuery(conditions1, conditions2) {
@@ -278,29 +284,35 @@ function urlQuery(conditions1, condition
           SELECT :query_type,
                  url,
                  :strippedURL,
                  frecency,
                  foreign_count > 0 AS bookmarked,
                  id
           FROM moz_places
           WHERE rev_host = :revHost
-                AND MAX(frecency, 0) >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD}
+                AND (
+                  MAX(frecency, 0) >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD}
+                  OR bookmarked
+                )
                 AND hidden = 0
                 ${conditions1}
           UNION ALL
           SELECT :query_type,
                  url,
                  :strippedURL,
                  frecency,
                  foreign_count > 0 AS bookmarked,
                  id
           FROM moz_places
           WHERE rev_host = :revHost || 'www.'
-                AND MAX(frecency, 0) >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD}
+                AND (
+                  MAX(frecency, 0) >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD}
+                  OR bookmarked
+                )
                 AND hidden = 0
                 ${conditions2}
           ORDER BY frecency DESC, id DESC
           LIMIT 1 `;
 }
 
 const SQL_URL_QUERY = urlQuery(
   `AND strip_prefix_and_userinfo(url) BETWEEN :strippedURL AND :strippedURL || X'FFFF'`,
--- a/toolkit/components/places/tests/unifiedcomplete/autofill_tasks.js
+++ b/toolkit/components/places/tests/unifiedcomplete/autofill_tasks.js
@@ -596,19 +596,103 @@ function addAutofillTasks(origins) {
     await check_autocomplete({
       search,
       matches: [],
     });
 
     await cleanup();
   });
 
+  // Bookmarked places should always be autofilled, even when they don't meet
+  // the threshold.
+  add_task(async function bookmarkBelowThreshold() {
+    // Add some visits to a URL so that the origin autofill threshold is large.
+    for (let i = 0; i < 3; i++) {
+      await PlacesTestUtils.addVisits([{
+        uri: "http://not-" + url,
+      }]);
+    }
+
+    // Now bookmark another URL.
+    await addBookmark({
+      uri: "http://" + url,
+    });
+
+    // Make sure the bookmarked origin and place frecencies are below the
+    // threshold so that the origin/URL otherwise would not be autofilled.
+    let placeFrecency =
+      await PlacesTestUtils.fieldInDB("http://" + url, "frecency");
+    let originFrecency = await getOriginFrecency("http://", host);
+    let threshold = await getOriginAutofillThreshold();
+    Assert.ok(placeFrecency < threshold,
+              `Place frecency should be below the threshold: ` +
+              `placeFrecency=${placeFrecency} threshold=${threshold}`);
+    Assert.ok(originFrecency < threshold,
+              `Origin frecency should be below the threshold: ` +
+              `originFrecency=${originFrecency} threshold=${threshold}`);
+
+    // The bookmark should be autofilled.
+    await check_autocomplete({
+      search,
+      autofilled: url,
+      completed: "http://" + url,
+      matches: [
+        {
+          value: url,
+          comment,
+          style: ["autofill", "heuristic"],
+        },
+        {
+          value: "http://not-" + url,
+          comment: "test visit for http://not-" + url,
+          style: ["favicon"],
+        },
+      ],
+    });
+
+    await cleanup();
+  });
+
+  // Bookmarked places should be autofilled when they *do* meet the threshold.
+  add_task(async function bookmarkAboveThreshold() {
+    // Bookmark a URL.
+    await addBookmark({
+      uri: "http://" + url,
+    });
+
+    // The frecencies of the place and origin should be >= the threshold.  In
+    // fact they should be the same as the threshold since the place is the only
+    // place in the database.
+    let placeFrecency =
+      await PlacesTestUtils.fieldInDB("http://" + url, "frecency");
+    let originFrecency = await getOriginFrecency("http://", host);
+    let threshold = await getOriginAutofillThreshold();
+    Assert.equal(placeFrecency, threshold);
+    Assert.equal(originFrecency, threshold);
+
+    // The bookmark should be autofilled.
+    await check_autocomplete({
+      search,
+      autofilled: url,
+      completed: "http://" + url,
+      matches: [
+        {
+          value: url,
+          comment,
+          style: ["autofill", "heuristic"],
+        },
+      ],
+    });
+
+    await cleanup();
+  });
+
   // Autofill should respect the browser.urlbar.suggest.history pref -- i.e., it
   // should complete only bookmarked pages when that pref is false.
-  add_task(async function bookmarked() {
+  add_task(async function suggestHistoryFalse() {
     // Force only bookmarked pages to be suggested and therefore only bookmarked
     // pages to be completed.
     Services.prefs.setBoolPref("browser.urlbar.suggest.history", false);
 
     // Add a non-bookmarked page.  It should not be suggested or completed.
     await PlacesTestUtils.addVisits([{
       uri: "http://" + url,
     }]);
@@ -633,17 +717,17 @@ function addAutofillTasks(origins) {
         },
       ],
     });
 
     await cleanup();
   });
 
   // Same as previous but the search contains a prefix.
-  add_task(async function bookmarkedPrefix() {
+  add_task(async function suggestHistoryFalsePrefix() {
     // Force only bookmarked pages to be suggested and therefore only bookmarked
     // pages to be completed.
     Services.prefs.setBoolPref("browser.urlbar.suggest.history", false);
 
     // Add a non-bookmarked page.  It should not be suggested or completed.
     await PlacesTestUtils.addVisits([{
       uri: "http://" + url,
     }]);
@@ -687,37 +771,22 @@ function addAutofillTasks(origins) {
     // tests disable the idle service, so in practice it should always be -1,
     // but in order to avoid possible intermittent failures in the future, don't
     // assume that.)
     let placeFrecency =
       await PlacesTestUtils.fieldInDB("http://" + url, "frecency");
     Assert.ok(placeFrecency <= 0);
 
     // Make sure the origin's frecency is 0.
-    let db = await PlacesUtils.promiseDBConnection();
-    let rows = await db.execute(`
-      SELECT frecency FROM moz_origins WHERE host = '${host}';
-    `);
-    Assert.equal(rows.length, 1);
-    let originFrecency = rows[0].getResultByIndex(0);
+    let originFrecency = await getOriginFrecency("http://", host);
     Assert.equal(originFrecency, 0);
 
     // Make sure the autofill threshold is 0.
-    rows = await db.execute(`
-      SELECT
-        IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_count"), 0),
-        IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_sum"), 0),
-        IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_sum_of_squares"), 0)
-    `);
-    let count = rows[0].getResultByIndex(0);
-    let sum = rows[0].getResultByIndex(1);
-    let squares = rows[0].getResultByIndex(2);
-    Assert.equal(count, 0);
-    Assert.equal(sum, 0);
-    Assert.equal(squares, 0);
+    let threshold = await getOriginAutofillThreshold();
+    Assert.equal(threshold, 0);
 
     await check_autocomplete({
       search,
       autofilled: url,
       completed: "http://" + url,
       matches: [
         {
           value: url,
@@ -725,8 +794,67 @@ function addAutofillTasks(origins) {
           style: ["autofill", "heuristic"],
         },
       ],
     });
 
     await cleanup();
   });
 }
+
+
+/**
+ * Returns the frecency of an origin.
+ *
+ * @param   {string} prefix
+ *          The origin's prefix, e.g., "http://".
+ * @param   {string} host
+ *          The origin's host.
+ * @returns {number} The origin's frecency.
+ */
+async function getOriginFrecency(prefix, host) {
+  let db = await PlacesUtils.promiseDBConnection();
+  let rows = await db.execute(`
+    SELECT frecency
+    FROM moz_origins
+    WHERE prefix = :prefix AND host = :host
+  `, { prefix, host });
+  Assert.equal(rows.length, 1);
+  return rows[0].getResultByIndex(0);
+}
+
+/**
+ * Returns the origin frecency stats.
+ *
+ * @returns {object}
+ *          An object { count, sum, squares }.
+ */
+async function getOriginFrecencyStats() {
+  let db = await PlacesUtils.promiseDBConnection();
+  let rows = await db.execute(`
+    SELECT
+      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_count"), 0),
+      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_sum"), 0),
+      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_sum_of_squares"), 0)
+  `);
+  let count = rows[0].getResultByIndex(0);
+  let sum = rows[0].getResultByIndex(1);
+  let squares = rows[0].getResultByIndex(2);
+  return { count, sum, squares };
+}
+
+/**
+ * Returns the origin autofill frecency threshold.
+ *
+ * @returns {number}
+ *          The threshold.
+ */
+async function getOriginAutofillThreshold() {
+  let { count, sum, squares } = await getOriginFrecencyStats();
+  if (!count) {
+    return 0;
+  }
+  if (count == 1) {
+    return sum;
+  }
+  let stddevMultiplier = UrlbarPrefs.get("autoFill.stddevMultiplier");
+  return (sum / count) + (stddevMultiplier * Math.sqrt((squares - ((sum * sum) / count)) / count));
+}
--- a/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
+++ b/toolkit/components/places/tests/unifiedcomplete/head_autocomplete.js
@@ -20,18 +20,20 @@ ChromeUtils.import("resource://testing-c
   /* import-globals-from ./autofill_tasks.js */
   let file = do_get_file("autofill_tasks.js", false);
   let uri = Services.io.newFileURI(file);
   XPCOMUtils.defineLazyScriptGetter(this, "addAutofillTasks", uri.spec);
 }
 
 // Put any other stuff relative to this test folder below.
 
-ChromeUtils.defineModuleGetter(this, "UrlbarProviderOpenTabs",
-  "resource:///modules/UrlbarProviderOpenTabs.jsm");
+XPCOMUtils.defineLazyModuleGetters(this, {
+  UrlbarPrefs: "resource:///modules/UrlbarPrefs.jsm",
+  UrlbarProviderOpenTabs: "resource:///modules/UrlbarProviderOpenTabs.jsm",
+});
 
 const TITLE_SEARCH_ENGINE_SEPARATOR = " \u00B7\u2013\u00B7 ";
 
 async function cleanup() {
   Services.prefs.clearUserPref("browser.urlbar.autocomplete.enabled");
   Services.prefs.clearUserPref("browser.urlbar.autoFill");
   Services.prefs.clearUserPref("browser.urlbar.autoFill.searchEngines");
   let suggestPrefs = [
--- a/toolkit/components/places/tests/unifiedcomplete/test_autofill_origins.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_autofill_origins.js
@@ -196,35 +196,19 @@ add_task(async function groupByHost() {
   ]);
 
   let httpFrec = frecencyForUrl("http://example.com/");
   let httpsFrec = frecencyForUrl("https://example.com/");
   let otherFrec = frecencyForUrl("https://mozilla.org/");
   Assert.ok(httpFrec < httpsFrec, "Sanity check");
   Assert.ok(httpsFrec < otherFrec, "Sanity check");
 
-  // Compute the autofill threshold.
-  let db = await PlacesUtils.promiseDBConnection();
-  let rows = await db.execute(`
-    SELECT
-      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_count"), 0),
-      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_sum"), 0),
-      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_sum_of_squares"), 0)
-  `);
-  let count = rows[0].getResultByIndex(0);
-  let sum = rows[0].getResultByIndex(1);
-  let squares = rows[0].getResultByIndex(2);
-  let stddevMultiplier =
-    Services.prefs.getFloatPref("browser.urlbar.autoFill.stddevMultiplier", 0.0);
-  let threshold =
-    (sum / count) +
-    (stddevMultiplier * Math.sqrt((squares - ((sum * sum) / count)) / count));
-
   // Make sure the frecencies of the three origins are as expected in relation
   // to the threshold.
+  let threshold = await getOriginAutofillThreshold();
   Assert.ok(httpFrec < threshold, "http origin should be < threshold");
   Assert.ok(httpsFrec < threshold, "https origin should be < threshold");
   Assert.ok(threshold <= otherFrec, "Other origin should cross threshold");
 
   Assert.ok(threshold <= httpFrec + httpsFrec,
             "http and https origin added together should cross threshold");
 
   // The https origin should be autofilled.
@@ -277,33 +261,19 @@ add_task(async function groupByHostNonDe
   ]);
 
   let httpFrec = frecencyForUrl("http://example.com/");
   let httpsFrec = frecencyForUrl("https://example.com/");
   let otherFrec = frecencyForUrl("https://mozilla.org/");
   Assert.ok(httpFrec < httpsFrec, "Sanity check");
   Assert.ok(httpsFrec < otherFrec, "Sanity check");
 
-  // Compute the autofill threshold.
-  let db = await PlacesUtils.promiseDBConnection();
-  let rows = await db.execute(`
-    SELECT
-      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_count"), 0),
-      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_sum"), 0),
-      IFNULL((SELECT value FROM moz_meta WHERE key = "origin_frecency_sum_of_squares"), 0)
-  `);
-  let count = rows[0].getResultByIndex(0);
-  let sum = rows[0].getResultByIndex(1);
-  let squares = rows[0].getResultByIndex(2);
-  let threshold =
-    (sum / count) +
-    (stddevMultiplier * Math.sqrt((squares - ((sum * sum) / count)) / count));
-
   // Make sure the frecencies of the three origins are as expected in relation
   // to the threshold.
+  let threshold = await getOriginAutofillThreshold();
   Assert.ok(httpFrec < threshold, "http origin should be < threshold");
   Assert.ok(httpsFrec < threshold, "https origin should be < threshold");
   Assert.ok(threshold <= otherFrec, "Other origin should cross threshold");
 
   Assert.ok(threshold <= httpFrec + httpsFrec,
             "http and https origin added together should cross threshold");
 
   // The https origin should be autofilled.