Closed Bug 1445836 Opened 6 years ago Closed 6 years ago

topSites places support

Categories

(WebExtensions :: Untriaged, enhancement, P1)

58 Branch
enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox63 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [topSites])

Attachments

(1 file)

We should decide whether to deprecate or keep support for "places".  It seems worthwhile to have a way to get the full topSites frecency for extensions.  The name can change, the underlying implementation could be handled different.  An alternative is to add support for frecency in the history api.
This would be for fx61 or later.  fx60 will be fixed to continue supporting "places".
Flags: needinfo?(mconca)
Geoff, can you step back from the implementation, and outline what you need?  I'm believe you want a list of links by frecency, unmodified by any Firefox logic.

Current state of ActivityStreams has logic for user blocked sites, de-duping by domain, maybe more.  
The places provider also has some level of de-duping and removing blocked sites.  I'm not certain there is much difference between the two now.

Will need input from Ed on long term support on some of this, but I could see the api changing to provide some options to turn off some of this functionality.  Or we could consider providing frecency results via the history api.
Flags: needinfo?(geoff)
A static analysis of all extensions on AMO currently using the topSites API shows that only one, New Tab Tools [1], is using the places parameter. Managing any change to the API should be relatively easy, and I see Shane brought Geoff (author) into the conversation (thanks).

Having a way for extensions to enumerate frequency in topSites (or history) is a good idea. To me, much depends on the engineering opinion of what isn't too hard to implement and what can be maintained into the future based on the underlying feature set.

We should absolutely target any change for release 61.

[1] https://addons.mozilla.org/en-US/firefox/addon/new-tab-tools/
Severity: normal → enhancement
Flags: needinfo?(mconca)
Whiteboard: [topSites]
Target Milestone: --- → mozilla61
I think if it more people (that is, anybody) knew about the existing API as implemented more extensions would use it. Seems silly to expect an extension to use the same (modified) list of links as Firefox does.

All I'm after is a list of links in descending order of frequency, with no modifications. I find 100 is plenty, although the ability to ask for more would be nice. I have no problem with changing my extension if necessary.
Flags: needinfo?(geoff)
Blocks: 1444579
No longer depends on: 1444579
Here is what is desired from a Product perspective for the topSites API:

1) A way for the topSites() API to enumerate sites as Firefox would enumerate them on the default New Tab page. This allows extension developers to offer a different New Tab, but with Top Sites reported as default Firefox would report them (maintaining consistency for users who want that).  This list could come from Activity Stream or places, whatever makes the most sense from an engineering point-of-view.  The source, though, should be the one used by the product and expected to be maintained into the future. Any changes to the internal algorithm for blocked or de-duped sites would always reflect out through the API.

2) A way for the topSites() API to enumerate a basic list of sites in descending order of frequency, no filtering or modifications. This allows extensions to provide their own algorithm and methods for displaying top sites to their users. It is preferable if this can be provided with new/additional parameters to the existing topSites() API.

Ed, would you mind commenting on the feasibility of implementing what is described above.  As Shane mentions in Comment 0, ignore the current API that uses "places" - the name can change, the underlying implementation can change.  I'm looking for a way to provide extension devs with a way to get 1) the default Firefox list of top sites, and 2) an unfiltered, ordered list of top sites.
Flags: needinfo?(edilee)
(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #5)
> 1) A way for the topSites() API to enumerate sites as Firefox would enumerate them on the default New Tab page.
For (1), there's a NewTabUtils getTopFrecentSites method that activity stream calls:
https://searchfox.org/mozilla-central/rev/59a9a86553e9bfd9277202748ff791fd9bc0713b/toolkit/modules/NewTabUtils.jsm#1073-1176

Currently it gets some number of urls with the highest frecency over an optional threshold and picks one url per "host" ignoring "www." as well as optionally removing blocked urls:
https://github.com/mozilla/activity-stream/blob/9339e04f9dd0cb5ac1b1e3c105cfd00a7ddd9ec8/system-addon/lib/TopSitesFeed.jsm#L87-L89

Additionally activity stream combines the urls from getTopFrecentSites with default sites and pinned sites with some deduping logic and filtering of adult sites based on an about:config preference. Not affecting the list of urls, activity stream also adds in favicons, custom screenshots, and thumbnails.

Sounds like the intention of (1) is to get at the exact url list after activity stream has done all of its processing. If so, there's currently no exposure of TopSitesFeed other than as a resource://…jsm, which I suppose could be imported and instantiated, although that would lead to duplicate instances with their own duplicate links cache, etc. Although there is an about:config preference that could turn off activity stream's instance, so a separate instance might be desired?

