Closed
Bug 1336946
Opened 7 years ago
Closed 7 years ago
Store list of pre-seed top web sites for autocompletion in a file.
Categories
(Firefox :: Address Bar, defect)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: sveta.orlik.code, Assigned: sveta.orlik.code)
References
Details
Attachments
(1 file)
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/55.0.2883.87 Chrome/55.0.2883.87 Safari/537.36 Steps to reproduce: Actual results: For now, the top sites for autocompletion on empty profiles are hard-coded. Expected results: Got to store the list of them in a file.
Component: Untriaged → Location Bar
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8839070 [details] Bug 1336946 - store the list of top URLs in a file, https://reviewboard.mozilla.org/r/113822/#review115366 This is almost done, really, but here are a few small nits / spelling corrections, and a suggestion for the test. :-) ::: toolkit/components/places/UnifiedComplete.js:564 (Diff revision 1) > } > > /** > * Storage object for Prefill Sites. > * add(url, title): adds a site to storage > - * populate() : populates the storage with data (hard-coded for now) > + * populate(json) : populates the storage with JSON data I would just use 'sites' as the parameter name (both in the documentation and the actual code a few lines below). What you're passing is effectively just an array (so we should say that rather than "JSON data" which implies it's a string containing JSON). ::: toolkit/components/places/UnifiedComplete.js:1069 (Diff revision 1) > if (site.uri.host.includes(this._searchString) || > site._matchTitle.includes(this._searchString)) { > let match = { > value: site.uri.spec, > comment: site.title, > + // icon: `page-icon:${site.uri.spec}`, Nit: please remove this again. :-) ::: toolkit/components/places/UnifiedComplete.js:2067 (Diff revision 1) > + fetch("chrome://global/content/unifiedcomplete-top-urls.json") > + .then((responce) => responce.json()) > + .then((json) => PrefillSiteStorage.populate(json)); Nit: here and elsewhere, in English 'response' has an 's' rather than a 'c'. Other nit: when there's only 1 parameter, you can omit the () around the parameters for arrow function definitions, so you can write this as: ```js .then(response => response.json()) .then(sites => PrefillSiteStorage.populate(sites)) ``` Other nit: please indent the .then calls with 2 more spaces (at least, I think that's style, unless eslint complains if you do that). Finally, please add a `.catch()` before the end of the statement, to report any errors: ``` .then(...) .catch(ex => Cu.reportError(ex)); ``` to avoid this failing silently. ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:120 (Diff revision 1) > > yield cleanup(); > }); > > +add_task(function* test_population_from_json() { > + let json = JSON.parse('\ Nit: you can use the backtick (`) to have multiline strings in new JS like this, then you don't need the backslashes :-) ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:130 (Diff revision 1) > + '); > + autocompleteObject.populatePrefillSiteStorage(json); > + > + do_print("Storage is populated from JSON correctly"); > + yield check_autocomplete({ > + search: "son-test", // not "json-test" to avoid style:"autofill" result We should probably also check the autofill result in addition to this test? ::: toolkit/components/places/tests/unifiedcomplete/test_prefill_sites.js:154 (Diff revision 1) > + do_print("The JSON is parsed"); > + let json = yield responce.json(); > + > + do_print("Storage is populated"); > + autocompleteObject.populatePrefillSiteStorage(json); It probably makes sense to check the JSON response yourself here, so e.g. use: ```js let sites = yield response.json(); autocompleteObject.populatePrefillSiteStorage(sites); let lastSite = sites.pop(); let url = NetUtil.newURI(lastSite[0]); // then verify autocomplete for `url.host` gets you the right site ? ```
Attachment #8839070 -
Flags: review?(gijskruitbosch+bugs)
Updated•7 years ago
|
Assignee: nobody → sveta.orlik.code
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8839070 [details] Bug 1336946 - store the list of top URLs in a file, https://reviewboard.mozilla.org/r/113822/#review115392 Nice, thanks!
Attachment #8839070 -
Flags: review?(gijskruitbosch+bugs) → review+
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/139038cf6a9c store the list of top URLs in a file, r=Gijs
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/139038cf6a9c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in
before you can comment on or make changes to this bug.
Description
•