1710518 - Come up with a general solution for placing Quick/Firefox Suggest results at configurable positions within the general bucket

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P3)

Description

[Drew Willcoxon :adw]

This is a continuation of bug 1709992. We're fixing that bug with a little bit of a hack by adding code to the muxer to specifically place quick suggest results at the end of the general bucket. TBH I call it a little hack because the muxer already has other code specifically for different types and providers. Regardless, it would be nice to come up with a more general solution.

I have a couple of ideas:

1. In addition to suggestedIndex, allow results to specify a suggestedGroup or suggestedBucket. Then the index would be relative to the group/bucket, not all results. A potential problem is that groups and buckets don't map one-to-one. A group may be in many buckets, so if you specify suggestedGroup, which bucket do we use? The first? If instead we have suggestedBucket, what do you specify? Because right now buckets don't have names or IDs. Maybe they should anyway.
2. Add a new result group and bucket for quick suggest, and modify the general bucket so it has two children, a general child and a quick suggest child. This has the advantage of working more naturally within our bucket system. Currently we don't have a way to say that a sibling bucket should always be filled with one result if possible, so we would need to add that -- something like a minResultCount to go along with the current maxResultCount.

Comment 1

[Drew Willcoxon :adw]

Bug 1711792 is changing the requirements for this, updating summary accordingly.

Comment 2

[Drew Willcoxon :adw]

There are three regressions/dependent bugs linked in bug 1709992 (as of right now) that we should be sure not to regress again here: bug 1710648, bug 1710657, bug 1712575

Comment 3

[Drew Willcoxon :adw]

We may not want to spend time on this because Natalie says the ultimate goal is to sort quick suggest results among other results as if it had a frecency (which of course is another challenge on its own). Also I think it's safe to say that the whole quick suggest feature is still being iterated on and is open to change.

Comment 4

[Drew Willcoxon :adw]

This will make bug 1723160 a little easier, so I'll take it after all.

Comment 5

[Drew Willcoxon :adw]

Attachment: Bug 1710518 - Introduce group-relative suggestedIndex and modify quick suggest results to use it.

This introduces a new result.isSuggestedIndexRelativeToGroup property. When
true, result.suggestedIndex is relative to the result's group.

We can get rid of some hardcoded quick-suggest things with this.

I considered a relativeSuggestedIndex property that would replace
suggestedIndex, but it would have made muxer._addSuggestedIndexResults a
little more complex because it would need to know whether it should use
relativeSuggestedIndex or suggestedIndex. UrlbarView._rowCanUpdateToResult
would also need to be updated since it checks for suggestedIndex results. But
there are trade-offs either way and I'm open to using relativeSuggestedIndex.

While working on this I noticed that we broke the position of quick suggest
results in bug 1662167. They're supposed to be last in the outer general group,
and currently the muxer is hardcoded to put them last in GENERAL, but after
that bug INPUT_HISTORY actually comes last. To fix that I added a new
GENERAL_PARENT group that's used for the outer general group. Quick suggest
results now use this group, combined with a relative suggestedIndex, to come
last. I wanted to rename GENERAL to PLACES or BOOKMARKS_HISTORY and use
GENERAL for the outer group, but we use GENERAL in a ton of places (tests
especially) and I didn't want to touch blame for it all. I'm open to better
names than GENERAL_PARENT. I considered FIREFOX_SUGGEST but I don't think
it's a good idea to tie it to specific branding that may change.

Comment 6

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff127bb5e319e768c29e46de181146a0c836b6d6

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd9241a36f5d
Introduce group-relative suggestedIndex and modify quick suggest results to use it. r=harry

Comment 8

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/fd9241a36f5d

Comment 9

[Drew Willcoxon :adw]

qe-verify- because this is a relatively low-level change that doesn't need to be individually manually verified.

Comment 10

[Drew Willcoxon :adw]

[Tracking Requested - why for this release]: Some urgent Firefox Suggest patches we want to uplift to a 92 dot release depend on this bug.

Comment 11

[(Out Sep 17-23) Ryan VanderMeulen]

We're tracking the metabug for this work.

Comment 12

[Drew Willcoxon :adw]

Attachment: 92/mozilla-release patch

Approval Request Comment
[Feature/Bug causing the regression]: Firefox Suggest offline/online rollouts
[User impact if declined]: Needed for important rollouts on 93 and 92
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No, low-level change with no visible changes
[List of other uplifts needed for the feature/fix]: See uplift coordination spreadsheet
[Is the change risky?]: Moderate risk relative to other uplifts needed in the patch stack
[Why is the change risky/not risky?]: This modifies how the address bar panel places Firefox Suggest suggestions. It touches code that affects all results shown in the panel. However, it comes with new tests, the address bar codebase has extensive existing coverage, and this has baked on 93 and 94 for a while now.
[String changes made/needed]:

Comment 13

[(Out Sep 17-23) Ryan VanderMeulen]

Comment on attachment 9240781 [details] [diff] [review]
92/mozilla-release patch

Approved for 92.0.1.

Comment 14

[(Out Sep 17-23) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-release/rev/6953b3781f3a