> 2) A way for the topSites() API to enumerate a basic list of sites in
> descending order of frequency, no filtering or modifications.
What exactly should a site be? As hinted at above, activity stream shows urls with some fuzzy logic of picking a highest frecency url of a www./non-www. host. I suppose if getTopFrecentSites's definition of site is acceptable, then seems like this API could be used passing in an argument to not filter out blocked urls.

Although, having multiple urls from the same host could be desired too. E.g., showing a "site" for each subreddit.
Flags: needinfo?(edilee) → needinfo?(mconca)
Thanks for the great info, Ed.  Release 61 got filled with other (higher priority) things, so I'm coming back to this now for 62.  For (1), I'm not excited to import and instantiate the TopSitesFeed jsm file - that sounds ... non-optimal. I'm going to loop Shane back into this conversation to get a real WebExtensions engineering perspective on this, but my preference would probably be to loosen the requirement to provide "exactly" what activity stream is showing.

So maybe the right answer is - simply use getTopFrecentSites as the underlying implementation for the topSites WebExtension API.

Thoughts, Shane?
Flags: needinfo?(mconca) → needinfo?(mixedpuppy)
I'm not concerned about the loading if the jsm.  it would be lazy.  But I'm not clear where we're landing on the feature functionality.

a) both topSites matching our default newtab (which is what the api is supposed to be) plus an unfiltered list
b) just matching our newtab
c) just the unfiltered list

I personally like (a) but am more concerned with long term stability of the api.

IIUC we'll get this done for 62.
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Priority: -- → P1
If there is no concern with loading the jsm, then I'm also in favor of (a).  Can the API be designed such that it 1) provides backward compatibility with the current Chrome API, and 2) also offers an unfiltered list?

Targeting this for 62 would be ideal.
I just re-read this, I didn't really catch that the TopSites.jsm was in an extension.  

So, I think the default should switch to using NewTabUtils getTopFrecentSites for the default, so long as there is long term commitment to that api.

We could consider adding a couple options to it, such as not reducing the result to one link per host.

We punt on any api that provides the activity stream results to extensions, which IIUC means topSites will not have access to a list matching what would appear on our newtab page.  Given that was the purpose of the api, the result is we will be different than other browsers.  I'm not certain it's a big deal.  We can document the difference.
Flags: needinfo?(mconca)
Yeah, as I mentioned in comment 7, using getTopFrecentSites probably makes the most sense. Let's confirm it will be around for a while (my assumption is it will, given that activity stream uses it) and see if it has any options we might want to expose.
Flags: needinfo?(mconca)
Proposed api changes

browser.topSites.get({
 providers: deprecated
 limit: max number of sites to return
 dedup: whether or not to dedup
 threshold: the frecency threshold (not sure what a maximum should be)
});

This requires some minor changes to NewTabUtils.jsm.  Patch coming.
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

The primary feedback I'm looking for is long term support of the api in NewTabUtils, at least as much as we can anticipate.  Secondary is the limits/etc. in top_sites.json.  I think these options I'm proposing make more sense as they get to the core of what was wanted from the api.  Defaults here should pretty much match what we might use in Firefox from this api.
Attachment #8976748 - Flags: feedback?(edilee)
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

https://reviewboard.mozilla.org/r/244846/#review255966

