Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-10-16 14:55:47 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1743008 - Fix the urlbar muxer to properly take into account the space taken up by group-relative suggested-index results. r=mak

Browse Source 
The problem is that the muxer doesn't update `usedLimits` when it adds
group-relative suggested-index results to a group. IOW it doesn't properly
account for the space taken up by group-relative suggested-index results. So in
the case described in the bug, the `GENERAL_PARENT` group is recorded as not
having any results at all since it doesn't have any normal results, even though
it does have the one quick suggest result. That leaves 9 spots for remote
suggestions, and we end up with 1 heuristic + 9 suggestions + 1 quick suggest =
11 results total.

Fixing that problem revealed another small problem where `_fillGroup` modifies
the passed-in `limits` object when it leaves room for the suggested-index result
at the beginning, which messes up the state in the caller and affects cases
where there's recursion due to flexed children not filling up.

In addition to these two fixes, I factored out the section where `_fillGroup`
fills child groups into its own `_fillGroupChildren` method. That way
`_fillGroup` can add group-relative suggested-index results in only one place,
at the end. Currently it does it in two places depending on whether the group
has any children. That also lets us do a slight optimization when recursing to
fill up flexed children. We don't need to worry about group-relative
suggested-index results in the group at that point, so we can call the new
`_fillGroupChildren` instead of `_fillGroup`.

