Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-10-19 08:15:31 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1694237 - Properly discard duplicate suggestions in urlbar results. r=mak

Browse Source 
This was a little more work than I first thought because fixing it uncovered
another problem: The recurse logic in the muxer isn't quite right. There are a
couple of problems actually:

1. By re-using the results from children that did not fill up, we skip updating
   `stateCopy` (and therefore `state`), which messes up subsequent buckets in
   the recursion because they're working with the wrong state.
2. By simply assigning `state = stateCopy` after handling children that didn't
   fill up, we're not really doing anything because at that point the function
   is done. The caller and subsequent buckets in the recursion won't see the
   updated state. We need to update `state` in place.

These problems were revealed in test_resultBuckets.js, which is pretty thorough.

To fix the actual problem that the bug is about (not deduping remote suggestions
and form history), we just need to keep a set of suggestions that the muxer has
seen so far, and then `_canAddResult` can discard dupe suggestions. This patch
adds `state.suggestions` for that, and it includes form history, remote
suggestions, and the heuristic query when it's a search result. This way the
relative ordering of form history vs. remote suggestions doesn't matter. We'll
dedupe whichever comes later.

A bunch of tasks in test_resultBuckets.js needed to be updated to account for
this because they were incorrectly not expecting dupes to be removed.

Differential Revision: https://phabricator.services.mozilla.com/D106030
This commit is contained in:
Drew Willcoxon 2021-02-23 17:17:31 +00:00
parent e6aa1d9959
commit 80e0f95944
3 changed files with 98 additions and 56 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
browser/components/urlbar/UrlbarMuxerUnifiedComplete.jsm
View File

