Bug 1525581 - Show Activity Stream cached data immediately on subsequent loads. r=k88hudson a=lizzard
authorAndrei Oprea <andrei.br92@gmail.com>
Fri, 15 Feb 2019 19:48:01 +0100
changeset 515977 14f5f0ae70f466bcdba4bae0878f93557fbcf17e
parent 515976 448d348e474bba880ad0f8bdce67943bc23604e9
child 515978 7899491917e338794985f1c29c88a213d47d374e
push id1953
push userffxbld-merge
push dateMon, 11 Mar 2019 12:10:20 +0000
treeherdermozilla-release@9c35dcbaa899 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersk88hudson, lizzard
bugs1525581
milestone66.0
Bug 1525581 - Show Activity Stream cached data immediately on subsequent loads. r=k88hudson a=lizzard
browser/components/newtab/lib/DiscoveryStreamFeed.jsm
browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
--- a/browser/components/newtab/lib/DiscoveryStreamFeed.jsm
+++ b/browser/components/newtab/lib/DiscoveryStreamFeed.jsm
@@ -7,16 +7,17 @@ ChromeUtils.import("resource://gre/modul
 XPCOMUtils.defineLazyGlobalGetters(this, ["fetch"]);
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 
 const {actionTypes: at, actionCreators: ac} = ChromeUtils.import("resource://activity-stream/common/Actions.jsm", {});
 const {PersistentCache} = ChromeUtils.import("resource://activity-stream/lib/PersistentCache.jsm", {});
 
 const CACHE_KEY = "discovery_stream";
 const LAYOUT_UPDATE_TIME = 30 * 60 * 1000; // 30 minutes
+const STARTUP_CACHE_EXPIRE_TIME = 7 * 24 * 60 * 60 * 1000; // 1 week
 const COMPONENT_FEEDS_UPDATE_TIME = 30 * 60 * 1000; // 30 minutes
 const SPOCS_FEEDS_UPDATE_TIME = 30 * 60 * 1000; // 30 minutes
 const CONFIG_PREF_NAME = "browser.newtabpage.activity-stream.discoverystream.config";
 const SPOC_IMPRESSION_TRACKING_PREF = "browser.newtabpage.activity-stream.discoverystream.spoc.impressions";
 const MAX_LIFETIME_CAP = 500; // Guard against misconfiguration on the server
 
 this.DiscoveryStreamFeed = class DiscoveryStreamFeed {
   constructor() {
@@ -86,49 +87,66 @@ this.DiscoveryStreamFeed = class Discove
       Cu.reportError(`Failed to fetch ${endpoint}: ${error.message}`);
     }
     // istanbul ignore next
     return null;
   }
 
   /**
    * Returns true if data in the cache for a particular key has expired or is missing.
-   * @param {{}} cacheData data returned from cache.get()
+   * @param {object} cachedData data returned from cache.get()
    * @param {string} key a cache key
    * @param {string?} url for "feed" only, the URL of the feed.
+   * @param {boolean} is this check done at initial browser load
    */
-  isExpired(cacheData, key, url) {
-    const {layout, spocs, feeds} = cacheData;
+  isExpired({cachedData, key, url, isStartup}) {
+    const {layout, spocs, feeds} = cachedData;
+    const updateTimePerComponent = {
+      "layout": LAYOUT_UPDATE_TIME,
+      "spocs": SPOCS_FEEDS_UPDATE_TIME,
+      "feed": COMPONENT_FEEDS_UPDATE_TIME,
+    };
+    const EXPIRATION_TIME = isStartup ? STARTUP_CACHE_EXPIRE_TIME : updateTimePerComponent[key];
     switch (key) {
       case "layout":
-        return (!layout || !(Date.now() - layout._timestamp < LAYOUT_UPDATE_TIME));
+        return (!layout || !(Date.now() - layout._timestamp < EXPIRATION_TIME));
       case "spocs":
-        return (!spocs || !(Date.now() - spocs.lastUpdated < SPOCS_FEEDS_UPDATE_TIME));
+        return (!spocs || !(Date.now() - spocs.lastUpdated < EXPIRATION_TIME));
       case "feed":
-        return (!feeds || !feeds[url] || !(Date.now() - feeds[url].lastUpdated < COMPONENT_FEEDS_UPDATE_TIME));
+        return (!feeds || !feeds[url] || !(Date.now() - feeds[url].lastUpdated < EXPIRATION_TIME));
       default:
+        // istanbul ignore next
         throw new Error(`${key} is not a valid key`);
     }
   }
 
+  async _checkExpirationPerComponent() {
+    const cachedData = await this.cache.get() || {};
+    const {feeds} = cachedData;
+    return {
+      layout: this.isExpired({cachedData, key: "layout"}),
+      spocs: this.isExpired({cachedData, key: "spocs"}),
+      feeds: !feeds || Object.keys(feeds).some(url => this.isExpired({cachedData, key: "feed", url})),
+    };
+  }
+
   /**
    * Returns true if any data for the cached endpoints has expired or is missing.
    */
   async checkIfAnyCacheExpired() {
-    const cachedData = await this.cache.get() || {};
-    const {feeds} = cachedData;
-    return this.isExpired(cachedData, "layout") ||
-      this.isExpired(cachedData, "spocs") ||
-      !feeds || Object.keys(feeds).some(url => this.isExpired(cachedData, "feed", url));
+    const expirationPerComponent = await this._checkExpirationPerComponent();
+    return expirationPerComponent.layout ||
+      expirationPerComponent.spocs ||
+      expirationPerComponent.feeds;
   }
 
-  async loadLayout(sendUpdate) {
+  async loadLayout(sendUpdate, isStartup) {
     const cachedData = await this.cache.get() || {};
     let {layout: layoutResponse} = cachedData;
-    if (this.isExpired(cachedData, "layout")) {
+    if (this.isExpired({cachedData, key: "layout", isStartup})) {
       layoutResponse = await this.fetchFromEndpoint(this.config.layout_endpoint);
       if (layoutResponse && layoutResponse.layout) {
         layoutResponse._timestamp = Date.now();
         await this.cache.set("layout", layoutResponse);
       } else {
         Cu.reportError("No response for response.layout prop");
       }
     }
@@ -145,44 +163,44 @@ this.DiscoveryStreamFeed = class Discove
     if (layoutResponse && layoutResponse.spocs && layoutResponse.spocs.url) {
       sendUpdate({
         type: at.DISCOVERY_STREAM_SPOCS_ENDPOINT,
         data: layoutResponse.spocs.url,
       });
     }
   }
 
-  async loadComponentFeeds(sendUpdate) {
+  async loadComponentFeeds(sendUpdate, isStartup) {
     const {DiscoveryStream} = this.store.getState();
     const newFeeds = {};
     if (DiscoveryStream && DiscoveryStream.layout) {
       for (let row of DiscoveryStream.layout) {
         if (!row || !row.components) {
           continue;
         }
         for (let component of row.components) {
           if (component && component.feed) {
             const {url} = component.feed;
-            newFeeds[url] = await this.getComponentFeed(url);
+            newFeeds[url] = await this.getComponentFeed(url, isStartup);
           }
         }
       }
 
       await this.cache.set("feeds", newFeeds);
       sendUpdate({type: at.DISCOVERY_STREAM_FEEDS_UPDATE, data: newFeeds});
     }
   }
 
-  async loadSpocs(sendUpdate) {
+  async loadSpocs(sendUpdate, isStartup) {
     const cachedData = await this.cache.get() || {};
     let spocs;
 
     if (this.showSpocs) {
       spocs = cachedData.spocs;
-      if (this.isExpired(cachedData, "spocs")) {
+      if (this.isExpired({cachedData, key: "spocs", isStartup})) {
         const endpoint = this.store.getState().DiscoveryStream.spocs.spocs_endpoint;
         const spocsResponse = await this.fetchFromEndpoint(endpoint);
         if (spocsResponse) {
           spocs = {
             lastUpdated: Date.now(),
             data: spocsResponse,
           };
 
@@ -258,58 +276,81 @@ this.DiscoveryStreamFeed = class Discove
     if (campaignCap) {
       const campaignCapExceeded = campaignImpressions
         .filter(i => (Date.now() - i) < (campaignCap.period * 1000)).length >= campaignCap.count;
       return !campaignCapExceeded;
     }
     return true;
   }
 
-  async getComponentFeed(feedUrl) {
+  async getComponentFeed(feedUrl, isStartup) {
     const cachedData = await this.cache.get() || {};
     const {feeds} = cachedData;
     let feed = feeds ? feeds[feedUrl] : null;
-    if (this.isExpired(cachedData, "feed", feedUrl)) {
+    if (this.isExpired({cachedData, key: "feed", url: feedUrl, isStartup})) {
       const feedResponse = await this.fetchFromEndpoint(feedUrl);
       if (feedResponse) {
         feed = {
           lastUpdated: Date.now(),
           data: feedResponse,
         };
       } else {
         Cu.reportError("No response for feed");
       }
     }
 
     return feed;
   }
 
   /**
+   * Called at startup to update cached data in the background.
+   */
+  async _maybeUpdateCachedData() {
+    const expirationPerComponent = await this._checkExpirationPerComponent();
+    // Pass in `store.dispatch` to send the updates only to main
+    if (expirationPerComponent.layout) {
+      await this.loadLayout(this.store.dispatch);
+    }
+    if (expirationPerComponent.spocs) {
+      await this.loadSpocs(this.store.dispatch);
+    }
+    if (expirationPerComponent.feeds) {
+      await this.loadComponentFeeds(this.store.dispatch);
+    }
+  }
+
+  /**
    * @typedef {Object} RefreshAllOptions
    * @property {boolean} updateOpenTabs - Sends updates to open tabs immediately if true,
    *                                      updates in background if false
-
+   * @property {boolean} isStartup - When the function is called at browser startup
+   *
    * Refreshes layout, component feeds, and spocs in order if caches have expired.
    * @param {RefreshAllOptions} options
    */
   async refreshAll(options = {}) {
-    const dispatch = options.updateOpenTabs ?
+    const {updateOpenTabs, isStartup} = options;
+    const dispatch = updateOpenTabs ?
       action => this.store.dispatch(ac.BroadcastToContent(action)) :
       this.store.dispatch;
 
-    await this.loadLayout(dispatch);
+    await this.loadLayout(dispatch, isStartup);
     await Promise.all([
-      this.loadComponentFeeds(dispatch).catch(error => Cu.reportError(`Error trying to load component feeds: ${error}`)),
-      this.loadSpocs(dispatch).catch(error => Cu.reportError(`Error trying to load spocs feed: ${error}`)),
+      this.loadComponentFeeds(dispatch, isStartup).catch(error => Cu.reportError(`Error trying to load component feeds: ${error}`)),
+      this.loadSpocs(dispatch, isStartup).catch(error => Cu.reportError(`Error trying to load spocs feed: ${error}`)),
     ]);
+    if (isStartup) {
+      await this._maybeUpdateCachedData();
+    }
+
     this.loaded = true;
   }
 
   async enable() {
-    await this.refreshAll({updateOpenTabs: true});
+    await this.refreshAll({updateOpenTabs: true, isStartup: true});
   }
 
   async disable() {
     await this.clearCache();
     // Reset reducer
     this.store.dispatch(ac.BroadcastToContent({type: at.DISCOVERY_STREAM_LAYOUT_RESET}));
     this.loaded = false;
   }
--- a/browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
+++ b/browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
@@ -1,16 +1,17 @@
 import {actionCreators as ac, actionTypes as at, actionUtils as au} from "common/Actions.jsm";
 import {combineReducers, createStore} from "redux";
 import {DiscoveryStreamFeed} from "lib/DiscoveryStreamFeed.jsm";
 import {reducers} from "common/Reducers.jsm";
 
 const CONFIG_PREF_NAME = "browser.newtabpage.activity-stream.discoverystream.config";
 const SPOC_IMPRESSION_TRACKING_PREF = "browser.newtabpage.activity-stream.discoverystream.spoc.impressions";
 const THIRTY_MINUTES = 30 * 60 * 1000;
+const ONE_WEEK = 7 * 24 * 60 * 60 * 1000; // 1 week
 
 describe("DiscoveryStreamFeed", () => {
   let feed;
   let sandbox;
   let configPrefStub;
   let fetchStub;
   let clock;
 
@@ -25,16 +26,18 @@ describe("DiscoveryStreamFeed", () => {
     fetchStub = sandbox.stub(global, "fetch");
 
     // Time
     clock = sinon.useFakeTimers();
 
     // Feed
     feed = new DiscoveryStreamFeed();
     feed.store = createStore(combineReducers(reducers));
+
+    sandbox.stub(feed, "_maybeUpdateCachedData").resolves();
   });
 
   afterEach(() => {
     clock.restore();
     sandbox.restore();
   });
 
   describe("#observe", () => {
@@ -647,16 +650,36 @@ describe("DiscoveryStreamFeed", () => {
   });
 
   describe("#isExpired", () => {
     it("should throw if the key is not valid", () => {
       assert.throws(() => {
         feed.isExpired({}, "foo");
       });
     });
+    it("should return false for layout on startup for content under 1 week", () => {
+      const layout = {_timestamp: Date.now()};
+      const result = feed.isExpired({cachedData: {layout}, key: "layout", isStartup: true});
+
+      assert.isFalse(result);
+    });
+    it("should return true for layout for isStartup=false", () => {
+      const layout = {_timestamp: Date.now()};
+      clock.tick(THIRTY_MINUTES + 1);
+      const result = feed.isExpired({cachedData: {layout}, key: "layout"});
+
+      assert.isTrue(result);
+    });
+    it("should return true for layout on startup for content over 1 week", () => {
+      const layout = {_timestamp: Date.now()};
+      clock.tick(ONE_WEEK + 1);
+      const result = feed.isExpired({cachedData: {layout}, key: "layout", isStartup: true});
+
+      assert.isTrue(result);
+    });
   });
 
   describe("#checkIfAnyCacheExpired", () => {
     let cache;
     beforeEach(() => {
       cache = {
         layout: {_timestamp: Date.now()},
         feeds: {"foo.com": {lastUpdated: Date.now()}},
@@ -714,16 +737,17 @@ describe("DiscoveryStreamFeed", () => {
   });
 
   describe("#refreshAll", () => {
     beforeEach(() => {
       sandbox.stub(feed, "loadLayout").resolves();
       sandbox.stub(feed, "loadComponentFeeds").resolves();
       sandbox.stub(feed, "loadSpocs").resolves();
       sandbox.spy(feed.store, "dispatch");
+      Object.defineProperty(feed, "showSpocs", {get: () => true});
     });
 
     it("should call layout, component, spocs update functions", async () => {
       await feed.refreshAll();
 
       assert.calledOnce(feed.loadLayout);
       assert.calledOnce(feed.loadComponentFeeds);
       assert.calledOnce(feed.loadSpocs);
@@ -758,10 +782,80 @@ describe("DiscoveryStreamFeed", () => {
       sandbox.stub(global.Promise, "all").resolves();
 
       await feed.refreshAll();
 
       assert.calledOnce(global.Promise.all);
       const {args} = global.Promise.all.firstCall;
       assert.equal(args[0].length, 2);
     });
+    describe("test startup cache behaviour", () => {
+      beforeEach(() => {
+        feed._maybeUpdateCachedData.restore();
+        sandbox.stub(feed.cache, "set").resolves();
+      });
+      it("should refresh layout on startup if it was served from cache", async () => {
+        feed.loadLayout.restore();
+        sandbox.stub(feed.cache, "get").resolves({layout: {_timestamp: Date.now(), layout: {}}});
+        sandbox.stub(feed, "fetchFromEndpoint").resolves({layout: {}});
+        clock.tick(THIRTY_MINUTES + 1);
+
+        await feed.refreshAll({isStartup: true});
+
+        assert.calledOnce(feed.fetchFromEndpoint);
+        // Once from cache, once to update the store
+        assert.calledTwice(feed.store.dispatch);
+        assert.equal(feed.store.dispatch.firstCall.args[0].type, at.DISCOVERY_STREAM_LAYOUT_UPDATE);
+      });
+      it("should not refresh layout on startup if it is under THIRTY_MINUTES", async () => {
+        feed.loadLayout.restore();
+        sandbox.stub(feed.cache, "get").resolves({layout: {_timestamp: Date.now(), layout: {}}});
+        sandbox.stub(feed, "fetchFromEndpoint").resolves({layout: {}});
+
+        await feed.refreshAll({isStartup: true});
+
+        assert.notCalled(feed.fetchFromEndpoint);
+      });
+      it("should refresh spocs on startup if it was served from cache", async () => {
+        feed.loadSpocs.restore();
+        sandbox.stub(feed.cache, "get").resolves({spocs: {lastUpdated: Date.now()}});
+        sandbox.stub(feed, "fetchFromEndpoint").resolves("data");
+        clock.tick(THIRTY_MINUTES + 1);
+
+        await feed.refreshAll({isStartup: true});
+
+        assert.calledOnce(feed.fetchFromEndpoint);
+        // Once from cache, once to update the store
+        assert.calledTwice(feed.store.dispatch);
+        assert.equal(feed.store.dispatch.firstCall.args[0].type, at.DISCOVERY_STREAM_SPOCS_UPDATE);
+      });
+      it("should not refresh spocs on startup if it is under THIRTY_MINUTES", async () => {
+        feed.loadSpocs.restore();
+        sandbox.stub(feed.cache, "get").resolves({spocs: {lastUpdated: Date.now()}});
+        sandbox.stub(feed, "fetchFromEndpoint").resolves("data");
+
+        await feed.refreshAll({isStartup: true});
+
+        assert.notCalled(feed.fetchFromEndpoint);
+      });
+      it("should refresh feeds on startup if it was served from cache", async () => {
+        feed.loadComponentFeeds.restore();
+
+        const fakeComponents = {components: [{feed: {url: "foo.com"}}]};
+        const fakeLayout = [fakeComponents];
+        const fakeDiscoveryStream = {DiscoveryStream: {layout: fakeLayout}};
+        sandbox.stub(feed.store, "getState").returns(fakeDiscoveryStream);
+
+        const fakeCache = {feeds: {"foo.com": {lastUpdated: Date.now(), data: "data"}}};
+        sandbox.stub(feed.cache, "get").resolves(fakeCache);
+        clock.tick(THIRTY_MINUTES + 1);
+        sandbox.stub(feed, "fetchFromEndpoint").resolves("data");
+
+        await feed.refreshAll({isStartup: true});
+
+        assert.calledOnce(feed.fetchFromEndpoint);
+        // Once from cache, once to update the store
+        assert.calledTwice(feed.store.dispatch);
+        assert.equal(feed.store.dispatch.firstCall.args[0].type, at.DISCOVERY_STREAM_FEEDS_UPDATE);
+      });
+    });
   });
 });