Add an API to get the top frecent sites

[gabrielluong]

Fixes #2163

Pull Request checklist

• Quality: This PR builds and tests run cleanly
  • automation/all_tests.sh runs to completion and produces no failures
  • Note: For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
  • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
• Dependencies: This PR follows our dependency management guidelines
  • Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

[gabrielluong]

@linacambridge This should be ready for review. I don't have permissions to request reviews.

[mhammond]

A bit of a drive-by - I don't think the name topSiteFrecency is very descriptive - I had no idea what it meant when first reading this. More generally though, I'm not sure it makes sense to have this as part of the API - I don't think we want consumers of this API to try and understand what an absolute frecency value actually means - ie, the number should be seen as only relative to other frecencies (ie, more or less frecent than another site) but not that, say "anything under X isn't frecent" or "anything over X is very frecent"

[gabrielluong]

A bit of a drive-by - I don't think the name topSiteFrecency is very descriptive - I had no idea what it meant when first reading this. More generally though, I'm not sure it makes sense to have this as part of the API - I don't think we want consumers of this API to try and understand what an absolute frecency value actually means - ie, the number should be seen as only relative to other frecencies (ie, more or less frecent than another site) but not that, say "anything under X isn't frecent" or "anything over X is very frecent"

Totally understandable. Some of the options and naming that were used here originated from prior art of NewTabUtils.jsm#getTopFrecentSites https://searchfox.org/mozilla-central/rev/fc91a093e40dde71d10ad219946b8ae775aca9eb/toolkit/modules/NewTabUtils.jsm#1279-1284,1367-1390.

I don't personally feel too strongly about adding this as part of the API. My main concern was that getTopFrecentSites would start returning the first visited site you go to on a fresh profile of Fenix. Adding topSiteFrecency or perhaps better named frecencyThreshold provided some flexibility to avoid that.

Let me know if this additional information changes your thoughts on the API, happy to make any changes to it and get some additional eyes on this APi. Thanks!

[mhammond]

My main concern was that getTopFrecentSites would start returning the first visited site you go to on a fresh profile of Fenix.

My main concern is that if Fenix starts specifying a frecency of (say) 200 to avoid this problem, it means we will be largely unable to ever change the algorithm - and we certainly want to reserve the right to change it (eg, see #610)

Indeed, this would argue for never actually returning the frecency - if we return the value it's inevitable that it will end up with meaning. I wonder if we should just return the order?

While I don't see that behaviour on a fresh profile as an actual problem, if we really do want to prevent that, I guess there are a couple of options:

• Use something like (local/remote) visit count as a proxy - eg, maybe a site must be visited twice to qualify? But even this is subtle - something visited twice by following a link isn't necessarily "better" than a site visited once because the user explicitly typed the address. Similarly, the site having 1 visit but being bookmarked probably means it should be returned.

• Have the implementation handle it internally - ie, still filter, but filter internally without the app having the ability to control the threshold. Given how subtle frecency is, this seems the best option, but it does remove flexibility from the containing app.

@lina probably has opinions here and I'll be happy to defer to them, so let's see what she says tomorrow.

[linabutler]

This looks great, modulo comments about hiding the frecency threshold param. Thanks so much for working on this, @gabrielluong!

[gabrielluong]

Thanks for the reviews and comments. I will make the changes to remove the frecency params before asking for this to land.

The pull request has been modified, dismissing previous reviews.

[linabutler]