Mostly questions about what we intend to expose and support going forwards. In particular frecency is something pretty Firefox specific so making it more cross-browser might make sense? (I'm not really sure what needs to be considered for cross-browser extension stuff.)

::: toolkit/components/extensions/parent/ext-topSites.js:14
(Diff revision 1)
>    getAPI(context) {
>      return {
>        topSites: {
> -        get: function(options) {
> -          return new Promise((resolve) => {
> -            NewTabUtils.links.populateCache(async () => {
> +        get: async function(options) {
> +          let links = await NewTabUtils.activityStreamLinks.getTopSites({
> +            ignoreBlocked: true,

Do we want to always ignore blocked? This means someone installing an extension will suddenly see all the pages that were removed. Although that gives the extension an opportunity to build its own blocking mechanism in case the default one is not desired.

::: toolkit/components/extensions/schemas/top_sites.json:41
(Diff revision 1)
>              "optional": true,
>              "description": "The title of the page."
> +          },
> +          "favicon": {
> +            "type": "string",
> +            "description": "Data URL for the favicon."

Do we want to expose the favicon now? Seems like extensions were okay without it (but what did they show…)? I suppose it's fine either way. I suppose this could get large, but at least it's not page thumbnail data uri large. ;)

::: toolkit/components/extensions/schemas/top_sites.json:76
(Diff revision 1)
> +              },
> +              "dedup": {
> +                "type": "boolean",
> +                "default": true,
> +                "optional": true,
> +                "description": "Deduping the top sites list will result in a single top site link per domain"

Maybe the flag could be named along the lines of "onePerDomain" ?

::: toolkit/components/extensions/schemas/top_sites.json:83
(Diff revision 1)
> +              "threshold": {
> +                "type": "integer",
> +                "default": 150,
> +                "minimum": 0,
> +                "optional": true,
> +                "description": "Firefox uses a frecency value (frequent and recent) for visisted links.  Links returned must have a frecency higher than the threshold (defaults to 150)."

I'm not sure if we want to expose frecency directly. A single plain page visit is 100 frecency, so I wonder if we can approximate this by having the extension specify a generic threshold? Internally we multiply that number (should allow fractional??) by 100 to get a frecency threshold.

The reason activity stream picked 150 is that an unvisited bookmark has 140 frecency. So applying some more magic conversion, extension says minimum 1 visit threshold becomes topsiteFrecency: (threshold + 1) * 100 - 1 = 199 ?? It's not exactly the same, but should be pretty close to what the extension might intend without actually checking visit count directly…

"A minimum threshold that is approximately the number of visits to a page, e.g., a threshold of 1 excludes pages that the user visited once."

::: toolkit/modules/NewTabUtils.jsm:1087
(Diff revision 1)
>    async getTopFrecentSites(aOptions) {
>      const options = Object.assign({
>        ignoreBlocked: false,
>        numItems: ACTIVITY_STREAM_DEFAULT_LIMIT,
> -      topsiteFrecency: ACTIVITY_STREAM_DEFAULT_FRECENCY
> +      topsiteFrecency: ACTIVITY_STREAM_DEFAULT_FRECENCY,
> +      dedup: true,

We can update the xpcshell test for the additional options similar to https://searchfox.org/mozilla-central/rev/3737701cfab93ccea04c0e9cab211ad10f931d87/toolkit/modules/tests/xpcshell/test_NewTabUtils.js#663-667

::: toolkit/modules/NewTabUtils.jsm:1139
(Diff revision 1)
>           other.lastVisitDate === link.lastVisitDate && other.url < link.url);
>      }
>  
> +    if (!options.dedup) {
> +      links = links.map(link => {
> +        link.type = "history";

I suppose it's just a minor optimization that we only add the type after deduping and block filtering, but we can just move this into the query to return "history" in a type column.

(I see that the extension API doesn't care about the type anyway, but probably good to keep the return consistent.)

::: toolkit/modules/NewTabUtils.jsm:1142
(Diff revision 1)
> +    if (!options.dedup) {
> +      links = links.map(link => {
> +        link.type = "history";
> +        return link;
> +      }).sort(isOtherBetter).slice(0, origNumItems);
> +      return this._faviconBytesToDataURI(await this._addFavicons(links));

The results are already coming back highest frecency first, so this additional sorting shouldn't really be necessary.

Also, the slicing shouldn't be necessary either as origNumItems is the same as options.numItems for this code path.
Attachment #8976748 - Flags: feedback?(edilee) → feedback+
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

https://reviewboard.mozilla.org/r/244846/#review255966

The use of the api by default needs to be compatible with other browsers.  The options we're adding here are perfectly fine to be firefox specific.

My larger concern is whether we forsee any changes that would make any of the proposed changes either impossible to keep, or preventing us from later moving forward.

> Do we want to always ignore blocked? This means someone installing an extension will suddenly see all the pages that were removed. Although that gives the extension an opportunity to build its own blocking mechanism in case the default one is not desired.

It could be an option on the api rather than default.  In fact, I think I'll do that.

> Do we want to expose the favicon now? Seems like extensions were okay without it (but what did they show…)? I suppose it's fine either way. I suppose this could get large, but at least it's not page thumbnail data uri large. ;)

Actually there are a handful of bugs around getting favicons.  This could also be an option on the api call.

> Maybe the flag could be named along the lines of "onePerDomain" ?

+1
Well, the main API change to NewTabUtils is adding the extra "onePerDomain" option, which seems reasonable. I suppose if activity stream ever went to showing multiple per domain, the API would then need to be maintained, but I guess that's unlikely and fine to support.

Similarly, the extension dependency makes it harder to switch away from topsiteFrecency to a theoretical topsiteMinVisits. Although in that case, we could fix up the existing callsites to convert to a minVisits if we don't want the API to support both frecency and minVisits. In particular for extensions, it's exposed as a "threshold" so changing around the internals should be okay?
Product: Toolkit → WebExtensions
(In reply to Ed Lee :Mardak from comment #15)
> Comment on attachment 8976748 [details]

> ::: toolkit/components/extensions/schemas/top_sites.json:76

> visit threshold becomes topsiteFrecency: (threshold + 1) * 100 - 1 = 199 ??
> It's not exactly the same, but should be pretty close to what the extension
> might intend without actually checking visit count directly…
> 
> "A minimum threshold that is approximately the number of visits to a page,
> e.g., a threshold of 1 excludes pages that the user visited once."

That doesn't seem to hold true.  Frecency seems to be about visit * 10, and least in my basic testing, but I'm not certain that would be consistent either.  The above calculation requires 20+ visits for the site to be included.

> ::: toolkit/modules/NewTabUtils.jsm:1142
> (Diff revision 1)
> > +      }).sort(isOtherBetter).slice(0, origNumItems);
> > +      return this._faviconBytesToDataURI(await this._addFavicons(links));
> 
> The results are already coming back highest frecency first, so this
> additional sorting shouldn't really be necessary.

The sorting does more than just frecency, so keeping it to remain consistent with the later sorting.
(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> (In reply to Ed Lee :Mardak from comment #15)
> > Comment on attachment 8976748 [details]
> 
> > ::: toolkit/components/extensions/schemas/top_sites.json:76
> 
> > visit threshold becomes topsiteFrecency: (threshold + 1) * 100 - 1 = 199 ??
> > It's not exactly the same, but should be pretty close to what the extension
> > might intend without actually checking visit count directly…
> > 
> > "A minimum threshold that is approximately the number of visits to a page,
> > e.g., a threshold of 1 excludes pages that the user visited once."
> 
> That doesn't seem to hold true.  Frecency seems to be about visit * 10, and
> least in my basic testing, but I'm not certain that would be consistent
> either.  The above calculation requires 20+ visits for the site to be
> included.

Given https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Places/Frecency_algorithm

I don't think it makes sense to try and correlate the threshold to visits in the way described above.  If we end up switching to a pure numVisits model in the future, we'll just have to deal with that.  Perhaps that option should simply be removed.
Flags: needinfo?(edilee)
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

https://reviewboard.mozilla.org/r/244846/#review261082

The webextensions part looks good to me, follows a first round of review comments (two small ones marked as issues, and one questions).

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:50
(Diff revision 2)
>        "permissions": [
>          "topSites",
>        ],
>      },
>      background() {
>        // Tests consistent behaviour when no providers are specified.

Nit, this inline comment looks outdated now, `no provider` should be probably changed to `no options`.

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:71
(Diff revision 2)
> -    extension.sendMessage(providers);
> +    extension.sendMessage(options);
>      return extension.awaitMessage("sites");
>    }
>  
> -  Assert.deepEqual(visits, await getSites(), "got topSites");
> -  Assert.deepEqual(visits, await getSites({}), "got topSites");
> +  Assert.deepEqual([visits[0], visits[2]], await getSites(), "got topSites");
> +  Assert.deepEqual(visits, await getSites({onePerDomain: false}), "got topSites");

it seems that we should also add a test for `ignoreBlocked: true`.
(I see that this patch already has a test at "NewTabUtils.jsm "unit level, but that test doesn't actually ensure that the option is being exposed and used correctly by the WebExtensions API method).

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js
(Diff revision 2)
> -  Assert.deepEqual(visits, await getSites({providers: ["places"]}), "got topSites");
> -  Assert.deepEqual(visits, await getSites({providers: ["activityStream"]}), "got topSites");
> -  Assert.deepEqual(visits, await getSites({providers: ["places", "activityStream"]}), "got topSites");

I've been wondering:
for the deprecated `providers` option, do we want to raise a warning but still support them for now? (to be completely removed after some versions)

If we plan to keep them as supported for some versions, it could be reasonable to keep the assertions that were previosuly testing their behavior, otherwise we could add a test that ensure that we raise the deprecation message as an error.
Attachment #8976748 - Flags: review?(lgreco)
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

https://reviewboard.mozilla.org/r/244846/#review261082

> I've been wondering:
> for the deprecated `providers` option, do we want to raise a warning but still support them for now? (to be completely removed after some versions)
> 
> If we plan to keep them as supported for some versions, it could be reasonable to keep the assertions that were previosuly testing their behavior, otherwise we could add a test that ensure that we raise the deprecation message as an error.

I don't think we need anything special here, we have one known consumer of "providers" who is cc'd on this bug.
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

https://reviewboard.mozilla.org/r/244846/#review261674

::: toolkit/components/extensions/schemas/top_sites.json:39
(Diff revision 3)
>            "title": {
>              "type": "string",
>              "optional": true,
>              "description": "The title of the page."
> +          },
> +          "favicon": {

It seems that `favicon` should also be optional.

This also seems to be the reason for the failures of the new xpcshell test in debug builds (where there is actually a validation of the properties included in a resolved result), e.g.:
- error logged on a windows debug build: https://treeherder.mozilla.org/logviewer.html#?job_id=185995731&repo=try&lineNumber=9884-9885
- error logged in a linux debug build: https://treeherder.mozilla.org/logviewer.html#?job_id=185991845&repo=try&lineNumber=1983-1984

We only validate the returns and callback parameters in debug builds:
- https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/toolkit/components/extensions/Schemas.jsm#2347-2353
- https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/toolkit/components/extensions/Schemas.jsm#2322-2327
- https://searchfox.org/mozilla-central/rev/403038737ba75af3842ba6b43b6e2fb47eb06609/toolkit/components/extensions/Schemas.jsm#2316-2319

Also, in comment 16 it seems that you were evaluating to include the `favicon` only if an options is being specified by the extension to explicitly ask the API method to include them, is this still something that you are planning to do in this patch?

It seems a good idea, given that the extensions are not currently expecting the favicon to be part of the results and that if the extension doesn't need the favicon we could spare a bit of memory by not including them.
Attachment #8976748 - Flags: review?(lgreco)
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

https://reviewboard.mozilla.org/r/244846/#review261902

::: toolkit/modules/NewTabUtils.jsm:1194
(Diff revisions 3 - 4)
> -    }
> +      }
>  
> -    // Pick out the top links using the same comparer as before
> +      // Pick out the top links using the same comparer as before
> -    links = [...hosts.values()].sort(isOtherBetter).slice(0, origNumItems);
> +      links = [...hosts.values()].sort(isOtherBetter).slice(0, origNumItems);
> +    } else {
> +      links = links.sort(isOtherBetter);

Oh, I was going to suggest just indenting the existing code to share the sorting/slicing, so this is getting pretty close. :)

Perhaps
```js
if (onePerDomain) {
  …
  links = [...hosts.values()];
}

// Pick out the top links…
links = links.sort(isOtherBetter).slice(0, origNumItems);
```
Attachment #8976748 - Flags: review?(edilee) → review+
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

https://reviewboard.mozilla.org/r/244846/#review261894

Hi Shane, the webextensions part of thispatch looks pretty good to me, this round of review comments in 98% of nits and optional changes.

The only reason why I'm not setting it as r+ yet is because of the two questions related to the minimum set on the limit parameter (and the test for the limit option currently missing) and some doubts about the option named "ignoreBlocked", on which I would like to hear your opinion first.

::: toolkit/components/extensions/schemas/top_sites.json:69
(Diff revision 4)
> +              },
> +              "limit": {
> +                "type": "integer",
> +                "default": 12,
> +                "maximum": 100,
> +                "minimum": 12,

I see that the default number of items used by activityStream is currently 12, but I'm wondering:
is there a reason to enforce the minimum to 12 on the API method?

::: toolkit/components/extensions/schemas/top_sites.json:79
(Diff revision 4)
> +                "type": "boolean",
> +                "default": true,
> +                "optional": true,
> +                "description": "Limit the result to a single top site link per domain"
> +              },
> +              "ignoreBlocked": {

I'm keeping to think about this option name, I'm not sure that "ignoreBlocked" makes immediately clear what is its purpose.

I'm wondering if calling this "includeBlocked" could make its purpose more immediately clear to the addon developer (personally, every time I read "ignoreBlocked" I had to come back to this description and double-check if "ignore" means "not  include them" or "include them").

(and I'm not saying that we have to change option name used internally by NewTabUtils, after all `limit` is named `numItems` internally and so there is already an option that is translated internally by the API method implementation, the only one that concerns me is the option name exposed to the addon developers)

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:8
(Diff revision 4)
>  ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");
>  ChromeUtils.import("resource://gre/modules/NewTabUtils.jsm");
> +ChromeUtils.import("resource://testing-common/PlacesTestUtils.jsm");
> +
> +// A small 1x1 test png
> +const image1x1 = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVQI12NgAAAAAgAB4iG8MwAAAABJRU5ErkJggg==";

Nit, we could use an upcase name for this const (but it is not a big deal, and so I'm not marking as an issue, it is just a totally optional change).

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:23
(Diff revision 4)
>        visit.visits.push({date: new Date(visitDate)});
>      }
>      visits.push(visit);
>    }
> +  // Stick a couple sites into history.
> +  for (let i = 0; i < 2; ++i) {

This patch is not explicitly testing the `limit` parameter, it seems reasonable to me to add more visits here and then test that setting the limit to its minimum value (currently 12) is returning the expected number entries.

(or if we decide that an extension should be allowed to also use 1 as a limit, we could just add the additional test case with the current 2 visits that we are creating in this test)

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:82
(Diff revision 4)
>  
> -  Assert.deepEqual(visits, await getSites(), "got topSites");
> -  Assert.deepEqual(visits, await getSites({}), "got topSites");
> -  Assert.deepEqual(visits, await getSites({providers: ["places"]}), "got topSites");
> -  Assert.deepEqual(visits, await getSites({providers: ["activityStream"]}), "got topSites");
> -  Assert.deepEqual(visits, await getSites({providers: ["places", "activityStream"]}), "got topSites");
> +  Assert.deepEqual([visits[0], visits[2]], await getSites(), "got topSites default");
> +  Assert.deepEqual(visits, await getSites({onePerDomain: false}), "got topSites all links");
> +
> +  NewTabUtils.activityStreamLinks.blockURL(visits[0]);
> +  ok(NewTabUtils.blockedLinks.isBlocked(visits[0]), "link is blocked");

Nit, it could be nice to turn this "link is blocked" message into a template string and make the blocked URL part of the assert message (which would make much more clear what we expect to happen while looking at the test logs)

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:84
(Diff revision 4)
> +  Assert.deepEqual([visits[2], visits[1]], await getSites(), "got topSites with blocked links");
> +  Assert.deepEqual([visits[0], visits[2]], await getSites({ignoreBlocked: true}), "got topSites ignoring blocked link");

Nit, I would prefer this two messages to be a bit more specific and change them to something like 

- "got topSites with blocked links filtered out".
- "got topSites with blocked links included"

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:89
(Diff revision 4)
> +  Assert.deepEqual([visits[2], visits[1]], await getSites(), "got topSites with blocked links");
> +  Assert.deepEqual([visits[0], visits[2]], await getSites({ignoreBlocked: true}), "got topSites ignoring blocked link");
> +
> +  // Test favicon result
> +  let topSites = await getSites({ignoreBlocked: true, includeFavicon: true});
> +  equal(topSites[0].favicon, image1x1, "recieved favicon");

Nit, typo: recieved => received
Attachment #8976748 - Flags: review?(lgreco)
Comment on attachment 8976748 [details]
Bug 1445836 - finalize topSites api on platform APIs that will be stable,

https://reviewboard.mozilla.org/r/244846/#review262034
Attachment #8976748 - Flags: review?(lgreco) → review+
In practice, users visiting a top site recently will get at least 100 frecency per visit, so estimating a threshold of 1 being similar to 1 visit as a minimum of 199 frecency would be pretty close. I agree that the actual frecency algorithm makes that not quite exact as what you ran in to with default bucket 10 instead of recent bucket 100 as well as other types of visits making it not quite 100, but it's a pretty close approximation for "more than 1 top site visit."

And as we also separately discussed, we don't need to expose a threshold and come up with a frecency calculation if we just use the default 150 minimum frecency that's already in NewTabUtils. It again is an approximation, but it probably won't make much of a difference for users with browsing history wanting to see their top sites with an extension.
Flags: needinfo?(edilee)
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fc0d9d4bf1bf
finalize topSites api on platform APIs that will be stable, r=Mardak,rpl
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/fc0d9d4bf1bf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I've updated https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/topSites/get for this, and the compat data: https://github.com/mdn/browser-compat-data/pull/2783 (but the compat data isn't yet deployed to the Wiki).

Please let me know if this looks good.
Flags: needinfo?(mixedpuppy)
lgtm
Flags: needinfo?(mixedpuppy)
You need to log in before you can comment on or make changes to this bug.