Bug 1445836 - finalize topSites api on platform APIs that will be stable, r=Mardak,rpl
authorShane Caraveo <scaraveo@mozilla.com>
Fri, 06 Jul 2018 09:33:50 -0300
changeset 425313 fc0d9d4bf1bf539107e72f5b9f99314e181c6989
parent 425312 dc6f1d3e6a11bcd394feb7f260a47d43d2413e3f
child 425314 b9e9d507a53cfb13c5183adba4cc4489f62e1cce
push id34243
push userbtara@mozilla.com
push dateFri, 06 Jul 2018 21:59:02 +0000
treeherdermozilla-central@79197e2d630a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMardak, rpl
bugs1445836
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1445836 - finalize topSites api on platform APIs that will be stable, r=Mardak,rpl MozReview-Commit-ID: Esj5sJweJ4K
toolkit/components/extensions/parent/ext-topSites.js
toolkit/components/extensions/schemas/top_sites.json
toolkit/components/extensions/test/xpcshell/test_ext_topSites.js
toolkit/modules/NewTabUtils.jsm
toolkit/modules/tests/xpcshell/test_NewTabUtils.js
--- a/toolkit/components/extensions/parent/ext-topSites.js
+++ b/toolkit/components/extensions/parent/ext-topSites.js
@@ -4,34 +4,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((resolve) => {
-            NewTabUtils.links.populateCache(async () => {
-              let urls;
-
-              // 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 = await NewTabUtils.activityStreamLinks.getTopSites();
-              }
-              resolve(urls.filter(link => !!link)
-                          .map(link => {
-                            return {
-                              url: link.url,
-                              title: link.title,
-                            };
-                          }));
-            }, false);
+        get: async function(options) {
+          let links = await NewTabUtils.activityStreamLinks.getTopSites({
+            ignoreBlocked: options.includeBlocked,
+            onePerDomain: options.onePerDomain,
+            numItems: options.limit,
+            includeFavicon: options.includeFavicon,
+          });
+          return links.map(link => {
+            return {
+              url: link.url,
+              title: link.title,
+              favicon: link.favicon,
+            };
           });
         },
       },
     };
   }
 };
--- a/toolkit/components/extensions/schemas/top_sites.json
+++ b/toolkit/components/extensions/schemas/top_sites.json
@@ -30,16 +30,21 @@
           "url": {
             "type": "string",
             "description": "The most visited URL."
           },
           "title": {
             "type": "string",
             "optional": true,
             "description": "The title of the page."
+          },
+          "favicon": {
+            "type": "string",
+            "optional": true,
+            "description": "Data URL for the favicon, if available."
           }
         }
       }
     ],
     "functions": [
       {
         "name": "get",
         "type": "function",
@@ -48,19 +53,45 @@
         "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\".",
+                "deprecated": "Please use the other options to tune the results received from topSites.",
                 "default": [],
                 "optional": true
+              },
+              "limit": {
+                "type": "integer",
+                "default": 12,
+                "maximum": 100,
+                "minimum": 1,
+                "optional": true,
+                "description": "The number of top sites to return, defaults to the value used by Firefox"
+              },
+              "onePerDomain": {
+                "type": "boolean",
+                "default": true,
+                "optional": true,
+                "description": "Limit the result to a single top site link per domain"
+              },
+              "includeBlocked": {
+                "type": "boolean",
+                "default": false,
+                "optional": true,
+                "description": "Include sites that the user has blocked from appearing on the Firefox new tab."
+              },
+              "includeFavicon": {
+                "type": "boolean",
+                "default": false,
+                "optional": true,
+                "description": "Include sites favicon if available."
               }
             },
             "default": {},
             "optional": true
           },
           {
             "name": "callback",
             "type": "function",
--- a/toolkit/components/extensions/test/xpcshell/test_ext_topSites.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_topSites.js
@@ -1,72 +1,104 @@
 "use strict";
 
 ChromeUtils.import("resource://gre/modules/PlacesUtils.jsm");
 ChromeUtils.import("resource://gre/modules/NewTabUtils.jsm");
+ChromeUtils.import("resource://testing-common/PlacesTestUtils.jsm");
+
+// A small 1x1 test png
+const IMAGE_1x1 = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAAAAAA6fptVAAAACklEQVQI12NgAAAAAgAB4iG8MwAAAABJRU5ErkJggg==";
 
 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();
 
-  // Stick a couple sites into history.
-  for (let i = 0; i < 2; ++i) {
-    let visit = {
-      url: `http://example.com${i}/`,
-      title: `visit${i}`,
-      visits: [],
-    };
+  function setVisit(visit) {
     for (let j = 0; j < numVisits; ++j) {
       visitDate -= 1000;
       visit.visits.push({date: new Date(visitDate)});
     }
     visits.push(visit);
   }
+  // Stick a couple sites into history.
+  for (let i = 0; i < 2; ++i) {
+    setVisit({
+      url: `http://example${i}.com/`,
+      title: `visit${i}`,
+      visits: [],
+    });
+    setVisit({
+      url: `http://www.example${i}.com/foobar`,
+      title: `visit${i}-www`,
+      visits: [],
+    });
+  }
   NewTabUtils.init();
-
   await PlacesUtils.history.insertMany(visits);
 
+  // Insert a favicon to show that favicons are not returned by default.
+  let faviconData = new Map();
+  faviconData.set("http://example0.com", IMAGE_1x1);
+  await PlacesTestUtils.addFavicons(faviconData);
+
   // Ensure our links show up in activityStream.
-  let links = await NewTabUtils.activityStreamLinks.getTopSites();
+  let links = await NewTabUtils.activityStreamLinks.getTopSites({onePerDomain: false, topsiteFrecency: 1});
+
   equal(links.length, visits.length, "Top sites has been successfully initialized");
 
   // Drop the visits.visits for later testing.
-  visits = visits.map(v => { return {url: v.url, title: v.title}; });
+  visits = visits.map(v => { return {url: v.url, title: v.title, favicon: undefined}; });
 
   // 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.test.onMessage.addListener(async providers => {
+      browser.test.onMessage.addListener(async options => {
         let sites;
-        if (typeof providers !== undefined) {
-          sites = await browser.topSites.get(providers);
+        if (typeof options !== undefined) {
+          sites = await browser.topSites.get(options);
         } else {
           sites = await browser.topSites.get();
         }
         browser.test.sendMessage("sites", sites);
       });
     },
   });
 
   await extension.startup();
 
