Bug 1126184: Part 2: Select a single tile to show as the first unpinned tile based on a user's top sites. r=adw

browser/modules/DirectoryLinksProvider.jsm

@@ -57,6 +57,9 @@ const ALLOWED_IMAGE_SCHEMES = new Set(["https", "data"]);
 // The frecency of a directory link
 const DIRECTORY_FRECENCY = 1000;
 
+// The frecency of a related link
+const RELATED_FRECENCY = Infinity;
+
 // Divide frecency by this amount for pings
 const PING_SCORE_DIVISOR = 10000;
 
@@ -472,18 +475,29 @@ let DirectoryLinksProvider = {
         this._topSitesWithRelatedLinks.add(site);
       }
     });
+    this._updateRelatedTile();
   },
 
+  /**
+   * Updates _topSitesWithRelatedLinks based on the link that was changed.
+   *
+   * @return true if _topSitesWithRelatedLinks was modified, false otherwise.
+   */
   _handleLinkChanged: function(aLink) {
     let changedLinkSite = NewTabUtils.extractSite(aLink.url);
-    if (!NewTabUtils.isTopPlacesSite(changedLinkSite)) {
+    let linkStored = this._topSitesWithRelatedLinks.has(changedLinkSite);
+
+    if (!NewTabUtils.isTopPlacesSite(changedLinkSite) && linkStored) {
       this._topSitesWithRelatedLinks.delete(changedLinkSite);
-      return;
+      return true;
     }
 
-    if (this._relatedLinks.has(changedLinkSite)) {
+    if (this._relatedLinks.has(changedLinkSite) &&
+        NewTabUtils.isTopPlacesSite(changedLinkSite) && !linkStored) {
       this._topSitesWithRelatedLinks.add(changedLinkSite);
+      return true;
     }
+    return false;
   },
 
   _populatePlacesLinks: function () {
@@ -495,7 +509,9 @@ let DirectoryLinksProvider = {
   onLinkChanged: function (aProvider, aLink) {
     // Make sure NewTabUtils.links handles the notification first.
     setTimeout(() => {
-      this._handleLinkChanged(aLink);
+      if (this._handleLinkChanged(aLink)) {
+        this._updateRelatedTile();
+      }
     }, 0);
   },
 
@@ -506,6 +522,63 @@ let DirectoryLinksProvider = {
     }, 0);
   },
 
+  /**
+   * Chooses and returns a related tile based on a user's top sites
+   * that we have an available related tile for.
+   *
+   * @return the chosen related tile, or undefined if there isn't one
+   */
+  _updateRelatedTile: function() {
+    let sortedLinks = NewTabUtils.getProviderLinks(this);
+
+    // Delete the current related tile, if one exists.
+    let initialLength = sortedLinks.length;
+    this.maxNumLinks = initialLength;
+    if (initialLength) {
+      let mostFrecentLink = sortedLinks[0];
+      if ("related" == mostFrecentLink.type) {
+        this._callObservers("onLinkChanged", {
+          url: mostFrecentLink.url,
+          frecency: 0,
+          lastVisitDate: mostFrecentLink.lastVisitDate,
+          type: "related",
+        }, 0, true);
+      }
+    }
+
+    if (this._topSitesWithRelatedLinks.size == 0) {
+      // There are no potential related links we can show.
+      return;
+    }
+
+    // Create a flat list of all possible links we can show as related.
+    // Note that many top sites may map to the same related links, but we only
+    // want to count each related link once (based on url), thus possibleLinks is a map
+    // from url to relatedLink. Thus, each link has an equal chance of being chosen at
+    // random from flattenedLinks if it appears only once.
+    let possibleLinks = new Map();
+    this._topSitesWithRelatedLinks.forEach(topSiteWithRelatedLink => {
+      let relatedLinksMap = this._relatedLinks.get(topSiteWithRelatedLink);
+      relatedLinksMap.forEach((relatedLink, url) => {
+        possibleLinks.set(url, relatedLink);
+      })
+    });
+    let flattenedLinks = [...possibleLinks.values()];
+
+    // Choose our related link at random
+    let relatedIndex = Math.floor(Math.random() * flattenedLinks.length);
+    let chosenRelatedLink = flattenedLinks[relatedIndex];
+
+    // Show the new directory tile.
+    this._callObservers("onLinkChanged", {
+      url: chosenRelatedLink.url,
+      frecency: RELATED_FRECENCY,
+      lastVisitDate: chosenRelatedLink.lastVisitDate,
+      type: "related",
+    });
+    return chosenRelatedLink;
+   },
+
   /**
    * Return the object to its pre-init state
    */

browser/modules/test/xpcshell/test_DirectoryLinksProvider.js

@@ -214,6 +214,131 @@ function run_test() {
   });
 }
 
