Bug 1105360: Only enhance tiles that are under the 'enhanced' key. r=adw
authorMarina Samuel <msamuel@mozilla.com>
Wed, 01 Apr 2015 17:26:46 -0400
changeset 237250 895cf25d11fdee52b78430bdf139e1b11baa22ce
parent 237249 c6b002187f587f7c5404bfbd172fe11c2d5e1a2d
child 237251 ab12986b946810eb5474e53266134bfc4c8bb0eb
push id57898
push usercbook@mozilla.com
push dateThu, 02 Apr 2015 12:14:17 +0000
treeherdermozilla-inbound@63c87250946e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1105360
milestone40.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 1105360: Only enhance tiles that are under the 'enhanced' key. r=adw
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
@@ -210,16 +210,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) {
@@ -306,23 +310,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);
@@ -410,18 +416,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
@@ -451,35 +456,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;