Bug 1445836 - finalize topSites api on platform APIs that will be stable, r?Mardak, rpl draft
authorShane Caraveo <scaraveo@mozilla.com>
Fri, 06 Jul 2018 09:33:50 -0300
changeset 814922 442fa520169e07f5b24e30e0da1a4f7c284701b6
parent 814073 a9dc5dc8e2b8513686ad1b1f28c9e4da6de62226
push id115379
push usermixedpuppy@gmail.com
push dateFri, 06 Jul 2018 12:34:30 +0000
reviewersMardak, rpl
bugs1445836
milestone63.0a1
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");