1547669 - Improve the TopSites WebExtension API

Status: RESOLVED FIXED

(WebExtensions :: General, task, P3)

Description

[Mike de Boer [:mikedeboer]]

For more info, see the design doc over at https://docs.google.com/document/d/11c-OTsscmArnqL02Kq1TUPKtkHl4avmC3pDdSJDC6JI/edit

There’s an existing topSites webextension API but not clear it has some things we need.

Comment 1

[Marco Bonardo [:mak]]

Fwiw, it's possible we could use the existing WebExt API as-is, because we dropped the idea of using large icons and an horizontal layout. The horizontal layout would require us to fetch an hi-dpi image somewhere. On the other side, by keeping the current layout, we can just use a page-icon:url favicon, that will fetch a favicon or use the default if we don't have one. One downside may be related to new profiles or permanent PB profiles, where we wouldn't have a favicon at hand (unless the entry is a token alias, then we can use the engine icon).

Comment 2

[Marco Bonardo [:mak]]

So, the existing topSites API is baked by NewTabUtils.getTopFrecentSites(), that is just returning a list of the most frecent urls. It is not returning the list shown on about:newtab, and in particular it only returns urls, no search engines. So I guess we'll need our own API, or to change the existing topSites API to also return the list from the new tab.

NewTabUtils.links.getLinks() returns what we want.

For the favicons problem, we could make the extensionprovider add an icon when it converts results, in the end it can use the engine icon for shortcuts and a page-icon: for other urls.

I'll try to look around for Shane to discuss the best path forward.

Comment 3

[Marco Bonardo [:mak]]

So I evaluated our options with Shane and Philipp, we are going to do the following:

1. add a new option to topSites.get(), as an enum string, that allows to switch mode between "frequent" and "newtab" (names are made up for now, we also need a name for the option, maybe "mode" or "sortingMode" or "resultSet"?)
2. the default for the API will remain the current result set, in the future the add-ons team will evaluate whether to change the default
3. add a new property to the MostVisitedURL object, to indicate if it's a search (maybe just a type property) and again default to the current type (I suppose at the beginning the only 2 types will be "url" or "search", where url is the default).
4. if possible, provide favicons when the includeFavicon option is true, if it's hard or slow we can leave the favicons out and document that "newtab" mode doesn't yet return favicons. We can add favicon urls in the UrlbarProviderExtension object, as I said in comment 2

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1547669 - Improve the TopSites WebExtension API with a "newtab" resultSet. r=mixedpuppy,k88hudson,adw

Adds a "newtab" resultSet to the topSites API, so that we can get the actual
list of results shown on the newtab page. The old behavior (most frecent pages)
is retained as the default for now.
This may not be working completely on Android, but the topSites API situation
there is foggy already, the test is marked as a skip and part of the code is
using Places methods not available there. Should likely be handled elsewhere.

Comment 5

[Marco Bonardo [:mak]]

Attachment: Bug 1547669 - Improve the TopSites WebExtension API with further options. r=mixedpuppy,Mardak,adw

Adds includePinned and includeSearchShortcuts options to the topSites API, so
that it's possible to get a list of results like it's shown on the newtab page.
Both options default to false, so that the existing behavior is preserved.
The API gets disabled on Android, the API always depended on not available APIs
and the test is disabled on Android, so it'd be untested there.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99f94dd8c8f1
Improve the TopSites WebExtension API with further options. r=mixedpuppy,Mardak,adw

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2308c2443fa
Backed out changeset 99f94dd8c8f1 for xpcshell failure at  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_json_parser.js. On a CLOSED TREE

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ab7f6c67d4b
Improve the TopSites WebExtension API with further options. r=mixedpuppy,Mardak,adw

Comment 9

[Shane Caraveo (:mixedpuppy)]

This is primarily a webextension api enhancement, moving to our component.

Comment 10

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/0ab7f6c67d4b

Comment 11

[Madalin Cotetiu]

Can you please provide some steps so we can manually verify this issue? If no manual testing is needed can you please mark it as "qe-verify- "

Comment 12

[Marco Bonardo [:mak]]

I don't think it needs manual QA, thanks.

Comment 13

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [::mak] from comment #5)

Adds includePinned and includeSearchShortcuts options to the topSites API, so
that it's possible to get a list of results like it's shown on the newtab page.

This doesn't seem to be working right: I'm not getting unvisited top sites listed on newtab (e.g. YouTube, Facebook, Reddit, Twitter). I haven't investigated very much yet.

Comment 14

[Drew Willcoxon :adw]

Oh I see Ed said this in phabricator:

This should match up pretty close as is if we don't want to exactly exactly match
TopStoriesFeed's default sites for small history

But we do want that for the top sites experiment, don't we? e.g. users who just switched to Firefox should see the default suggested sites, same as new tab, no? Verdi?

Comment 15

[Marco Bonardo [:mak]]

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

This doesn't seem to be working right: I'm not getting unvisited top sites listed on newtab (e.g. YouTube, Facebook, Reddit, Twitter). I haven't investigated very much yet.

I didn't add it because I didn't want to surprise the user with stuff that he didn't visit/know about. The new tab page has more implications for which it wants to be filled out, and did their own investigation about it. The urlbar had its own way to provide "empty" results in the preloaded-top-sites feature, that has never been finished nor enabled (it's still behind a disabled pref). Considered that, the risk looked too big to just cargo-cult the empty-list from new tab page.

Comment 16

[Verdi [:verdi] Best to slack me]

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

Oh I see Ed said this in phabricator:

This should match up pretty close as is if we don't want to exactly exactly match
TopStoriesFeed's default sites for small history

But we do want that for the top sites experiment, don't we? e.g. users who just switched to Firefox should see the default suggested sites, same as new tab, no? Verdi?

This is correct. The idea of the top sites feature is that it should always exactly match the top sites displayed in activity stream.

Comment 17

[Irene Smith]

Tracked in mdn/sprints 1914 (in progress) for documentation purposes.

Comment 18

[Irene Smith]

Added the following two values to the definition of topSites.get:

includePinned {{optional_inline}}
Boolean. Includes sites that the user has pinned to the Firefox new tab..
Defaults to false.
includeSearchShortcuts {{optional_inline}}
Boolean. Includes search shortcuts that appear on the Firefox new tab.
Defaults to false.

The release notes also have an entry:

The topSites.get() method now has new options available — includePinned and includeSearchShortcuts (bug 1547669).

Let me know if I missed anything.

Comment 19

[Marco Bonardo [:mak]]

It looks good to me, thank you!