Differential Revision: https://phabricator.services.mozilla.com/D132526
This commit is contained in:
Drew Willcoxon 2021-12-02 23:11:59 +00:00
parent 95768e21ee
commit c14bb93ba4
2 changed files with 240 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -204,27 +204,25 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
* as described above. They will be used as the limits for the group.
* @param {object} state
* The muxer state.
* @param {array} [flexDataArray]
* Non-recursive callers should leave this null. See `_updateFlexData` for a
* description.
* @returns {array}
* `[results, usedLimits, hasMoreResults]` -- see `_addResults`.
*/
_fillGroup(group, limits, state, flexDataArray = null) {
// Get the group's suggestedIndex results.
_fillGroup(group, limits, state) {
// Get the group's suggestedIndex results. Reminder: `group.group` is a
// `RESULT_GROUP` constant.
let suggestedIndexResults;
if ("group" in group) {
// Reminder: `group.group` is a `RESULT_GROUP`.
suggestedIndexResults = state.suggestedIndexResultsByGroup.get(
group.group
);
if (suggestedIndexResults) {
// Subtract their span and count from the group's limits so there will
// be room for them later.
// Subtract them from the group's limits so there will be room for them
// later. Create a new `limits` object so we don't modify the caller's.
let span = suggestedIndexResults.reduce((sum, result) => {
sum += UrlbarUtils.getSpanForResult(result);
return sum;
}, 0);
limits = { ...limits };
limits.availableSpan = Math.max(limits.availableSpan - span, 0);
limits.maxResultCount = Math.max(
limits.maxResultCount - suggestedIndexResults.length,
@ -233,15 +231,43 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
}
}
// If there are no child groups, fill the group directly.
if (!group.children) {
let [results, ...rest] = this._addResults(group.group, limits, state);
if (suggestedIndexResults) {
this._addSuggestedIndexResults(suggestedIndexResults, results, state);
// Fill the group. If it has children, fill them recursively. Otherwise fill
// the group directly.
let [results, usedLimits, hasMoreResults] = group.children
? this._fillGroupChildren(group, limits, state)
: this._addResults(group.group, limits, state);
// Add the group's suggestedIndex results.
if (suggestedIndexResults) {
let suggestedIndexUsedLimits = this._addSuggestedIndexResults(
suggestedIndexResults,
results,
state
);
for (let [key, value] of Object.entries(suggestedIndexUsedLimits)) {
usedLimits[key] += value;
}
return [results, ...rest];
}
return [results, usedLimits, hasMoreResults];
}
/**
* Helper for `_fillGroup` that fills a group's children.
*
* @param {object} group
* The result group to fill. It's assumed to have a `children` property.
* @param {object} limits
* An object with optional `availableSpan` and `maxResultCount` properties
* as described in `_fillGroup`.
* @param {object} state
* The muxer state.
* @param {array} flexDataArray
* See `_updateFlexData`.
* @returns {array}
* `[results, usedLimits, hasMoreResults]` -- see `_addResults`.
*/
_fillGroupChildren(group, limits, state, flexDataArray = null) {
// If the group has flexed children, update the data we use during flex
// calculations.
//
@ -320,7 +346,7 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
// If the children are flexed and some underfilled but others still have
// more results, do another pass.
if (anyChildUnderfilled && anyChildHasMoreResults) {
[results, usedLimits, anyChildHasMoreResults] = this._fillGroup(
[results, usedLimits, anyChildHasMoreResults] = this._fillGroupChildren(
group,
limits,
stateCopy,
@ -333,10 +359,6 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
}
}
if (suggestedIndexResults) {
this._addSuggestedIndexResults(suggestedIndexResults, results, state);
}
return [results, usedLimits, anyChildHasMoreResults];
}
@ -520,10 +542,6 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
* the same `RESULT_GROUP`. This is not related to the group's limits.
*/
_addResults(groupConst, limits, state) {
// We modify `limits` below. As a defensive measure, don't modify the
// caller's object.
limits = { ...limits };
let usedLimits = {};
for (let key of Object.keys(limits)) {
usedLimits[key] = 0;
@ -537,6 +555,8 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
groupConst == UrlbarUtils.RESULT_GROUP.FORM_HISTORY &&
!UrlbarPrefs.get("maxHistoricalSearchSuggestions")
) {
// Create a new `limits` object so we don't modify the caller's.
limits = { ...limits };
limits.maxResultCount = 0;
}
@ -1037,11 +1057,19 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
* inserted.
* @param {object} state
* Global state that we use to make decisions during this sort.
* @returns {object}
* A `usedLimits` object that describes the total span and count of all the
* added results. See `_addResults`.
*/
_addSuggestedIndexResults(suggestedIndexResults, sortedResults, state) {
let usedLimits = {
availableSpan: 0,
maxResultCount: 0,
};
if (!suggestedIndexResults?.length) {
// This is just a slight optimization; no need to continue.
return;
return usedLimits;
}
// Partition the results into positive- and negative-index arrays. Positive
@ -1075,10 +1103,14 @@ class MuxerUnifiedComplete extends UrlbarMuxer {
? Math.min(result.suggestedIndex, sortedResults.length)
: Math.max(result.suggestedIndex + sortedResults.length + 1, 0);
sortedResults.splice(index, 0, result);
usedLimits.availableSpan += UrlbarUtils.getSpanForResult(result);
usedLimits.maxResultCount++;
this._updateStatePostAdd(result, state);
}
}
}
return usedLimits;
}
}

212
browser/components/urlbar/tests/unit/test_suggestedIndexRelativeToGroup.js
View File

@ -9,36 +9,36 @@ const MAX_RESULTS = 10;
const RESULT_GROUPS_PREF = "browser.urlbar.resultGroups";
const MAX_RICH_RESULTS_PREF = "browser.urlbar.maxRichResults";
// Default result groups used in the tests below.
const RESULT_GROUPS = {
children: [
{
group: UrlbarUtils.RESULT_GROUP.GENERAL_PARENT,
flexChildren: true,
children: [
{
flex: 1,
group: UrlbarUtils.RESULT_GROUP.FORM_HISTORY,
},
{
flex: 1,
group: UrlbarUtils.RESULT_GROUP.GENERAL,
},
{
flex: 1,
group: UrlbarUtils.RESULT_GROUP.REMOTE_SUGGESTION,
},
],
},
],
};
add_task(async function test() {
// Set a specific maxRichResults for sanity's sake.
Services.prefs.setIntPref(MAX_RICH_RESULTS_PREF, MAX_RESULTS);
// Set the result groups we'll test with.
setResultGroups({
children: [
{
group: UrlbarUtils.RESULT_GROUP.GENERAL_PARENT,
flexChildren: true,
children: [
{
flex: 1,
group: UrlbarUtils.RESULT_GROUP.FORM_HISTORY,
},
{
flex: 1,
group: UrlbarUtils.RESULT_GROUP.GENERAL,
},
{
flex: 1,
group: UrlbarUtils.RESULT_GROUP.REMOTE_SUGGESTION,
},
],
},
],
});
// Create the non-suggestedIndex results we'll use in all tests. For each test
// we'll copy this array and append suggestedIndex results.
// Create the default non-suggestedIndex results we'll use for tests that
// don't specify `otherResults`.
let basicResults = [
...makeHistoryResults(),
...makeFormHistoryResults(),
@ -64,6 +64,11 @@ add_task(async function test() {
// The number of results in the slice.
// * {number} [offset]
// Can be used to offset the starting index of the slice in the results.
// * {array} [otherResults]
// An array of results besides the group-relative suggestedIndex results
// that the provider should return. If not specified `basicResults` is used.
// * {array} [resultGroups]
// The result groups to use. If not specified `RESULT_GROUPS` is used.
let tests = [
{
desc: "First result in GENERAL",
@ -305,15 +310,166 @@ add_task(async function test() {
},
],
},
{
desc: "Results in sibling group, no other results in same group",
otherResults: makeFormHistoryResults(),
suggestedIndexResults: [
{
suggestedIndex: -1,
group: UrlbarUtils.RESULT_GROUP.GENERAL,
},
],
expected: [
{
group: UrlbarUtils.RESULT_GROUP.FORM_HISTORY,
count: 9,
},
{
group: UrlbarUtils.RESULT_GROUP.GENERAL,
suggestedIndex: -1,
},
],
},
{
desc:
"Results in sibling group, no other results in same group, has child group",
resultGroups: {
flexChildren: true,
children: [
{
flex: 2,
group: UrlbarUtils.RESULT_GROUP.REMOTE_SUGGESTION,
},
{
flex: 1,
group: UrlbarUtils.RESULT_GROUP.GENERAL_PARENT,
children: [{ group: UrlbarUtils.RESULT_GROUP.GENERAL }],
},
],
},
otherResults: makeRemoteSuggestionResults(),
suggestedIndexResults: [
{
suggestedIndex: -1,
group: UrlbarUtils.RESULT_GROUP.GENERAL_PARENT,
},
],
expected: [
{
group: UrlbarUtils.RESULT_GROUP.REMOTE_SUGGESTION,
count: 9,
},
{
group: UrlbarUtils.RESULT_GROUP.GENERAL_PARENT,
suggestedIndex: -1,
},
],
},
{
desc: "Complex group nesting with global suggestedIndex with resultSpan",
resultGroups: {
children: [
{
maxResultCount: 1,
children: [{ group: UrlbarUtils.RESULT_GROUP.HEURISTIC_TEST }],
},
{
flexChildren: true,
children: [
{
flex: 2,
group: UrlbarUtils.RESULT_GROUP.REMOTE_SUGGESTION,
},
{
flex: 1,
group: UrlbarUtils.RESULT_GROUP.GENERAL_PARENT,
children: [{ group: UrlbarUtils.RESULT_GROUP.GENERAL }],
},
],
},
],
},
otherResults: [
// heuristic
Object.assign(
new UrlbarResult(
UrlbarUtils.RESULT_TYPE.SEARCH,
UrlbarUtils.RESULT_SOURCE.SEARCH,
{
engine: "test",
suggestion: "foo",
lowerCaseSuggestion: "foo",
}
),
{
heuristic: true,
group: UrlbarUtils.RESULT_GROUP.HEURISTIC_TEST,
}
),
// global suggestedIndex with resultSpan = 2
Object.assign(
new UrlbarResult(
UrlbarUtils.RESULT_TYPE.SEARCH,
UrlbarUtils.RESULT_SOURCE.SEARCH,
{
engine: "test",
}
),
{
suggestedIndex: 1,
resultSpan: 2,
}
),
// remote suggestions
...makeRemoteSuggestionResults(),
],
suggestedIndexResults: [
{
suggestedIndex: -1,
group: UrlbarUtils.RESULT_GROUP.GENERAL_PARENT,
},
],
expected: [
{
group: UrlbarUtils.RESULT_GROUP.HEURISTIC_TEST,
count: 1,
},
{
group: UrlbarUtils.RESULT_GROUP.SUGGESTED_INDEX,
suggestedIndex: 1,
resultSpan: 2,
count: 1,
},
{
group: UrlbarUtils.RESULT_GROUP.REMOTE_SUGGESTION,
count: 6,
},
{
group: UrlbarUtils.RESULT_GROUP.GENERAL_PARENT,
suggestedIndex: -1,
},
],
},
];
let controller = UrlbarTestUtils.newMockController();
for (let { desc, suggestedIndexResults, expected } of tests) {
for (let {
desc,
suggestedIndexResults,
expected,
resultGroups,
otherResults,
} of tests) {
info(`Running test: ${desc}`);
setResultGroups(resultGroups || RESULT_GROUPS);
// Make the array of all results and do a search.
let results = basicResults.concat(
let results = (otherResults || basicResults).concat(
makeSuggestedIndexResults(suggestedIndexResults)
);
let provider = registerBasicTestProvider(results);