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)

defect
Not set
normal

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.
Depends on: 1211726
Component: Untriaged → Location Bar
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
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)
Assignee: nobody → sveta.orlik.code
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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
https://hg.mozilla.org/mozilla-central/rev/139038cf6a9c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Depends on: 1342551
You need to log in before you can comment on or make changes to this bug.