Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items draft
authorUrsula Sarracini <usarracini@mozilla.com>
Wed, 21 Feb 2018 11:42:53 -0500
changeset 758005 e999c9cb4086d0eaa64c8c53cfe6e61c6401dd28
parent 757244 2c000486eac466da6623e4d7f7f1fd4e318f60e8
push id99908
push userusarracini@mozilla.com
push dateWed, 21 Feb 2018 18:36:28 +0000
bugs1439684
milestone60.0a1
Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items MozReview-Commit-ID: LLHD9TKnKA8
toolkit/modules/NewTabUtils.jsm
toolkit/modules/tests/xpcshell/test_NewTabUtils.js
--- 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;