1634279 - Allow accessing the TopSites feed even if Top Sites are disabled

Status: VERIFIED FIXED

(Firefox :: New Tab Page, task, P2)

Description

[Marco Bonardo [:mak]]

For the urlbar we need a way to access the TopSites feed even when Top Sites are disabled. The code seems to always clear the TopSites store when the pref is set. Though it's complicate to follow all the code paths in AS, so we're blocked on it, hints would be welcome.
We currently use AboutNewTab.getTopSites().

Comment 1

[Jessilyn Davis]

Hey Mak! Our team doesn't have bandwidth to tackle this this release. It it something that you/Search team can tackle? We'd be happy to review a patch.

Comment 2

[Marco Bonardo [:mak]]

if you can give us some pointers about where to attack the problem, it would be very useful.

Comment 3

[Jessilyn Davis]

Scott, can you point in the right direction and/or tag in the right person (from UJET/Tim?) to answer?

Comment 4

[Scott [:thecount] Downe]

Hey Marco,

This feels kinda tricky to me.

It'll probably first help to understand what causes the store to be dropped when the pref is set.

I'll start with what I know/can piece together, hopefully that helps.

The way these feed prefs work is each pref is attached to a jsm, in this case, the jsm is resource://activity-stream/lib/TopSitesFeed.jsm

You can see here: https://searchfox.org/mozilla-central/source/browser/components/newtab/lib/ActivityStream.jsm#657

All of these prefs have a factory which calls some constructor if the value is true.

Feeds have a constructor and lifecycle methods, examples, init, observe, and onAction as examples used by topsitesfeed.jsm.

If you set the feed pref to false, which is what happens when you turn off topsites in about:preferences#home it removes that feed, along with all it's state. I'm not sure yet where in the code it clears the state, but this isn't the only concern around this. Even if you keep that state around, if you restart the browser, the jsm never gets run so that state would never be rehydrated.

This makes what you're trying to do kinda tricky.

I am thinking if we're ok with topsites running in the background for everyone even if they toggle it off, and only having that toggle control the visibility of topsites on newtab, we need to decouple the prefs somehow.

Comment 5

[Marco Bonardo [:mak]]

Yes, potentially we could leave things as they are, but introduce a new pref controlled by about:preferences (replace the current one) that will only hide TopSites from being shown, not disable the feed. Though at that point we'd need to migrate the old pref to the new one, and set the old pref to true. There we should understand if there could be privacy problems with that (I guess not, but better safe than sorry).
The TopSites feed anyway is likely running for most users, so having it around should be ok from a perf point of view.

Comment 6

[Marco Bonardo [:mak]]

We have 2 possible approaches here:

1. Rename the feed, so it will depend on a new pref, that of course will be true by default. Introduce a new pref and migrate the old feed pref into it. This pref only controls whether Top Sites are visible in the New Tab page, but not influence the feed itself, that will have its own pref. If the user downgrades, the old pref will still be there to control the feed with the old name.

2. this is based on an experimental patch by Scott; create a new feed, make both the old and the new feed use the same factory, then one feed is used by the New Tab page, while the other can be use by the urlbar. It's unclear though why disabling one feed doesn't disable both, and if this may have unexpected effects since it was never tried before.

Off-hand approach one seems less risky.

Comment 7

[Harry Twyford]

The pref I'm adding to control the top sites feed is feeds.topsitesData, accessible only in about:config. feeds.topsites will become a pref that simply controls whether Top Sites are shown on NTP.

In the current version of my patch, disabling feeds.topsitesData but enabling feeds.topsites leads to Top Sites being shown on NTP, but they're never refreshed. Should we bother addressing this case? The only way to hit it is to mess with about:config. I plan on landing this patch in a stack with bug 1623666 and bug 1627858, so there will never really be any reason (that I can discern) for a user to disable feeds.topsitesData, let alone disabling it and leaving feeds.topsites enabled.

Comment 8

[Harry Twyford]

Attachment: Bug 1634279 - Allow accessing the TopSites feed even if Top Sites are hidden on the new tab page. r?thecount,dao,mixedpuppy

Creates a new pref, "...activity-stream.feeds.topSitesData" to control whether TopSitesFeed is enabled. The existing "...activity-stream-feeds.topSites" pref is kept to allow users to downgrade and to ensure that people who disabled Top Sites in the past will continue to not see Top Sites on the New Tab Page -- the difference being that Top Sites will now be generated for those users.

Please note that, per comment 7 in the bug, this patch has no special handling for the case where the user disables topsitesData in about:config but enables topsites. I left that out since handling that looks like it would require a decent amount of work given Activity Stream's special pref handling in PrefsFeed/AboutPreferences.jsm. I'm happy to add handling there if we deem it necessary.

Comment 9

[Harry Twyford]

This patch will tweak the behaviour of the topSites.get() WebExt API. Summary of the changes:

Firefox 78 introduces a new preference to enable Top Sites, which is on by default. If the user disables this preference, the newtab parameter passed to topSites.get() will be ignored. A list of most-visited sites – rather than a list of Top Sites as they appear on about:newtab – will be returned regardless of the value of the newtab parameter.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a842554c303
Allow accessing the TopSites feed even if Top Sites are hidden on the new tab page. r=dao,thecount,mixedpuppy

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4a842554c303

Comment 12

[Marius Coman [:mcoman], Ecosystem QA]

I have verified this task and I can confirm the following:

• The browser.newtabpage.activity-stream.feeds.system.topsites pref was added to the "about:config" page.
• The Top Sites tiles are still displayed on the New Tab page if the browser.newtabpage.activity-stream.feeds.system.topsites is set to false
• The "Top Sites" are never refreshed if the browser.newtabpage.activity-stream.feeds.system.topsites is set to false

Verified using the latest Firefox Beta (78.0b9 Build ID - 20200619002543) and the latest Firefox Nightly (79.0a1 Build ID - 20200622093309) installed on Windows 10 x64, Mac 10.15, and Ubuntu 18.04 x64.

Comment 13

[Will Bamberg [:wbamberg]]

I've updated the compat data for this in https://github.com/mdn/browser-compat-data/pull/6301, to add a note based on https://bugzilla.mozilla.org/show_bug.cgi?id=1634279#c9 and this note is now being shown on pages: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/topSites/get#Browser_compatibility.

I'm marking this dev-doc-complete, but please let me know if you need anything else.