topSites.get results should be based on frecency only

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Untriaged
P5
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: darktrojan, Assigned: darktrojan, Mentored)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8864343 [details] [diff] [review]
1361899-1.diff
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #8864343 - Flags: review?(aswan)
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.
Attachment #8864343 - Flags: review?(aswan) → review-
(Assignee)

Comment 3

a year ago
> 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.
(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.
(Assignee)

Comment 5

a year ago
> 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 })
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.
I'm happy to go forward now, would you like to continue the patch?
Flags: needinfo?(geoff)

Updated

a year ago
Mentor: mixedpuppy
Priority: -- → P5
Whiteboard: triaged
(Assignee)

Comment 8

a year ago
Created attachment 8865721 [details] [diff] [review]
1361899-2.diff
Attachment #8864343 - Attachment is obsolete: true
Flags: needinfo?(geoff)
Attachment #8865721 - Flags: review?(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.
Attachment #8865721 - Flags: review?(mixedpuppy) → feedback+
(Assignee)

Comment 10

a year ago
Created attachment 8866589 [details] [diff] [review]
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.
Attachment #8865721 - Attachment is obsolete: true
Attachment #8866589 - Flags: review?(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
(Assignee)

Comment 12

a year ago
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.
I'm not clear on what you are saying or suggesting.
Flags: needinfo?(geoff)
(Assignee)

Comment 14

a year ago
Created attachment 8867582 [details] [diff] [review]
1361899-4.diff

Sorry, I think I misinterpreted what you wanted. Is this more like it?
Attachment #8866589 - Attachment is obsolete: true
Attachment #8866589 - Flags: review?(mixedpuppy)
Flags: needinfo?(geoff)
(Assignee)

Updated

a year ago
Attachment #8867582 - Flags: review?(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.
Attachment #8867582 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 16

a year ago
Created attachment 8869689 [details] [diff] [review]
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.
Attachment #8867582 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 17

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3117287bcc9d
Allow extensions to specify provider when calling topSites.get. r=mixedpuppy
Keywords: checkin-needed

Comment 18

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3117287bcc9d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 months ago
Depends on: 1444579
You need to log in before you can comment on or make changes to this bug.