Bug 1525494 - Parallel component feed loads. r=Mardak a=lizzard
authork88hudson <k88hudson@gmail.com>
Sat, 16 Feb 2019 00:49:16 +0200
changeset 515987 3c371055b3815f02899b6a5055496d29e35f79b6
parent 515986 0bb65ba5bb92dd0722091e18e33b5c2e1d80521d
child 515988 5d44e50ff0c250f05778c896e2f00387f51b0fe6
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)
reviewersMardak, lizzard
bugs1525494
milestone66.0
Bug 1525494 - Parallel component feed loads. r=Mardak a=lizzard Reviewers: Mardak Reviewed By: Mardak Bug #: 1525494 Differential Revision: https://phabricator.services.mozilla.com/D19716
browser/components/newtab/content-src/components/ASRouterAdmin/ASRouterAdmin.jsx
browser/components/newtab/content-src/lib/selectLayoutRender.js
browser/components/newtab/data/content/activity-stream.bundle.js
browser/components/newtab/lib/DiscoveryStreamFeed.jsm
browser/components/newtab/test/unit/content-src/lib/selectLayoutRender.test.js
browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
--- a/browser/components/newtab/content-src/components/ASRouterAdmin/ASRouterAdmin.jsx
+++ b/browser/components/newtab/content-src/components/ASRouterAdmin/ASRouterAdmin.jsx
@@ -63,17 +63,17 @@ class DiscoveryStreamAdmin extends React
     return (
       <React.Fragment>
         <Row>
           <td className="min">Feed url</td>
           <td>{feed.url}</td>
         </Row>
         <Row>
           <td className="min">Data last fetched</td>
-          <td>{relativeTime(feeds[feed.url] ? feeds[feed.url].lastUpdated : null) || "(no data)"}</td>
+          <td>{relativeTime(feeds.data[feed.url] ? feeds.data[feed.url].lastUpdated : null) || "(no data)"}</td>
         </Row>
       </React.Fragment>
     );
   }
 
   render() {
     const {config, lastUpdated, layout} = this.props.state;
     return (<div>
--- a/browser/components/newtab/content-src/lib/selectLayoutRender.js
+++ b/browser/components/newtab/content-src/lib/selectLayoutRender.js
@@ -40,13 +40,22 @@ export const selectLayoutRender = create
 
       // Loops through all the components and adds a .data property
       // containing data from feeds
       components: row.components.map(component => {
         if (!component.feed || !feeds.data[component.feed.url]) {
           return component;
         }
 
-        return {...component, data: maybeInjectSpocs(feeds.data[component.feed.url].data, component.spocs)};
+        let {data} = feeds.data[component.feed.url];
+
+        if (component && component.properties && component.properties.offset) {
+          data = {
+            ...data,
+            recommendations: data.recommendations.slice(component.properties.offset),
+          };
+        }
+
+        return {...component, data: maybeInjectSpocs(data, component.spocs)};
       }),
     }));
   }
 );
--- a/browser/components/newtab/data/content/activity-stream.bundle.js
+++ b/browser/components/newtab/data/content/activity-stream.bundle.js
@@ -2579,17 +2579,17 @@ class DiscoveryStreamAdmin extends react
         react__WEBPACK_IMPORTED_MODULE_4___default.a.createElement(
           "td",
           { className: "min" },
           "Data last fetched"
         ),
         react__WEBPACK_IMPORTED_MODULE_4___default.a.createElement(
           "td",
           null,
-          relativeTime(feeds[feed.url] ? feeds[feed.url].lastUpdated : null) || "(no data)"
+          relativeTime(feeds.data[feed.url] ? feeds.data[feed.url].lastUpdated : null) || "(no data)"
         )
       )
     );
   }
 
   render() {
     const { config, lastUpdated, layout } = this.props.state;
     return react__WEBPACK_IMPORTED_MODULE_4___default.a.createElement(
@@ -7778,17 +7778,25 @@ function layoutRender(layout, feeds, spo
 
     // Loops through all the components and adds a .data property
     // containing data from feeds
     components: row.components.map(component => {
       if (!component.feed || !feeds.data[component.feed.url]) {
         return component;
       }
 
-      return Object.assign({}, component, { data: maybeInjectSpocs(feeds.data[component.feed.url].data, component.spocs) });
+      let { data } = feeds.data[component.feed.url];
+
+      if (component && component.properties && component.properties.offset) {
+        data = Object.assign({}, data, {
+          recommendations: data.recommendations.slice(component.properties.offset)
+        });
+      }
+
+      return Object.assign({}, component, { data: maybeInjectSpocs(data, component.spocs) });
     })
   }));
 });
 // EXTERNAL MODULE: ./content-src/components/TopSites/TopSites.jsx
 var TopSites = __webpack_require__(31);
 
 // CONCATENATED MODULE: ./content-src/components/DiscoveryStreamComponents/TopSites/TopSites.jsx
 