-  function getSites(providers) {
-    extension.sendMessage(providers);
+  function getSites(options) {
+    extension.sendMessage(options);
     return extension.awaitMessage("sites");
   }
 
-  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");
+  Assert.deepEqual([visits[0], visits[2]],
+                   await getSites(),
+                   "got topSites default");
+  Assert.deepEqual(visits,
+                   await getSites({onePerDomain: false}),
+                   "got topSites all links");
+
+  NewTabUtils.activityStreamLinks.blockURL(visits[0]);
+  ok(NewTabUtils.blockedLinks.isBlocked(visits[0]), `link ${visits[0].url} is blocked`);
+
+  Assert.deepEqual([visits[2], visits[1]],
+                   await getSites(),
+                   "got topSites with blocked links filtered out");
+  Assert.deepEqual([visits[0], visits[2]],
+                   await getSites({includeBlocked: true}),
+                   "got topSites with blocked links included");
+
+  // Test favicon result
+  let topSites = await getSites({includeBlocked: true, includeFavicon: true});
+  equal(topSites[0].favicon, IMAGE_1x1, "received favicon");
+
+  equal(1, (await getSites({limit: 1, includeBlocked: true})).length, "limit 1 topSite");
 
   NewTabUtils.uninit();
   await extension.unload();
   await PlacesUtils.history.clear();
 });
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -1084,57 +1084,65 @@ var ActivityStreamProvider = {
 
   /*
    * Gets the top frecent sites for Activity Stream.
    *
    * @param {Object} aOptions
    *   {bool} ignoreBlocked: Do not filter out blocked links.
    *   {int}  numItems: Maximum number of items to return.
    *   {int}  topsiteFrecency: Minimum amount of frecency for a site.
+   *   {bool} onePerDomain: Dedupe the resulting list.
+   *   {bool} includeFavicon: Include favicons if available.
    *
    * @returns {Promise} Returns a promise with the array of links as payload.
    */
   async getTopFrecentSites(aOptions) {
     const options = Object.assign({
       ignoreBlocked: false,
       numItems: ACTIVITY_STREAM_DEFAULT_LIMIT,
-      topsiteFrecency: ACTIVITY_STREAM_DEFAULT_FRECENCY
+      topsiteFrecency: ACTIVITY_STREAM_DEFAULT_FRECENCY,
+      onePerDomain: true,
+      includeFavicon: true,
     }, aOptions || {});
 
     // Double the item count in case the host is deduped between with www or
     // not-www (i.e., 2 hosts) and an extra buffer for multiple pages per host.
     const origNumItems = options.numItems;
-    options.numItems *= 2 * 10;
+    if (options.onePerDomain) {
+      options.numItems *= 2 * 10;
+    }
 
     // Keep this query fast with frecency-indexed lookups (even with excess
     // rows) and shift the more complex logic to post-processing afterwards
     const sqlQuery = `
       SELECT
         ${this._commonBookmarkGuidSelect},
         frecency,
         guid,
         last_visit_date / 1000 AS lastVisitDate,
         rev_host,
         title,
-        url
+        url,
+        "history" as type
       FROM moz_places h
       WHERE frecency >= :frecencyThreshold
         ${this._commonPlacesWhere}
       ORDER BY frecency DESC
       LIMIT :limit
     `;
 
     let links = await this.executePlacesQuery(sqlQuery, {
       columns: [
         "bookmarkGuid",
         "frecency",
         "guid",
         "lastVisitDate",
         "title",
-        "url"
+        "url",
+        "type"
       ],
       params: this._getCommonParams(options, {
         frecencyThreshold: options.topsiteFrecency
       })
     });
 
     // Determine if the other link is "better" (larger frecency, more recent,
     // lexicographically earlier url)
@@ -1152,42 +1160,47 @@ var ActivityStreamProvider = {
         if (isOtherBetter(link, other)) {
           link = other;
         }
         combiner(link, other);
       }
       map.set(host, link);
     }
 
-    // Clean up the returned links by removing blocked, deduping, etc.
-    const exactHosts = new Map();
-    for (const link of links) {
-      if (!options.ignoreBlocked && BlockedLinks.isBlocked(link)) {
-        continue;
-      }
-
-      link.type = "history";
-
-      // First we want to find the best link for an exact host
-      setBetterLink(exactHosts, link, url => url.match(/:\/\/([^\/]+)/));
+    // Remove any blocked links.
+    if (!options.ignoreBlocked) {
+      links = links.filter(link => !BlockedLinks.isBlocked(link));
     }
 
-    // Clean up exact hosts to dedupe as non-www hosts
-    const hosts = new Map();
-    for (const link of exactHosts.values()) {
-      setBetterLink(hosts, link, url => url.match(/:\/\/(?:www\.)?([^\/]+)/),
-        // Combine frecencies when deduping these links
-        (targetLink, otherLink) => {
-          targetLink.frecency = link.frecency + otherLink.frecency;
-        });
+    if (options.onePerDomain) {
+      // De-dup the links.
+      const exactHosts = new Map();
+      for (const link of links) {
+        // First we want to find the best link for an exact host
+        setBetterLink(exactHosts, link, url => url.match(/:\/\/([^\/]+)/));
+      }
+
+      // Clean up exact hosts to dedupe as non-www hosts
+      const hosts = new Map();
+      for (const link of exactHosts.values()) {
+        setBetterLink(hosts, link, url => url.match(/:\/\/(?:www\.)?([^\/]+)/),
+          // Combine frecencies when deduping these links
+          (targetLink, otherLink) => {
+            targetLink.frecency = link.frecency + otherLink.frecency;
+          });
+      }
+
+      links = [...hosts.values()];
     }
+    // Pick out the top links using the same comparer as before
+    links = links.sort(isOtherBetter).slice(0, origNumItems);
 
-    // Pick out the top links using the same comparer as before
-    links = [...hosts.values()].sort(isOtherBetter).slice(0, origNumItems);
-
+    if (!options.includeFavicon) {
+      return links;
+    }
     // Get the favicons as data URI for now (until we use the favicon protocol)
     return this._faviconBytesToDataURI(await this._addFavicons(links));
   },
 
   /**
    * Gets a specific bookmark given some info about it
    *
    * @param {Obj} aInfo
--- a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
+++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
@@ -674,16 +674,41 @@ add_task(async function getTopFrecentSit
   links = await provider.getTopSites();
   Assert.equal(links.length, 0, "adding a single visit doesn't exceed default threshold");
 
   links = await provider.getTopSites({topsiteFrecency: 100});
   Assert.equal(links.length, 1, "adding a visit yields a link");
   Assert.equal(links[0].url, testURI, "added visit corresponds to added url");
 });
 
+add_task(async function getTopFrecentSites_no_dedup() {
+  await setUpActivityStreamTest();
+
+  let provider = NewTabUtils.activityStreamLinks;
+  let links = await provider.getTopSites({topsiteFrecency: 100});
+  Assert.equal(links.length, 0, "empty history yields empty links");
+
+  // Add a visits in reverse order they will be returned in when not deduped.
+  let testURIs = [{uri: "http://www.mozilla.com/"}, {uri: "http://mozilla.com/"}];
+  await PlacesTestUtils.addVisits(testURIs);
+
+  links = await provider.getTopSites();
+  Assert.equal(links.length, 0, "adding a single visit doesn't exceed default threshold");
+
+  links = await provider.getTopSites({topsiteFrecency: 100});
+  Assert.equal(links.length, 1, "adding a visit yields a link");
+  // Plain domain is returned when deduped.
+  Assert.equal(links[0].url, testURIs[1].uri, "added visit corresponds to added url");
+
+  links = await provider.getTopSites({topsiteFrecency: 100, onePerDomain: false});
+  Assert.equal(links.length, 2, "adding a visit yields a link");
+  Assert.equal(links[0].url, testURIs[1].uri, "added visit corresponds to added url");
+  Assert.equal(links[1].url, testURIs[0].uri, "added visit corresponds to added url");
+});
+
 add_task(async function getTopFrecentSites_dedupeWWW() {
   await setUpActivityStreamTest();
 
   let provider = NewTabUtils.activityStreamLinks;
 
   let links = await provider.getTopSites({topsiteFrecency: 100});
   Assert.equal(links.length, 0, "empty history yields empty links");