Closed
Bug 1125117
Opened 9 years ago
Closed 9 years ago
Use the new keywords API in autocomplete
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file, 1 obsolete file)
92.66 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Updated•9 years ago
|
Blocks: placesAsyncBookmarks
Comment 1•9 years ago
|
||
Are these bugs good-first candidates in your estimation?
Assignee | ||
Comment 2•9 years ago
|
||
no, not good first. The other problem is that we need them fixed asap so I'll likely work on them next week.
Updated•9 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Updated•9 years ago
|
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Assignee | ||
Updated•9 years ago
|
No longer blocks: placesAsyncBookmarks
Updated•9 years ago
|
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Updated•9 years ago
|
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Comment 5•9 years ago
|
||
let's see how far we are https://treeherder.mozilla.org/#/jobs?repo=try&revision=650820e5830c
Assignee | ||
Comment 6•9 years ago
|
||
Fixed m-b https://treeherder.mozilla.org/#/jobs?repo=try&revision=af9ba9913ea3
Attachment #8583164 -
Attachment is obsolete: true
Attachment #8583355 -
Flags: review?(ttaubert)
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/54f37e45c70b
Flags: in-testsuite? → in-testsuite+
Target Milestone: --- → mozilla39
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
Flags: needinfo?(mak77)
Assignee | ||
Comment 11•9 years ago
|
||
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)
You need to log in
before you can comment on or make changes to this bug.
Description
•