--- a/browser/components/newtab/lib/DiscoveryStreamFeed.jsm
+++ b/browser/components/newtab/lib/DiscoveryStreamFeed.jsm
@@ -163,35 +163,94 @@ this.DiscoveryStreamFeed = class Discove
     if (layoutResponse && layoutResponse.spocs && layoutResponse.spocs.url) {
       sendUpdate({
         type: at.DISCOVERY_STREAM_SPOCS_ENDPOINT,
         data: layoutResponse.spocs.url,
       });
     }
   }
 
+  /**
+   * buildFeedPromise - Adds the promise result to newFeeds and
+   *                    pushes a promise to newsFeedsPromises.
+   * @param {Object} Has both newFeedsPromises (Array) and newFeeds (Object)
+   * @param {Boolean} isStartup We have different cache handling for startup.
+   * @returns {Function} We return a function so we can contain
+   *                     the scope for isStartup and the promises object.
+   *                     Combines feed results and promises for each component with a feed.
+   */
+  buildFeedPromise({newFeedsPromises, newFeeds}, isStartup) {
+    return component => {
+      const {url} = component.feed;
+
+      if (!newFeeds[url]) {
+        // We initially stub this out so we don't fetch dupes,
+        // we then fill in with the proper object inside the promise.
+        newFeeds[url] = {};
+
+        const feedPromise = this.getComponentFeed(url, isStartup);
+
+        feedPromise.then(data => {
+          newFeeds[url] = data;
+        }).catch(/* istanbul ignore next */ error => {
+          Cu.reportError(`Error trying to load component feed ${url}: ${error}`);
+        });
+
+        newFeedsPromises.push(feedPromise);
+      }
+    };
+  }
+
+  /**
+   * reduceFeedComponents - Filters out components with no feeds, and combines
+   *                        all feeds on this component with the feeds from other components.
+   * @param {Boolean} isStartup We have different cache handling for startup.
+   * @returns {Function} We return a function so we can contain the scope for isStartup.
+   *                     Reduces feeds into promises and feed data.
+   */
+  reduceFeedComponents(isStartup) {
+    return (accumulator, row) => {
+      row.components
+        .filter(component => component && component.feed)
+        .forEach(this.buildFeedPromise(accumulator, isStartup));
+      return accumulator;
+    };
+  }
+
+  /**
+   * buildFeedPromises - Filters out rows with no components,
+   *                     and gets us a promise for each unique feed.
+   * @param {Object} layout This is the Discovery Stream layout object.
+   * @param {Boolean} isStartup We have different cache handling for startup.
+   * @returns {Object} An object with newFeedsPromises (Array) and newFeeds (Object),
+   *                   we can Promise.all newFeedsPromises to get completed data in newFeeds.
+   */
+  buildFeedPromises(layout, isStartup) {
+    const initialData = {
+      newFeedsPromises: [],
+      newFeeds: {},
+    };
+    return layout
+      .filter(row => row && row.components)
+      .reduce(this.reduceFeedComponents(isStartup), initialData);
+  }
+
   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, isStartup);
-          }
-        }
-      }
+
+    if (!DiscoveryStream || !DiscoveryStream.layout) {
+      return;
+    }
 
