Bug 1602728 - Always autofill bookmarks when suggest.history=false and suggest.bookmark=true regardless of frecency threshold. r=mak a=RyanVM
authorDrew Willcoxon <adw@mozilla.com>
Tue, 14 Jan 2020 16:48:42 +0000
changeset 571173 320b0ab4cfe30e63ee86b3a2a1ee9e2bbc83fd25
parent 571172 b267babdbb3a81b815de9a4b56242d68d0e5f014
child 571174 52fba9ebcb6f9f1460edc899a24b4fd41517cac6
push id12554
push usercbrindusan@mozilla.com
push dateWed, 15 Jan 2020 02:09:58 +0000
treeherdermozilla-beta@087b0cab93cc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, RyanVM
bugs1602728
milestone73.0
Bug 1602728 - Always autofill bookmarks when suggest.history=false and suggest.bookmark=true regardless of frecency threshold. r=mak a=RyanVM Differential Revision: https://phabricator.services.mozilla.com/D59438
toolkit/components/places/UnifiedComplete.jsm
toolkit/components/places/tests/unifiedcomplete/autofill_tasks.js
--- a/toolkit/components/places/UnifiedComplete.jsm
+++ b/toolkit/components/places/UnifiedComplete.jsm
@@ -166,17 +166,17 @@ const SQL_AUTOFILL_WITH = `
       WHEN 0 THEN 0.0
       WHEN 1 THEN sum
       ELSE (sum / count) + (:stddevMultiplier * sqrt((squares - ((sum * sum) / count)) / count))
       END
     FROM frecency_stats
   )
 `;
 
-const SQL_AUTOFILL_FRECENCY_THRESHOLD = `(
+const SQL_AUTOFILL_FRECENCY_THRESHOLD = `host_frecency >= (
   SELECT value FROM autofill_frecency_threshold
 )`;
 
 function originQuery({ select = "", where = "", having = "" }) {
   return `${SQL_AUTOFILL_WITH}
           SELECT :query_type,
                  fixed_up_host || '/',
                  IFNULL(:prefix, prefix) || moz_origins.host || '/',
@@ -192,75 +192,73 @@ function originQuery({ select = "", wher
                      FROM moz_places
                      WHERE moz_places.origin_id = moz_origins.id
                    ) AS bookmarked
                    ${select}
             FROM moz_origins
             WHERE host BETWEEN :searchString AND :searchString || X'FFFF'
                   ${where}
             GROUP BY host
-            HAVING host_frecency >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD}
-                   ${having}
+            HAVING ${having}
             UNION ALL
             SELECT host,
                    fixup_url(host) AS fixed_up_host,
                    TOTAL(frecency) AS host_frecency,
                    (
                      SELECT TOTAL(foreign_count) > 0
                      FROM moz_places
                      WHERE moz_places.origin_id = moz_origins.id
                    ) AS bookmarked
                    ${select}
             FROM moz_origins
             WHERE host BETWEEN 'www.' || :searchString AND 'www.' || :searchString || X'FFFF'
                   ${where}
             GROUP BY host
