Bug 1525413 - Parallelize and wait for endpoint requests before rendering Discovery Stream. r=Mardak a=lizzard
authork88hudson <k88hudson@gmail.com>
Thu, 14 Feb 2019 18:59:09 +0100
changeset 515950 0f8d776acfbf488e9c357b923f0b401cbc368713
parent 515949 0fcc6713d0248efd3494e5a6a66ec77021c81552
child 515951 24c97a1ec2485ab1ca019cc6ada2036451bce287
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
bugs1525413
milestone66.0
Bug 1525413 - Parallelize and wait for endpoint requests before rendering Discovery Stream. r=Mardak a=lizzard
browser/components/newtab/common/Reducers.jsm
browser/components/newtab/content-src/components/Base/Base.jsx
browser/components/newtab/content-src/components/Base/_Base.scss
browser/components/newtab/content-src/components/DiscoveryStreamBase/DiscoveryStreamBase.jsx
browser/components/newtab/content-src/lib/selectLayoutRender.js
browser/components/newtab/css/activity-stream-linux.css
browser/components/newtab/css/activity-stream-mac.css
browser/components/newtab/css/activity-stream-windows.css
browser/components/newtab/data/content/activity-stream.bundle.js
browser/components/newtab/lib/DiscoveryStreamFeed.jsm
browser/components/newtab/prerendered/static/activity-stream-initial-state.js
browser/components/newtab/test/unit/common/Reducers.test.js
browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/DiscoveryStreamBase.test.jsx
browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
--- a/browser/components/newtab/common/Reducers.jsm
+++ b/browser/components/newtab/common/Reducers.jsm
@@ -49,17 +49,20 @@ const INITIAL_STATE = {
   },
   // This is the new pocket configurable layout state.
   DiscoveryStream: {
     // This is a JSON-parsed copy of the discoverystream.config pref value.
     config: {enabled: false, layout_endpoint: ""},
     layout: [],
     lastUpdated: null,
     feeds: {
-      // "https://foo.com/feed1": {lastUpdated: 123, data: []}
+      data: {
+        // "https://foo.com/feed1": {lastUpdated: 123, data: []}
+      },
+      loaded: false,
     },
     spocs: {
       spocs_endpoint: "",
       lastUpdated: null,
       data: {}, // {spocs: []}
       loaded: false,
     },
   },
