1298211 - need topSites implementation

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P3)

Description

[Shane Caraveo (:mixedpuppy)]

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

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

Comment 1

[Shane Caraveo (:mixedpuppy)]

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

Comment 2

[:shell escalante]

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.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: 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.

Comment 4

[Andy McKay]

Comment on attachment 8796362 [details] [diff] [review]
topSites draft

Don't much about getLinks under the hood, but this looks good.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: topSites

test added

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1298211 - Implement chrome.topSites,

Review commit: https://reviewboard.mozilla.org/r/85354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85354/

Comment 7

[Andrew Swan [:aswan]]

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,`

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800457 [details]
Bug 1298211 - Implement chrome.topSites,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85354/diff/1-2/

Comment 9

[Shane Caraveo (:mixedpuppy)]

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

Comment 10

[Andrew Swan [:aswan]]

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: "[e-mail]"},
> +      },

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() { ... }`.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8800457 [details]
Bug 1298211 - Implement chrome.topSites,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85354/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cbf9f5b47d56
Implement chrome.topSites, r=aswan

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbf9f5b47d56
https://hg.mozilla.org/mozilla-central/rev/b8dd08d6aa5a

Comment 15

[Will Bamberg [:wbamberg]]

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.

Comment 16

[Shane Caraveo (:mixedpuppy)]

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

Comment 17

[Will Bamberg [:wbamberg]]

Thanks Shane. I've noted that the number of sites is browser-specific.