+add_task(function test_updateRelatedTile() {
+  let topSites = ["site0.com", "1040.com", "site2.com", "hrblock.com", "site4.com", "freetaxusa.com", "site6.com"];
+
+  // Initial setup
+  let data = {"en-US": [relatedTile1, relatedTile2, relatedTile3, someOtherSite]};
+  let dataURI = 'data:application/json,' + JSON.stringify(data);
+
+  let testObserver = new TestFirstRun();
+  DirectoryLinksProvider.addObserver(testObserver);
+
+  yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
+  let links = yield fetchData();
+
+  let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite;
+  NewTabUtils.isTopPlacesSite = function(site) {
+    return topSites.indexOf(site) >= 0;
+  }
+
+  let origGetProviderLinks = NewTabUtils.getProviderLinks;
+  NewTabUtils.getProviderLinks = function(provider) {
+    return links;
+  }
+
+  do_check_eq(DirectoryLinksProvider._updateRelatedTile(), undefined);
+
+  function TestFirstRun() {
+    this.promise = new Promise(resolve => {
+      this.onLinkChanged = (directoryLinksProvider, link) => {
+        links.unshift(link);
+        let possibleLinks = [relatedTile1.url, relatedTile2.url, relatedTile3.url];
+
+        isIdentical([...DirectoryLinksProvider._topSitesWithRelatedLinks], ["hrblock.com", "1040.com", "freetaxusa.com"]);
+        do_check_true(possibleLinks.indexOf(link.url) > -1);
+        do_check_eq(link.frecency, Infinity);
+        do_check_eq(link.type, "related");
+        resolve();
+      };
+    });
+  }
+
+  function TestChangingRelatedTile() {
+    this.count = 0;
+    this.promise = new Promise(resolve => {
+      this.onLinkChanged = (directoryLinksProvider, link) => {
+        this.count++;
+        let possibleLinks = [relatedTile1.url, relatedTile2.url, relatedTile3.url];
+
+        do_check_true(possibleLinks.indexOf(link.url) > -1);
+        do_check_eq(link.type, "related");
+        do_check_true(this.count <= 2);
+
+        if (this.count == 1) {
+          // The removed related link is the one we added initially.
+          do_check_eq(link.url, links.shift().url);
+          do_check_eq(link.frecency, 0);
+        } else {
+          links.unshift(link);
+          do_check_eq(link.frecency, Infinity);
+        }
+        isIdentical([...DirectoryLinksProvider._topSitesWithRelatedLinks], ["hrblock.com", "freetaxusa.com"]);
+        resolve();
+      }
+    });
+  }
+
+  function TestRemovingRelatedTile() {
+    this.count = 0;
+    this.promise = new Promise(resolve => {
+      this.onLinkChanged = (directoryLinksProvider, link) => {
+        this.count++;
+
+        do_check_eq(link.type, "related");
+        do_check_eq(this.count, 1);
+        do_check_eq(link.frecency, 0);
+        do_check_eq(link.url, links.shift().url);
+        isIdentical([...DirectoryLinksProvider._topSitesWithRelatedLinks], []);
+        resolve();
+      }
+    });
+  }
+
+  // Test first call to '_updateRelatedTile()', called when fetching directory links.
+  yield testObserver.promise;
+  DirectoryLinksProvider.removeObserver(testObserver);
+
+  // Removing a top site that doesn't have a related link should
+  // not change the current related tile.
+  let removedTopsite = topSites.shift();
+  do_check_eq(removedTopsite, "site0.com");
+  do_check_false(NewTabUtils.isTopPlacesSite(removedTopsite));
+  let updateRelatedTile = DirectoryLinksProvider._handleLinkChanged({
+    url: "http://" + removedTopsite,
+    type: "history",
+  });
+  do_check_false(updateRelatedTile);
+
+  // Removing a top site that has a related link should
+  // remove any current related tile and add a new one.
+  testObserver = new TestChangingRelatedTile();
+  DirectoryLinksProvider.addObserver(testObserver);
+  removedTopsite = topSites.shift();
+  do_check_eq(removedTopsite, "1040.com");
+  do_check_false(NewTabUtils.isTopPlacesSite(removedTopsite));
+  DirectoryLinksProvider.onLinkChanged(DirectoryLinksProvider, {
+    url: "http://" + removedTopsite,
+    type: "history",
+  });
+  yield testObserver.promise;
+  do_check_eq(testObserver.count, 2);
+  DirectoryLinksProvider.removeObserver(testObserver);
+
+  // Removing all top sites with related links should remove
+  // the current related link and not replace it.
+  topSites = [];
+  testObserver = new TestRemovingRelatedTile();
+  DirectoryLinksProvider.addObserver(testObserver);
+  DirectoryLinksProvider.onManyLinksChanged();
+  yield testObserver.promise;
+
+  // Cleanup
+  yield promiseCleanDirectoryLinksProvider();
+  NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
+  NewTabUtils.getProviderLinks = origGetProviderLinks;
+});
+
 add_task(function test_relatedLinksMap() {
   let data = {"en-US": [relatedTile1, relatedTile2, relatedTile3, someOtherSite]};
   let dataURI = 'data:application/json,' + JSON.stringify(data);
@@ -252,6 +377,12 @@ add_task(function test_topSitesWithRelatedLinks() {
     return topSites.indexOf(site) >= 0;
   }
 
+  // Mock out getProviderLinks() so we don't have to populate cache in NewTabUtils
+  let origGetProviderLinks = NewTabUtils.getProviderLinks;
+  NewTabUtils.getProviderLinks = function(provider) {
+    return [];
+  }
+
   // We start off with no top sites with related links.
   do_check_eq(DirectoryLinksProvider._topSitesWithRelatedLinks.size, 0);
 
@@ -288,6 +419,7 @@ add_task(function test_topSitesWithRelatedLinks() {
 
   // Cleanup.
   NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
+  NewTabUtils.getProviderLinks = origGetProviderLinks;
 });
 
 add_task(function test_reportSitesAction() {

toolkit/modules/NewTabUtils.jsm

@@ -967,8 +967,11 @@ let Links = {
    * @param aLink The link that changed.  If the link is new, it must have all
    *              of the _sortProperties.  Otherwise, it may have as few or as
    *              many as is convenient.
+   * @param aIndex The current index of the changed link in the sortedLinks
+                   cache in _providers. Defaults to -1 if the provider doesn't know the index
+   * @param aDeleted Boolean indicating if the provider has deleted the link.
    */
-  onLinkChanged: function Links_onLinkChanged(aProvider, aLink) {
+  onLinkChanged: function Links_onLinkChanged(aProvider, aLink, aIndex=-1, aDeleted=false) {
     if (!("url" in aLink))
       throw new Error("Changed links must have a url property");
 
@@ -988,19 +991,33 @@ let Links = {
       // Update our copy's position in O(lg n) by first removing it from its
       // list.  It's important to do this before modifying its properties.
       if (this._sortProperties.some(prop => prop in aLink)) {
-        let idx = this._indexOf(sortedLinks, existingLink);
+        let idx = aIndex;
+        if (idx < 0) {
+          idx = this._indexOf(sortedLinks, existingLink);
+        } else if (this.compareLinks(aLink, sortedLinks[idx]) != 0) {
+          throw new Error("aLink should be the same as sortedLinks[idx]");
+        }
+
         if (idx < 0) {
           throw new Error("Link should be in _sortedLinks if in _linkMap");
         }
         sortedLinks.splice(idx, 1);
-        // Update our copy's properties.
-        for (let prop of this._sortProperties) {
-          if (prop in aLink) {
-            existingLink[prop] = aLink[prop];
+
+        if (aDeleted) {
+          updatePages = true;
+          linkMap.delete(existingLink.url);
+          this._decrementSiteMap(siteMap, existingLink);
+        } else {
+          // Update our copy's properties.
+          for (let prop of this._sortProperties) {
+            if (prop in aLink) {
+              existingLink[prop] = aLink[prop];
+            }
           }
+
+          // Finally, reinsert our copy below.
+          insertionLink = existingLink;
         }
-        // Finally, reinsert our copy below.
-        insertionLink = existingLink;
       }
       // Update our copy's title in O(1).
       if ("title" in aLink && aLink.title != existingLink.title) {
@@ -1230,6 +1247,10 @@ this.NewTabUtils = {
     return false;
   },
 
+  getProviderLinks: function(aProvider) {
+    return Links._providers.get(aProvider).sortedLinks;
+  },
+
   isTopSiteGivenProvider: function(aSite, aProvider) {
     return Links._providers.get(aProvider).siteMap.has(aSite);
   },

toolkit/modules/tests/xpcshell/test_NewTabUtils.js

@@ -12,6 +12,41 @@ function run_test() {
   run_next_test();
 }
 
+add_task(function notifyLinkDelete() {
+  let expectedLinks = makeLinks(0, 3, 1);
+
+  let provider = new TestProvider(done => done(expectedLinks));
+  provider.maxNumLinks = expectedLinks.length;
+
+  NewTabUtils.initWithoutProviders();
+  NewTabUtils.links.addProvider(provider);
+  yield new Promise(resolve => NewTabUtils.links.populateCache(resolve));
+
+  do_check_links(NewTabUtils.links.getLinks(), expectedLinks);
+
+  // Remove a link.
+  let removedLink = expectedLinks[2];
+  provider.notifyLinkChanged(removedLink, 2, true);
+  let links = NewTabUtils.links._providers.get(provider);
+
+  // Check that sortedLinks is correctly updated.
+  do_check_links(NewTabUtils.links.getLinks(), expectedLinks.slice(0, 2));
+
+  // Check that linkMap is accurately updated.
+  do_check_eq(links.linkMap.size, 2);
+  do_check_true(links.linkMap.get(expectedLinks[0].url));
+  do_check_true(links.linkMap.get(expectedLinks[1].url));
+  do_check_false(links.linkMap.get(removedLink.url));
+
+  // Check that siteMap is correctly updated.
+  do_check_eq(links.siteMap.size, 2);
+  do_check_true(links.siteMap.has(NewTabUtils.extractSite(expectedLinks[0].url)));
+  do_check_true(links.siteMap.has(NewTabUtils.extractSite(expectedLinks[1].url)));
+  do_check_false(links.siteMap.has(NewTabUtils.extractSite(removedLink.url)));
+
+  NewTabUtils.links.removeProvider(provider);
+});
+
 add_task(function populatePromise() {
   let count = 0;
   let expectedLinks = makeLinks(0, 10, 2);
@@ -267,16 +302,19 @@ TestProvider.prototype = {
   addObserver: function (observer) {
     this._observers.add(observer);
   },
-  notifyLinkChanged: function (link) {
-    this._notifyObservers("onLinkChanged", link);
+  notifyLinkChanged: function (link, index=-1, deleted=false) {
+    this._notifyObservers("onLinkChanged", link, index, deleted);
   },
   notifyManyLinksChanged: function () {
     this._notifyObservers("onManyLinksChanged");
   },
-  _notifyObservers: function (observerMethodName, arg) {
+  _notifyObservers: function () {
+    let observerMethodName = arguments[0];
+    let args = Array.prototype.slice.call(arguments, 1);
+    args.unshift(this);
     for (let obs of this._observers) {
       if (obs[observerMethodName])
-        obs[observerMethodName](this, arg);
+        obs[observerMethodName].apply(NewTabUtils.links, args);
     }
   },
 };