fix(systemaddon): Limit the number of topsites being rendered.

[piatra]

Closes #2945.
Cause of bug seems more of an edge case: if you switch from AS to Tiles you could get different sites presented to you and pin > 12 sites. I managed to get 14 pinned sites on my profile. These show up on PINNED_SITES_UPDATED actions because the reducer does not slice the results.
I moved the slice in the actual component this way we could catch the issue if it happens from other sources as well.
It should not be a problem for people coming over from Tiles because they only had at most 12 tiles.

[coveralls]

Coverage remained the same at 86.407% when pulling c186a0a on piatra:bug-2945-topsite-overflow into 62be2b7 on mozilla:master.

[Mardak]

Any reason not to do this from the reducer? I also see there is basically the same .slice-12 logic in TopSitesFeed which would be annoying to make sure they're kept in sync.

[piatra]

@Mardak agreed. I initially tried to do this in the reducer and import the 12 constant from TopSitesFeed but that results in a circular dependency. So I decided it made more sense (if I have to duplicate the constant) to do it in the component. Should I create a constants file for this?

[Mardak]

@k88hudson any suggestions on this? If we just put the 12 const only in Reducer it would result in more-than-necessary work in TopSitesFeed.refresh and in the passing around of data for TOP_SITES_UPDATED.

Perhaps a step back, the reason why the extra pinned sites appear is that there's a PINNED_SITES_UPDATED that updates the TopSites state. And now taking a closer look at TopSitesFeed._getPinnedWithData, it seems like there could be logic from refresh that should be there too? Maybe instead of PINNED_SITES_UPDATED we just call refresh to dispatch TOP_SITES_UPDATED ?

[k88hudson]

The only disadvantage to handling PINNED_SITES_UPDATED with a refresh is that it might cause some lag on slower computers, which is probably something we don't want;

In terms of sharing a constant, why not just define it in Reducers.jsm and import in TopSites?

[Mardak]

After some chatting with @rlr on irc. It sounds like refresh should be called infrequently probably to avoid re-querying SQL via NewTabUtils.activityStreamLinks.getTopSites(). And there is existing refresh-once-per-15-minutes logic on NEW_TAB_LOAD. If we are only concerned about the SQL overhead, we can cache it locally, e.g.,

async getFrecent(force) {
  if (force || 15 minutes passed) {
    this._frecent = await NewTabUtils.activityStreamLinks.getTopSites()
  }
  return this._frecent;
}
async getLinksWithDefaults(force) {
  let frecent = await this.getFrecent(force);
}

[Mardak]

I suppose frequent refreshes also trigger more broadcasting..

[Mardak]

Move the const from TopSitesFeed to export from Reducers and use that value to slice from both TopSitesFeed and Reducers

[piatra]

@Mardak updated.

[coveralls]

Coverage remained the same at 86.407% when pulling 5f5be92 on piatra:bug-2945-topsite-overflow into 62be2b7 on mozilla:master.

[as-pine-proxy]

Treeherder: 67dfebe927b739952eac628db49ecb70d14f5624