Bug 1126184: Part 2: Select a single tile to show as the first unpinned tile based on a user's top sites. r=adw
authorMarina Samuel <msamuel@mozilla.com>
Fri, 13 Mar 2015 11:45:34 -0400
changeset 233751 973fab97c0b21818a63d9dc4c726040b241c77b2
parent 233750 dab6b12a0ff6c2d5f56eedb9ca4c94086399e14f
child 233752 d0e5df2bd844693cd17ac36f2f4d81ddd593f6b5
push id28423
push userphilringnalda@gmail.com
push dateMon, 16 Mar 2015 02:35:36 +0000
treeherdermozilla-central@436686833af0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1126184
milestone39.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 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
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++) {