@@ -460,17 +463,24 @@ function DiscoveryStream(prevState = INI
     // The reason this is a separate action is so it doesn't trigger a listener update on init
     case at.DISCOVERY_STREAM_CONFIG_SETUP:
       return {...prevState, config: action.data || {}};
     case at.DISCOVERY_STREAM_LAYOUT_UPDATE:
       return {...prevState, lastUpdated: action.data.lastUpdated || null, layout: action.data.layout || []};
     case at.DISCOVERY_STREAM_LAYOUT_RESET:
       return {...prevState, lastUpdated: INITIAL_STATE.DiscoveryStream.lastUpdated, layout: INITIAL_STATE.DiscoveryStream.layout};
     case at.DISCOVERY_STREAM_FEEDS_UPDATE:
-      return {...prevState, feeds: action.data || prevState.feeds};
+      return {
+        ...prevState,
+        feeds: {
+          ...prevState.feeds,
+          data: action.data || prevState.feeds.data,
+          loaded: true,
+        },
+      };
     case at.DISCOVERY_STREAM_SPOCS_ENDPOINT:
       return {
         ...prevState,
         spocs: {
           ...INITIAL_STATE.DiscoveryStream.spocs,
           spocs_endpoint: action.data || INITIAL_STATE.DiscoveryStream.spocs.spocs_endpoint,
         },
       };
--- a/browser/components/newtab/content-src/components/Base/Base.jsx
+++ b/browser/components/newtab/content-src/components/Base/Base.jsx
@@ -157,16 +157,17 @@ export class BaseContent extends React.P
     const searchHandoffEnabled = prefs["improvesearch.handoffToAwesomebar"];
 
     if (isDiscoveryStream) {
       this.disableDarkTheme();
     }
 
     const outerClassName = [
       "outer-wrapper",
+      isDiscoveryStream && "ds-outer-wrapper-search-alignment",
       shouldBeFixedToTop && "fixed-to-top",
       prefs.showSearch && this.state.fixedSearch && !noSectionsEnabled && "fixed-search",
       prefs.showSearch && noSectionsEnabled && "only-search",
     ].filter(v => v).join(" ");
 
     return (
       <div>
         <div className={outerClassName}>
--- a/browser/components/newtab/content-src/components/Base/_Base.scss
+++ b/browser/components/newtab/content-src/components/Base/_Base.scss
@@ -49,16 +49,24 @@ main {
   }
 
   .hide-main & {
     visibility: hidden;
   }
 
 }
 
+.ds-outer-wrapper-search-alignment {
+  main {
+    // This override is to ensure while Discovery Stream loads,
+    // the search bar does not jump around. (it sticks to the top)
+    margin: 0 auto;
+  }
+}
+
 .base-content-fallback {
   // Make the error message be centered against the viewport
   height: 100vh;
 }
 
 .body-wrapper {
   // Hide certain elements so the page structure is fixed, e.g., placeholders,
   // while avoiding flashes of changing content, e.g., icons and text
--- a/browser/components/newtab/content-src/components/DiscoveryStreamBase/DiscoveryStreamBase.jsx
+++ b/browser/components/newtab/content-src/components/DiscoveryStreamBase/DiscoveryStreamBase.jsx
@@ -111,24 +111,16 @@ export class _DiscoveryStreamBase extend
           }
         });
       });
     });
   }
 
   renderComponent(component, embedWidth) {
     let rows;
-    const {spocs} = this.props.DiscoveryStream;
-
-    // TODO: Can we make this a bit better visually while it loads?
-    // If this component expects spocs,
-    // wait until spocs are loaded before attempting to use it.
-    if (component.spocs && !spocs.loaded) {
-      return null;
-    }
 
     switch (component.type) {
       case "TopSites":
         return (<TopSites header={component.header} />);
       case "Message":
         return (
           <DSMessage
             title={component.header && component.header.title}
@@ -205,16 +197,22 @@ export class _DiscoveryStreamBase extend
     // to unmount and mount a new instance for new styles.
     const json = JSON.stringify(styles);
     return (<style key={json} data-styles={json} ref={this.onStyleMount} />);
   }
 
   render() {
     const {layoutRender} = this.props.DiscoveryStream;
     const styles = [];
+    const {spocs, feeds} = this.props.DiscoveryStream;
+
+    if (!spocs.loaded || !feeds.loaded) {
+      return null;
+    }
+
     return (
       <div className="discovery-stream ds-layout">
         {layoutRender.map((row, rowIndex) => (
           <div key={`row-${rowIndex}`} className={`ds-column ds-column-${row.width}`}>
             <div className="ds-column-grid">
               {row.components.map((component, componentIndex) => {
                 styles[rowIndex] = [...styles[rowIndex] || [], component.styles];
                 return (<div key={`component-${componentIndex}`}>
--- a/browser/components/newtab/content-src/lib/selectLayoutRender.js
+++ b/browser/components/newtab/content-src/lib/selectLayoutRender.js
@@ -36,17 +36,17 @@ export const selectLayoutRender = create
     }
 
     return layout.map(row => ({
       ...row,
 
       // Loops through all the components and adds a .data property
       // containing data from feeds
       components: row.components.map(component => {
-        if (!component.feed || !feeds[component.feed.url]) {
+        if (!component.feed || !feeds.data[component.feed.url]) {
           return component;
         }
 
-        return {...component, data: maybeInjectSpocs(feeds[component.feed.url].data, component.spocs)};
+        return {...component, data: maybeInjectSpocs(feeds.data[component.feed.url].data, component.spocs)};
       }),
     }));
   }
 );
--- a/browser/components/newtab/css/activity-stream-linux.css
+++ b/browser/components/newtab/css/activity-stream-linux.css
@@ -354,16 +354,19 @@ main {
     main {
       width: 1042px; } }
   main section {
     margin-bottom: 20px;
     position: relative; }
   .hide-main main {
     visibility: hidden; }
 
+.ds-outer-wrapper-search-alignment main {
+  margin: 0 auto; }
+
 .base-content-fallback {
   height: 100vh; }
 
 
 .body-wrapper .section-title,
 .body-wrapper .sections-list .section:last-of-type,
 .body-wrapper .topics {
   opacity: 0; }
--- a/browser/components/newtab/css/activity-stream-mac.css
+++ b/browser/components/newtab/css/activity-stream-mac.css
@@ -357,16 +357,19 @@ main {
     main {
       width: 1042px; } }
   main section {
     margin-bottom: 20px;
     position: relative; }
   .hide-main main {
     visibility: hidden; }
 
+.ds-outer-wrapper-search-alignment main {
+  margin: 0 auto; }
+
 .base-content-fallback {
   height: 100vh; }
 
 
 .body-wrapper .section-title,
 .body-wrapper .sections-list .section:last-of-type,
 .body-wrapper .topics {
   opacity: 0; }
--- a/browser/components/newtab/css/activity-stream-windows.css
+++ b/browser/components/newtab/css/activity-stream-windows.css
@@ -354,16 +354,19 @@ main {
     main {
       width: 1042px; } }
   main section {
     margin-bottom: 20px;
     position: relative; }
   .hide-main main {
     visibility: hidden; }
 
+.ds-outer-wrapper-search-alignment main {
+  margin: 0 auto; }
+
 .base-content-fallback {
   height: 100vh; }
 
 
 .body-wrapper .section-title,
 .body-wrapper .sections-list .section:last-of-type,
 .body-wrapper .topics {
   opacity: 0; }
--- a/browser/components/newtab/data/content/activity-stream.bundle.js
+++ b/browser/components/newtab/data/content/activity-stream.bundle.js
@@ -2391,17 +2391,17 @@ class BaseContent extends react__WEBPACK
     const noSectionsEnabled = !prefs["feeds.topsites"] && props.Sections.filter(section => section.enabled).length === 0;
     const isDiscoveryStream = props.DiscoveryStream.config && props.DiscoveryStream.config.enabled;
     const searchHandoffEnabled = prefs["improvesearch.handoffToAwesomebar"];
 
     if (isDiscoveryStream) {
       this.disableDarkTheme();
     }
 
-    const outerClassName = ["outer-wrapper", shouldBeFixedToTop && "fixed-to-top", prefs.showSearch && this.state.fixedSearch && !noSectionsEnabled && "fixed-search", prefs.showSearch && noSectionsEnabled && "only-search"].filter(v => v).join(" ");
+    const outerClassName = ["outer-wrapper", isDiscoveryStream && "ds-outer-wrapper-search-alignment", shouldBeFixedToTop && "fixed-to-top", prefs.showSearch && this.state.fixedSearch && !noSectionsEnabled && "fixed-search", prefs.showSearch && noSectionsEnabled && "only-search"].filter(v => v).join(" ");
 
     return react__WEBPACK_IMPORTED_MODULE_9___default.a.createElement(
       "div",
       null,
       react__WEBPACK_IMPORTED_MODULE_9___default.a.createElement(
         "div",
         { className: outerClassName },
         react__WEBPACK_IMPORTED_MODULE_9___default.a.createElement(
@@ -7735,21 +7735,21 @@ function layoutRender(layout, feeds, spo
     return data;
   }
 
   return layout.map(row => Object.assign({}, row, {
 
     // Loops through all the components and adds a .data property
     // containing data from feeds
     components: row.components.map(component => {
-      if (!component.feed || !feeds[component.feed.url]) {
+      if (!component.feed || !feeds.data[component.feed.url]) {
         return component;
       }
 
-      return Object.assign({}, component, { data: maybeInjectSpocs(feeds[component.feed.url].data, component.spocs) });
+      return Object.assign({}, component, { data: maybeInjectSpocs(feeds.data[component.feed.url].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
 
@@ -7894,24 +7894,16 @@ class DiscoveryStreamBase_DiscoveryStrea
           }
         });
       });
     });
   }
 
   renderComponent(component, embedWidth) {
     let rows;
-    const { spocs } = this.props.DiscoveryStream;
-
-    // TODO: Can we make this a bit better visually while it loads?
-    // If this component expects spocs,
-    // wait until spocs are loaded before attempting to use it.
-    if (component.spocs && !spocs.loaded) {
-      return null;
-    }
 
     switch (component.type) {
       case "TopSites":
         return external_React_default.a.createElement(TopSites_TopSites_TopSites, { header: component.header });
       case "Message":
         return external_React_default.a.createElement(DSMessage_DSMessage, {
           title: component.header && component.header.title,
           subtitle: component.header && component.header.subtitle,
@@ -7986,16 +7978,22 @@ class DiscoveryStreamBase_DiscoveryStrea
     // to unmount and mount a new instance for new styles.
     const json = JSON.stringify(styles);
     return external_React_default.a.createElement("style", { key: json, "data-styles": json, ref: this.onStyleMount });
   }
 
   render() {
     const { layoutRender } = this.props.DiscoveryStream;
     const styles = [];
+    const { spocs, feeds } = this.props.DiscoveryStream;
+
+    if (!spocs.loaded || !feeds.loaded) {
+      return null;
+    }
+
     return external_React_default.a.createElement(
       "div",
       { className: "discovery-stream ds-layout" },
       layoutRender.map((row, rowIndex) => external_React_default.a.createElement(
         "div",
         { key: `row-${rowIndex}`, className: `ds-column ds-column-${row.width}` },
         external_React_default.a.createElement(
           "div",
@@ -11582,17 +11580,20 @@ const INITIAL_STATE = {
   },
   // This is the new pocket configurable layout state.
   DiscoveryStream: {
     // This is a JSON-parsed copy of the discoverystream.config pref value.
     config: { enabled: false, layout_endpoint: "" },
     layout: [],
     lastUpdated: null,
     feeds: {
-      // "https://foo.com/feed1": {lastUpdated: 123, data: []}
+      data: {
+        // "https://foo.com/feed1": {lastUpdated: 123, data: []}
+      },
+      loaded: false
     },
     spocs: {
       spocs_endpoint: "",
       lastUpdated: null,
       data: {}, // {spocs: []}
       loaded: false
     }
   },
@@ -11994,17 +11995,22 @@ function DiscoveryStream(prevState = INI
     // The reason this is a separate action is so it doesn't trigger a listener update on init
     case Actions["actionTypes"].DISCOVERY_STREAM_CONFIG_SETUP:
       return Object.assign({}, prevState, { config: action.data || {} });
     case Actions["actionTypes"].DISCOVERY_STREAM_LAYOUT_UPDATE:
       return Object.assign({}, prevState, { lastUpdated: action.data.lastUpdated || null, layout: action.data.layout || [] });
     case Actions["actionTypes"].DISCOVERY_STREAM_LAYOUT_RESET:
       return Object.assign({}, prevState, { lastUpdated: INITIAL_STATE.DiscoveryStream.lastUpdated, layout: INITIAL_STATE.DiscoveryStream.layout });
     case Actions["actionTypes"].DISCOVERY_STREAM_FEEDS_UPDATE:
-      return Object.assign({}, prevState, { feeds: action.data || prevState.feeds });
+      return Object.assign({}, prevState, {
+        feeds: Object.assign({}, prevState.feeds, {
+          data: action.data || prevState.feeds.data,
+          loaded: true
+        })
+      });
     case Actions["actionTypes"].DISCOVERY_STREAM_SPOCS_ENDPOINT:
       return Object.assign({}, prevState, {
         spocs: Object.assign({}, INITIAL_STATE.DiscoveryStream.spocs, {
           spocs_endpoint: action.data || INITIAL_STATE.DiscoveryStream.spocs.spocs_endpoint
         })
       });
     case Actions["actionTypes"].DISCOVERY_STREAM_SPOCS_UPDATE:
       if (action.data) {
--- a/browser/components/newtab/lib/DiscoveryStreamFeed.jsm
+++ b/browser/components/newtab/lib/DiscoveryStreamFeed.jsm
@@ -291,18 +291,20 @@ this.DiscoveryStreamFeed = class Discove
    * @param {RefreshAllOptions} options
    */
   async refreshAll(options = {}) {
     const dispatch = options.updateOpenTabs ?
       action => this.store.dispatch(ac.BroadcastToContent(action)) :
       this.store.dispatch;
 
     await this.loadLayout(dispatch);
-    await this.loadComponentFeeds(dispatch);
-    await this.loadSpocs(dispatch);
+    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.loaded = true;
   }
 
   async enable() {
     await this.refreshAll({updateOpenTabs: true});
   }
 
   async disable() {
--- a/browser/components/newtab/prerendered/static/activity-stream-initial-state.js
+++ b/browser/components/newtab/prerendered/static/activity-stream-initial-state.js
@@ -71,17 +71,20 @@ window.gActivityStreamPrerenderedState =
   },
   "DiscoveryStream": {
     "config": {
       "enabled": false,
       "layout_endpoint": ""
     },
     "layout": [],
     "lastUpdated": null,
-    "feeds": {},
+    "feeds": {
+      "data": {},
+      "loaded": false
+    },
     "spocs": {
       "spocs_endpoint": "",
       "lastUpdated": null,
       "data": {},
       "loaded": false
     }
   },
   "Search": {
--- a/browser/components/newtab/test/unit/common/Reducers.test.js
+++ b/browser/components/newtab/test/unit/common/Reducers.test.js
@@ -663,16 +663,28 @@ describe("Reducers", () => {
       const state = DiscoveryStream(undefined, {type: at.DISCOVERY_STREAM_LAYOUT_UPDATE, data: {layout: ["test"], lastUpdated: 123}});
       assert.equal(state.layout[0], "test");
       assert.equal(state.lastUpdated, 123);
     });
     it("should set config data with DISCOVERY_STREAM_CONFIG_CHANGE", () => {
       const state = DiscoveryStream(undefined, {type: at.DISCOVERY_STREAM_CONFIG_CHANGE, data: {enabled: true}});
       assert.deepEqual(state.config, {enabled: true});
     });
+    it("should load feeds with DISCOVERY_STREAM_FEEDS_UPDATE", () => {
+      const data = {
+        "https://foo.com/feed1": {lastUpdated: 123, data: [1, 2, 3]},
+      };
+
+      const state = DiscoveryStream(undefined, {type: at.DISCOVERY_STREAM_FEEDS_UPDATE, data});
+
+      assert.deepEqual(state.feeds, {
+        data,
+        loaded: true,
+      });
+    });
     it("should set spoc_endpoint with DISCOVERY_STREAM_SPOCS_ENDPOINT", () => {
       const state = DiscoveryStream(undefined, {type: at.DISCOVERY_STREAM_SPOCS_ENDPOINT, data: "foo.com"});
       assert.equal(state.spocs.spocs_endpoint, "foo.com");
     });
     it("should set spocs with DISCOVERY_STREAM_SPOCS_UPDATE", () => {
       const data = {
         lastUpdated: 123,
         spocs: [1, 2, 3],
new file mode 100644
--- /dev/null
+++ b/browser/components/newtab/test/unit/content-src/components/DiscoveryStreamComponents/DiscoveryStreamBase.test.jsx
@@ -0,0 +1,37 @@
+import {_DiscoveryStreamBase as DiscoveryStreamBase} from "content-src/components/DiscoveryStreamBase/DiscoveryStreamBase";
+import React from "react";
+import {shallow} from "enzyme";
+
+describe("<DiscoveryStreamBase>", () => {
+  it("should render once we have feeds and spocs", () => {
+    const discoveryStreamProps = {
+      spocs: {loaded: true},
+      feeds: {loaded: true},
+      layoutRender: [],
+    };
+
+    const wrapper = shallow(<DiscoveryStreamBase DiscoveryStream={discoveryStreamProps} />);
+
+    assert.equal(wrapper.find(".ds-layout").length, 1);
+  });
+  it("should not render anything without spocs", () => {
+    const discoveryStreamProps = {
+      spocs: {loaded: false},
+      feeds: {loaded: true},
+    };
+
+    const wrapper = shallow(<DiscoveryStreamBase DiscoveryStream={discoveryStreamProps} />);
+
+    assert.equal(wrapper.find(".ds-layout").length, 0);
+  });
+  it("should not render anything without feeds", () => {
+    const discoveryStreamProps = {
+      spocs: {loaded: true},
+      feeds: {loaded: false},
+    };
+
+    const wrapper = shallow(<DiscoveryStreamBase DiscoveryStream={discoveryStreamProps} />);
+
+    assert.equal(wrapper.find(".ds-layout").length, 0);
+  });
+});
--- a/browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
+++ b/browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
@@ -741,10 +741,27 @@ describe("DiscoveryStreamFeed", () => {
       await feed.refreshAll({updateOpenTabs: false});
       [feed.loadLayout, feed.loadComponentFeeds, feed.loadSpocs]
         .forEach(fn => {
           assert.calledOnce(fn);
           const result = fn.firstCall.args[0]({type: "FOO"});
           assert.deepEqual(result, {type: "FOO"});
         });
     });
+    it("should set loaded to true if loadSpocs and loadComponentFeeds fails", async () => {
+      feed.loadComponentFeeds.rejects("loadComponentFeeds error");
+      feed.loadSpocs.rejects("loadSpocs error");
+
+      await feed.refreshAll();
+
+      assert.isTrue(feed.loaded);
+    });
+    it("should call loadComponentFeeds and loadSpocs in Promise.all", async () => {
+      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);
+    });
   });
 });