1071481 - host of search engines is not autoFilled anymore if it's not the most frecent result

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Recent changes to the awesomebar have somehow regressed autoFill, I have google.com in my typed domains but it's not autoFilled anymore.

Need to investigate which change caused this and a new test for it.

Comment 1

[Marco Bonardo [:mak]]

Marco, please add this to the current iteration.

Comment 2

[Marco Mucci [:MarcoM]]

Added to IT 35.2

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #1)
> Marco, please add this to the current iteration.

Comment 3

[Marco Bonardo [:mak]]

The problem here is that when we match search engines, if we get a match we call addFrecencyMatch. This will add the match to a list but not yet to the result.

Later, before a new match is being added, we compare its frecency with the entries in the frecencies list and if an entry with an higher frecency is found we add it to the result.

This means at the time we call addFrecencyMatch we still don't know if that result will end up being the first or not, but we return hasFirstResult = true, that means we don't run any more heuristics for the first result (no host/url autofill).

So basically:
1. I have typed google.com
2. I have google.com search engine

I'd expect "go" to autofill at least one of these 2, but when I type "go" the search engine is at third post and the typed host is not even tried.

The easy solution here is to drop support for frecency matches mixup for now, to proper support those we need to be able to insert matches in a result in random order, that will come with an autocomplete rewrite.

There are actually also other issues we had even before the regression, like calling setDefaultIndex(0) even if then the entry won't be the first one. and the fact if it is not the first entry and the user tries to remove it, it will fail. now since the first entry will look special it is less surprising that removing it may fail, but not for the other entries.

So yeah, I think we should make the code simpler for now and re-evaluate adding more complex behavior in future, when we have the right way to do it.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1

This may be a little bit tricky to verify for QE, it requires having a result that matches the same string as the search engine host but with an higher frecency (like a google search repeated an hundred times).
Since this has an automated test,if QE-verify ends up being tricky to complete, I'd just rely on the test.

Comment 5

[Robert Kaiser]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #4)
> Since this has an automated test,if QE-verify ends up being tricky to
> complete, I'd just rely on the test.

Settings the flags accordingly, thanks (feel free to do that on such on assessment in the future right away).

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8494002 [details] [diff] [review]
patch v1

Review of attachment 8494002 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/UnifiedComplete.js
@@ +70,1 @@
>  const FRECENCY_SEARCHENGINES_DEFAULT = 1000;

Maybe the const should be renamed?

@@ +834,5 @@
>      let match;
>      switch (queryType) {
>        case QUERYTYPE_AUTOFILL_HOST:
>          this._result.setDefaultIndex(0);
> +        // Fallback.

ITYM "fall through"

::: toolkit/components/places/tests/unifiedcomplete/test_searchEngine.js
@@ +23,5 @@
>  
>    yield cleanup();
>  });
> +
> +add_task(function* test_searchEngine_autoFill() {

Sorry, my turn to bitrot someone :) I renamed this file to test_searchEngine_restyle.js and added test_searchEngine_current.js - so you'll want to move this new test to a test_searchEngine_domain.js or something.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Blair McBride [:Unfocused] from comment #6)
> > +        // Fallback.
> 
> ITYM "fall through"

lol, not sure what I was thinking :)

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v2

fixed. waiting for bug 1067888 to reland.

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/fa7e44ef6938

Comment 10

[Marco Bonardo [:mak]]

Oops, forgot to hg add the test :/
https://hg.mozilla.org/integration/fx-team/rev/be31d8414372

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa7e44ef6938
https://hg.mozilla.org/mozilla-central/rev/be31d8414372