1568617 - Extend the top sites API to optionally return TopSitesFeed sites for small history

Status: RESOLVED FIXED

(WebExtensions :: General, task, P1)

Description

[Drew Willcoxon :adw]

Verdi says that the top sites experiment should show the same sites that newtab does for small histories, i.e. the built-in default suggested sites (bug 1547669 comment 16). So we'll need to add that to the top sites webextension API, behind an option.

Comment 1

[Ed Lee :Mardak]

Just commenting here that an early patch for bug 1547669 had some discussion about defaults and activity-stream data:
https://phabricator.services.mozilla.com/D36200#1085550

And specifically about default sites for small history, those urls are available from
browser.newtabpage.activity-stream.default.sites

… set as (runtime) default prefs based on the geo/region:
https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/browser/components/newtab/lib/ActivityStream.jsm#70-104

Comment 2

[Drew Willcoxon :adw]

Thanks Ed. I think what you said here in Marco's abandoned revision is basically what we should do: https://phabricator.services.mozilla.com/D36200?id=123964#1084157 Export a function in a JSM related to newtab or activity stream that we can call from the extension API that returns exactly what newtab uses.

Comment 3

[Drew Willcoxon :adw]

The default top sites don't have favicons if they're unvisited. I see they have a tippyTopIcon like this:

"tippyTopIcon": "resource://activity-stream/data/content/tippytop/images/[e-mail]",

I guess we can use those as favicons? We'll need to either convert them to data URIs like favicons and include them in the sites returned by browser.topSites.get() like other icons, or for our purposes with our top sites experiment, we could just use the resource URIs directly in our urlbar back-end via UrlbarProviderExtension (I think).

Comment 4

[Drew Willcoxon :adw]

...or just pass along the resource URI. That seems to work.

Comment 5

[Drew Willcoxon :adw]

Attachment: Bug 1568617 - Add a `newtab` option to browser.urlbar.topSites.get that returns the top sites exactly as shown on newtab

This is based on D39589, which moves the top sites API from toolkit to browser.

• Add a newtab option to browser.urlbar.topSites.get (similar to the abandoned D36200) that returns exactly the top sites shown on newtab
• Add a AboutNewTab.getTopSites function that returns those sites from activity stream
• Keep the changes made in bug 1547669

There are a couple of missing things related to the default top sites that this doesn't address but ideally we would have. I think we can come back to these if necessary.

• Actual favicons for the default top sites instead of using the bigger tile images since they don't seem to be the same for every site.
• Proper names names for the default sites. There's a hostname property, but it would be nice to have e.g. "YouTube" instead of "youtube".

Comment 6

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4747bc77f218783c2a39364a3158b356307d811

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b33efbfec65c
Add a `newtab` option to browser.urlbar.topSites.get that returns the top sites exactly as shown on newtab r=mixedpuppy,Mardak

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b33efbfec65c

Comment 9

[Drew Willcoxon :adw]

Attachment: Beta/69 patch

Beta/Release Uplift Approval Request

• User impact if declined: This bug supports the top-sites experiment that the quantumbar/search team would like to conduct on 69.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: 1569366 (must be uplifted first)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a large-ish patch, but most of it is tests. The patch adds a new option to the browser.topSites.get() webextension API. The non-test part is small. Existing webextensions that use the top-sites API aren't affected.
• String changes made/needed: None

Comment 10

[Ryan VanderMeulen [:RyanVM]]

I'm not crazy about uplifting new APIs into Beta more than halfway through the cycle. Is there a particular reason we aren't asking QA to test this ahead of any uplifts?

Comment 11

[Drew Willcoxon :adw]

Does QA test new additions to webextension APIs? This is covered by automated tests, and I've been testing it myself manually since I'm writing an extension that's using the API.

I don't blame you for hesitating to uplift, but this new API only affects the browser.topSites.get() function, and it's behind a new option flag that you must pass in to get().

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9081690 [details] [diff] [review]
Beta/69 patch

Thanks for the extra info. Sounds like it's a pretty self-contained change and includes good test coverage. Approved for 69.0b10. I'll defer to the Addons QA team to decide on whether this needs additional testing or not.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3c1c65ddeac2

Comment 14

[Irene Smith]

TopSites.get is documented here: TopSites.get. Does this issue describe a change to that?

Comment 15

[Drew Willcoxon :adw]

Yes, we added a new boolean option to get called newtab. If true, get returns the sites that exactly appear on the user's new-tab page (and all other options are ignored except limit and includeFavicon). newtab is false by default.

Comment 16

[Irene Smith]

(In reply to Drew Willcoxon :adw from comment #15)

Yes, we added a new boolean option to get called newtab. If true, get returns the sites that exactly appear on the user's new-tab page (and all other options are ignored except limit and includeFavicon). newtab is false by default.

Added the following:

newtab Optional
Boolean. If included, the method returns the list of pages returned when the user opens a new tab. If included, and set to true, the method ignores all other parameters except limit and includeFavicon. Defaults to false.

Also adding this to the release notes:

Added a new parameter to the topSites.get() method that causes the method to return the list of pages that appear when the user opens a new tab (bug 1568617).

Let me know if I missed anything.

Comment 17

[Drew Willcoxon :adw]

Thanks!

(In reply to Irene Smith from comment #16)

If included, ... If included, and set to true, ...

"If included and set to true" applies to both of the sentences in your description, so I would just say "if set to true." You could include the option but set it to false, and in that case the option would not apply.