@ -62,7 +62,10 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
strippedUrlToTopPrefixAndTitle: new Map(),
canShowPrivateSearch: context.results.length > 1,
canShowTailSuggestions: true,
formHistorySuggestions: new Set(),
// Form history and remote suggestions added so far. Used for deduping
// suggestions. Also includes the heuristic query string if the heuristic
// is a search result. All strings in the set are lowercased.
suggestions: new Set(),
canAddTabToSearch: true,
// When you add state, update _copyState() as necessary.
};
@ -139,11 +142,11 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
*/
_copyState(state) {
let copy = Object.assign({}, state, {
formHistorySuggestions: new Set(state.formHistorySuggestions),
resultsByGroup: new Map(),
strippedUrlToTopPrefixAndTitle: new Map(
state.strippedUrlToTopPrefixAndTitle
),
suggestions: new Set(state.suggestions),
});
for (let [group, results] of state.resultsByGroup) {
copy.resultsByGroup.set(group, [...results]);
@ -172,7 +175,6 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
// Set up some flex state for the bucket.
let stateCopy;
let flexSum = 0;
let childResultsByIndex = [];
let unfilledChildIndexes = [];
let unfilledChildResultCount = 0;
if (bucket.flexChildren) {
@ -213,7 +215,6 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
// Recurse and fill the child bucket.
let childResults = this._fillBuckets(child, childMaxResultCount, state);
childResultsByIndex.push(childResults);
results = results.concat(childResults);
if (bucket.flexChildren && childResults.length < childMaxResultCount) {
@ -237,17 +238,23 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
i < bucket.children.length && results.length < maxResultCount;
i++
) {
if (unfilledChildIndexes.length && i == unfilledChildIndexes[0]) {
// This is one of the children that didn't fill up.
unfilledChildIndexes.shift();
results = results.concat(childResultsByIndex[i]);
continue;
}
let child = bucket.children[i];
let flex = typeof child.flex == "number" ? child.flex : 0;
let childMaxResultCount = flex
? Math.round(remainingResultCount * (flex / flexSumFilled))
: remainingResultCount;
let childMaxResultCount;
if (unfilledChildIndexes.length && i == unfilledChildIndexes[0]) {
// This is one of the children that didn't fill up. Since it didn't
// fill up, the max result count to use in this pass isn't important
// as long as it's >= the number of results it was able to fill. We
// can't re-use its results from the first pass (even though they're
// still correct) because we need to properly update `stateCopy` and
// therefore re-fill the child.
unfilledChildIndexes.shift();
childMaxResultCount = maxResultCount;
} else {
let flex = typeof child.flex == "number" ? child.flex : 0;
childMaxResultCount = flex
? Math.round(remainingResultCount * (flex / flexSumFilled))
: remainingResultCount;
}
let childResults = this._fillBuckets(
child,
childMaxResultCount,
@ -255,7 +262,11 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
);
results = results.concat(childResults);
}
state = stateCopy;
// Update `state` in place so that it's also updated in the caller.
for (let [key, value] of Object.entries(stateCopy)) {
state[key] = value;
}
}
return results;
@ -438,23 +449,12 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
return false;
}
// Discard form history that dupes the heuristic or previous added form
// history (for restyleSearch).
// Discard form history and remote suggestions that dupe previously added
// suggestions or the heuristic.
if (
result.type == UrlbarUtils.RESULT_TYPE.SEARCH &&
result.source == UrlbarUtils.RESULT_SOURCE.HISTORY &&
(result.payload.lowerCaseSuggestion === state.heuristicResultQuery ||
state.formHistorySuggestions.has(result.payload.lowerCaseSuggestion))
) {
return false;
}
// Discard remote search suggestions that dupe the heuristic.
if (
result.type == UrlbarUtils.RESULT_TYPE.SEARCH &&
result.source == UrlbarUtils.RESULT_SOURCE.SEARCH &&
result.payload.lowerCaseSuggestion &&
result.payload.lowerCaseSuggestion === state.heuristicResultQuery
state.suggestions.has(result.payload.lowerCaseSuggestion)
) {
return false;
}
@ -469,7 +469,7 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
}
// Discard SERPs from browser history that dupe either the heuristic or
// previously added form history.
// previously added suggestions.
if (
result.source == UrlbarUtils.RESULT_SOURCE.HISTORY &&
result.type == UrlbarUtils.RESULT_TYPE.URL
@ -477,10 +477,7 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
let submission = Services.search.parseSubmissionURL(result.payload.url);
if (submission) {
let resultQuery = submission.terms.toLocaleLowerCase();
if (
state.heuristicResultQuery === resultQuery ||
state.formHistorySuggestions.has(resultQuery)
) {
if (state.suggestions.has(resultQuery)) {
// If the result's URL is the same as a brand new SERP URL created
// from the query string modulo certain URL params, then treat the
// result as a dupe and discard it.
@ -598,7 +595,7 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
result.type == UrlbarUtils.RESULT_TYPE.SEARCH &&
result.payload.query
) {
state.heuristicResultQuery = result.payload.query.toLocaleLowerCase();
state.suggestions.add(result.payload.query.toLocaleLowerCase());
}
}
@ -614,12 +611,12 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
state.canShowPrivateSearch = false;
}
// Update form history suggestions.
// Update suggestions.
if (
result.type == UrlbarUtils.RESULT_TYPE.SEARCH &&
result.source == UrlbarUtils.RESULT_SOURCE.HISTORY
result.payload.lowerCaseSuggestion
) {
state.formHistorySuggestions.add(result.payload.lowerCaseSuggestion);
state.suggestions.add(result.payload.lowerCaseSuggestion);
}
// Avoid multiple tab-to-search results.

48
browser/components/urlbar/tests/unit/test_resultBuckets.js
View File

@ -627,7 +627,9 @@ add_resultBuckets_task({
// remote suggestions: round(10 * (1 / 3)) = 3
...makeIndexRange(MAX_RESULTS, 3),
// form history: round(10 * (1 / 3)) = 3
...makeIndexRange(0, 3),
// The first three form history results dupe the three remote suggestions,
// so they should not be included.
...makeIndexRange(3, 3),
],
});
@ -661,7 +663,9 @@ add_resultBuckets_task({
// remote suggestions: round(10 * (1 / 4)) = 3
...makeIndexRange(MAX_RESULTS, 3),
// form history: round(10 * (1 / 4)) = 3, but context.maxResults is 10, so 2
...makeIndexRange(0, 2),
// The first three form history results dupe the three remote suggestions,
// so they should not be included.
...makeIndexRange(3, 2),
],
});
@ -695,7 +699,9 @@ add_resultBuckets_task({
// remote suggestions: round(10 * (2 / 4)) = 5
...makeIndexRange(MAX_RESULTS, 5),
// form history: round(10 * (1 / 4)) = 3, but context.maxResults is 10, so 2
...makeIndexRange(0, 2),
// The first five form history results dupe the five remote suggestions, so
// they should not be included.
...makeIndexRange(5, 2),
],
});
@ -729,7 +735,9 @@ add_resultBuckets_task({
// remote suggestions: round(10 * (1 / 4)) = 3
...makeIndexRange(MAX_RESULTS, 3),
// form history: round(10 * (2 / 4)) = 5, but context.maxResults is 10, so 4
...makeIndexRange(0, 4),
// The first three form history results dupe the three remote suggestions,
// so they should not be included.
...makeIndexRange(3, 4),
],
});
@ -763,7 +771,9 @@ add_resultBuckets_task({
// remote suggestions
...makeIndexRange(MAX_RESULTS, 1),
// form history: 10 - (2 + 1) = 7
...makeIndexRange(0, 7),
// The first form history result dupes the remote suggestion, so it should
// not be included.
...makeIndexRange(1, 7),
],
});
@ -797,7 +807,9 @@ add_resultBuckets_task({
// remote suggestions
...makeIndexRange(MAX_RESULTS, 1),
// form history: 10 - (2 + 1) = 7
...makeIndexRange(0, 7),
// The first form history result dupes the remote suggestion, so it should
// not be included.
...makeIndexRange(1, 7),
],
});
@ -862,7 +874,9 @@ add_resultBuckets_task({
// remote suggestions
...makeIndexRange(MAX_RESULTS, 1),
// form history: round(9 * (1 / (2 + 0 + 1))) = 3
...makeIndexRange(0, 3),
// The first form history result dupes the remote suggestion, so it should
// not be included.
...makeIndexRange(1, 3),
],
});
@ -975,7 +989,9 @@ add_resultBuckets_task({
// remote suggestions: round(7 * (2 / (2 + 1))) = 5
...makeIndexRange(0, 5),
// form history: round(3 * (2 / (2 + 1))) = 2
...makeIndexRange(MAX_RESULTS, 2),
// The first five form history results dupe the five remote suggestions, so
// they should not be included.
...makeIndexRange(MAX_RESULTS + 5, 2),
// general: round(3 * (1 / (2 + 1))) = 1
...makeIndexRange(2 * MAX_RESULTS + 2, 1),
],
@ -1019,7 +1035,9 @@ add_resultBuckets_task({
// remote suggestions: round(7 * (2 / (2 + 1))) = 5
...makeIndexRange(0, 5),
// form history: round(3 * (2 / (2 + 1))) = 2
...makeIndexRange(MAX_RESULTS, 3),
// The first five form history results dupe the five remote suggestions, so
// they should not be included.
...makeIndexRange(MAX_RESULTS + 5, 3),
],
});
@ -1058,7 +1076,9 @@ add_resultBuckets_task({
// remote suggestions: round(7 * (2 / (2 + 1))) = 5
...makeIndexRange(0, 5),
// form history: round(3 * (2 / (2 + 1))) = 2
...makeIndexRange(MAX_RESULTS, 3),
// The first five form history results dupe the five remote suggestions, so
// they should not be included.
...makeIndexRange(MAX_RESULTS + 5, 3),
],
});
@ -1111,7 +1131,9 @@ add_resultBuckets_task({
// outer 2: form history & general: round(10 * (1 / (2 + 1))) = 3
// inner 1: form history: round(3 * (2 / (2 + 1))) = 2
...makeIndexRange(MAX_RESULTS, 2),
// The first five form history results dupe the five remote suggestions, so
// they should not be included.
...makeIndexRange(MAX_RESULTS + 5, 2),
// inner 2: general: round(3 * (1 / (2 + 1))) = 1
...makeIndexRange(2 * MAX_RESULTS + 2, 1),
],
@ -1164,7 +1186,9 @@ add_resultBuckets_task({
// outer 2: form history & general: round(10 * (1 / (2 + 1))) = 3
// inner 1: form history: round(3 * (2 / (2 + 0))) = 3
...makeIndexRange(MAX_RESULTS, 3),
// The first seven form history results dupe the seven remote suggestions,
// so they should not be included.
...makeIndexRange(MAX_RESULTS + 7, 3),
// inner 2: general: no results
],
});