-            HAVING host_frecency >= ${SQL_AUTOFILL_FRECENCY_THRESHOLD}
-                   ${having}
+            HAVING ${having}
           ) AS grouped_hosts
           JOIN moz_origins ON moz_origins.host = grouped_hosts.host
           ORDER BY frecency DESC, id DESC
           LIMIT 1 `;
 }
 
 const QUERY_ORIGIN_HISTORY_BOOKMARK = originQuery({
-  having: `OR bookmarked`,
+  having: `bookmarked OR ${SQL_AUTOFILL_FRECENCY_THRESHOLD}`,
 });
 
 const QUERY_ORIGIN_PREFIX_HISTORY_BOOKMARK = originQuery({
   where: `AND prefix BETWEEN :prefix AND :prefix || X'FFFF'`,
-  having: `OR bookmarked`,
+  having: `bookmarked OR ${SQL_AUTOFILL_FRECENCY_THRESHOLD}`,
 });
 
 const QUERY_ORIGIN_HISTORY = originQuery({
   select: `, (
     SELECT TOTAL(visit_count) > 0
     FROM moz_places
     WHERE moz_places.origin_id = moz_origins.id
    ) AS visited`,
-  having: `AND (visited OR NOT bookmarked)`,
+  having: `visited AND ${SQL_AUTOFILL_FRECENCY_THRESHOLD}`,
 });
 
 const QUERY_ORIGIN_PREFIX_HISTORY = originQuery({
   select: `, (
     SELECT TOTAL(visit_count) > 0
     FROM moz_places
     WHERE moz_places.origin_id = moz_origins.id
    ) AS visited`,
   where: `AND prefix BETWEEN :prefix AND :prefix || X'FFFF'`,
-  having: `AND (visited OR NOT bookmarked)`,
+  having: `visited AND ${SQL_AUTOFILL_FRECENCY_THRESHOLD}`,
 });
 
 const QUERY_ORIGIN_BOOKMARK = originQuery({
-  having: `AND bookmarked`,
+  having: `bookmarked`,
 });
 
 const QUERY_ORIGIN_PREFIX_BOOKMARK = originQuery({
   where: `AND prefix BETWEEN :prefix AND :prefix || X'FFFF'`,
-  having: `AND bookmarked`,
+  having: `bookmarked`,
 });
 
 // Result row indexes for urlQuery()
 const QUERYINDEX_URL_URL = 1;
 const QUERYINDEX_URL_STRIPPED_URL = 2;
 const QUERYINDEX_URL_FRECENCY = 3;
 
 function urlQuery(where1, where2) {
--- a/toolkit/components/places/tests/unifiedcomplete/autofill_tasks.js
+++ b/toolkit/components/places/tests/unifiedcomplete/autofill_tasks.js
@@ -892,20 +892,40 @@ function addAutofillTasks(origins) {
   //   search for: bookmark
   //   prefix search: no
   //   prefix matches search: n/a
   //   origin matches search: yes
   //
   // Expected result:
   //   should autofill: yes
   add_task(async function suggestHistoryFalse_bookmark_0() {
-    Services.prefs.setBoolPref("browser.urlbar.suggest.history", false);
+    // Add the bookmark.
     await addBookmark({
       uri: "http://" + url,
     });
+
+    // Make the bookmark fall below the autofill frecency threshold so we ensure
+    // the bookmark is always autofilled in this case, even if it doesn't meet
+    // the threshold.
+    let meetsThreshold = true;
+    while (meetsThreshold) {
+      // Add a visit to another origin to boost the threshold.
+      await PlacesTestUtils.addVisits("http://foo-" + url);
+      let originFrecency = await getOriginFrecency("http://", host);
+      let threshold = await getOriginAutofillThreshold();
+      meetsThreshold = threshold <= originFrecency;
+    }
+
+    // At this point, the bookmark doesn't meet the threshold, but it should
+    // still be autofilled.
+    let originFrecency = await getOriginFrecency("http://", host);
+    let threshold = await getOriginAutofillThreshold();
+    Assert.ok(originFrecency < threshold);
+
+    Services.prefs.setBoolPref("browser.urlbar.suggest.history", false);
     await check_autocomplete({
       search,
       autofilled: url,
       completed: "http://" + url,
       matches: [
         {
           value: url,
           comment,
@@ -954,16 +974,39 @@ function addAutofillTasks(origins) {
   //   search for: bookmark
   //   prefix search: yes
   //   prefix matches search: yes
   //   origin matches search: yes
   //
   // Expected result:
   //   should autofill: yes
   add_task(async function suggestHistoryFalse_bookmark_prefix_0() {
+    // Add the bookmark.
+    await addBookmark({
+      uri: "http://" + url,
+    });
+
+    // Make the bookmark fall below the autofill frecency threshold so we ensure
+    // the bookmark is always autofilled in this case, even if it doesn't meet
+    // the threshold.
+    let meetsThreshold = true;
+    while (meetsThreshold) {
+      // Add a visit to another origin to boost the threshold.
+      await PlacesTestUtils.addVisits("http://foo-" + url);
+      let originFrecency = await getOriginFrecency("http://", host);
+      let threshold = await getOriginAutofillThreshold();
+      meetsThreshold = threshold <= originFrecency;
+    }
+
+    // At this point, the bookmark doesn't meet the threshold, but it should
+    // still be autofilled.
+    let originFrecency = await getOriginFrecency("http://", host);
+    let threshold = await getOriginAutofillThreshold();
+    Assert.ok(originFrecency < threshold);
+
     Services.prefs.setBoolPref("browser.urlbar.suggest.history", false);
     await addBookmark({
       uri: "http://" + url,
     });
     await check_autocomplete({
       search: "http://" + search,
       autofilled: "http://" + url,
       completed: "http://" + url,