Closed Bug 1645324 Opened 5 years ago Closed 5 years ago

Move the omnibox heuristic result to the omnibox provider

Categories

(Firefox :: Address Bar, task, P3)

Product:
Firefox
Component:
Address Bar
Type:
task
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 80
Iteration:
80.1 - June 29 - July 12
Tracking Flags:
Tracking Status
firefox80 --- fixed

People

(Reporter: mak, Assigned: bugzilla)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1645324 - Move the omnibox heuristic result to the omnibox provider. r?adw!
47 bytes, text/x-phabricator-request
Details | Review

Bug 1642710 esposed the limits we have regarding the heuristic result, and providers composition, because yhere we would like (at least) to:

  1. have an extension heuristic result
  2. have extenstion results
  3. eventually fill up remaining space with search suggestions (or what we like)

If one would think this from scratch, it sounds like the new provide should provide both the heuristic result and other results, and then some other provider should fill up space.
And this has various problems for our current architecture:

We don't properly support multiple heuristic results, we should probably make the muxer wait for the heuristic result of all the PROVIDER_TYPE.HEURISTIC providers, and then pick one and discard the others, before returning any result. Or this provider should be restricting, but then we have problem 2
If we define a provider priority, then that provider is the only one running, but we'd like to fill up space with another provider... A possible idea: have a filling provider (search suggestion sounds like a good candidate) that can be invoked if a provider has an allowFillupResults property, or allow a restricting provider to define who is its filling provider, if any.
If this provider takes control, other providers should not spend time searching unless requested to fill up, since it may be pointless work.

After this brief analysis, it looks to me that the omnibox provider should be a restricting HEURISTIC provider, and that we need a way to define which providers should be used to fill remaining space, if any.

Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Iteration: --- → 79.2 - June 15 - June 28
Points: --- → 3

Expanding on what Marco said about the muxer picking a heuristic provider, we could tackle this by defining a heuristicOrder in the muxer that resolves priority ties between heuristic providers. Following the order currently defined in UnifiedComplete, it could look something like

const heuristicOrder = [
  "UrlbarProviderOmnibox",
  "UrlbarProviderKeywords", // Doesn't yet exist, would cover non-token aliases and keywords
  "UrlbarProviderAutofill", // Doesn't yet exist
  "UrlbarProviderTokenAliasEngines",
  "UrlbarProviderUnifiedComplete", // Covers Places matches
  "UrlbarProviderSearchSuggestions", // Handles searching with the current engine
];

That way, a provider can offer a heuristic result while not restricting non-heuristic results from other providers, much like how UnifiedComplete works now. This would allow us to use the usual priority system for fillups. We'd need to ensure that each provider is uniquely ranked, which is why I propose defining this as an ordered list in the Muxer rather than a heuristicPriority value returned by each provider.

There are a number of // Doesn't yet exist comments above... We could work on this in the short term just by starting with "UrlbarProviderOmnibox", "UrlbarProviderUnifiedComplete", and "UrlbarProviderSearchSuggestions", in that order, then adding to the list as work on bug 1623636 progresses.

Drew, any thoughts on these proposals?

Flags: needinfo?(adw)

I think it makes sense that it's the muxer's job to pick the heuristic, and that multiple providers can provide a heuristic.

I don't think I agree with the idea about designating providers to be fill-up providers. That seems a little kludgy to me. Ideally providers look at the query context, decide whether they want to be active, and then go off and produce all their results independent of what the muxer will do or what any other later stage in the process will do. I guess I'm open to the idea though if it turns out to be an important optimization.