33
browser/components/urlbar/tests/unit/test_search_suggestions.js
View File

@ -1542,6 +1542,31 @@ add_task(async function formHistory() {
],
});
// Add a form history entry that dupes the first remote suggestion and do a
// search that triggers both. The form history should be included but the
// remote suggestion should not since it dupes the form history.
let suggestionPrefix = "dupe";
let dupeSuggestion = makeRemoteSuggestionResults(context, {
suggestionPrefix,
})[0].payload.suggestion;
Assert.ok(dupeSuggestion, "Sanity check: dupeSuggestion is defined");
await UrlbarTestUtils.formHistory.add([dupeSuggestion]);
context = createContext(suggestionPrefix, { isPrivate: false });
await check_results({
context,
matches: [
makeSearchResult(context, { engineName: ENGINE_NAME, heuristic: true }),
makeFormHistoryResult(context, {
suggestion: dupeSuggestion,
engineName: ENGINE_NAME,
}),
...makeRemoteSuggestionResults(context, { suggestionPrefix }).slice(1),
],
});
await UrlbarTestUtils.formHistory.remove([dupeSuggestion]);
// Add these form history strings to use below.
let formHistoryStrings = ["foo", "foobar", "fooquux"];
await UrlbarTestUtils.formHistory.add(formHistoryStrings);
@ -1571,8 +1596,8 @@ add_task(async function formHistory() {
// Add a visit that matches "foo" and will autofill so that the heuristic is
// not a search result. Now the "foo" and "foobar" form history should be
// included. The "foo" remote suggestion should also be included since the
// heuristic is not a search result.
// included. The "foo" remote suggestion should not be included since it
// dupes the "foo" form history.
await PlacesTestUtils.addVisits("http://foo.example.com/");
context = createContext("foo", { isPrivate: false });
await check_results({
@ -1596,10 +1621,6 @@ add_task(async function formHistory() {
suggestion: "fooquux",
engineName: ENGINE_NAME,
}),
makeSearchResult(context, {
suggestion: "foo",
engineName: ENGINE_NAME,
}),
...makeRemoteSuggestionResults(context, {
suggestionPrefix: "foo",
}),