Closed Bug 1568617 Opened 4 months ago Closed 4 months ago

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

Categories

(WebExtensions :: General, task, P1)

task
Points:
3

Tracking

(firefox69 fixed, firefox70 fixed)

RESOLVED FIXED
mozilla70
Iteration:
70.2 - Jul 22 - Aug 4
Tracking Status
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

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.

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

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.

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/twitter-com@2x.png",

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).

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

Depends on: 1569366

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".
Pushed by dwillcoxon@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Attached patch Beta/69 patchSplinter Review

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
Attachment #9081690 - Flags: approval-mozilla-beta?
Flags: qe-verify-
Flags: in-testsuite+

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?

Flags: needinfo?(adw)
Flags: needinfo?(adw)
Summary: Extend the top sites API to optionally return TopStoriesFeed sites for small history → Extend the top sites API to optionally return TopSitesFeed sites for small history

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().

Depends on: 1570492
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.
Attachment #9081690 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1570492
No longer depends on: 1570492

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

Flags: needinfo?(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.

Flags: needinfo?(adw)

(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.

Flags: needinfo?(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.

Flags: needinfo?(adw)
You need to log in before you can comment on or make changes to this bug.