Bug 1444579 - fix use of places provider for topSites api, r?Gijs,kmag draft
authorShane Caraveo <scaraveo@mozilla.com>
Wed, 14 Mar 2018 19:58:15 -0500
changeset 767791 4ea652fb09482044e545d37b860067b11a56a7d2
parent 767347 80b4777a6421d8df4bb27ac23fb607c318a3006c
push id102693
push usermixedpuppy@gmail.com
push dateThu, 15 Mar 2018 00:58:47 +0000
reviewersGijs, kmag
bugs1444579
milestone61.0a1
Bug 1444579 - fix use of places provider for topSites api, r?Gijs,kmag MozReview-Commit-ID: 8GTl9BLQdge
toolkit/components/extensions/ext-topSites.js
toolkit/components/extensions/schemas/top_sites.json
toolkit/components/extensions/test/xpcshell/test_ext_topSites.js
toolkit/modules/NewTabUtils.jsm
--- a/toolkit/components/extensions/ext-topSites.js
+++ b/toolkit/components/extensions/ext-topSites.js
@@ -5,30 +5,27 @@
 ChromeUtils.defineModuleGetter(this, "NewTabUtils",
                                "resource://gre/modules/NewTabUtils.jsm");
 
 this.topSites = class extends ExtensionAPI {
   getAPI(context) {
     return {
       topSites: {
         get: function(options) {
-          return new Promise(function(resolve) {
-            NewTabUtils.links.populateCache(function() {
+          return new Promise((resolve) => {
+            NewTabUtils.links.populateCache(async () => {
               let urls;
 
-              if (options && options.providers && options.providers.length > 0) {
-                let urlLists = options.providers.map(function(p) {
-                  let provider = NewTabUtils[`${p}Provider`];
-                  return provider ? NewTabUtils.getProviderLinks(provider).slice() : [];
-                });
-                urls = NewTabUtils.links.mergeLinkLists(urlLists);
+              // The placesProvider is a superset of activityStream if sites are blocked, etc.,
+              // there is no need to attempt a merge of multiple provider lists.
+              if (options.providers.includes("places")) {
+                urls = NewTabUtils.getProviderLinks(NewTabUtils.placesProvider).slice();
               } else {
-                urls = NewTabUtils.links.getLinks();
+                urls = await NewTabUtils.activityStreamLinks.getTopSites();
               }
-
               resolve(urls.filter(link => !!link)
                           .map(link => {
                             return {
                               url: link.url,
                               title: link.title,
                             };
                           }));
             }, false);
--- a/toolkit/components/extensions/schemas/top_sites.json
+++ b/toolkit/components/extensions/schemas/top_sites.json
@@ -49,19 +49,21 @@
           {
             "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\".",
+                "default": [],
                 "optional": true
               }
             },
+            "default": {},
             "optional": true
           },
           {
             "name": "callback",
             "type": "function",
             "parameters": [
               {
                 "name": "results",
--- a/toolkit/components/extensions/test/xpcshell/test_ext_topSites.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_topSites.js
@@ -1,121 +1,72 @@
 "use strict";
 
+ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");
 ChromeUtils.import("resource://gre/modules/NewTabUtils.jsm");
 
-
-function TestProvider(getLinksFn) {
-  this.getLinks = getLinksFn;
-  this._observers = new Set();
-}
+add_task(async function test_topSites() {
+  let visits = [];
+  const numVisits = 15; // To make sure we get frecency.
+  let visitDate = new Date(1999, 9, 9, 9, 9).getTime();
 
-TestProvider.prototype = {
-  addObserver: function(observer) {
-    this._observers.add(observer);
-  },
-  notifyLinkChanged: function(link, index = -1, deleted = false) {
-    this._notifyObservers("onLinkChanged", link, index, deleted);
-  },
-  notifyManyLinksChanged: function() {
-    this._notifyObservers("onManyLinksChanged");
-  },
-  _notifyObservers: function(observerMethodName, ...args) {
-    args.unshift(this);
-    for (let obs of this._observers) {
-      if (obs[observerMethodName]) {
-        obs[observerMethodName].apply(NewTabUtils.links, args);
-      }
+  // Stick a couple sites into history.
+  for (let i = 0; i < 2; ++i) {
+    let visit = {
+      url: `http://example.com${i}/`,
+      title: `visit${i}`,
+      visits: [],
+    };
+    for (let j = 0; j < numVisits; ++j) {
+      visitDate -= 1000;
+      visit.visits.push({date: new Date(visitDate)});
     }
-  },
-};
+    visits.push(visit);
+  }
+  NewTabUtils.init();
+
+  await PlacesUtils.history.insertMany(visits);
 
-add_task(async function test_topSites() {
-  // Important: To avoid test failures due to clock jitter on Windows XP, call
-  // Date.now() once here, not each time through the loop.
-  let now = Date.now() * 1000;
-  let provider1 = new TestProvider(done => {
-    let data = [{url: "http://example.com/", title: "site#-1", frecency: 9, lastVisitDate: now},
-                {url: "http://example0.com/", title: "site#0", frecency: 8, lastVisitDate: now},
-                {url: "http://example3.com/", title: "site#3", frecency: 5, lastVisitDate: now}];
-    done(data);
-  });
-  let provider2 = new TestProvider(done => {
-    let data = [{url: "http://example1.com/", title: "site#1", frecency: 7, lastVisitDate: now},
-                {url: "http://example2.com/", title: "site#2", frecency: 6, lastVisitDate: now}];
-    done(data);
-  });
+  // Ensure our links show up in activityStream.
+  let links = await NewTabUtils.activityStreamLinks.getTopSites();
+  equal(links.length, visits.length, "Top sites has been successfully initialized");
 
-  NewTabUtils.initWithoutProviders();
-  NewTabUtils.links.addProvider(provider1);
-  NewTabUtils.links.addProvider(provider2);
-  NewTabUtils.test1Provider = provider1;
-  NewTabUtils.test2Provider = provider2;
+  // Drop the visits.visits for later testing.
+  visits = visits.map(v => { return {url: v.url, title: v.title}; });
 
   // Test that results from all providers are returned by default.
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
       "permissions": [
         "topSites",
       ],
     },
     background() {
       // Tests consistent behaviour when no providers are specified.
-      browser.topSites.get(result => {
-        browser.test.sendMessage("done1", result);
-      });
-      browser.topSites.get({}, result => {
-        browser.test.sendMessage("done2", result);
-      });
-      browser.topSites.get({providers: []}, result => {
-        browser.test.sendMessage("done3", result);
-      });
-      // Tests that results are merged correctly.
-      browser.topSites.get({providers: ["test2", "test1"]}, result => {
-        browser.test.sendMessage("done4", result);
-      });
-      // Tests that only the specified provider is used.
-      browser.topSites.get({providers: ["test2"]}, result => {
-        browser.test.sendMessage("done5", result);
-      });
-      // Tests that specifying a non-existent provider returns an empty array.
-      browser.topSites.get({providers: ["fake"]}, result => {
-        browser.test.sendMessage("done6", result);
+      browser.test.onMessage.addListener(async providers => {
+        let sites;
+        if (typeof providers !== undefined) {
+          sites = await browser.topSites.get(providers);
+        } else {
+          sites = await browser.topSites.get();
+        }
+        browser.test.sendMessage("sites", sites);
       });
     },
   });
 
   await extension.startup();
 
-  let expected1 = [{url: "http://example.com/", title: "site#-1"},
-                   {url: "http://example0.com/", title: "site#0"},
-                   {url: "http://example1.com/", title: "site#1"},
-                   {url: "http://example2.com/", title: "site#2"},
-                   {url: "http://example3.com/", title: "site#3"}];
-
-  let actual1 = await extension.awaitMessage("done1");
-  Assert.deepEqual(expected1, actual1, "got topSites");
-
-  let actual2 = await extension.awaitMessage("done2");
-  Assert.deepEqual(expected1, actual2, "got topSites");
-
-  let actual3 = await extension.awaitMessage("done3");
-  Assert.deepEqual(expected1, actual3, "got topSites");
+  function getSites(providers) {
+    extension.sendMessage(providers);
+    return extension.awaitMessage("sites");
+  }
 
-  let actual4 = await extension.awaitMessage("done4");
-  Assert.deepEqual(expected1, actual4, "got topSites");
-
-  let expected5 = [{url: "http://example1.com/", title: "site#1"},
-                   {url: "http://example2.com/", title: "site#2"}];
-
-  let actual5 = await extension.awaitMessage("done5");
-  Assert.deepEqual(expected5, actual5, "got topSites");
+  Assert.deepEqual(visits, await getSites(), "got topSites");
+  Assert.deepEqual(visits, await getSites({}), "got topSites");
+  Assert.deepEqual(visits, await getSites({providers: ["places"]}), "got topSites");
+  Assert.deepEqual(visits, await getSites({providers: ["activityStream"]}), "got topSites");
+  Assert.deepEqual(visits, await getSites({providers: ["places", "activityStream"]}), "got topSites");
 
-  let actual6 = await extension.awaitMessage("done6");
-  Assert.deepEqual([], actual6, "got topSites");
-
+  NewTabUtils.uninit();
   await extension.unload();
-
-  NewTabUtils.links.removeProvider(provider1);
-  NewTabUtils.links.removeProvider(provider2);
-  delete NewTabUtils.test1Provider;
-  delete NewTabUtils.test2Provider;
+  await PlacesUtils.history.clear();
 });
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -2133,11 +2133,12 @@ var NewTabUtils = {
     BlockedLinks.resetCache();
     Links.populateCache(aCallback, true);
   },
 
   links: Links,
   allPages: AllPages,
   pinnedLinks: PinnedLinks,
   blockedLinks: BlockedLinks,
+  placesProvider: PlacesProvider,
   activityStreamLinks: ActivityStreamLinks,
   activityStreamProvider: ActivityStreamProvider
 };