We need to do a few things here, all of which require confirmation by Product:

1. store search history when searches start from the urlbar (see comment 6)
2. enable local search history (browser.urlbar.maxHistoricalSearchSuggestions. Define yes/no, how many entries)
3. evaluate whether we want to revive a simpler implementation of reverse parsing search urls (browser.urlbar.restyleSearches, figure out bug 1096247 or simplify reach of the feature, so restyle only some...)

Updating Fx75 tracking flags to reflect QA triage decision taken with :mdeboer in QA-Search weekly sync meeting.

This should fit nicely with Harry's work in bug 1626946.

Hmm... bug 1626946 might actually be the bulk of this work. It restyles search history results and adds a new payload property that indicates a search suggestion result is a restyled history result. With that, we don't need form history at all, do we? Form history is useful in the search bar and the old newtab search field, where browsing history isn't also searched. Here we can just use history, at least if the frecency is high enough to appear in the results. That might be the only reason we'd want to use form history -- if we want a stronger guarantee that non-frecent search history appears. But probably we don't. The only other thing we need is to make sure restyled results appear first, and maybe a history icon or badge.

Marco, bug 1626946 actually does a good job of recalling historical searches. Comment 11 here has my initial thoughts about that, so please read that first.

I think we should rely 100% on historical searches bubbling up through the usual frecency mechanism from Places history, and we shouldn't rely on form history at all. Therefore I propose that we remove the maxHistoricalSearchSuggestions pref and hardcode that value to zero.

If you agree with that, then we can close this bug or mark it as a dupe of bug 1626946 (but see below). I'll file a new bug for removing maxHistoricalSearchSuggestions. And there may also be follow-up bugs to bug 1626946 -- I'm thinking specifically of using a different favicon for historical searches, which we can ask Verdi about.

However, comment 0 here also talks about the ability to easily edit your search terms. So I suggest we leave this open but make it specifically about that, and not about restyling searches or historical searches.

What do you think?

