need topSites implementation

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
WebExtensions: Untriaged
P3
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla52
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [topSites][design-decision-approved]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a year ago
I don't see this feature tracked in bugzilla or in docs, so adding it.

https://developer.chrome.com/extensions/topSites

Updated

a year ago
Whiteboard: [topSites]
(Assignee)

Comment 1

a year ago
Use case would be a newtab override (bug 1234150) asking for the topSites to be displayed.

Comment 2

a year ago
Currently this isn't prioritized for next few releases - so if it's critical ASAP - please discuss with andym about potentially picking up /collaborating on API.
Priority: -- → P3
Whiteboard: [topSites] → [topSites] triaged

Updated

a year ago
Whiteboard: [topSites] triaged → [topSites][design-decision-approved]triaged
(Assignee)

Updated

a year ago
Blocks: 1303905
(Assignee)

Comment 3

a year ago
Created attachment 8796362 [details] [diff] [review]
topSites draft

This is a quick implementation based on top of NewTabUtils.links.  The nice thing about that is cacheing, updates, etc. are all handled.  A downside [maybe] is that it is more than frecency based history links, such as prefilled links for new users.  An upside to that is that NewTabUtils supports data providers that could slip new stuff into topSites, which may be useful as a future experimental api.  We could implement our own caching and directly use NewTabUtils.placesProvider for a pure history based list.

Google docs imply frecency/history in the API, but not strongly.  Top line doc is it returns a list of urls used on newtab.

One internal difference is it appears chromium limits to 20 returned urls, newtabutils limits to 100.  As this is not documented, I'm not inclined to be bothered by it.

I'm not clear on the proper use of the second param for extensions.registerSchemaAPI.  ie. what value is preferable under what circumstances.

Tests yet to be written.
Assignee: nobody → mixedpuppy
Attachment #8796362 - Flags: feedback?(kmaglione+bmo)
Attachment #8796362 - Flags: feedback?(amckay)
Comment on attachment 8796362 [details] [diff] [review]
topSites draft

Don't much about getLinks under the hood, but this looks good.
Attachment #8796362 - Flags: feedback?(amckay) → feedback+
(Assignee)

Comment 5

a year ago
Created attachment 8797730 [details] [diff] [review]
topSites

test added
Attachment #8796362 - Attachment is obsolete: true
Attachment #8796362 - Flags: feedback?(kmaglione+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8797730 - Attachment is obsolete: true

Comment 7

a year ago
mozreview-review
Comment on attachment 8800457 [details]
Bug 1298211 - Implement chrome.topSites,

https://reviewboard.mozilla.org/r/85354/#review84284

Great start, some comments below.
One other observartion: you have this all under browser/components/extensions but NewSitesUtils.jsm is in toolkit/, can this api go into toolkit/components/extensions?  (that would make it available on fennec as well)

