Bug 1105360: Only enhance tiles that are under the 'enhanced' key. r=adw, a=sylvestre
authorMarina Samuel <msamuel@mozilla.com>
Wed, 01 Apr 2015 17:26:46 -0400
changeset 258202 311733df5675
parent 258201 da2535172770
child 258203 96e8fba7c4c4
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
bugs1105360
milestone38.0
Bug 1105360: Only enhance tiles that are under the 'enhanced' key. r=adw, a=sylvestre
browser/base/content/test/newtab/browser_newtab_enhanced.js
browser/modules/DirectoryLinksProvider.jsm
browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
toolkit/modules/NewTabUtils.jsm
--- a/browser/base/content/test/newtab/browser_newtab_enhanced.js
+++ b/browser/base/content/test/newtab/browser_newtab_enhanced.js
@@ -1,19 +1,32 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 const PRELOAD_PREF = "browser.newtab.preload";
 
 gDirectorySource = "data:application/json," + JSON.stringify({
-  "directory": [{
+  "enhanced": [{
     url: "http://example.com/",
     enhancedImageURI: "data:image/png;base64,helloWORLD",
     title: "title",
+    type: "organic",
+  }],
+  "directory": [{
+    url: "http://example1.com/",
+    enhancedImageURI: "data:image/png;base64,helloWORLD2",
+    title: "title1",
     type: "organic"
+  }],
+  "suggested": [{
+    url: "http://example1.com/2",
+    imageURI: "data:image/png;base64,helloWORLD3",
+    title: "title2",
+    type: "affiliate",
+    frecent_sites: ["test.com"]
   }]
 });
 
 function runTests() {
   let origEnhanced = NewTabUtils.allPages.enhanced;
   registerCleanupFunction(() => {
     Services.prefs.clearUserPref(PRELOAD_PREF);
     NewTabUtils.allPages.enhanced = origEnhanced;
@@ -28,17 +41,17 @@ function runTests() {
     let siteNode = cell.site.node;
     return {
       type: siteNode.getAttribute("type"),
       enhanced: siteNode.querySelector(".enhanced-content").style.backgroundImage,
       title: siteNode.querySelector(".newtab-title").textContent,
     };
   }
 
-  // Make the page have a directory link followed by a history link
+  // Make the page have a directory link, enhanced link, and history link
   yield setLinks("-1");
 
   // Test with enhanced = false
   yield addNewTabPageTab();
   yield customizeNewTabPage("classic");
   let {type, enhanced, title} = getData(0);
   isnot(type, "enhanced", "history link is not enhanced");
   is(enhanced, "", "history link has no enhanced image");
@@ -47,35 +60,83 @@ function runTests() {
   is(getData(1), null, "there is only one link and it's a history link");
 
   // Test with enhanced = true
   yield addNewTabPageTab();
   yield customizeNewTabPage("enhanced");
   ({type, enhanced, title} = getData(0));
   is(type, "organic", "directory link is organic");
   isnot(enhanced, "", "directory link has enhanced image");
+  is(title, "title1");
+
+  ({type, enhanced, title} = getData(1));
+  is(type, "enhanced", "history link is enhanced");
+  isnot(enhanced, "", "history link has enhanced image");
   is(title, "title");
 
-  is(getData(1), null, "history link pushed out by directory link");
+  is(getData(2), null, "there are only 2 links, directory and enhanced history");
 
   // Test with a pinned link
   setPinnedLinks("-1");
   yield addNewTabPageTab();
   ({type, enhanced, title} = getData(0));
-  is(type, "history", "pinned history link is not enhanced");
-  is(enhanced, "", "pinned history link doesn't have enhanced image");
-  is(title, "site#-1");
+  is(type, "enhanced", "pinned history link is enhanced");
+  isnot(enhanced, "", "pinned history link has enhanced image");
+  is(title, "title");
 
-  is(getData(1), null, "directory link pushed out by pinned history link");
+  ({type, enhanced, title} = getData(1));
+  is(type, "organic", "directory link is organic");
+  isnot(enhanced, "", "directory link has enhanced image");
+  is(title, "title1");
+
+  is(getData(2), null, "directory link pushed out by pinned history link");
 
   // Test pinned link 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, "directory link still pushed out by pinned history link");
 
   ok(getContentDocument().getElementById("newtab-intro-what"),
      "'What is this page?' link exists");
+
+  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");
+
+  // 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");
+
+
+  // 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));
+  is(type, "affiliate", "suggested link is affiliate");
+  is(enhanced, "", "suggested link has no enhanced image");
+  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");
 }
--- a/browser/modules/DirectoryLinksProvider.jsm
+++ b/browser/modules/DirectoryLinksProvider.jsm
@@ -205,16 +205,20 @@ let DirectoryLinksProvider = {
   _removePrefsObserver: function DirectoryLinksProvider_removeObserver() {
     for (let pref in this._observedPrefs) {
       let prefName = this._observedPrefs[pref];
       Services.prefs.removeObserver(prefName, this);
     }
   },
 
   _cacheSuggestedLinks: function(link) {
+    if (!link.frecent_sites) {
+      // Don't cache links that don't have the expected 'frecent_sites'.
+      return;
+    }
     for (let suggestedSite of link.frecent_sites) {
       let suggestedMap = this._suggestedLinks.get(suggestedSite) || new Map();
       suggestedMap.set(link.url, link);
       this._suggestedLinks.set(suggestedSite, suggestedMap);
     }
   },
 
   _fetchAndCacheLinks: function DirectoryLinksProvider_fetchAndCacheLinks(uri) {
@@ -301,23 +305,25 @@ let DirectoryLinksProvider = {
 
   /**
    * Reads directory links file and parses its content
    * @return a promise resolved to an object with keys 'directory' and 'suggested',
    *         each containing a valid list of links,
    *         or {'directory': [], 'suggested': []} if read or parse fails.
    */
   _readDirectoryLinksFile: function DirectoryLinksProvider_readDirectoryLinksFile() {
-    let emptyOutput = {directory: [], suggested: []};
+    let emptyOutput = {directory: [], suggested: [], enhanced: []};
     return OS.File.read(this._directoryFilePath).then(binaryData => {
       let output;
       try {
         let json = gTextDecoder.decode(binaryData);
         let linksObj = JSON.parse(json);
-        output = {directory: linksObj.directory || [], suggested: linksObj.suggested || []};
+        output = {directory: linksObj.directory || [],
+                  suggested: linksObj.suggested || [],
+                  enhanced:  linksObj.enhanced  || []};
       }
       catch (e) {
         Cu.reportError(e);
       }
       return output || emptyOutput;
     },
     error => {
       Cu.reportError(error);
@@ -405,18 +411,17 @@ let DirectoryLinksProvider = {
     return this._fetchAndCacheLinksIfNecessary();
   },
 
   /**
    * Get the enhanced link object for a link (whether history or directory)
    */
   getEnhancedLink: function DirectoryLinksProvider_getEnhancedLink(link) {
     // Use the provided link if it's already enhanced
-    return link.type == "history" ? null :
-           link.enhancedImageURI && link ? link :
+    return link.enhancedImageURI && link ? link :
            this._enhancedLinks.get(NewTabUtils.extractSite(link.url));
   },
 
   /**
    * Check if a url's scheme is in a Set of allowed schemes
    */
   isURLAllowed: function DirectoryLinksProvider_isURLAllowed(url, allowed) {
     // Assume no url is an allowed url
@@ -446,35 +451,36 @@ let DirectoryLinksProvider = {
 
       let validityFilter = function(link) {
         // Make sure the link url is allowed and images too if they exist
         return this.isURLAllowed(link.url, ALLOWED_LINK_SCHEMES) &&
                this.isURLAllowed(link.imageURI, ALLOWED_IMAGE_SCHEMES) &&
                this.isURLAllowed(link.enhancedImageURI, ALLOWED_IMAGE_SCHEMES);
       }.bind(this);
 
-      let setCommonProperties = function(link, length, position) {
-        // Stash the enhanced image for the site
-        if (link.enhancedImageURI) {
-          this._enhancedLinks.set(NewTabUtils.extractSite(link.url), link);
-        }
-        link.lastVisitDate = length - position;
-      }.bind(this);
-
       rawLinks.suggested.filter(validityFilter).forEach((link, position) => {
-        setCommonProperties(link, rawLinks.suggested.length, position);
+        link.lastVisitDate = rawLinks.suggested.length - position;
 
         // We cache suggested tiles here but do not push any of them in the links list yet.
         // The decision for which suggested tile to include will be made separately.
         this._cacheSuggestedLinks(link);
         this._frequencyCaps.set(link.url, DEFAULT_FREQUENCY_CAP);
       });
 
+      rawLinks.enhanced.filter(validityFilter).forEach((link, position) => {
+        link.lastVisitDate = rawLinks.enhanced.length - position;
+
+        // Stash the enhanced image for the site
+        if (link.enhancedImageURI) {
+          this._enhancedLinks.set(NewTabUtils.extractSite(link.url), link);
+        }
+      });
+
       let links = rawLinks.directory.filter(validityFilter).map((link, position) => {
-        setCommonProperties(link, rawLinks.directory.length, position);
+        link.lastVisitDate = rawLinks.directory.length - position;
         link.frecency = DIRECTORY_FRECENCY;
         return link;
       });
 
       // Allow for one link suggestion on top of the default directory links
       this.maxNumLinks = links.length + 1;
 
       return links;
--- a/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
+++ b/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
@@ -1021,26 +1021,26 @@ add_task(function test_DirectoryLinksPro
   do_check_eq(links.length, 2);
 
   // The only remaining enhancedImages should be http and https and data
   do_check_eq(links[0].enhancedImageURI, data["directory"][3].enhancedImageURI);
   do_check_eq(links[1].enhancedImageURI, data["directory"][5].enhancedImageURI);
 });
 
 add_task(function test_DirectoryLinksProvider_getEnhancedLink() {
-  let data = {"directory": [
+  let data = {"enhanced": [
     {url: "http://example.net", enhancedImageURI: "data:,net1"},
     {url: "http://example.com", enhancedImageURI: "data:,com1"},
     {url: "http://example.com", enhancedImageURI: "data:,com2"},
   ]};
   let dataURI = 'data:application/json,' + JSON.stringify(data);
   yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
 
   let links = yield fetchData();
-  do_check_eq(links.length, 3);
+  do_check_eq(links.length, 0); // There are no directory links.
 
   function checkEnhanced(url, image) {
     let enhanced = DirectoryLinksProvider.getEnhancedLink({url: url});
     do_check_eq(enhanced && enhanced.enhancedImageURI, image);
   }
 
   // Get the expected image for the same site
   checkEnhanced("http://example.net/", "data:,net1");
@@ -1061,27 +1061,73 @@ add_task(function test_DirectoryLinksPro
 
   // Undefined for not enhanced
   checkEnhanced("http://sub.example.net/", undefined);
   checkEnhanced("http://example.org", undefined);
   checkEnhanced("http://localhost", undefined);
   checkEnhanced("http://127.0.0.1", undefined);
 
   // Make sure old data is not cached
-  data = {"directory": [
+  data = {"enhanced": [
     {url: "http://example.com", enhancedImageURI: "data:,fresh"},
   ]};
   dataURI = 'data:application/json,' + JSON.stringify(data);
   yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
   links = yield fetchData();
-  do_check_eq(links.length, 1);
+  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 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"}
+    ]
+  };
+  let dataURI = 'data:application/json,' + JSON.stringify(data);
+  yield promiseSetupDirectoryLinksProvider({linksURL: dataURI});
+
+  // Wait for links to get loaded
+  let gLinks = NewTabUtils.links;
+  gLinks.addProvider(DirectoryLinksProvider);
+  gLinks.populateCache();
+  yield new Promise(resolve => {
+    NewTabUtils.allPages.register({
+      observe: _ => _,
+      update() {
+        NewTabUtils.allPages.unregister(this);
+        resolve();
+      }
+    });
+  });
+
+  // Check that we've saved the directory tile.
+  let links = yield fetchData();
+  do_check_eq(links.length, 1);
+  do_check_eq(links[0].title, "DirectoryTitle");
+  do_check_eq(links[0].enhancedImageURI, "data:,net2");
+
+  // 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;
+  gLinks.removeProvider(DirectoryLinksProvider);
+});
+
 add_task(function test_DirectoryLinksProvider_setDefaultEnhanced() {
   function checkDefault(expected) {
     Services.prefs.clearUserPref(kNewtabEnhancedPref);
     do_check_eq(Services.prefs.getBoolPref(kNewtabEnhancedPref), expected);
   }
 
   // Use the default donottrack prefs (enabled = false)
   Services.prefs.clearUserPref("privacy.donottrackheader.enabled");
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -1009,21 +1009,17 @@ let Links = {
         sortedLinks.splice(idx, 1);
 
         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];
-            }
-          }
+          Object.assign(existingLink, aLink);
 
           // 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;