diff --git a/browser/modules/DirectoryLinksProvider.jsm b/browser/modules/DirectoryLinksProvider.jsm index c61688a28518..1c1595e2f397 100644 --- a/browser/modules/DirectoryLinksProvider.jsm +++ b/browser/modules/DirectoryLinksProvider.jsm @@ -15,6 +15,7 @@ const XMLHttpRequest = Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/Task.jsm"); +Cu.import("resource://gre/modules/Timer.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm"); @@ -89,6 +90,11 @@ let DirectoryLinksProvider = { */ _relatedLinks: new Map(), + /** + * A set of top sites that we can provide related links for + */ + _topSitesWithRelatedLinks: new Set(), + get _observedPrefs() Object.freeze({ enhanced: PREF_NEWTAB_ENHANCED, linksURL: PREF_DIRECTORY_SOURCE, @@ -432,7 +438,10 @@ let DirectoryLinksProvider = { }).catch(ex => { Cu.reportError(ex); return []; - }).then(aCallback); + }).then(links => { + aCallback(links); + this._populatePlacesLinks(); + }); }, init: function DirectoryLinksProvider_init() { @@ -441,6 +450,9 @@ let DirectoryLinksProvider = { // 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); + return Task.spawn(function() { // get the last modified time of the links file if it exists let doesFileExists = yield OS.File.exists(this._directoryFilePath); @@ -453,6 +465,47 @@ let DirectoryLinksProvider = { }.bind(this)); }, + _handleManyLinksChanged: function() { + this._topSitesWithRelatedLinks.clear(); + this._relatedLinks.forEach((relatedLinks, site) => { + if (NewTabUtils.isTopPlacesSite(site)) { + this._topSitesWithRelatedLinks.add(site); + } + }); + }, + + _handleLinkChanged: function(aLink) { + let changedLinkSite = NewTabUtils.extractSite(aLink.url); + if (!NewTabUtils.isTopPlacesSite(changedLinkSite)) { + this._topSitesWithRelatedLinks.delete(changedLinkSite); + return; + } + + if (this._relatedLinks.has(changedLinkSite)) { + this._topSitesWithRelatedLinks.add(changedLinkSite); + } + }, + + _populatePlacesLinks: function () { + NewTabUtils.links.populateProviderCache(NewTabUtils.placesProvider, () => { + this._handleManyLinksChanged(); + }); + }, + + onLinkChanged: function (aProvider, aLink) { + // Make sure NewTabUtils.links handles the notification first. + setTimeout(() => { + this._handleLinkChanged(aLink); + }, 0); + }, + + onManyLinksChanged: function () { + // Make sure NewTabUtils.links handles the notification first. + setTimeout(() => { + this._handleManyLinksChanged(); + }, 0); + }, + /** * Return the object to its pre-init state */ diff --git a/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js b/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js index 0ba8c96540ca..5255ee618470 100644 --- a/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js +++ b/browser/modules/test/xpcshell/test_DirectoryLinksProvider.js @@ -19,6 +19,8 @@ Cu.import("resource://gre/modules/XPCOMUtils.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "NetUtil", "resource://gre/modules/NetUtil.jsm"); +XPCOMUtils.defineLazyModuleGetter(this, "NewTabUtils", + "resource://gre/modules/NewTabUtils.jsm"); do_get_profile(); @@ -58,6 +60,42 @@ const BinaryInputStream = CC("@mozilla.org/binaryinputstream;1", "setInputStream"); let gLastRequestPath; + +let relatedTile1 = { + url: "http://turbotax.com", + type: "related", + lastVisitDate: 4, + related: [ + "taxact.com", + "hrblock.com", + "1040.com", + "taxslayer.com" + ] +}; +let relatedTile2 = { + url: "http://irs.gov", + type: "related", + lastVisitDate: 3, + related: [ + "taxact.com", + "hrblock.com", + "freetaxusa.com", + "taxslayer.com" + ] +}; +let relatedTile3 = { + url: "http://hrblock.com", + type: "related", + lastVisitDate: 2, + related: [ + "taxact.com", + "freetaxusa.com", + "1040.com", + "taxslayer.com" + ] +}; +let someOtherSite = {url: "http://someothersite.com", title: "Not_A_Related_Site"}; + function getHttpHandler(path) { let code = 200; let body = JSON.stringify(kHttpHandlerData[path]); @@ -161,6 +199,7 @@ function run_test() { server.registerPrefixHandler(kExamplePath, getHttpHandler(kExamplePath)); server.registerPrefixHandler(kFailPath, getHttpHandler(kFailPath)); server.start(kDefaultServerPort); + NewTabUtils.init(); run_next_test(); @@ -176,40 +215,6 @@ function run_test() { } add_task(function test_relatedLinksMap() { - let relatedTile1 = { - url: "http://turbotax.com", - type: "related", - lastVisitDate: 4, - related: [ - "taxact.com", - "hrblock.com", - "1040.com", - "taxslayer.com" - ] - }; - let relatedTile2 = { - url: "http://irs.gov", - type: "related", - lastVisitDate: 3, - related: [ - "taxact.com", - "hrblock.com", - "freetaxusa.com", - "taxslayer.com" - ] - }; - let relatedTile3 = { - url: "http://hrblock.com", - type: "related", - lastVisitDate: 2, - related: [ - "taxact.com", - "freetaxusa.com", - "1040.com", - "taxslayer.com" - ] - }; - let someOtherSite = {url: "http://someothersite.com", title: "Not_A_Related_Site"}; let data = {"en-US": [relatedTile1, relatedTile2, relatedTile3, someOtherSite]}; let dataURI = 'data:application/json,' + JSON.stringify(data); @@ -240,6 +245,51 @@ add_task(function test_relatedLinksMap() { yield promiseCleanDirectoryLinksProvider(); }); +add_task(function test_topSitesWithRelatedLinks() { + let topSites = ["site0.com", "1040.com", "site2.com", "hrblock.com", "site4.com", "freetaxusa.com", "site6.com"]; + let origIsTopPlacesSite = NewTabUtils.isTopPlacesSite; + NewTabUtils.isTopPlacesSite = function(site) { + return topSites.indexOf(site) >= 0; + } + + // We start off with no top sites with related links. + do_check_eq(DirectoryLinksProvider._topSitesWithRelatedLinks.size, 0); + + let data = {"en-US": [relatedTile1, relatedTile2, relatedTile3, someOtherSite]}; + let dataURI = 'data:application/json,' + JSON.stringify(data); + + yield promiseSetupDirectoryLinksProvider({linksURL: dataURI}); + let links = yield fetchData(); + + // Check we've populated related links as expected. + do_check_eq(DirectoryLinksProvider._relatedLinks.size, 5); + + // When many sites change, we update _topSitesWithRelatedLinks as expected. + let expectedTopSitesWithRelatedLinks = ["hrblock.com", "1040.com", "freetaxusa.com"]; + DirectoryLinksProvider._handleManyLinksChanged(); + isIdentical([...DirectoryLinksProvider._topSitesWithRelatedLinks], expectedTopSitesWithRelatedLinks); + + // Removing site6.com as a topsite has no impact on _topSitesWithRelatedLinks. + let popped = topSites.pop(); + DirectoryLinksProvider._handleLinkChanged({url: "http://" + popped}); + isIdentical([...DirectoryLinksProvider._topSitesWithRelatedLinks], expectedTopSitesWithRelatedLinks); + + // Removing freetaxusa.com as a topsite will remove it from _topSitesWithRelatedLinks. + popped = topSites.pop(); + expectedTopSitesWithRelatedLinks.pop(); + DirectoryLinksProvider._handleLinkChanged({url: "http://" + popped}); + isIdentical([...DirectoryLinksProvider._topSitesWithRelatedLinks], expectedTopSitesWithRelatedLinks); + + // Re-adding freetaxusa.com as a topsite will add it to _topSitesWithRelatedLinks. + topSites.push(popped); + expectedTopSitesWithRelatedLinks.push(popped); + DirectoryLinksProvider._handleLinkChanged({url: "http://" + popped}); + isIdentical([...DirectoryLinksProvider._topSitesWithRelatedLinks], expectedTopSitesWithRelatedLinks); + + // Cleanup. + NewTabUtils.isTopPlacesSite = origIsTopPlacesSite; +}); + add_task(function test_reportSitesAction() { yield DirectoryLinksProvider.init(); let deferred, expectedPath, expectedPost; diff --git a/toolkit/modules/NewTabUtils.jsm b/toolkit/modules/NewTabUtils.jsm index 01284234af45..fc5c11c11c72 100644 --- a/toolkit/modules/NewTabUtils.jsm +++ b/toolkit/modules/NewTabUtils.jsm @@ -12,6 +12,7 @@ const Cu = Components.utils; Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); +Cu.import("resource://gre/modules/Task.jsm"); XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils", "resource://gre/modules/PlacesUtils.jsm"); @@ -714,18 +715,13 @@ let Links = { */ maxNumLinks: LINKS_GET_LINKS_LIMIT, - /** - * The link providers. - */ - _providers: new Set(), - /** * 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. * linkMap is a Map from link URLs to link objects. */ - _providerLinks: new Map(), + _providers: new Map(), /** * The properties of link objects used to sort them. @@ -746,7 +742,7 @@ let Links = { * @param aProvider The link provider. */ addProvider: function Links_addProvider(aProvider) { - this._providers.add(aProvider); + this._providers.set(aProvider, null); aProvider.addObserver(this); }, @@ -757,7 +753,6 @@ let Links = { removeProvider: function Links_removeProvider(aProvider) { if (!this._providers.delete(aProvider)) throw new Error("Unknown provider"); - this._providerLinks.delete(aProvider); }, /** @@ -790,7 +785,7 @@ let Links = { } let numProvidersRemaining = this._providers.size; - for (let provider of this._providers) { + for (let [provider, links] of this._providers) { this._populateProviderCache(provider, () => { if (--numProvidersRemaining == 0) executeCallbacks(); @@ -840,7 +835,9 @@ let Links = { * Resets the links cache. */ resetCache: function Links_resetCache() { - this._providerLinks.clear(); + for (let provider of this._providers.keys()) { + this._providers.set(provider, null); + } }, /** @@ -878,6 +875,14 @@ let Links = { } }, + 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); + }, + /** * Calls getLinks on the given provider and populates our cache for it. * @param aProvider The provider whose cache will be populated. @@ -885,28 +890,42 @@ let Links = { * @param aForce When true, populates the provider's cache even when it's * already filled. */ - _populateProviderCache: function Links_populateProviderCache(aProvider, aCallback, aForce) { - if (this._providerLinks.has(aProvider) && !aForce) { - aCallback(); - } else { - aProvider.getLinks(links => { - // Filter out null and undefined links so we don't have to deal with - // them in getLinks when merging links from providers. - links = links.filter((link) => !!link); - this._providerLinks.set(aProvider, { - sortedLinks: links, - siteMap: links.reduce((map, link) => { + _populateProviderCache: function (aProvider, aCallback, aForce) { + let cache = this._providers.get(aProvider); + let createCache = !cache; + if (createCache) { + cache = { + // Start with a resolved promise. + populatePromise: new Promise(resolve => resolve()), + }; + this._providers.set(aProvider, cache); + } + // Chain the populatePromise so that calls are effectively queued. + cache.populatePromise = cache.populatePromise.then(() => { + return new Promise(resolve => { + if (!createCache && !aForce) { + aCallback(); + resolve(); + return; + } + aProvider.getLinks(links => { + // Filter out null and undefined links so we don't have to deal with + // them in getLinks when merging links from providers. + links = links.filter((link) => !!link); + cache.sortedLinks = links; + cache.siteMap = links.reduce((map, link) => { this._incrementSiteMap(map, link); return map; - }, new Map()), - linkMap: links.reduce((map, link) => { + }, new Map()); + cache.linkMap = links.reduce((map, link) => { map.set(link.url, link); return map; - }, new Map()), + }, new Map()); + aCallback(); + resolve(); }); - aCallback(); }); - } + }); }, /** @@ -916,8 +935,10 @@ let Links = { _getMergedProviderLinks: function Links__getMergedProviderLinks() { // Build a list containing a copy of each provider's sortedLinks list. let linkLists = []; - for (let links of this._providerLinks.values()) { - linkLists.push(links.sortedLinks.slice()); + for (let links of this._providers.values()) { + if (links) { + linkLists.push(links.sortedLinks.slice()); + } } function getNextLink() { @@ -951,7 +972,7 @@ let Links = { if (!("url" in aLink)) throw new Error("Changed links must have a url property"); - let links = this._providerLinks.get(aProvider); + let links = this._providers.get(aProvider); if (!links) // This is not an error, it just means that between the time the provider // was added and the future time we call getLinks on it, it notified us of @@ -1210,7 +1231,7 @@ this.NewTabUtils = { }, isTopSiteGivenProvider: function(aSite, aProvider) { - return Links._providerLinks.get(aProvider).siteMap.has(aSite); + return Links._providers.get(aProvider).siteMap.has(aSite); }, isTopPlacesSite: function(aSite) { @@ -1248,5 +1269,6 @@ this.NewTabUtils = { linkChecker: LinkChecker, pinnedLinks: PinnedLinks, blockedLinks: BlockedLinks, - gridPrefs: GridPrefs + gridPrefs: GridPrefs, + placesProvider: PlacesProvider }; diff --git a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js index e9bed568acff..cf26b1993f67 100644 --- a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js +++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js @@ -6,11 +6,36 @@ const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components; Cu.import("resource://gre/modules/NewTabUtils.jsm"); Cu.import("resource://gre/modules/Promise.jsm"); +Cu.import("resource://gre/modules/Task.jsm"); function run_test() { run_next_test(); } +add_task(function populatePromise() { + let count = 0; + let expectedLinks = makeLinks(0, 10, 2); + + let getLinksFcn = Task.async(function* (callback) { + //Should not be calling getLinksFcn twice + count++; + do_check_eq(count, 1); + yield Promise.resolve(); + callback(expectedLinks); + }); + + let provider = new TestProvider(getLinksFcn); + + NewTabUtils.initWithoutProviders(); + NewTabUtils.links.addProvider(provider); + + NewTabUtils.links.populateProviderCache(provider, () => {}); + NewTabUtils.links.populateProviderCache(provider, () => { + do_check_links(NewTabUtils.links.getLinks(), expectedLinks); + NewTabUtils.links.removeProvider(provider); + }); +}); + add_task(function isTopSiteGivenProvider() { let expectedLinks = makeLinks(0, 10, 2); @@ -22,7 +47,7 @@ add_task(function isTopSiteGivenProvider() { NewTabUtils.initWithoutProviders(); NewTabUtils.links.addProvider(provider); - NewTabUtils.links.populateCache(function () {}, false); + yield new Promise(resolve => NewTabUtils.links.populateCache(resolve)); do_check_eq(NewTabUtils.isTopSiteGivenProvider("example2.com", provider), true); do_check_eq(NewTabUtils.isTopSiteGivenProvider("example1.com", provider), false); @@ -62,8 +87,7 @@ add_task(function multipleProviders() { NewTabUtils.links.addProvider(evenProvider); NewTabUtils.links.addProvider(oddProvider); - // This is sync since the providers' getLinks are sync. - NewTabUtils.links.populateCache(function () {}, false); + yield new Promise(resolve => NewTabUtils.links.populateCache(resolve)); let links = NewTabUtils.links.getLinks(); let expectedLinks = makeLinks(NewTabUtils.links.maxNumLinks, @@ -83,8 +107,7 @@ add_task(function changeLinks() { NewTabUtils.initWithoutProviders(); NewTabUtils.links.addProvider(provider); - // This is sync since the provider's getLinks is sync. - NewTabUtils.links.populateCache(function () {}, false); + yield new Promise(resolve => NewTabUtils.links.populateCache(resolve)); do_check_links(NewTabUtils.links.getLinks(), expectedLinks); @@ -124,8 +147,12 @@ add_task(function changeLinks() { // Notify of many links changed. expectedLinks = makeLinks(0, 3, 1); provider.notifyManyLinksChanged(); - // NewTabUtils.links will now repopulate its cache, which is sync since - // the provider's getLinks is sync. + + // Since _populateProviderCache() is async, we must wait until the provider's + // populate promise has been resolved. + yield NewTabUtils.links._providers.get(provider).populatePromise; + + // NewTabUtils.links will now repopulate its cache do_check_links(NewTabUtils.links.getLinks(), expectedLinks); NewTabUtils.links.removeProvider(provider); @@ -138,15 +165,14 @@ add_task(function oneProviderAlreadyCached() { NewTabUtils.initWithoutProviders(); NewTabUtils.links.addProvider(provider1); - // This is sync since the provider's getLinks is sync. - NewTabUtils.links.populateCache(function () {}, false); + yield new Promise(resolve => NewTabUtils.links.populateCache(resolve)); do_check_links(NewTabUtils.links.getLinks(), links1); let links2 = makeLinks(10, 20, 1); let provider2 = new TestProvider(done => done(links2)); NewTabUtils.links.addProvider(provider2); - NewTabUtils.links.populateCache(function () {}, false); + yield new Promise(resolve => NewTabUtils.links.populateCache(resolve)); do_check_links(NewTabUtils.links.getLinks(), links2.concat(links1)); NewTabUtils.links.removeProvider(provider1); @@ -162,8 +188,7 @@ add_task(function newLowRankedLink() { NewTabUtils.initWithoutProviders(); NewTabUtils.links.addProvider(provider); - // This is sync since the provider's getLinks is sync. - NewTabUtils.links.populateCache(function () {}, false); + yield new Promise(resolve => NewTabUtils.links.populateCache(resolve)); do_check_links(NewTabUtils.links.getLinks(), links); // Notify of a new link that's low-ranked enough not to make the list.