1361899 - topSites.get results should be based on frecency only

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, enhancement, P5)

Description

[Geoff Lankow (:darktrojan)]

I see some discussion of this in bug 1298211 comment 3. The currently returned list includes pinned sites on the new tab page, and is also filtered so there is only one entry per domain. I think extension writers (including myself) should be able to access the unmodified data and decide for themselves how they want to use it.

Also, there is a buggy behaviour in that the provider cache might not be populated when the code is run.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1361899-1.diff

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8864343 [details] [diff] [review]
1361899-1.diff

This is purposely based on our newtab module.  It would be better to add frecency support to the history api.  I'd alternately be willing to consider an optional param to topSites for various alternative results.

Comment 3

[Geoff Lankow (:darktrojan)]

> This is purposely based on our newtab module.

What's the purpose? Extension users don't want a copy of what they already have (which also now can't be modified except by using the built-in page), they want some thing different.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Geoff Lankow (:darktrojan) from comment #3)
> > This is purposely based on our newtab module.
> 
> What's the purpose? 

We have existing use cases for it.

As I said, I'd consider an optional param to topsites to return different results, or additions to the history api which would achieve the same result and possibly be more flexible (though that may be more of an issue technically).  As I also vaguely mentioned in the original implementation bug, it could be nice to have addons be providers to the newtab module.  An option on get() could fit in well with that.

Comment 5

[Geoff Lankow (:darktrojan)]

> An option on get() could fit in well with that.

Okay, that seems like a sensible approach. Do you have any suggestions as to what that might look like? I imagine something like:

browser.topSites.get({ historyOnly: true })

Comment 6

[Shane Caraveo (:mixedpuppy)]

I might suggest this instead:

browser.topSites.get({ providers: ["places", "activitystream"] })

No option defaults to current behavior.

Let me also get feedback from someone more involved on the newtab stuff before we settle on anything.

Comment 7

[Shane Caraveo (:mixedpuppy)]

I'm happy to go forward now, would you like to continue the patch?

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1361899-2.diff

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8865721 [details] [diff] [review]
1361899-2.diff


> this.topSites = class extends ExtensionAPI {

>+        get: function(options) {
>+          return new Promise(function(resolve) {
>+            NewTabUtils.links.populateCache(function() {
>+              let urls;
>+
>+              if (options && Array.isArray(options.providers)) {

Should also handle an empty list (options.providers = []).  The type in schema ensures it is an array if it is present.

>diff --git a/toolkit/components/extensions/schemas/top_sites.json b/toolkit/components/extensions/schemas/top_sites.json
>--- a/toolkit/components/extensions/schemas/top_sites.json
>+++ b/toolkit/components/extensions/schemas/top_sites.json
>@@ -42,16 +42,24 @@
>     "functions": [
>       {
>         "name": "get",
>         "type": "function",
>         "description": "Gets a list of top sites.",
>         "async": "callback",
>         "parameters": [
>           {
>+            "type": "object",
>+            "name": "options",
>+            "properties": {
>+              "providers": {"type": "array", "items": { "type": "string" }, "description": "Which providers to get top sites from. Possible values are \"places\" and \"activityStream\".", "optional": true}

Format this line.


A couple thoughts, probably as followup bugs.

- It would be nice to have named providers in NewTabUtils.jsm rather than having to hard code provider classes.
- We should add frecency and lastVisitDate to the url objects returned by topsites.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1361899-3.diff

Okay, it now responds to an empty array as if none was specified. I've also tidied up the test as I wasn't that happy with it.

> It would be nice to have named providers in NewTabUtils.jsm rather than
having to hard code provider classes.

I've been trying to avoid changes to NewTabUtils.jsm in this bug, will follow up once this has landed.

> We should add frecency and lastVisitDate to the url objects returned by
topsites.

Yeah, probably.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8866589 [details] [diff] [review]
1361899-3.diff



>diff --git a/toolkit/components/extensions/ext-topSites.js b/toolkit/components/extensions/ext-topSites.js
>--- a/toolkit/components/extensions/ext-topSites.js
>+++ b/toolkit/components/extensions/ext-topSites.js
>@@ -4,23 +4,43 @@

>+                if (options && options.providers.includes("test")) {
>+                  urlLists.push(NewTabUtils.getProviderLinks(NewTabUtils.testProvider).slice());
>+                }

I got thinking about test code in here.  I think it's possible to have a problem if an addon passes "test" as a provider.  Maybe something like this:

for p in options.providers
  if p in [...allowed providers] and NewTabUtils has `${p}Provider` then
    getProviderLinks

if urlList > 0
  urls = mergeLinkLists
else
  urls = getlinks

Comment 12

[Geoff Lankow (:darktrojan)]

The only difference is you'd get an empty array instead of falling back to the default behaviour. I don't see that as much of a problem, and if we start registering providers with names it will go away.

Comment 13

[Shane Caraveo (:mixedpuppy)]

I'm not clear on what you are saying or suggesting.

Comment 14

[Geoff Lankow (:darktrojan)]

Attachment: 1361899-4.diff

Sorry, I think I misinterpreted what you wanted. Is this more like it?

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8867582 [details] [diff] [review]
1361899-4.diff

This is looking fine but the test wouldn't apply it against a current pull of m-c, so I need an update.  I'll want to see a try run, then we good.

Comment 16

[Geoff Lankow (:darktrojan)]

Attachment: 1361899-5.diff

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=616bc6587ff5f8650e31bf9e1e53aeb64b18e59f (it applied once I accounted for the async/await change last week).

Let's land this.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3117287bcc9d
Allow extensions to specify provider when calling topSites.get. r=mixedpuppy

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3117287bcc9d