-      await this.cache.set("feeds", newFeeds);
-      sendUpdate({type: at.DISCOVERY_STREAM_FEEDS_UPDATE, data: newFeeds});
-    }
+    const {newFeedsPromises, newFeeds} = this.buildFeedPromises(DiscoveryStream.layout, isStartup);
+
+    // Each promise has a catch already built in, so no need to catch here.
+    await Promise.all(newFeedsPromises);
+    await this.cache.set("feeds", newFeeds);
+    sendUpdate({type: at.DISCOVERY_STREAM_FEEDS_UPDATE, data: newFeeds});
   }
 
   async loadSpocs(sendUpdate, isStartup) {
     const cachedData = await this.cache.get() || {};
     let spocs;
 
     if (this.showSpocs) {
       spocs = cachedData.spocs;
--- a/browser/components/newtab/test/unit/content-src/lib/selectLayoutRender.test.js
+++ b/browser/components/newtab/test/unit/content-src/lib/selectLayoutRender.test.js
@@ -42,16 +42,26 @@ describe("selectLayoutRender", () => {
 
     const result = selectLayoutRender(store.getState());
 
     assert.lengthOf(result, 1);
     assert.propertyVal(result[0], "width", 3);
     assert.deepEqual(result[0].components[0], FAKE_LAYOUT[0].components[0]);
   });
 
+  it("should return feed data offset by layout set prop", () => {
+    const fakeLayout = [{width: 3, components: [{type: "foo", properties: {offset: 1}, feed: {url: "foo.com"}}]}];
+    store.dispatch({type: at.DISCOVERY_STREAM_LAYOUT_UPDATE, data: {layout: fakeLayout}});
+    store.dispatch({type: at.DISCOVERY_STREAM_FEEDS_UPDATE, data: FAKE_FEEDS});
+
+    const result = selectLayoutRender(store.getState());
+
+    assert.deepEqual(result[0].components[0].data, {recommendations: ["bar"]});
+  });
+
   it("should return spoc result for rolls below the probability", () => {
     const fakeSpocConfig = {positions: [{index: 0}, {index: 1}], probability: 0.5};
     const fakeLayout = [{width: 3, components: [{type: "foo", feed: {url: "foo.com"}, spocs: fakeSpocConfig}]}];
     const fakeSpocsData = {lastUpdated: 0, spocs: {spocs: ["fooSpoc", "barSpoc"]}};
 
     store.dispatch({type: at.DISCOVERY_STREAM_LAYOUT_UPDATE, data: {layout: fakeLayout}});
     store.dispatch({type: at.DISCOVERY_STREAM_FEEDS_UPDATE, data: FAKE_FEEDS});
     store.dispatch({type: at.DISCOVERY_STREAM_SPOCS_UPDATE, data: fakeSpocsData});
--- a/browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
+++ b/browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
@@ -101,29 +101,94 @@ describe("DiscoveryStreamFeed", () => {
 
       await feed.loadLayout(feed.store.dispatch);
 
       assert.equal(feed.store.getState().DiscoveryStream.spocs.spocs_endpoint, "foo.com");
     });
   });
 
   describe("#loadComponentFeeds", () => {
-    it("should populate feeds cache", async () => {
-      const fakeComponents = {components: [{feed: {url: "foo.com"}}]};
-      const fakeLayout = [fakeComponents, {components: [{}]}, {}];
-      const fakeDiscoveryStream = {DiscoveryStream: {layout: fakeLayout}};
+    let fakeCache;
+    let fakeDiscoveryStream;
+    beforeEach(() => {
+      fakeDiscoveryStream = {
+        DiscoveryStream: {
+          layout: [
+            {components: [{feed: {url: "foo.com"}}]},
+            {components: [{}]},
+            {},
+          ],
+        },
+      };
+      fakeCache = {};
       sandbox.stub(feed.store, "getState").returns(fakeDiscoveryStream);
       sandbox.stub(feed.cache, "set").returns(Promise.resolve());
-      const fakeCache = {feeds: {"foo.com": {"lastUpdated": Date.now(), "data": "data"}}};
+    });
+
+    afterEach(() => {
+      sandbox.restore();
+    });
+
+    it("should not dispatch updates when layout is not defined", async () => {
+      fakeDiscoveryStream = {
+        DiscoveryStream: {},
+      };
+      feed.store.getState.returns(fakeDiscoveryStream);
+      sandbox.spy(feed.store, "dispatch");
+
+      await feed.loadComponentFeeds(feed.store.dispatch);
+
+      assert.notCalled(feed.store.dispatch);
+    });
+
+    it("should populate feeds cache", async () => {
+      fakeCache = {feeds: {"foo.com": {"lastUpdated": Date.now(), "data": "data"}}};
       sandbox.stub(feed.cache, "get").returns(Promise.resolve(fakeCache));
 
       await feed.loadComponentFeeds(feed.store.dispatch);
 
       assert.calledWith(feed.cache.set, "feeds", {"foo.com": {"data": "data", "lastUpdated": 0}});
     });
+
+    it("should send at.DISCOVERY_STREAM_FEEDS_UPDATE with new feed data",
+      async () => {
+        sandbox.stub(feed.cache, "get").returns(Promise.resolve(fakeCache));
+        sandbox.spy(feed.store, "dispatch");
+
+        await feed.loadComponentFeeds(feed.store.dispatch);
+
+        assert.calledWith(feed.store.dispatch, {
+          type: at.DISCOVERY_STREAM_FEEDS_UPDATE,
+          data: {"foo.com": null},
+        });
+      });
+
+    it("should return number of promises equal to unique urls",
+      async () => {
+        sandbox.stub(feed.cache, "get").returns(Promise.resolve(fakeCache));
+        sandbox.stub(global.Promise, "all").resolves();
+        fakeDiscoveryStream = {
+          DiscoveryStream: {
+            layout: [
+              {components: [{feed: {url: "foo.com"}}, {feed: {url: "bar.com"}}]},
+              {components: [{feed: {url: "foo.com"}}]},
+              {},
+              {components: [{feed: {url: "baz.com"}}]},
+            ],
+          },
+        };
+        feed.store.getState.returns(fakeDiscoveryStream);
+
+        await feed.loadComponentFeeds(feed.store.dispatch);
+
+        assert.calledOnce(global.Promise.all);
+        const {args} = global.Promise.all.firstCall;
+        assert.equal(args[0].length, 3);
+      }
+    );
   });
 
   describe("#getComponentFeed", () => {
     it("should fetch fresh data if cache is empty", async () => {
       const fakeCache = {};
       sandbox.stub(feed.cache, "get").returns(Promise.resolve(fakeCache));
       sandbox.stub(feed, "fetchFromEndpoint").resolves("data");