Bug 1126184: Part 1: Make DirectoryLinksProvider listen to PlacesProvider and update its _topSitesWithRelatedLinks cache. r=adw

This commit is contained in:
Marina Samuel 2015-03-13 11:45:31 -04:00
parent 1881e2771b
commit ca512976a2
4 changed files with 228 additions and 78 deletions

View File

@ -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
*/

View File

@ -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;

View File

@ -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
};

View File

@ -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.