1422867 - Wrong number of Top Sites displayed

Status: NEW

(Firefox :: New Tab Page, defect, P3)

Description

[Andrei Oprea [:andreio]]

I get 11 topsites + 1 empty placeholder although I have a rich enough history + pinned sites.

There is an issue with the way we filter through the pinned topsites (which have undefined entries in order to store position). `getLinksWithDefaults` method returns an array of length 12 but upon inspection it has 11 elements inside.

Debugging the issue I have it tracked down to:
https://github.com/mozilla/activity-stream/blob/master/system-addon/lib/TopSitesFeed.jsm#L129

This information is based on my profile
> withPinned.length // 12
> checkedAdult.length // 10
> pinned.length // 15 Array [ null, null, null, null, null, null, null, null, null, null, 5 more… ] first 9 are undefined them the rest are defined

The result is an array of length 12 with only 11 elements.

Comment 1

[Andrei Oprea [:andreio]]

Maybe fix would be to just filter out null elements before we slice?

Perma-link for previous comment.
https://github.com/mozilla/activity-stream/blob/ce92c43ba20c2f2cc7b18b75ab1b2de3eeab84f9/system-addon/lib/TopSitesFeed.jsm#L129

Comment 2

[Andrei Oprea [:andreio]]

Should we just close this? When initially submitted you could not
* Drag and drop topsites
* Set a topsite in place of an empty placeholder
I think these two features do a lot of good mitigating the issue. 
The source for the bug was switching from tiles to AS where you could have pinned links overflowing the AS topsites limit. I don't think it's likely for users to run into this anymore.

Comment 3

[Ed Lee :Mardak]

I believe the underlying issue is that there are pinned sites that are not visible yet are deduping sites that could have been shown.

There have been recent changes to drag-n-drop but that probably actually makes things worse in that you can drag sites to the second row. And along with the wider layout changes, we switched to a single row, which means there's 8 potentially out of view pinned sites.

Being able to set a top site in an empty placeholder could help the user fill in some site, but if the user doesn't actively do something, it will look like there's missing top sites. Although looking at the screenshots of the original issues, it looks like there isn't even a placeholder and just an empty spot?