It's been mentioned here, but just to be clear, we'll want to avoid flickering of the heuristic, meaning we don't want to pick one heuristic just to replace it with another a moment later. So the muxer will need to wait for all active heuristic providers to return their heuristic results before it can do anything. It will need a way to tell which active heuristic providers are still pending. Earlier versions of my form history patches added a context.activeProviders array in the providers manager, right after active providers are determined. I would think we'd need that for this. (context.activeProviders was an array of provider names, not the providers themselves, because the context must be serializable since it's passed to webextensions.)

Heuristic providers should add their heuristic results first so that the muxer isn't stuck waiting while their non-heuristic results are added. Maybe we could enforce that through the API somehow (startHeuristicQuery below for example), but it could probably just be left to convention.

We currently have a context.heuristicResult property that's set by the providers manager. The point of that property is to allow the muxer to determine whether there's a heuristic in O(1) time instead of O(number of results). That probably needs to become an array or set.

Finally, the providers manager queries heuristic providers immediately. For other providers it waits for a small timeout before calling their startQuery. If we end up with many heuristic providers, we may want to rethink that distinction. For example, maybe the delay isn't useful. Or maybe we add another provider method called startHeuristicQuery that would be called immediately on heuristic providers, while startQuery would always be called on a delay for both heuristic and non-heuristic providers.

Flags: needinfo?(adw)
Depends on: 1645521

Depends on D80293

Iteration: 79.2 - June 15 - June 28 → 80.1 - June 29 - July 12

Comment on attachment 9157786 [details]
Bug 1645324 - Part 1 - Add ProviderHeuristicFallback. r?adw!

Revision D80291 was moved to bug 1645521. Setting attachment 9157786 [details] to obsolete.

Attachment #9157786 - Attachment is obsolete: true

Comment on attachment 9157788 [details]
Bug 1645324 - Part 2 - Allow for multiple heuristic providers and enable ProviderHeuristicFallback. r?adw!

Revision D80293 was moved to bug 1645521. Setting attachment 9157788 [details] to obsolete.

Attachment #9157788 - Attachment is obsolete: true

Comment on attachment 9157789 [details]
Bug 1645324 - Part 3 - Port unifiedcomplete tests. r?adw!

Revision D80294 was moved to bug 1645521. Setting attachment 9157789 [details] to obsolete.

Attachment #9157789 - Attachment is obsolete: true
Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/65f908569875 Move the omnibox heuristic result to the omnibox provider. r=adw

See bug 1645521 comment 6.

Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/366f7bfe267c Move the omnibox heuristic result to the omnibox provider. r=adw

https://hg.mozilla.org/mozilla-central/rev/366f7bfe267c

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80
Regressions: 1652024

Backed out as requested by harry for causing bug 1652024.

Backout link: https://hg.mozilla.org/integration/autoland/rev/a82cdd81945436f60f13e552140ea0d2886c84b6

Status: RESOLVED → REOPENED
status-firefox80: fixed → ---
Resolution: FIXED → ---
Target Milestone: Firefox 80 → ---
Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bf67c2159244 Move the omnibox heuristic result to the omnibox provider. r=adw

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

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80
Regressions: 1652280

Backed out: https://hg.mozilla.org/integration/autoland/rev/863e463684183a334f15b465d932372e62d24a4f

Status: RESOLVED → REOPENED
status-firefox80: fixed → ---
Resolution: FIXED → ---
Target Milestone: Firefox 80 → ---

I'm pretty sure this change has introduced a bug that is a little difficult to reproduce reliably.

Before, even if the preview hadn't updated to reflect the last couple of keystrokes, they would be included when hitting enter.
Now I have to wait for the preview to update, or whatever I typed will get trimmed or cropped to what the preview had updated to.

This means that if I type quickly and hit enter to google it, I often end up searching for fewer words than I intended.

I can attach a video of this happening if required, but you should be able to trigger it yourself if you type quickly enough.

I think you're running into a different form of bug 1652024, which should be fixed when this re-lands with the patches for bug 1648468 attached.

Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e4bbcec6a28 Move the omnibox heuristic result to the omnibox provider. r=adw

https://hg.mozilla.org/mozilla-central/rev/9e4bbcec6a28

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 80

Christian, can you still reproduce the issue you described on the latest Nightly?

Flags: needinfo?(cers)

(In reply to Harry Twyford [:harry] from comment #25)

Christian, can you still reproduce the issue you described on the latest Nightly?

No, the issue has been resolved, thanks!

Flags: needinfo?(cers)
Regressions: 1653697
No longer regressions: 1653697
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: