Use the new keywords API in autocomplete

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla39
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Autocomplete needs to use the new keywords API, so we can remove the cache from the old one.
Flags: qe-verify-
Flags: in-testsuite?
Flags: firefox-backlog+
Are these bugs good-first candidates in your estimation?
no, not good first.
The other problem is that we need them fixed asap so I'll likely work on them next week.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Duplicate of this bug: 1095424
Blocks: 1140395
No longer blocks: placesAsyncBookmarks
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Blocks: 1146461
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Posted patch patch v1 (obsolete) — Splinter Review
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.
Attachment #8583355 - Flags: review?(ttaubert) → review+
(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.
 https://hg.mozilla.org/integration/fx-team/rev/54f37e45c70b
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla39
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
Flags: needinfo?(mak77)
https://hg.mozilla.org/mozilla-central/rev/e4496ef0ca0b
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Depends on: 1178075
You need to log in before you can comment on or make changes to this bug.