(In reply to Drew Willcoxon :adw from comment #12)

I think we should rely 100% on historical searches bubbling up through the usual frecency mechanism from Places history, and we shouldn't rely on form history at all. Therefore I propose that we remove the maxHistoricalSearchSuggestions pref and hardcode that value to zero.

There are a couple cases where form history may still be useful, and I can't think of cases where it would be bad, unless it's to save on I/O? I'd like to hear more about the concerns.

The cases coming to my mind are:

1. Search history is shared across all the search fields, history is not. Of course long term we aim at a single SAP, so this is probably ok.
2. If history is disabled or cleared on shutdown, we could still use search history, and it would be useful to the user. For this maybe it could be worth to still store search history from the urlbar, and enable it only when history is disabled or cleared on shutdown?
3. if the search url changes (and they do but I don't know how often), history won't be reverse-parsed.

I fear this requires a broader discussion also involving PM, because I can't tell how bad 2. and 3. may be.
My off-hand suggestion would be to store search history at least, so we can make use of it if/when necessary, but I'm not sold on a solution yet.

However, comment 0 here also talks about the ability to easily edit your search terms. So I suggest we leave this open but make it specifically about that, and not about restyling searches or historical searches.

What do we put in the input field when a reverse-parsed result is selected? We should probably only reverse parse engines with a token alias, and put in the input field "@token search terms" (unless it's the default engine, then it's only "search terms") and it looks pretty easy to edit at that point.

If we use form history, then we need to decide how many form history results to include vs. other results. We also need to decide the "frecency" threshold for form history results since we probably don't want to suggest a search you did one time a few years ago.

Looking at FormHistory.jsm and FormAutoComplete.jsm, it does look like form history has a frecency concept too, like Places history. So we would use that. But it's not the same as Places history. So we could use it to sort and define a cutoff for form history results among themselves, but we couldn't use it to compare against other types of results. We couldn't use it as a hint about how many form history results we should include vs. other results.

On the other hand, if we rely on SERPs in Places history instead, then the decisions about how many historical search results to include and what their frecency threshold should be are predetermined. It just works. We already have a system for deciding how many of one type of result we should include vs. other types, and how they should be ordered. It's UnifiedComplete and urlbar. Form history is useful for the search bar because it doesn't have access to your SERPs in your Places history, but of course the urlbar does.

Places history actually is shared across all SAPs, because once you hit enter and do a search, you visit the SERP, and it's added to history. So searches you do from the search bar will naturally be included as potential search history in the urlbar. And that includes all the searches you've ever done that are still in history.

If you clear your Places history, imo it's reasonable to expect that your search history is cleared too, especially your search history from the urlbar. So here again using past SERPs in Places history instead of form history just works naturally.

Also, form history doesn't remember which engine you used to do a search. If we want certain handling around which engine is current vs. which engine a past search was done with, form history doesn't allow us to do that, so we'd need to build a way.

I don't mind storing form history too, but I think the case for using Places history as I've outlined is compelling.

(In reply to Marco Bonardo [:mak] from comment #13)

What do we put in the input field when a reverse-parsed result is selected? We should probably only reverse parse engines with a token alias, and put in the input field "@token search terms" (unless it's the default engine, then it's only "search terms") and it looks pretty easy to edit at that point.

Currently restyled search history results retain their engine, so if you search for "foobar" on Google and then change your default to Bing and type "foo", you'll see "<Google favicon> foobar". When you key down to it, "foobar" gets put in the input, and when you hit enter, you do a search for foobar on Google. We should clarify with Verdi because it's not specified at all, and we can iterate on it.

I'm not sure how much all of this addresses Alice's comment though because you still need to start typing to see a search history result. Alice may be asking for something like how Safari keeps your search terms in the input while you're on the SERP.

Ok so the plan changed, according to today's UX meeting:

1. We won't restyle history results
2. We want to use FormHistory, for which we already have browser.urlbar.maxHistoricalSearchSuggestions
3. Historical searches should look and work the same as in the legacy search bar
4. if maxHistoricalSearchSuggestions is positive, we want to try hiding historical results that would be restyled (we can use the same code to identify them)
5. suggestions are engine-agnostic, like they are in the legacy search bar, and shared across SAP
6. if there are enough local results, we won't hit the network at all for remote results

(In reply to Drew Willcoxon :adw from comment #14)

If we use form history, then we need to decide how many form history results to include vs. other results. We also need to decide the "frecency" threshold for form history results since we probably don't want to suggest a search you did one time a few years ago.

It was never a problem in the legacy search bar, why is it now? Is it because we have less space?

Looking at FormHistory.jsm and FormAutoComplete.jsm, it does look like form history has a frecency concept too, like Places history. So we would use that. But it's not the same as Places history. So we could use it to sort and define a cutoff for form history results among themselves, but we couldn't use it to compare against other types of results.

That's fine, local and remote results are in the suggestions bucket, for now we won't mixup.

On the other hand, if we rely on SERPs in Places history instead, then the decisions about how many historical search results to include and what their frecency threshold should be are predetermined. It just works. We already have a system for deciding how many of one type of result we should include vs. other types, and how they should be ordered. It's UnifiedComplete and urlbar. Form history is useful for the search bar because it doesn't have access to your SERPs in your Places history, but of course the urlbar does.

It's possible to augment form history score using history, but sounds like food for follow-up work than MVP.

Places history actually is shared across all SAPs, because once you hit enter and do a search, you visit the SERP, and it's added to history. So searches you do from the search bar will naturally be included as potential search history in the urlbar. And that includes all the searches you've ever done that are still in history.

I'd be prone to check what form history does there. Imo we should have a time threshold, if suggestions are older than N days, we should not show them. Is Form History decayed? I don't recall.

If you clear your Places history, imo it's reasonable to expect that your search history is cleared too, especially your search history from the urlbar. So here again using past SERPs in Places history instead of form history just works naturally.

I'm not sure, but of course we could listen to onClearHistory and clear search history for SAPs IF the legacy search bar is not used.

Also, form history doesn't remember which engine you used to do a search. If we want certain handling around which engine is current vs. which engine a past search was done with, form history doesn't allow us to do that, so we'd need to build a way.

We don't want that.

I don't mind storing form history too, but I think the case for using Places history as I've outlined is compelling.

We'll keep both anyway, history entries in the future will have a normal visit score (instead of typed), for now we can hide them and see.

(In reply to Marco Bonardo [:mak] from comment #15)

Ok so the plan changed, according to today's UX meeting:

1. We won't restyle history results
2. We want to use FormHistory, for which we already have browser.urlbar.maxHistoricalSearchSuggestions
3. Historical searches should look and work the same as in the legacy search bar
4. if maxHistoricalSearchSuggestions is positive, we want to try hiding historical results that would be restyled (we can use the same code to identify them)
5. suggestions are engine-agnostic, like they are in the legacy search bar, and shared across SAP
6. if there are enough local results, we won't hit the network at all for remote results

We get all of this for free with bug 1626946, plus we don't need to do extra work to hook up FormHistory. (We'd have to tweak it so suggestions are engine-agnostic. It's a small change.) I missed whatever meeting this was discussed in, but I really don't understand why whoever said we need to use FormHistory said it. If I could get it past review, I'm tempted to simply flip the restyle-searches pref in this bug because I don't think anyone looking at the final UX could tell the difference.

Anyhow I'll stop talking about it now and use FormHistory.

mconnor explained it to me on Slack. The point I was missing (or a point) is that we consider it a bug that SERP history results have a high frecency, which is due at least in part to the fact that we give them the typed boost. We want their frecency to be lower than the links you click in SERPs. In a world where that's fixed, SERP history might not rise to the top of urlbar results the way they do now, so they might not be present to be restyled in the first place. That's why we want to use FormHistory.

(In reply to Marco Bonardo [:mak] from comment #15)

6. if there are enough local results, we won't hit the network at all for remote results

SearchSuggestionController fetches remote suggestions and form history at the same time and then combines them, so if we want this to be true, we'll need to modify it to wait until form history is fetched before fetching remote suggestions.

I'm running into some flickering when I enable deduping. I think it's due to the fact that for the first time we now have a provider whose results affect results from other providers. Specifically, the search suggestion provider can cause the muxer to remove history results from the UnifiedComplete provider. That causes flickering because we show results as soon as they're added by their providers.

In my profile, I've done a search for "foobar". When I type "foo", close the view, and then open it again (e.g., by clicking the input), the second result is my foobar SERP, and then it immediately becomes "foo -- Search in a Private Window".

I think this is what's happening:

1. The UnifiedComplete provider adds its history results, in this case a SERP for foobar
2. The muxer allows the foobar SERP, and it's shown in the view
3. The search suggestions provider adds its results, including "foobar" from form history
4. The muxer removes the foobar SERP because it dupes the foobar form history. And then because all the remaining results are search suggestions, it also allows the private-window search result, which effectively replaces the foobar SERP in the view.

So it seems like we need to wait for form history results before adding history results. We don't have a way to do that right now.

I've modified the UrlbarProviderSearchSuggestions class so that it can provide either form history or remote suggestions but not both. That allows us to have a separate form history provider and a remote suggestions provider. I've also implemented a provider dependency mechanism so that you can specify that one provider must be queried and finish before another, and I use that to force the form history provider to add all its results before the UnifiedComplete provider is queried. Those two things together fix the flickering I mentioned earlier.

Now I'm running into flickering of the first result. The form history provider finishes and adds a first result, and then the UnifiedComplete provider adds the heuristic result, replacing the first form history result.

I think I can fix that by factoring out the heuristic logic from UnifiedComplete into a heuristic provider, and then every other provider would depend on it to ensure that it finishes first.

Or, we could stop adding results to the view as soon as they're available. That would let the muxer have more time to dedupe and sort results. But we'd need to have a timeout to make sure the view isn't starved, so that may just postpone the problem, depending on the timeout we choose.

An alternative to provider dependencies might be to have the muxer buffer results until it knows it can dedupe and sort them so there's no flickering, even if more results are added. The problem is that practically speaking result types depend on the provider, and the muxer doesn't have a way to tell when a provider is finished. So for example if it receives history results but it hasn't received form history results, should it keep waiting for them, or will they never come? If it knows that the form history provider has finished, then it can know it shouldn't wait.

I added a context.pendingProviders property, and a provider.resultGroups property. With these two, the muxer knows which providers are active and still adding results, and which result groups those providers return. So that means the muxer can do these things for example:

• If a pending provider returns results in the heuristic group and a heuristic result hasn't been added yet, then the muxer bails and the providers manager doesn't notify the controller of new results. i.e., the muxer "waits" for the heuristic result.
• If a pending provider returns results in the form history group, then the muxer waits for those results.
• If no pending provider returns results in the form history group and there are no form history results, then the muxer knows there won't be any form history results, so it's safe to add history results that might be duped by form history.

I'm not sure whether provider.resultGroups should be provider.resultTypes instead, but this approach seems better than the provider dependencies I mentioned earlier.

This works well and it's polished, but it's a big patch that I think I can split up to make it easier to review. Although I might change my mind when I start to try. It also needs tests. It implements what I described in comment 22.

Let's see how this does against current tests... https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9b16006558a917e0de836e37f1c45ecbf08b638

(In reply to Drew Willcoxon :adw from comment #18)

SearchSuggestionController fetches remote suggestions and form history at the same time and then combines them, so if we want this to be true, we'll need to modify it to wait until form history is fetched before fetching remote suggestions.

I don't think it's a strict requirement but it was one of the things that came out during the discussion and everyone liked the idea. I suppose it could be evaluated if it creates troubles.

(In reply to Drew Willcoxon :adw from comment #19)

I'm running into some flickering when I enable deduping. I think it's due to the fact that for the first time we now have a provider whose results affect results from other providers. Specifically, the search suggestion provider can cause the muxer to remove history results from the UnifiedComplete provider. That causes flickering because we show results as soon as they're added by their providers.

Could we instead mark them as "stale" so they are just removed later?
Though, this require coordination with the view that may not be trivial...

(In reply to Drew Willcoxon :adw from comment #20)

Or, we could stop adding results to the view as soon as they're available. That would let the muxer have more time to dedupe and sort results. But we'd need to have a timeout to make sure the view isn't starved, so that may just postpone the problem, depending on the timeout we choose.

Long term I think we should have a chunked behavior with 16ms timers

Some notes from mconnor in today's meeting:

• Land enabled on Nightly and Beta with two form history suggestions (maxHistoricalSearchSuggestions == 2), which is how many Chrome shows
• Deduping question 1: If we have a form history result for foo and a SERP history result for foo but the SERP isn't from page 1 of the results (e.g. it's from page 2 or 10), then we still want to show only the form history result
• Deduping question 2: If we have a form history result for foo and a SERP history result for foo but the SERP is from a different engine than the current engine, then we want to show only the form history result, but he doesn't feel strongly about it
• Deduping question 3: If we have a form history result for foo and the heuristic result is a foo search, then we do not want to show the form history result since it dupes the heuristic

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

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

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

Query.start shouldn't await provider.tryMethod("isActive"). Instead it
should call all providers at once and then await all the promises. Also make
some other simplifications and changes to Query.start in preparation for later
patches.

Factor out notifyResults into Query._notifyResultsFromProvider because the
next patch will use it.

By returning false (or not adding results to context.results), the muxer
signals that it needs more results in order to decide how to sort them and to
sort them without flickering. After the providers manager calls each provider's
startQuery, it removes the provider from context.activeProviders and calls
_notifyResultsFromProvider one more time so that the muxer can resort if
necessary given the new context.activeProviders.

We can also be a little faster and more efficient by making only two passes over
the results, and by keeping a context.heuristicResult property that's updated
by the providers manager. A new provider.providesHeuristicResult property lets
the muxer know when to expect a heuristic.

This also excludes SERPs from browser history that dupe the heuristic search
result. I'll build on this in a later patch to dedupe SERPs and form history
too, and keep in mind that we want to dedupe form history and the heuristic
result as well.

This also fixes a bug with the search suggestions provider, where it didn't
include a suggestion that matched the user's search string even if the heuristic
result was not a search result (e.g., if it's an autofill). That deduping logic
should be done in the muxer.

Finally this removes browser_restyleSearches.js since the muxer changes removed
part of the restyling functionality. I'll file a follow-up to remove other parts
of restyling.

This is straightforward: Change UrlbarProviderSearchSuggestions from a module
to a class, and let it to take a suggestions type, either form history or
remote. For both types, the provider requests maxResults from the search
suggestions controller, and the muxer will be responsible for muxing and
limiting the results.

Form history results have the new type RESULT_TYPE.FORM_HISTORY. I considered
making them the SEARCH type with a new isFormHistory property, but that
wouldn't have let me make the muxer improvements in the previous and next patch.
The muxer can be smarter if it knows about form history as a separate type.

The TODO in test_search_suggestions.js is addressed in a later patch.

Integrate form history in the muxer, including deduping and limiting the number
of form history results.

Add provider.resultTypes so the muxer knows which result types it's still
expecting. To avoid flicker, the muxer bails if it's still expecting form
history.

Handle the new FORM_HISTORY result type everywhere else in urlbar, except for
telemetry, which is coming in the next patch. This includes adding to and
removing from FormHistory and updating the view.

I needed to make new histograms because we ran out of space in the old ones.

This is the last patch except for new tests, so form history is enabled and
usable with this. This also updates existing tests so they pass with form
history enabled.

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

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

https://hg.mozilla.org/mozilla-central/rev/68a3175d343f

https://hg.mozilla.org/mozilla-central/rev/5830d5c98f29

This is a conversation in follow-up from comment 26 that I had with mconnor on Slack. We talk more about how to handle deduping.

I missed my initial comment.

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

• https://treeherder.mozilla.org/#/jobs?repo=try&revision=6734c32c45c965c8409fa58f85b8601d701e7bb2
  • With maxHistoricalSearchSuggestions == 2
• https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a457a2bb8e6e182fceecd1df72026cb950f9e95
  • With maxHistoricalSearchSuggestions == 0 to simulate late beta and release

I forgot to include the browser.engagement.navigation.urlbar scalar telemetry.

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

I reproduced this issue using 57.0a1 (2017-09-09) on Windows 10 x64.
I can confirm the fix, I verified using Fx 78.0 on Windows 10 x64 and macOS 10.13.