diff --git a/browser/components/urlbar/UrlbarProviderAutofill.jsm b/browser/components/urlbar/UrlbarProviderAutofill.jsm index 0a96cb0f2dac..78404452cd10 100644 --- a/browser/components/urlbar/UrlbarProviderAutofill.jsm +++ b/browser/components/urlbar/UrlbarProviderAutofill.jsm @@ -24,35 +24,6 @@ XPCOMUtils.defineLazyModuleGetters(this, { UrlbarUtils: "resource:///modules/UrlbarUtils.jsm", }); -// Sqlite result row index constants. -const QUERYINDEX = { - QUERYTYPE: 0, - URL: 1, - TITLE: 2, - BOOKMARKED: 3, - BOOKMARKTITLE: 4, - TAGS: 5, - VISITCOUNT: 6, - TYPED: 7, - PLACEID: 8, - SWITCHTAB: 9, - FRECENCY: 10, -}; - -// Result row indexes for originQuery() -const QUERYINDEX_ORIGIN = { - AUTOFILLED_VALUE: 1, - URL: 2, - FRECENCY: 3, -}; - -// Result row indexes for urlQuery() -const QUERYINDEX_URL = { - URL: 1, - STRIPPED_URL: 2, - FRECENCY: 3, -}; - // AutoComplete query type constants. // Describes the various types of queries that we can process rows for. const QUERYTYPE = { @@ -89,48 +60,42 @@ 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 || '/', - frecency, - bookmarked, - id - FROM ( - SELECT host, - 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 :searchString AND :searchString || X'FFFF' - ${where} - GROUP BY host - 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 ${having} - ) AS grouped_hosts - JOIN moz_origins ON moz_origins.host = grouped_hosts.host - ORDER BY frecency DESC, id DESC - LIMIT 1 `; +function originQuery(where) { + // `frecency`, `bookmarked` and `visited` are partitioned by the fixed host, + // without `www.`. `host_prefix` instead is partitioned by full host, because + // we assume a prefix may not work regardless of `www.`. + let selectVisited = where.includes("visited") + ? `MAX(EXISTS( + SELECT 1 FROM moz_places WHERE origin_id = o.id AND visit_count > 0 + )) OVER (PARTITION BY fixup_url(host)) > 0` + : "0"; + return `/* do not warn (bug no): cannot use an index to sort */ + ${SQL_AUTOFILL_WITH}, + origins(id, prefix, host_prefix, host, fixed, host_frecency, frecency, bookmarked, visited) AS ( + SELECT + id, + prefix, + first_value(prefix) OVER (PARTITION BY host ORDER BY frecency DESC), + host, + fixup_url(host), + TOTAL(frecency) OVER (PARTITION BY fixup_url(host)), + frecency, + MAX(EXISTS( + SELECT 1 FROM moz_places WHERE origin_id = o.id AND foreign_count > 0 + )) OVER (PARTITION BY fixup_url(host)), + ${selectVisited} + FROM moz_origins o + WHERE (host BETWEEN :searchString AND :searchString || X'FFFF') + OR (host BETWEEN 'www.' || :searchString AND 'www.' || :searchString || X'FFFF') + ) + SELECT :query_type AS query_type, + iif(instr(:searchString, "www."), host, fixed) || '/' AS host_fixed, + ifnull(:prefix, host_prefix) || host || '/' AS url + FROM origins + ${where} + ORDER BY frecency DESC, id DESC + LIMIT 1 + `; } function urlQuery(where1, where2) { @@ -139,9 +104,9 @@ function urlQuery(where1, where2) { // rows as possible. Keep in mind that we run this query every time the user // types a key when the urlbar value looks like a URL with a path. return `/* do not warn (bug no): cannot use an index to sort */ - SELECT :query_type, + SELECT :query_type AS query_type, url, - :strippedURL, + :strippedURL AS stripped_url, frecency, foreign_count > 0 AS bookmarked, visit_count > 0 AS visited, @@ -150,9 +115,9 @@ function urlQuery(where1, where2) { WHERE rev_host = :revHost ${where1} UNION ALL - SELECT :query_type, + SELECT :query_type AS query_type, url, - :strippedURL, + :strippedURL AS stripped_url, frecency, foreign_count > 0 AS bookmarked, visit_count > 0 AS visited, @@ -164,42 +129,29 @@ function urlQuery(where1, where2) { LIMIT 1 `; } // Queries -const QUERY_ORIGIN_HISTORY_BOOKMARK = originQuery({ - having: `bookmarked OR ${SQL_AUTOFILL_FRECENCY_THRESHOLD}`, -}); +const QUERY_ORIGIN_HISTORY_BOOKMARK = originQuery( + `WHERE bookmarked OR ${SQL_AUTOFILL_FRECENCY_THRESHOLD}` +); -const QUERY_ORIGIN_PREFIX_HISTORY_BOOKMARK = originQuery({ - where: `AND prefix BETWEEN :prefix AND :prefix || X'FFFF'`, - having: `bookmarked OR ${SQL_AUTOFILL_FRECENCY_THRESHOLD}`, -}); +const QUERY_ORIGIN_PREFIX_HISTORY_BOOKMARK = originQuery( + `WHERE prefix BETWEEN :prefix AND :prefix || X'FFFF' + AND (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: `visited AND ${SQL_AUTOFILL_FRECENCY_THRESHOLD}`, -}); +const QUERY_ORIGIN_HISTORY = originQuery( + `WHERE 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: `visited AND ${SQL_AUTOFILL_FRECENCY_THRESHOLD}`, -}); +const QUERY_ORIGIN_PREFIX_HISTORY = originQuery( + `WHERE prefix BETWEEN :prefix AND :prefix || X'FFFF' + AND visited AND ${SQL_AUTOFILL_FRECENCY_THRESHOLD}` +); -const QUERY_ORIGIN_BOOKMARK = originQuery({ - having: `bookmarked`, -}); +const QUERY_ORIGIN_BOOKMARK = originQuery(`WHERE bookmarked`); -const QUERY_ORIGIN_PREFIX_BOOKMARK = originQuery({ - where: `AND prefix BETWEEN :prefix AND :prefix || X'FFFF'`, - having: `bookmarked`, -}); +const QUERY_ORIGIN_PREFIX_BOOKMARK = originQuery( + `WHERE prefix BETWEEN :prefix AND :prefix || X'FFFF' AND bookmarked` +); const QUERY_URL_HISTORY_BOOKMARK = urlQuery( `AND (bookmarked OR frecency > 20) @@ -550,18 +502,16 @@ class ProviderAutofill extends UrlbarProvider { * @param {UrlbarQueryContext} queryContext */ _onResultRow(row, cancel, queryContext) { - let queryType = row.getResultByIndex(QUERYINDEX.QUERYTYPE); + let queryType = row.getResultByName("query_type"); let autofilledValue, finalCompleteValue; switch (queryType) { case QUERYTYPE.AUTOFILL_ORIGIN: - autofilledValue = row.getResultByIndex( - QUERYINDEX_ORIGIN.AUTOFILLED_VALUE - ); - finalCompleteValue = row.getResultByIndex(QUERYINDEX_ORIGIN.URL); + autofilledValue = row.getResultByName("host_fixed"); + finalCompleteValue = row.getResultByName("url"); break; case QUERYTYPE.AUTOFILL_URL: - let url = row.getResultByIndex(QUERYINDEX_URL.URL); - let strippedURL = row.getResultByIndex(QUERYINDEX_URL.STRIPPED_URL); + let url = row.getResultByName("url"); + let strippedURL = row.getResultByName("stripped_url"); // We autofill urls to-the-next-slash. // http://mozilla.org/foo/bar/baz will be autofilled to: // - http://mozilla.org/f[oo/] @@ -690,7 +640,7 @@ class ProviderAutofill extends UrlbarProvider { [query, params] = this._getUrlQuery(queryContext); } - // _getrlQuery doesn't always return a query. + // _getUrlQuery doesn't always return a query. if (query) { await conn.executeCached(query, params, (row, cancel) => { this._onResultRow(row, cancel, queryContext); diff --git a/browser/components/urlbar/tests/unit/head.js b/browser/components/urlbar/tests/unit/head.js index 33085d856260..e0e978ee3c64 100644 --- a/browser/components/urlbar/tests/unit/head.js +++ b/browser/components/urlbar/tests/unit/head.js @@ -757,12 +757,14 @@ async function check_results({ ); } } - - Assert.equal( - context.results.length, - matches.length, - "Found the expected number of results." - ); + if (context.results.length != matches.length) { + info("Returned results: " + JSON.stringify(context.results)); + Assert.equal( + context.results.length, + matches.length, + "Found the expected number of results." + ); + } Assert.deepEqual( matches.map(m => m.payload), @@ -786,3 +788,67 @@ async function check_results({ } }); } + +/** + * Returns the frecency of an origin. + * + * @param {string} prefix + * The origin's prefix, e.g., "http://". + * @param {string} aHost + * The origin's host. + * @returns {number} The origin's frecency. + */ +async function getOriginFrecency(prefix, aHost) { + let db = await PlacesUtils.promiseDBConnection(); + let rows = await db.execute( + ` + SELECT frecency + FROM moz_origins + WHERE prefix = :prefix AND host = :host + `, + { prefix, host: aHost } + ); + 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) + ); +} diff --git a/browser/components/urlbar/tests/unit/test_autofill_bookmarked.js b/browser/components/urlbar/tests/unit/test_autofill_bookmarked.js new file mode 100644 index 000000000000..27bed75d56b3 --- /dev/null +++ b/browser/components/urlbar/tests/unit/test_autofill_bookmarked.js @@ -0,0 +1,106 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +// This is a specific autofill test to ensure we pick the correct bookmarked +// state of an origin. Regardless of the order of origins, we should always pick +// the correct bookmarked status. + +add_task(async function() { + registerCleanupFunction(async () => { + Services.prefs.clearUserPref("browser.urlbar.suggest.searches"); + }); + Services.prefs.setBoolPref("browser.urlbar.suggest.searches", false); + + let host = "example.com"; + // Add a bookmark to the http version, but ensure the https version has an + // higher frecency. + let bookmark = await PlacesUtils.bookmarks.insert({ + url: `http://${host}`, + parentGuid: PlacesUtils.bookmarks.unfiledGuid, + }); + for (let i = 0; i < 3; i++) { + await PlacesTestUtils.addVisits(`https://${host}`); + } + // ensure both fall below the threshold. + for (let i = 0; i < 15; i++) { + await PlacesTestUtils.addVisits(`https://not-${host}`); + } + + async function check_autofill() { + let threshold = await getOriginAutofillThreshold(); + let httpOriginFrecency = await getOriginFrecency("http://", host); + Assert.less( + httpOriginFrecency, + threshold, + "Http origin frecency should be below the threshold" + ); + let httpsOriginFrecency = await getOriginFrecency("https://", host); + Assert.less( + httpsOriginFrecency, + threshold, + "Https origin frecency should be below the threshold" + ); + Assert.less( + httpOriginFrecency, + httpsOriginFrecency, + "Http origin frecency should be below the https origin frecency" + ); + + // The http version should be filled because it's bookmarked, but with the + // https prefix that is more frecent. + let context = createContext("ex", { isPrivate: false }); + await check_results({ + context, + autofilled: `${host}/`, + completed: `https://${host}/`, + matches: [ + makeVisitResult(context, { + uri: `https://${host}/`, + title: `https://${host}`, + heuristic: true, + }), + makeVisitResult(context, { + uri: `https://not-${host}/`, + title: `test visit for https://not-${host}/`, + }), + ], + }); + } + + await check_autofill(); + + // Now remove the bookmark, ensure to remove the orphans, then reinsert the + // bookmark; thus we physically invert the order of the rows in the table. + await checkOriginsOrder(host, ["http://", "https://"]); + await PlacesUtils.bookmarks.remove(bookmark); + await PlacesUtils.withConnectionWrapper("removeOrphans", async db => { + db.execute(`DELETE FROM moz_places WHERE url = :url`, { + url: `http://${host}/`, + }); + db.execute( + `DELETE FROM moz_origins WHERE prefix = "http://" AND host = :host`, + { host } + ); + }); + await PlacesUtils.bookmarks.insert({ + url: `http://${host}`, + parentGuid: PlacesUtils.bookmarks.unfiledGuid, + }); + + await checkOriginsOrder(host, ["https://", "http://"]); + + await check_autofill(); +}); + +async function checkOriginsOrder(host, prefixOrder) { + await PlacesUtils.withConnectionWrapper("removeOrphans", async db => { + let prefixes = ( + await db.execute( + `SELECT prefix FROM moz_origins WHERE host = :host ORDER BY ROWID`, + { host } + ) + ).map(r => r.getResultByIndex(0)); + Assert.deepEqual(prefixes, prefixOrder); + }); +} diff --git a/browser/components/urlbar/tests/unit/test_autofill_origins.js b/browser/components/urlbar/tests/unit/test_autofill_origins.js index ac22a7408a2a..f31969137497 100644 --- a/browser/components/urlbar/tests/unit/test_autofill_origins.js +++ b/browser/components/urlbar/tests/unit/test_autofill_origins.js @@ -633,44 +633,3 @@ add_task(async function suggestHistoryFalse_bookmark_prefix_multiple() { await cleanup(); }); - -/** - * 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) - ); -} diff --git a/browser/components/urlbar/tests/unit/test_autofill_originsAndQueries.js b/browser/components/urlbar/tests/unit/test_autofill_originsAndQueries.js index f44e358cf46d..28df201423bc 100644 --- a/browser/components/urlbar/tests/unit/test_autofill_originsAndQueries.js +++ b/browser/components/urlbar/tests/unit/test_autofill_originsAndQueries.js @@ -2354,68 +2354,3 @@ add_autofill_task( await cleanup(); } ); -// } - -/** - * Returns the frecency of an origin. - * - * @param {string} prefix - * The origin's prefix, e.g., "http://". - * @param {string} aHost - * The origin's host. - * @returns {number} The origin's frecency. - */ -async function getOriginFrecency(prefix, aHost) { - let db = await PlacesUtils.promiseDBConnection(); - let rows = await db.execute( - ` - SELECT frecency - FROM moz_origins - WHERE prefix = :prefix AND host = :host - `, - { prefix, host: aHost } - ); - 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) - ); -} diff --git a/browser/components/urlbar/tests/unit/xpcshell.ini b/browser/components/urlbar/tests/unit/xpcshell.ini index 8708748810fb..4ea8118c1bdc 100644 --- a/browser/components/urlbar/tests/unit/xpcshell.ini +++ b/browser/components/urlbar/tests/unit/xpcshell.ini @@ -6,6 +6,7 @@ support-files = data/engine-tail-suggestions.xml [test_autofill_about_urls.js] +[test_autofill_bookmarked.js] [test_autofill_functional.js] [test_autofill_origins.js] [test_autofill_originsAndQueries.js]