Bug 1105360: Only enhance tiles that are under the 'enhanced' key. r=adw

browser/base/content/test/newtab/browser_newtab_enhanced.js

@@ -4,11 +4,24 @@
 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"]
   }]
 });
 
@@ -33,7 +46,7 @@ function runTests() {
     };
   }
 
-  // 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
@@ -52,19 +65,29 @@ function runTests() {
   ({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();
@@ -78,4 +101,42 @@ function runTests() {
 
   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");
 }

browser/modules/DirectoryLinksProvider.jsm

@@ -215,6 +215,10 @@ let DirectoryLinksProvider = {
   },
 
   _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);
@@ -311,13 +315,15 @@ let DirectoryLinksProvider = {
    *         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);
@@ -415,8 +421,7 @@ let DirectoryLinksProvider = {
    */
   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));
   },
 
@@ -456,16 +461,8 @@ let DirectoryLinksProvider = {
                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.
@@ -473,8 +470,17 @@ let DirectoryLinksProvider = {
         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;
       });

browser/modules/test/xpcshell/test_DirectoryLinksProvider.js

@@ -1026,7 +1026,7 @@ add_task(function test_DirectoryLinksProvider_getAllowedEnhancedImages() {
 });
 
 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"},
@@ -1035,7 +1035,7 @@ add_task(function test_DirectoryLinksProvider_getEnhancedLink() {
   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});
@@ -1066,17 +1066,63 @@ add_task(function test_DirectoryLinksProvider_getEnhancedLink() {
   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);

toolkit/modules/NewTabUtils.jsm

@@ -1014,11 +1014,7 @@ let Links = {
           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;