Bug 1654316 - Autofill is not properly considering bookmarked status of origins. r=adw

The old query was picking a random bookmarked status out of the pages in an origin,
depending on the physical position of the origin rows in the table.
The new query uses window functions to partition the data by origin, so it can
query origins just once and sum www and non-www origin's frecency scores before
comparing to the autofill threshold.

Differential Revision: https://phabricator.services.mozilla.com/D84657
This commit is contained in:
Marco Bonardo 2020-07-24 12:01:15 +00:00
Родитель 69914972f2
Коммит 48d23c80d5
6 изменённых файлов: 243 добавлений и 226 удалений

Просмотреть файл

@ -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);

Просмотреть файл

@ -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)
);
}

Просмотреть файл

@ -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);
});
}

Просмотреть файл

@ -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)
);
}

Просмотреть файл

@ -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)
);
}

Просмотреть файл

@ -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]