Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items
MozReview-Commit-ID: LLHD9TKnKA8
--- a/toolkit/modules/NewTabUtils.jsm
+++ b/toolkit/modules/NewTabUtils.jsm
@@ -17,16 +17,19 @@ ChromeUtils.defineModuleGetter(this, "Pa
"resource://gre/modules/PageThumbs.jsm");
ChromeUtils.defineModuleGetter(this, "BinarySearch",
"resource://gre/modules/BinarySearch.jsm");
ChromeUtils.defineModuleGetter(this, "pktApi",
"chrome://pocket/content/pktApi.jsm");
+ChromeUtils.defineModuleGetter(this, "Pocket",
+ "chrome://pocket/content/Pocket.jsm");
+
XPCOMUtils.defineLazyGetter(this, "gCryptoHash", function() {
return Cc["@mozilla.org/security/hash;1"].createInstance(Ci.nsICryptoHash);
});
XPCOMUtils.defineLazyGetter(this, "gUnicodeConverter", function() {
let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
.createInstance(Ci.nsIScriptableUnicodeConverter);
converter.charset = "utf8";
@@ -56,16 +59,17 @@ const TOPIC_GATHER_TELEMETRY = "gather-t
const ACTIVITY_STREAM_DEFAULT_FRECENCY = 150;
// Some default query limit for Activity Stream requests
const ACTIVITY_STREAM_DEFAULT_LIMIT = 12;
// Some default seconds ago for Activity Stream recent requests
const ACTIVITY_STREAM_DEFAULT_RECENT = 5 * 24 * 60 * 60;
+const POCKET_UPDATE_TIME = 60 * 60 * 1000; // 1 hour
/**
* Calculate the MD5 hash for a string.
* @param aValue
* The string to convert.
* @return The base64 representation of the MD5 hash.
*/
function toHash(aValue) {
let value = gUnicodeConverter.convertToByteArray(aValue);
@@ -1032,23 +1036,16 @@ var ActivityStreamProvider = {
.map(item => ({
description: item.excerpt,
preview_image_url: item.image && item.image.src,
title: item.resolved_title,
url: item.resolved_url,
pocket_id: item.item_id
}));
- // Add bookmark guid for Pocket items that are also bookmarks
- for (let item of items) {
- const bookmarkData = await this.getBookmark({url: item.url});
- if (bookmarkData) {
- item.bookmarkGuid = bookmarkData.bookmarkGuid;
- }
- }
return this._processHighlights(items, aOptions, "pocket");
},
/**
* Get most-recently-created visited bookmarks for Activity Stream.
*
* @param {Object} aOptions
* {num} bookmarkSecondsAgo: Maximum age of added bookmark.
@@ -1300,17 +1297,20 @@ var ActivityStreamProvider = {
return items;
}
};
/**
* A set of actions which influence what sites shown on the Activity Stream page
*/
var ActivityStreamLinks = {
- /**
+ _savedPocketStories: null,
+ _pocketLastUpdated: 0,
+
+ /**
* Block a url
*
* @param {Object} aLink
* The link which contains a URL to add to the block list
*/
blockURL(aLink) {
BlockedLinks.block(aLink);
},
@@ -1363,41 +1363,76 @@ var ActivityStreamLinks = {
deleteHistoryEntry(aUrl) {
const url = aUrl;
PinnedLinks.unpin({url});
return PlacesUtils.history.remove(url);
},
/**
* Helper function which makes the call to the Pocket API to delete an item from
- * a user's saved to Pocket feed.
+ * a user's saved to Pocket feed. Also, invalidate the Pocket stories cache
*
* @param {Integer} aItemID
* The unique pocket ID used to find the item to be deleted
*
*@returns {Promise} Returns a promise at completion
*/
deletePocketEntry(aItemID) {
+ this._savedPocketStories = null;
return new Promise((success, error) => pktApi.deleteItem(aItemID, {success, error}));
},
/**
* Helper function which makes the call to the Pocket API to archive an item from
- * a user's saved to Pocket feed.
+ * a user's saved to Pocket feed. Also, invalidate the Pocket stories cache
*
* @param {Integer} aItemID
* The unique pocket ID used to find the item to be archived
*
*@returns {Promise} Returns a promise at completion
*/
archivePocketEntry(aItemID) {
+ this._savedPocketStories = null;
return new Promise((success, error) => pktApi.archiveItem(aItemID, {success, error}));
},
/**
+ * Helper function which makes the call to the Pocket API to save an item to
+ * a user's saved to Pocket feed if they are logged in. Also, invalidate the
+ * Pocket stories cache
+ *
+ * @param {String} aUrl
+ * The URL belonging to the story being saved
+ * @param {String} aTitle
+ * The title belonging to the story being saved
+ * @param {Browser} aBrowser
+ * The target browser to show the doorhanger in
+ *
+ *@returns {Promise} Returns a promise at completion
+ */
+ addPocketEntry(aUrl, aTitle, aBrowser) {
+ // If the user is not logged in, show the panel to prompt them to log in
+ if (!pktApi.isUserLoggedIn()) {
+ Pocket.savePage(aBrowser, aUrl, aTitle);
+ return Promise.resolve(null);
+ }
+
+ // If the user is logged in, just save the link to Pocket and Activity Stream
+ // will update the page
+ this._savedPocketStories = null;
+ return new Promise((success, error) => {
+ pktApi.addLink(aUrl, {
+ title: aTitle,
+ success,
+ error
+ });
+ });
+ },
+
+ /**
* Get the Highlights links to show on Activity Stream
*
* @param {Object} aOptions
* {bool} excludeBookmarks: Don't add bookmark items.
* {bool} excludeHistory: Don't add history items.
* {bool} excludePocket: Don't add Pocket items.
* {bool} withFavicons: Add favicon data: URIs, when possible.
* {int} numItems: Maximum number of (bookmark or history) items to return.
@@ -1410,17 +1445,24 @@ var ActivityStreamLinks = {
// First get bookmarks if we want them
if (!aOptions.excludeBookmarks) {
results.push(...await ActivityStreamProvider.getRecentBookmarks(aOptions));
}
// Add the Pocket items if we need more and want them
if (aOptions.numItems - results.length > 0 && !aOptions.excludePocket) {
- results.push(...await ActivityStreamProvider.getRecentlyPocketed(aOptions));
+ // If we do not have saved to Pocket stories already cached, or it has been
+ // more than 1 hour since we last got Pocket stories, invalidate the cache,
+ // get new stories, and update the timestamp
+ if (!this._savedPocketStories || (Date.now() - this._pocketLastUpdated > POCKET_UPDATE_TIME)) {
+ this._savedPocketStories = await ActivityStreamProvider.getRecentlyPocketed(aOptions);
+ this._pocketLastUpdated = Date.now();
+ }
+ results.push(...this._savedPocketStories);
}
// Add in history if we need more and want them
if (aOptions.numItems - results.length > 0 && !aOptions.excludeHistory) {
// Use the same numItems as bookmarks above in case we remove duplicates
const history = await ActivityStreamProvider.getRecentHistory(aOptions);
// Only include a url once in the result preferring the bookmark
--- a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
+++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js
@@ -527,77 +527,125 @@ add_task(async function getHighlightsWit
resolved_title: "A title for foo",
resolved_url: "http://www.foo.com",
item_id: "123",
status: "0"
},
"456": {
item_id: "456",
status: "2",
- },
- "789": {
- resolved_url: bookmark.url,
- excerpt: bookmark.description,
- resolved_title: bookmark.title,
- image: {src: bookmark.preview_image_url},
- status: "0",
- item_id: "789"
}
}
};
await PlacesUtils.bookmarks.insert(bookmark);
await PlacesTestUtils.addVisits(bookmark.url);
NewTabUtils.activityStreamProvider.fetchSavedPocketItems = () => fakeResponse;
let provider = NewTabUtils.activityStreamLinks;
+
+ // Force a cache invalidation
+ NewTabUtils.activityStreamLinks._pocketLastUpdated = Date.now() - (70 * 60 * 1000);
let links = await provider.getHighlights();
- // We should have 1 bookmark followed by 2 pocket stories in highlights
+ // We should have 1 bookmark followed by 1 pocket story in highlights
// We should not have stored the second pocket item since it was deleted
- Assert.equal(links.length, 3, "Should have 3 links in highlights");
+ Assert.equal(links.length, 2, "Should have 2 links in highlights");
// First highlight should be a bookmark
Assert.equal(links[0].url, bookmark.url, "The first link is the bookmark");
// Second highlight should be a Pocket item with the correct fields to display
let pocketItem = fakeResponse.list["123"];
let currentLink = links[1];
Assert.equal(currentLink.url, pocketItem.resolved_url, "Correct Pocket item");
Assert.equal(currentLink.type, "pocket", "Attached the correct type");
Assert.equal(currentLink.preview_image_url, pocketItem.image.src, "Correct preview image was added");
Assert.equal(currentLink.title, pocketItem.resolved_title, "Correct title was added");
Assert.equal(currentLink.description, pocketItem.excerpt, "Correct description was added");
Assert.equal(currentLink.pocket_id, pocketItem.item_id, "item_id was preserved");
- Assert.equal(currentLink.bookmarkGuid, undefined, "Should not have a bookmarkGuid");
+
+ NewTabUtils.activityStreamLinks._savedPocketStories = null;
+});
+
+add_task(async function getHighlightsWithPocketCached() {
+ await setUpActivityStreamTest();
+
+ let fakeResponse = {
+ list: {
+ "123": {
+ image: {src: "foo.com/img.png"},
+ excerpt: "A description for foo",
+ resolved_title: "A title for foo",
+ resolved_url: "http://www.foo.com",
+ item_id: "123",
+ status: "0"
+ },
+ "456": {
+ item_id: "456",
+ status: "2",
+ }
+ }
+ };
+
+ NewTabUtils.activityStreamProvider.fetchSavedPocketItems = () => fakeResponse;
+ let provider = NewTabUtils.activityStreamLinks;
- // Third highlight should still be a Pocket item but since it was bookmarked, it has a bookmarkGuid
- pocketItem = fakeResponse.list["789"];
- currentLink = links[2];
- Assert.equal(currentLink.url, pocketItem.resolved_url, "Correct Pocket item");
- Assert.ok(currentLink.bookmarkGuid, "Attached a bookmarkGuid for this Pocket item");
+ let links = await provider.getHighlights();
+ Assert.equal(links.length, 1, "Sanity check that we got 1 link back for highlights");
+ Assert.equal(links[0].url, fakeResponse.list["123"].resolved_url, "Sanity check that it was the pocket story");
+
+ // Update what the response would be
+ fakeResponse.list["789"] = {
+ image: {src: "bar.com/img.png"},
+ excerpt: "A description for bar",
+ resolved_title: "A title for bar",
+ resolved_url: "http://www.bar.com",
+ item_id: "789",
+ status: "0"
+ };
+
+ // Call getHighlights again - this time we should get the cached links since we just updated
+ links = await provider.getHighlights();
+ Assert.equal(links.length, 1, "We still got 1 link back for highlights");
+ Assert.equal(links[0].url, fakeResponse.list["123"].resolved_url, "It was still the same pocket story");
+
+ // Now force a cache invalidation and call getHighlights again
+ NewTabUtils.activityStreamLinks._pocketLastUpdated = Date.now() - (70 * 60 * 1000);
+ links = await provider.getHighlights();
+ Assert.equal(links.length, 2, "This time we got fresh links with the new response");
+ Assert.equal(links[0].url, fakeResponse.list["123"].resolved_url, "First link is unchanged");
+ Assert.equal(links[1].url, fakeResponse.list["789"].resolved_url, "Second link is the new link");
+
+ NewTabUtils.activityStreamLinks._savedPocketStories = null;
});
add_task(async function getHighlightsWithPocketFailure() {
await setUpActivityStreamTest();
NewTabUtils.activityStreamProvider.fetchSavedPocketItems = function() {
throw new Error();
};
let provider = NewTabUtils.activityStreamLinks;
+
+ // Force a cache invalidation
+ NewTabUtils.activityStreamLinks._pocketLastUpdated = Date.now() - (70 * 60 * 1000);
let links = await provider.getHighlights();
Assert.equal(links.length, 0, "Return empty links if we reject the promise");
});
add_task(async function getHighlightsWithPocketNoData() {
await setUpActivityStreamTest();
NewTabUtils.activityStreamProvider.fetchSavedPocketItems = () => {};
let provider = NewTabUtils.activityStreamLinks;
+
+ // Force a cache invalidation
+ NewTabUtils.activityStreamLinks._pocketLastUpdated = Date.now() - (70 * 60 * 1000);
let links = await provider.getHighlights();
Assert.equal(links.length, 0, "Return empty links if we got no data back from the response");
});
add_task(async function getTopFrecentSites() {
await setUpActivityStreamTest();
let provider = NewTabUtils.activityStreamLinks;