diff --git a/toolkit/modules/NewTabUtils.jsm b/toolkit/modules/NewTabUtils.jsm index 65bcaf2b2445..0286affe86a5 100644 --- a/toolkit/modules/NewTabUtils.jsm +++ b/toolkit/modules/NewTabUtils.jsm @@ -22,6 +22,9 @@ ChromeUtils.defineModuleGetter(this, "BinarySearch", 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); }); @@ -61,6 +64,7 @@ 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 @@ -1037,13 +1041,6 @@ var ActivityStreamProvider = { 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"); }, @@ -1305,7 +1302,10 @@ var ActivityStreamProvider = { * 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 @@ -1368,7 +1368,7 @@ var ActivityStreamLinks = { /** * 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 @@ -1376,12 +1376,13 @@ var ActivityStreamLinks = { *@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 @@ -1389,9 +1390,43 @@ var ActivityStreamLinks = { *@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 * @@ -1415,7 +1450,14 @@ var ActivityStreamLinks = { // 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 diff --git a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js index fc316d2a502d..1b59e8603782 100644 --- a/toolkit/modules/tests/xpcshell/test_NewTabUtils.js +++ b/toolkit/modules/tests/xpcshell/test_NewTabUtils.js @@ -532,14 +532,6 @@ add_task(async function getHighlightsWithPocketSuccess() { "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" } } }; @@ -549,11 +541,14 @@ add_task(async function getHighlightsWithPocketSuccess() { 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"); @@ -567,13 +562,60 @@ add_task(async function getHighlightsWithPocketSuccess() { 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"); - // 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"); + 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; + + 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() { @@ -583,6 +625,9 @@ add_task(async function getHighlightsWithPocketFailure() { 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"); }); @@ -593,6 +638,9 @@ add_task(async function getHighlightsWithPocketNoData() { 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"); });