Closed Bug 1298211 Opened 4 years ago Closed 4 years ago

need topSites implementation

Categories

(WebExtensions :: Untriaged, defect, P3)

defect

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [topSites][design-decision-approved]triaged)

Attachments

(1 file, 2 obsolete files)

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

https://developer.chrome.com/extensions/topSites
Whiteboard: [topSites]
Use case would be a newtab override (bug 1234150) asking for the topSites to be displayed.
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
Whiteboard: [topSites] triaged → [topSites][design-decision-approved]triaged
Blocks: 1303905
Attached patch topSites draft (obsolete) — Splinter Review
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+
Attached patch topSites (obsolete) — Splinter Review
test added
Attachment #8796362 - Attachment is obsolete: true
Attachment #8796362 - Flags: feedback?(kmaglione+bmo)
Attachment #8797730 - Attachment is obsolete: true
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)
New patch moves this into toolkit rather than browser, simplifies test and moves it to xpcshell.
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+
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
https://hg.mozilla.org/mozilla-central/rev/cbf9f5b47d56
https://hg.mozilla.org/mozilla-central/rev/b8dd08d6aa5a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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)
: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.
Depends on: 1444579
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.