Bug 1139052 - Don't show suggested tiles for users with fewer than 8 history tiles. r=adw, a=lizzard
authorMarina Samuel <msamuel@mozilla.com>
Tue, 14 Apr 2015 14:39:50 -0400
changeset 267368 069da5c02e22aeacfb758d3ab6fedcb9dd781bd2
parent 267367 0ca2f685a10af7adf66df6525f6662f36c43fa9d
child 267369 728c8bbc58434cf0b5ce48ec128e69d556e4decb
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw, lizzard
bugs1139052
milestone39.0a2
Bug 1139052 - Don't show suggested tiles for users with fewer than 8 history tiles. r=adw, a=lizzard
browser/base/content/test/newtab/browser_newtab_enhanced.js
browser/modules/DirectoryLinksProvider.jsm
browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
--- a/browser/base/content/test/newtab/browser_newtab_enhanced.js
+++ b/browser/base/content/test/newtab/browser_newtab_enhanced.js
@@ -104,27 +104,28 @@ function runTests() {
 
   yield unpinCell(0);
 
 
 
   // Test that a suggested tile is not enhanced by a directory tile
   let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite;
   NewTabUtils.isTopPlacesSite = () => true;
-  yield setLinks("-1");
+  yield setLinks("-1,2,3,4,5,6,7,8");
 
   // Test with enhanced = false
   yield addNewTabPageTab();
   yield customizeNewTabPage("classic");
   ({type, enhanced, title} = getData(0));
   isnot(type, "enhanced", "history link is not enhanced");
   is(enhanced, "", "history link has no enhanced image");
   is(title, "site#-1");
 
-  is(getData(1), null, "there is only one link and it's a history link");
+  isnot(getData(7), null, "there are 8 history links");
+  is(getData(8), null, "there are 8 history links");
 
 
   // Test with enhanced = true
   yield addNewTabPageTab();
   yield customizeNewTabPage("enhanced");
 
   // Suggested link was not enhanced by directory link with same domain
   ({type, enhanced, title} = getData(0));
@@ -133,10 +134,10 @@ function runTests() {
   is(title, "title2");
 
   // Enhanced history link shows up second
   ({type, enhanced, title} = getData(1));
   is(type, "enhanced", "pinned history link is enhanced");
   isnot(enhanced, "", "pinned history link has enhanced image");
   is(title, "title");
 
-  is(getData(2), null, "there is only a suggested link followed by an enhanced history link");
+  is(getData(9), null, "there is a suggested link followed by an enhanced history link and the remaining history links");
 }
--- a/browser/modules/DirectoryLinksProvider.jsm
+++ b/browser/modules/DirectoryLinksProvider.jsm
@@ -60,16 +60,19 @@ const ALLOWED_IMAGE_SCHEMES = new Set(["
 const DIRECTORY_FRECENCY = 1000;
 
 // The frecency of a suggested link
 const SUGGESTED_FRECENCY = Infinity;
 
 // Default number of times to show a link
 const DEFAULT_FREQUENCY_CAP = 5;
 
+// The min number of visible (not blocked) history tiles to have before showing suggested tiles
+const MIN_VISIBLE_HISTORY_TILES = 8;
+
 // 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.
@@ -559,17 +562,17 @@ let DirectoryLinksProvider = {
     NewTabUtils.links.populateProviderCache(NewTabUtils.placesProvider, () => {
       this._handleManyLinksChanged();
     });
   },
 
   onLinkChanged: function (aProvider, aLink) {
     // Make sure NewTabUtils.links handles the notification first.
     setTimeout(() => {
-      if (this._handleLinkChanged(aLink)) {
+      if (this._handleLinkChanged(aLink) || this._shouldUpdateSuggestedTile()) {
         this._updateSuggestedTile();
       }
     }, 0);
   },
 
   onManyLinksChanged: function () {
     // Make sure NewTabUtils.links handles the notification first.
     setTimeout(() => {
@@ -587,16 +590,48 @@ let DirectoryLinksProvider = {
     this._frequencyCaps.set(url, remainingViews);
 
     // Reached the number of views, so pick a new one.
     if (remainingViews <= 0) {
       this._updateSuggestedTile();
     }
   },
 
+  _getCurrentTopSiteCount: function() {
+    let visibleTopSiteCount = 0;
+    for (let link of NewTabUtils.links.getLinks().slice(0, MIN_VISIBLE_HISTORY_TILES)) {
+      if (link && (link.type == "history" || link.type == "enhanced")) {
+        visibleTopSiteCount++;
+      }
+    }
+    return visibleTopSiteCount;
+  },
+
+  _shouldUpdateSuggestedTile: function() {
+    let sortedLinks = NewTabUtils.getProviderLinks(this);
+
+    let mostFrecentLink = {};
+    if (sortedLinks && sortedLinks.length) {
+      mostFrecentLink = sortedLinks[0]
+    }
+
+    let currTopSiteCount = this._getCurrentTopSiteCount();
+    if ((!mostFrecentLink.targetedSite && currTopSiteCount >= MIN_VISIBLE_HISTORY_TILES) ||
+        (mostFrecentLink.targetedSite && currTopSiteCount < MIN_VISIBLE_HISTORY_TILES)) {
+      // If mostFrecentLink has a targetedSite then mostFrecentLink is a suggested link.
+      // If we have enough history links (8+) to show a suggested tile and we are not
+      // already showing one, then we should update (to *attempt* to add a suggested tile).
+      // OR if we don't have enough history to show a suggested tile (<8) and we are
+      // currently showing one, we should update (to remove it).
+      return true;
+    }
+
+    return false;
+  },
+
   /**
    * Chooses and returns a suggested tile based on a user's top sites
    * that we have an available suggested tile for.
    *
    * @return the chosen suggested tile, or undefined if there isn't one
    */
   _updateSuggestedTile: function() {
     let sortedLinks = NewTabUtils.getProviderLinks(this);
@@ -616,18 +651,19 @@ let DirectoryLinksProvider = {
           url: mostFrecentLink.url,
           frecency: SUGGESTED_FRECENCY,
           lastVisitDate: mostFrecentLink.lastVisitDate,
           type: mostFrecentLink.type,
         }, 0, true);
       }
     }
 
-    if (this._topSitesWithSuggestedLinks.size == 0) {
-      // There are no potential suggested links we can show.
+    if (this._topSitesWithSuggestedLinks.size == 0 || !this._shouldUpdateSuggestedTile()) {
+      // There are no potential suggested links we can show or not
+      // enough history for a suggested tile.
       return;
     }
 
     // Create a flat list of all possible links we can show as suggested.
     // Note that many top sites may map to the same suggested links, but we only
     // want to count each suggested link once (based on url), thus possibleLinks is a map
     // from url to suggestedLink. Thus, each link has an equal chance of being chosen at
     // random from flattenedLinks if it appears only once.
--- a/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
+++ b/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
@@ -218,16 +218,52 @@ 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_shouldUpdateSuggestedTile() {
+  let suggestedLink = {
+    targetedSite: "somesite.com"
+  };
+
+  // DirectoryLinksProvider has no suggested tile and no top sites => no need to update
+  do_check_eq(DirectoryLinksProvider._getCurrentTopSiteCount(), 0);
+  isIdentical(NewTabUtils.getProviderLinks(), []);
+  do_check_eq(DirectoryLinksProvider._shouldUpdateSuggestedTile(), false);
+
+  // DirectoryLinksProvider has a suggested tile and no top sites => need to update
+  let origGetProviderLinks = NewTabUtils.getProviderLinks;
+  NewTabUtils.getProviderLinks = (provider) => [suggestedLink];
+
+  do_check_eq(DirectoryLinksProvider._getCurrentTopSiteCount(), 0);
+  isIdentical(NewTabUtils.getProviderLinks(), [suggestedLink]);
+  do_check_eq(DirectoryLinksProvider._shouldUpdateSuggestedTile(), true);
+
+  // DirectoryLinksProvider has a suggested tile and 8 top sites => no need to update
+  let origCurrentTopSiteCount = DirectoryLinksProvider._getCurrentTopSiteCount;
+  DirectoryLinksProvider._getCurrentTopSiteCount = () => 8;
+
+  do_check_eq(DirectoryLinksProvider._getCurrentTopSiteCount(), 8);
+  isIdentical(NewTabUtils.getProviderLinks(), [suggestedLink]);
+  do_check_eq(DirectoryLinksProvider._shouldUpdateSuggestedTile(), false);
+
+  // DirectoryLinksProvider has no suggested tile and 8 top sites => need to update
+  NewTabUtils.getProviderLinks = origGetProviderLinks;
+  do_check_eq(DirectoryLinksProvider._getCurrentTopSiteCount(), 8);
+  isIdentical(NewTabUtils.getProviderLinks(), []);
+  do_check_eq(DirectoryLinksProvider._shouldUpdateSuggestedTile(), true);
+
+  // Cleanup
+  DirectoryLinksProvider._getCurrentTopSiteCount = origCurrentTopSiteCount;
+});
+
 add_task(function test_updateSuggestedTile() {
   let topSites = ["site0.com", "1040.com", "site2.com", "hrblock.com", "site4.com", "freetaxusa.com", "site6.com"];
 
   // Initial setup
   let data = {"suggested": [suggestedTile1, suggestedTile2, suggestedTile3], "directory": [someOtherSite]};
   let dataURI = 'data:application/json,' + JSON.stringify(data);
 
   let testObserver = new TestFirstRun();
@@ -241,16 +277,19 @@ add_task(function test_updateSuggestedTi
     return topSites.indexOf(site) >= 0;
   }
 
   let origGetProviderLinks = NewTabUtils.getProviderLinks;
   NewTabUtils.getProviderLinks = function(provider) {
     return links;
   }
 
+  let origCurrentTopSiteCount = DirectoryLinksProvider._getCurrentTopSiteCount;
+  DirectoryLinksProvider._getCurrentTopSiteCount = () => 8;
+
   do_check_eq(DirectoryLinksProvider._updateSuggestedTile(), undefined);
 
   function TestFirstRun() {
     this.promise = new Promise(resolve => {
       this.onLinkChanged = (directoryLinksProvider, link) => {
         links.unshift(link);
         let possibleLinks = [suggestedTile1.url, suggestedTile2.url, suggestedTile3.url];
 
@@ -341,16 +380,17 @@ add_task(function test_updateSuggestedTi
   DirectoryLinksProvider.addObserver(testObserver);
   DirectoryLinksProvider.onManyLinksChanged();
   yield testObserver.promise;
 
   // Cleanup
   yield promiseCleanDirectoryLinksProvider();
   NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
   NewTabUtils.getProviderLinks = origGetProviderLinks;
+  DirectoryLinksProvider._getCurrentTopSiteCount = origCurrentTopSiteCount;
 });
 
 add_task(function test_suggestedLinksMap() {
   let data = {"suggested": [suggestedTile1, suggestedTile2, suggestedTile3, suggestedTile4], "directory": [someOtherSite]};
   let dataURI = 'data:application/json,' + JSON.stringify(data);
 
   yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
   let links = yield fetchData();
@@ -431,16 +471,19 @@ add_task(function test_topSitesWithSugge
   NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
   NewTabUtils.getProviderLinks = origGetProviderLinks;
 });
 
 add_task(function test_suggestedAttributes() {
   let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite;
   NewTabUtils.isTopPlacesSite = () => true;
 
+  let origCurrentTopSiteCount = DirectoryLinksProvider._getCurrentTopSiteCount;
+  DirectoryLinksProvider._getCurrentTopSiteCount = () => 8;
+
   let frecent_sites = ["top.site.com"];
   let imageURI = "https://image/";
   let title = "the title";
   let type = "affiliate";
   let url = "http://test.url/";
   let data = {
     suggested: [{
       frecent_sites,
@@ -473,25 +516,29 @@ add_task(function test_suggestedAttribut
   do_check_eq(link.imageURI, imageURI);
   do_check_eq(link.targetedSite, frecent_sites[0]);
   do_check_eq(link.title, title);
   do_check_eq(link.type, type);
   do_check_eq(link.url, url);
 
   // Cleanup.
   NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
+  DirectoryLinksProvider._getCurrentTopSiteCount = origCurrentTopSiteCount;
   gLinks.removeProvider(DirectoryLinksProvider);
   DirectoryLinksProvider.removeObserver(gLinks);
 });
 
 add_task(function test_frequencyCappedSites_views() {
   Services.prefs.setCharPref(kPingUrlPref, "");
   let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite;
   NewTabUtils.isTopPlacesSite = () => true;
 
+  let origCurrentTopSiteCount = DirectoryLinksProvider._getCurrentTopSiteCount;
+  DirectoryLinksProvider._getCurrentTopSiteCount = () => 8;
+
   let testUrl = "http://frequency.capped/link";
   let targets = ["top.site.com"];
   let data = {
     suggested: [{
       type: "affiliate",
       frecent_sites: targets,
       url: testUrl
     }],
@@ -543,26 +590,30 @@ add_task(function test_frequencyCappedSi
   checkFirstTypeAndLength("affiliate", 2);
   synthesizeAction("view");
   checkFirstTypeAndLength("affiliate", 2);
   synthesizeAction("view");
   checkFirstTypeAndLength("organic", 1);
 
   // Cleanup.
   NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
+  DirectoryLinksProvider._getCurrentTopSiteCount = origCurrentTopSiteCount;
   gLinks.removeProvider(DirectoryLinksProvider);
   DirectoryLinksProvider.removeObserver(gLinks);
   Services.prefs.setCharPref(kPingUrlPref, kPingUrl);
 });
 
 add_task(function test_frequencyCappedSites_click() {
   Services.prefs.setCharPref(kPingUrlPref, "");
   let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite;
   NewTabUtils.isTopPlacesSite = () => true;
 
+  let origCurrentTopSiteCount = DirectoryLinksProvider._getCurrentTopSiteCount;
+  DirectoryLinksProvider._getCurrentTopSiteCount = () => 8;
+
   let testUrl = "http://frequency.capped/link";
   let targets = ["top.site.com"];
   let data = {
     suggested: [{
       type: "affiliate",
       frecent_sites: targets,
       url: testUrl
     }],
@@ -608,16 +659,17 @@ add_task(function test_frequencyCappedSi
   checkFirstTypeAndLength("affiliate", 2);
   synthesizeAction("view");
   checkFirstTypeAndLength("affiliate", 2);
   synthesizeAction("click");
   checkFirstTypeAndLength("organic", 1);
 
   // Cleanup.
   NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
+  DirectoryLinksProvider._getCurrentTopSiteCount = origCurrentTopSiteCount;
   gLinks.removeProvider(DirectoryLinksProvider);
   DirectoryLinksProvider.removeObserver(gLinks);
   Services.prefs.setCharPref(kPingUrlPref, kPingUrl);
 });
 
 add_task(function test_reportSitesAction() {
   yield DirectoryLinksProvider.init();
   let deferred, expectedPath, expectedPost;
@@ -1084,16 +1136,18 @@ add_task(function test_DirectoryLinksPro
   do_check_eq(links.length, 0); // There are no directory links.
   checkEnhanced("http://example.net", undefined);
   checkEnhanced("http://example.com", "data:,fresh");
 });
 
 add_task(function test_DirectoryLinksProvider_enhancedURIs() {
   let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite;
   NewTabUtils.isTopPlacesSite = () => true;
+  let origCurrentTopSiteCount = DirectoryLinksProvider._getCurrentTopSiteCount;
+  DirectoryLinksProvider._getCurrentTopSiteCount = () => 8;
 
   let data = {
     "suggested": [
       {url: "http://example.net", enhancedImageURI: "data:,net1", title:"SuggestedTitle", frecent_sites: ["test.com"]}
     ],
     "directory": [
       {url: "http://example.net", enhancedImageURI: "data:,net2", title:"DirectoryTitle"}
     ]
@@ -1124,16 +1178,17 @@ add_task(function test_DirectoryLinksPro
   // Check that the suggested tile with the same URL replaces the directory tile.
   links = gLinks.getLinks();
   do_check_eq(links.length, 1);
   do_check_eq(links[0].title, "SuggestedTitle");
   do_check_eq(links[0].enhancedImageURI, "data:,net1");
 
   // Cleanup.
   NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
+  DirectoryLinksProvider._getCurrentTopSiteCount = origCurrentTopSiteCount;
   gLinks.removeProvider(DirectoryLinksProvider);
 });
 
 add_task(function test_DirectoryLinksProvider_setDefaultEnhanced() {
   function checkDefault(expected) {
     Services.prefs.clearUserPref(kNewtabEnhancedPref);
     do_check_eq(Services.prefs.getBoolPref(kNewtabEnhancedPref), expected);
   }