Bug 1151876 - Blocking a targeted tile should remove its associated suggested tile in the next new tab. r=adw
authorMarina Samuel <msamuel@mozilla.com>
Tue, 14 Apr 2015 14:34:32 -0400
changeset 270494 3c1b43d1b703746a212b9050ddbad6dd0f5e7579
parent 270493 73daa8f0f5f0f2b057ebddd89af6f76132d5e16f
child 270495 9b3b216d2e98792ac2bfc464fb69efdf5c1d3484
push id863
push userraliiev@mozilla.com
push dateMon, 03 Aug 2015 13:22:43 +0000
treeherdermozilla-release@f6321b14228d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs1151876
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 1151876 - Blocking a targeted tile should remove its associated suggested tile in the next new tab. r=adw
browser/base/content/test/newtab/browser_newtab_block.js
browser/modules/DirectoryLinksProvider.jsm
toolkit/modules/NewTabUtils.jsm
--- a/browser/base/content/test/newtab/browser_newtab_block.js
+++ b/browser/base/content/test/newtab/browser_newtab_block.js
@@ -1,17 +1,31 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 /*
  * These tests make sure that blocking/removing sites from the grid works
  * as expected. Pinned tabs should not be moved. Gaps will be re-filled
  * if more sites are available.
  */
+
+gDirectorySource = "data:application/json," + JSON.stringify({
+  "suggested": [{
+    url: "http://suggested.com/",
+    imageURI: "data:image/png;base64,helloWORLD3",
+    title: "title",
+    type: "affiliate",
+    frecent_sites: ["example0.com"]
+  }]
+});
+
 function runTests() {
+  let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite;
+  NewTabUtils.isTopPlacesSite = (site) => false;
+
   // we remove sites and expect the gaps to be filled as long as there still
   // are some sites available
   yield setLinks("0,1,2,3,4,5,6,7,8,9");
   setPinnedLinks("");
 
   yield addNewTabPageTab();
   checkGrid("0,1,2,3,4,5,6,7,8");
 
@@ -53,9 +67,21 @@ function runTests() {
   yield setLinks("0,1,2,3,4,5,6,7,8,9");
   setPinnedLinks(",,,,,,,,8");
 
   yield addNewTabPageTab();
   checkGrid("0,1,2,3,4,5,6,7,8p");
 
   yield blockCell(0);
   checkGrid("1,2,3,4,5,6,7,9,8p");
+
+  // Test that blocking the targeted site also removes its associated suggested tile
+  NewTabUtils.isTopPlacesSite = origIsTopPlacesSite;
+  yield restore();
+  yield setLinks("0,1,2,3,4,5,6,7,8,9");
+  yield addNewTabPageTab();
+  yield customizeNewTabPage("enhanced");
+  checkGrid("http://suggested.com/,0,1,2,3,4,5,6,7,8,9");
+
+  yield blockCell(1);
+  yield addNewTabPageTab();
+  checkGrid("1,2,3,4,5,6,7,8,9");
 }
--- a/browser/modules/DirectoryLinksProvider.jsm
+++ b/browser/modules/DirectoryLinksProvider.jsm
@@ -504,16 +504,17 @@ let DirectoryLinksProvider = {
   init: function DirectoryLinksProvider_init() {
     this._setDefaultEnhanced();
     this._addPrefsObserver();
     // setup directory file path and last download timestamp
     this._directoryFilePath = OS.Path.join(OS.Constants.Path.localProfileDir, DIRECTORY_LINKS_FILE);
     this._lastDownloadMS = 0;
 
     NewTabUtils.placesProvider.addObserver(this);
+    NewTabUtils.links.addObserver(this);
 
     return Task.spawn(function() {
       // get the last modified time of the links file if it exists
       let doesFileExists = yield OS.File.exists(this._directoryFilePath);
       if (doesFileExists) {
         let fileInfo = yield OS.File.stat(this._directoryFilePath);
         this._lastDownloadMS = Date.parse(fileInfo.lastModificationDate);
       }
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -467,50 +467,64 @@ let PinnedLinks = {
   }
 };
 
 /**
  * Singleton that keeps track of all blocked links in the grid.
  */
 let BlockedLinks = {
   /**
+   * A list of objects that are observing blocked link changes.
+   */
+  _observers: [],
+
+  /**
    * The cached list of blocked links.
    */
   _links: null,
 
   /**
+   * Registers an object that will be notified when the blocked links change.
+   */
+  addObserver: function (aObserver) {
+    this._observers.push(aObserver);
+  },
+
+  /**
    * The list of blocked links.
    */
   get links() {
     if (!this._links)
       this._links = Storage.get("blockedLinks", {});
 
     return this._links;
   },
 
   /**
-   * Blocks a given link.
+   * Blocks a given link. Adjusts siteMap accordingly, and notifies listeners.
    * @param aLink The link to block.
    */
   block: function BlockedLinks_block(aLink) {
+    this._callObservers("onLinkBlocked", aLink);
     this.links[toHash(aLink.url)] = 1;
     this.save();
 
     // Make sure we unpin blocked links.
     PinnedLinks.unpin(aLink);
   },
 
   /**
-   * Unblocks a given link.
+   * Unblocks a given link. Adjusts siteMap accordingly, and notifies listeners.
    * @param aLink The link to unblock.
    */
   unblock: function BlockedLinks_unblock(aLink) {
     if (this.isBlocked(aLink)) {
       delete this.links[toHash(aLink.url)];
       this.save();
+      this._callObservers("onLinkUnblocked", aLink);
     }
   },
 
   /**
    * Saves the current list of blocked links.
    */
   save: function BlockedLinks_save() {
     Storage.set("blockedLinks", this.links);
@@ -532,16 +546,28 @@ let BlockedLinks = {
     return Object.keys(this.links).length == 0;
   },
 
   /**
    * Resets the links cache.
    */
   resetCache: function BlockedLinks_resetCache() {
     this._links = null;
+  },
+
+  _callObservers(methodName, ...args) {
+    for (let obs of this._observers) {
+      if (typeof(obs[methodName]) == "function") {
+        try {
+          obs[methodName](...args);
+        } catch (err) {
+          Cu.reportError(err);
+        }
+      }
+    }
   }
 };
 
 /**
  * Singleton that serves as the default link provider for the grid. It queries
  * the history to retrieve the most frequently visited sites.
  */
 let PlacesProvider = {
@@ -714,16 +740,18 @@ let Links = {
    * The maximum number of links returned by getLinks.
    */
   maxNumLinks: LINKS_GET_LINKS_LIMIT,
 
   /**
    * A mapping from each provider to an object { sortedLinks, siteMap, linkMap }.
    * sortedLinks is the cached, sorted array of links for the provider.
    * siteMap is a mapping from base domains to URL count associated with the domain.
+   *         The count does not include blocked URLs. siteMap is used to look up a
+   *         user's top sites that can be targeted with a suggested tile.
    * linkMap is a Map from link URLs to link objects.
    */
   _providers: new Map(),
 
   /**
    * The properties of link objects used to sort them.
    */
   _sortProperties: [
@@ -733,16 +761,28 @@ let Links = {
   ],
 
   /**
    * List of callbacks waiting for the cache to be populated.
    */
   _populateCallbacks: [],
 
   /**
+   * A list of objects that are observing links updates.
+   */
+  _observers: [],
+
+  /**
+   * Registers an object that will be notified when links updates.
+   */
+  addObserver: function (aObserver) {
+    this._observers.push(aObserver);
+  },
+
+  /**
    * Adds a link provider.
    * @param aProvider The link provider.
    */
   addProvider: function Links_addProvider(aProvider) {
     this._providers.set(aProvider, null);
     aProvider.addObserver(this);
   },
 
@@ -856,30 +896,69 @@ let Links = {
         throw new Error("Comparable link missing required property: " + prop);
     }
     return aLink2.frecency - aLink1.frecency ||
            aLink2.lastVisitDate - aLink1.lastVisitDate ||
            aLink1.url.localeCompare(aLink2.url);
   },
 
   _incrementSiteMap: function(map, link) {
+    if (NewTabUtils.blockedLinks.isBlocked(link)) {
+      // Don't count blocked URLs.
+      return;
+    }
     let site = NewTabUtils.extractSite(link.url);
     map.set(site, (map.get(site) || 0) + 1);
   },
 
   _decrementSiteMap: function(map, link) {
+    if (NewTabUtils.blockedLinks.isBlocked(link)) {
+      // Blocked URLs are not included in map.
+      return;
+    }
     let site = NewTabUtils.extractSite(link.url);
     let previousURLCount = map.get(site);
     if (previousURLCount === 1) {
       map.delete(site);
     } else {
       map.set(site, previousURLCount - 1);
     }
   },
 
+  /**
+    * Update the siteMap cache based on the link given and whether we need
+    * to increment or decrement it. We do this by iterating over all stored providers
+    * to find which provider this link already exists in. For providers that
+    * have this link, we will adjust siteMap for them accordingly.
+    *
+    * @param aLink The link that will affect siteMap
+    * @param increment A boolean for whether to increment or decrement siteMap
+    */
+  _adjustSiteMapAndNotify: function(aLink, increment=true) {
+    for (let [provider, cache] of this._providers) {
+      // We only update siteMap if aLink is already stored in linkMap.
+      if (cache.linkMap.get(aLink.url)) {
+        if (increment) {
+          this._incrementSiteMap(cache.siteMap, aLink);
+          continue;
+        }
+        this._decrementSiteMap(cache.siteMap, aLink);
+      }
+    }
+    this._callObservers("onLinkChanged", aLink);
+  },
+
+  onLinkBlocked: function(aLink) {
+    this._adjustSiteMapAndNotify(aLink, false);
+  },
+
+  onLinkUnblocked: function(aLink) {
+    this._adjustSiteMapAndNotify(aLink);
+  },
+
   populateProviderCache: function(provider, callback) {
     if (!this._providers.has(provider)) {
       throw new Error("Can only populate provider cache for existing provider.");
     }
 
     return this._populateProviderCache(provider, callback, false);
   },
 
@@ -1090,16 +1169,28 @@ let Links = {
     // Make sure to update open about:newtab instances. If there are no opened
     // pages we can just wait for the next new tab to populate the cache again.
     if (AllPages.length && AllPages.enabled)
       this.populateCache(function () { AllPages.update() }, true);
     else
       this.resetCache();
   },
 
+  _callObservers(methodName, ...args) {
+    for (let obs of this._observers) {
+      if (typeof(obs[methodName]) == "function") {
+        try {
+          obs[methodName](this, ...args);
+        } catch (err) {
+          Cu.reportError(err);
+        }
+      }
+    }
+  },
+
   /**
    * Adds a sanitization observer and turns itself into a no-op after the first
    * invokation.
    */
   _addObserver: function Links_addObserver() {
     Services.obs.addObserver(this, "browser:purge-session-history", true);
     this._addObserver = function () {};
   },
@@ -1230,16 +1321,17 @@ this.NewTabUtils = {
     // Strip off common subdomains of the same site (e.g., www, load balancer)
     return uri.asciiHost.replace(/^(m|mobile|www\d*)\./, "");
   },
 
   init: function NewTabUtils_init() {
     if (this.initWithoutProviders()) {
       PlacesProvider.init();
       Links.addProvider(PlacesProvider);
+      BlockedLinks.addObserver(Links);
     }
   },
 
   initWithoutProviders: function NewTabUtils_initWithoutProviders() {
     if (!this._initialized) {
       this._initialized = true;
       ExpirationFilter.init();
       Telemetry.init();