Bug 1126184: Part 2: Select a single tile to show as the first unpinned tile based on a user's top sites. r=adw, a=sylvestre
authorMarina Samuel <msamuel@mozilla.com>
Fri, 13 Mar 2015 11:45:34 -0400
changeset 258189 48564fb0e663
parent 258188 869ba4681d1b
child 258190 1a007e477655
push id4619
push useredilee@gmail.com
push date2015-04-02 05:50 +0000
treeherdermozilla-beta@daf8a9291a9b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, sylvestre
bugs1126184
milestone38.0
Bug 1126184: Part 2: Select a single tile to show as the first unpinned tile based on a user's top sites. r=adw, a=sylvestre
browser/modules/DirectoryLinksProvider.jsm
browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
toolkit/modules/NewTabUtils.jsm
toolkit/modules/tests/xpcshell/test_NewTabUtils.js
--- a/browser/modules/DirectoryLinksProvider.jsm
+++ b/browser/modules/DirectoryLinksProvider.jsm
@@ -52,16 +52,19 @@ const PREF_NEWTAB_ENHANCED = "browser.ne
 const ALLOWED_LINK_SCHEMES = new Set(["http", "https"]);
 
 // Only allow link image urls that are https or data
 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;
 
 // Allowed ping actions remotely stored as columns: case-insensitive [a-z0-9_]
 const PING_ACTIONS = ["block", "click", "pin", "sponsored", "sponsored_link", "unpin", "view"];
 
 /**
  * Singleton that serves as the provider of directory links.
@@ -467,51 +470,121 @@ let DirectoryLinksProvider = {
 
   _handleManyLinksChanged: function() {
     this._topSitesWithRelatedLinks.clear();
     this._relatedLinks.forEach((relatedLinks, site) => {
       if (NewTabUtils.isTopPlacesSite(site)) {
         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 () {
     NewTabUtils.links.populateProviderCache(NewTabUtils.placesProvider, () => {
       this._handleManyLinksChanged();
     });
   },
 
   onLinkChanged: function (aProvider, aLink) {
     // Make sure NewTabUtils.links handles the notification first.
     setTimeout(() => {
-      this._handleLinkChanged(aLink);
+      if (this._handleLinkChanged(aLink)) {
+        this._updateRelatedTile();
+      }
     }, 0);
   },
 
   onManyLinksChanged: function () {
     // Make sure NewTabUtils.links handles the notification first.
     setTimeout(() => {
       this._handleManyLinksChanged();
     }, 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
    */
   reset: function DirectoryLinksProvider_reset() {
     delete this.__linksURL;
     this._removePrefsObserver();
     this._removeObservers();
   },
 
--- a/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
+++ b/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
@@ -209,16 +209,141 @@ function run_test() {
     DirectoryLinksProvider.reset();
     Services.prefs.clearUserPref(kLocalePref);
     Services.prefs.clearUserPref(kSourceUrlPref);
     Services.prefs.clearUserPref(kPingUrlPref);
     Services.prefs.clearUserPref(kNewtabEnhancedPref);
   });
 }
 
+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);
 
   yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
   let links = yield fetchData();
 
   // Ensure the related tiles were not considered directory tiles.
@@ -247,16 +372,22 @@ add_task(function test_relatedLinksMap()
 
 add_task(function test_topSitesWithRelatedLinks() {
   let topSites = ["site0.com", "1040.com", "site2.com", "hrblock.com", "site4.com", "freetaxusa.com", "site6.com"];
   let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite;
   NewTabUtils.isTopPlacesSite = function(site) {
     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);
 
   let data = {"en-US": [relatedTile1, relatedTile2, relatedTile3, someOtherSite]};
   let dataURI = 'data:application/json,' + JSON.stringify(data);
 
   yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
   let links = yield fetchData();
@@ -283,16 +414,17 @@ add_task(function test_topSitesWithRelat
   // Re-adding freetaxusa.com as a topsite will add it to _topSitesWithRelatedLinks.
   topSites.push(popped);
   expectedTopSitesWithRelatedLinks.push(popped);
   DirectoryLinksProvider._handleLinkChanged({url: "http://" + popped});
   isIdentical([...DirectoryLinksProvider._topSitesWithRelatedLinks], expectedTopSitesWithRelatedLinks);
 
   // Cleanup.
   NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
+  NewTabUtils.getProviderLinks = origGetProviderLinks;
 });
 
 add_task(function test_reportSitesAction() {
   yield DirectoryLinksProvider.init();
   let deferred, expectedPath, expectedPost;
   let done = false;
   server.registerPrefixHandler(kPingPath, (aRequest, aResponse) => {
     if (done) {
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -962,18 +962,21 @@ let Links = {
   },
 
   /**
    * Called by a provider to notify us when a single link changes.
    * @param aProvider The provider whose link changed.
    * @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");
 
     let links = this._providers.get(aProvider);
     if (!links)
       // This is not an error, it just means that between the time the provider
       // was added and the future time we call getLinks on it, it notified us of
       // a change.
@@ -983,29 +986,43 @@ let Links = {
     let existingLink = linkMap.get(aLink.url);
     let insertionLink = null;
     let updatePages = false;
 
     if (existingLink) {
       // 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) {
         existingLink.title = aLink.title;
         updatePages = true;
       }
     }
     else if (this._sortProperties.every(prop => prop in aLink)) {
@@ -1225,16 +1242,20 @@ this.NewTabUtils = {
       this._initialized = true;
       ExpirationFilter.init();
       Telemetry.init();
       return true;
     }
     return false;
   },
 
+  getProviderLinks: function(aProvider) {
+    return Links._providers.get(aProvider).sortedLinks;
+  },
+
   isTopSiteGivenProvider: function(aSite, aProvider) {
     return Links._providers.get(aProvider).siteMap.has(aSite);
   },
 
   isTopPlacesSite: function(aSite) {
     return this.isTopSiteGivenProvider(aSite, PlacesProvider);
   },
 
--- a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
+++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
@@ -7,16 +7,51 @@ const { classes: Cc, interfaces: Ci, res
 Cu.import("resource://gre/modules/NewTabUtils.jsm");
 Cu.import("resource://gre/modules/Promise.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 
 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);
 
   let getLinksFcn = Task.async(function* (callback) {
     //Should not be calling getLinksFcn twice
     count++;
     do_check_eq(count, 1);
@@ -262,26 +297,29 @@ function TestProvider(getLinksFn) {
   this.getLinks = getLinksFn;
   this._observers = new Set();
 }
 
 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);
     }
   },
 };
 
 function do_check_links(actualLinks, expectedLinks) {
   do_check_true(Array.isArray(actualLinks));
   do_check_eq(actualLinks.length, expectedLinks.length);
   for (let i = 0; i < expectedLinks.length; i++) {