1125117 - Use the new keywords API in autocomplete

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Autocomplete needs to use the new keywords API, so we can remove the cache from the old one.

Comment 1

[Mike Hoye [:mhoye]]

Are these bugs good-first candidates in your estimation?

Comment 2

[Marco Bonardo [:mak]]

no, not good first.
The other problem is that we need them fixed asap so I'll likely work on them next week.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1

Comment 5

[Marco Bonardo [:mak]]

let's see how far we are
https://treeherder.mozilla.org/#/jobs?repo=try&revision=650820e5830c

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Fixed m-b
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af9ba9913ea3

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8583355 [details] [diff] [review]
patch v1.1

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

Mostly nits, no actual complaints. LGTM!

::: browser/base/content/test/general/browser_autocomplete_autoselect.js
@@ +18,4 @@
>  
> +  registerCleanupFunction(function* () {
> +    yield PlacesTestUtils.clearHistory();
> +  });

The old function did return the promise already, but ok.

::: browser/base/content/test/general/browser_autocomplete_enter_race.js
@@ +7,5 @@
>      Services.prefs.clearUserPref("browser.urlbar.unifiedcomplete");
>    });
>  
>    let itemId =
>      PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,

Wanna update this too while you're here?

::: toolkit/components/places/UnifiedComplete.js
@@ +758,5 @@
>  
> +    if (this._searchTokens.length > 0) {
> +      // This may be a Places keyword.
> +      hasFirstResult = yield this._matchPlacesKeyword();
> +      dump("\n\nMatched keyword: " + hasFirstResult + "\n\n");

Forgot to remove a dump() call.

::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +1451,5 @@
> +      // it will override default keywords behavior.  Note that keywords are
> +      // hashed on first use, so while the first query may delay a little bit,
> +      // next ones will just hit the memory hash.
> +      let dontAutoFill = this._currentSearchString.length == 0 || !this._db ||
> +                         (yield PlacesUtils.keywords.fetch(this._currentSearchString));

Maybe move this out like:

let dontAutoFill = this._currentSearchString.length == 0 || !this._db;

if (!dontAutoFill) {
  dontAutoFill = yield PlacesUtils.keywords.fetch(this._currentSearchString);
}

OTOH this doesn't make the condition as clear as it's now... Your call.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> Maybe move this out like:
> 
> let dontAutoFill = this._currentSearchString.length == 0 || !this._db;
> 
> if (!dontAutoFill) {
>   dontAutoFill = yield PlacesUtils.keywords.fetch(this._currentSearchString);
> }
> 
> OTOH this doesn't make the condition as clear as it's now... Your call.

I'll keep my code, cause I find it more readable. Regardless it won't be executed if any of first 2 conditions is true.

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/fx-team/rev/20b4f88db066 for bc1 orange:

https://treeherder.mozilla.org/logviewer.html#?job_id=2421747&repo=fx-team

Comment 11

[Marco Bonardo [:mak]]

sigh, I ran the test before pushing... but I ran the wrong one,
browser_autocomplete_autoselect.js instead of browser_autocomplete_enter_race.js :(

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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