::: browser/components/extensions/ext-topSites.js:15
(Diff revision 1)
> +extensions.registerSchemaAPI("topSites", "addon_parent", context => {
> +  return {
> +    topSites: {
> +      get: function() {
> +        let urls = NewTabUtils.links.getLinks().reduce((urls, link) => {
> +          if (link)

So getLinks() gives you back an array that has some empty or null arguments?  Hm.  I think this would be a little nicer to read as a `.filter()` to remove empty entries and then `.map()` to create the smaller objects to be returned.

::: browser/components/extensions/test/browser/browser.ini:89
(Diff revision 1)
>  [browser_ext_tabs_sendMessage.js]
>  [browser_ext_tabs_update.js]
>  [browser_ext_tabs_zoom.js]
>  [browser_ext_tabs_update_url.js]
>  [browser_ext_topwindowid.js]
> +[browser_ext_topSites.js]

I think this test could actually be run in xpcshell instead of a full blown mochitest?

::: browser/components/extensions/test/browser/browser_ext_topSites.js:1
(Diff revision 1)
> +var tmp = {};

var -> let

::: browser/components/extensions/test/browser/browser_ext_topSites.js:4
(Diff revision 1)
> +var tmp = {};
> +Cu.import("resource://gre/modules/NewTabUtils.jsm", tmp);
> +Cu.import("resource://testing-common/PlacesTestUtils.jsm", tmp);
> +var {NewTabUtils, PlacesTestUtils} = tmp;

var -> let
Any reason not to just use regular Cu.import() without a second argument though?

::: browser/components/extensions/test/browser/browser_ext_topSites.js:7
(Diff revision 1)
> +Cu.import("resource://gre/modules/NewTabUtils.jsm", tmp);
> +Cu.import("resource://testing-common/PlacesTestUtils.jsm", tmp);
> +var {NewTabUtils, PlacesTestUtils} = tmp;
> +
> +
> +// NOTE: setup, setLinks and fillHistory copied from browser/base/content/test/newtab/head.js

There's a bunch of stylistic stuff in those functions that we would do differently if we were writing here but maybe its easier to keep it the same for comparing?  How about instead moving the shared code to PlacesTestUtils.jsm so it doesn't need to be duplicated here?

::: browser/components/extensions/test/browser/browser_ext_topSites.js:93
(Diff revision 1)
> +  registerCleanupFunction(function() {
> +    return new Promise(resolve => {
> +      function cleanupAndFinish() {
> +        PlacesTestUtils.clearHistory().then(() => {
> +          NewTabUtils.restore();
> +          executeSoon(resolve);

I don't think you need executeSoon()

::: browser/components/extensions/test/browser/browser_ext_topSites.js:99
(Diff revision 1)
> +        });
> +      }
> +
> +      let callbacks = NewTabUtils.links._populateCallbacks;
> +      let numCallbacks = callbacks.length;
> +  

whitespace

::: browser/components/extensions/test/browser/browser_ext_topSites.js:110
(Diff revision 1)
> +  });
> +  yield setLinks("-1,0,1,2,3");
> +});
> +
> +add_task(function* test_topSites() {
> +  function background(options) {

options isn't used, remove?

::: browser/components/extensions/test/browser/browser_ext_topSites.js:117
(Diff revision 1)
> +      browser.test.assertEq(expect.length, result.length, "got topSites");
> +      for (let i = 0; i < expect.length; i++) {
> +        browser.test.assertEq(expect[i].url, result[i].url, "urls match");
> +        browser.test.assertEq(expect[i].title, result[i].title, "titles match");
> +      }

This can all be `Assert.deepEqual(expect, result)`

::: browser/components/extensions/test/browser/browser_ext_topSites.js:132
(Diff revision 1)
> +    manifest: {
> +      "permissions": [
> +        "topSites"
> +      ]
> +    },
> +    background: `(${background})()`,

This whole line can just be `background,`
Attachment #8800457 - Flags: review?(aswan)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
New patch moves this into toolkit rather than browser, simplifies test and moves it to xpcshell.

Comment 10

a year ago
mozreview-review
Comment on attachment 8800457 [details]
Bug 1298211 - Implement chrome.topSites,

https://reviewboard.mozilla.org/r/85354/#review85284

A handful of little style things, but looks good!

::: toolkit/components/extensions/ext-topSites.js:13
(Diff revision 2)
> +extensions.registerSchemaAPI("topSites", "addon_parent", context => {
> +  return {
> +    topSites: {
> +      get: function() {
> +        let urls = NewTabUtils.links.getLinks()
> +                              .filter(link => { return !!link; })

nit: this can just be `link => !!link` (no need for braces and return)

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:19
(Diff revision 2)
> +    this._notifyObservers("onLinkChanged", link, index, deleted);
> +  },
> +  notifyManyLinksChanged: function () {
> +    this._notifyObservers("onManyLinksChanged");
> +  },
> +  _notifyObservers: function () {

nit: this can be `_notifyObservers(observerMethodName, ...args) {`

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:35
(Diff revision 2)
> +  let places = [];
> +  for (let i = 0; i < aLinks.length; i++) {
> +    let link = aLinks[i];
> +    places.push({
> +      url: link.url,
> +      title: link.title,
> +      lastVisitDate: now - i,
> +      frecency: frecency++
> +    });
> +  }
> +  return places;

nit: this could all be written as `aLinks.map((link, i) => { ... })`.
Also, we don't use the aName convention in webextensions, the parameter should just be `links`

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:57
(Diff revision 2)
> +      "applications": {
> +       "gecko": {id: "test-webextension@mozilla.com"},
> +      },

This isn't harmful but it shouldn't be necessary?

::: toolkit/components/extensions/test/xpcshell/test_ext_topSites.js:64
(Diff revision 2)
> +      },
> +      "permissions": [
> +        "topSites"
> +      ]
> +    },
> +    background: "(" + function() {

nit: this can just be `background() { ... }`.
Attachment #8800457 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)

Comment 12

a year ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cbf9f5b47d56
Implement chrome.topSites, r=aswan

Comment 13

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/b8dd08d6aa5a
Implement chrome.topSites: Follow-up: Disable test on Android. r=me on a CLOSED TREE to fix xpcshell bustage

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cbf9f5b47d56
https://hg.mozilla.org/mozilla-central/rev/b8dd08d6aa5a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Updated

a year ago
Keywords: dev-doc-needed
I've written some docs for this here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/topSites

Please let me know if this covers it.
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 16

11 months ago
:wbamberg That looks good.  The only edit I might suggest [and it's not necessary, just extra info] is a note on a difference between Chrome and Firefox.  The number of sites returned is specific to the implementation.  e.g. Chrome returns 20 top sites, Firefox returns 100.  Firefox results may also return non-history related items, such as prefilled links for new users.  This also means Firefox could return more than what newtab might contain.
Flags: needinfo?(mixedpuppy)
Thanks Shane. I've noted that the number of sites is browser-specific.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.