Bug 1717891 - Spoc position pref r=thecount
authorGavin Lazar Suntop <gavin@gsuntop.com>
Fri, 23 Jul 2021 16:34:12 +0000
changeset 586542 7cb66eb9a96fb6bc31ab53817659b76c687632f6
parent 586541 eec37405fe16fc4df745a55b149c186712a0c5f0
child 586543 ede907c3def22d5bbdefc682a73a8c7585636a6f
push id38638
push usermlaza@mozilla.com
push dateFri, 23 Jul 2021 21:32:32 +0000
treeherdermozilla-central@eeb3f6140f69 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersthecount
bugs1717891
milestone92.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1717891 - Spoc position pref r=thecount Differential Revision: https://phabricator.services.mozilla.com/D119435
browser/app/profile/firefox.js
browser/components/newtab/lib/DiscoveryStreamFeed.jsm
browser/components/newtab/lib/PrefsFeed.jsm
browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
browser/components/newtab/test/unit/lib/PrefsFeed.test.js
browser/components/newtab/test/unit/unit-entry.js
toolkit/components/nimbus/FeatureManifest.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1475,16 +1475,17 @@ pref("browser.newtabpage.activity-stream
 
 // The pref that controls if ASRouter uses the remote fluent files.
 // It's enabled by default, but could be disabled to force ASRouter to use the local files.
 pref("browser.newtabpage.activity-stream.asrouter.useRemoteL10n", true);
 
 // These prefs control if Discovery Stream is enabled.
 pref("browser.newtabpage.activity-stream.discoverystream.enabled", true);
 pref("browser.newtabpage.activity-stream.discoverystream.hardcoded-basic-layout", false);
+pref("browser.newtabpage.activity-stream.discoverystream.spoc-positions", "2,4,11,20");
 pref("browser.newtabpage.activity-stream.discoverystream.spocs-endpoint", "");
 pref("browser.newtabpage.activity-stream.discoverystream.spocs-endpoint-query", "");
 
 // List of regions that do not get stories, regardless of locale-list-config.
 pref("browser.newtabpage.activity-stream.discoverystream.region-stories-block", "FR");
 // List of locales that get stories, regardless of region-stories-config.
 #ifdef NIGHTLY_BUILD
   pref("browser.newtabpage.activity-stream.discoverystream.locale-list-config", "en-US,en-CA,en-GB");
--- a/browser/components/newtab/lib/DiscoveryStreamFeed.jsm
+++ b/browser/components/newtab/lib/DiscoveryStreamFeed.jsm
@@ -404,32 +404,63 @@ this.DiscoveryStreamFeed = class Discove
 
     for (let [key, val] of params.entries()) {
       urlObject.searchParams.append(key, val);
     }
 
     return urlObject.toString();
   }
 
+  parseSpocPositions(csvPositions) {
+    let spocPositions;
+
+    // Only accept parseable non-negative integers
+    try {
+      spocPositions = csvPositions.map(index => {
+        let parsedInt = parseInt(index, 10);
+
+        if (!isNaN(parsedInt) && parsedInt >= 0) {
+          return parsedInt;
+        }
+
+        throw new Error("Bad input");
+      });
+    } catch (e) {
+      // Catch spoc positions that are not numbers or negative, and do nothing.
+      // We have hard coded backup positions.
+      spocPositions = undefined;
+    }
+
+    return spocPositions;
+  }
+
   async loadLayout(sendUpdate, isStartup) {
     let layoutResp = {};
     let url = "";
+
     if (!this.config.hardcoded_layout) {
       layoutResp = await this.fetchLayout(isStartup);
     }
 
     if (!layoutResp || !layoutResp.layout) {
       const isBasic =
         this.config.hardcoded_basic_layout ||
         this.store.getState().Prefs.values[PREF_HARDCODED_BASIC_LAYOUT] ||
         this.store.getState().Prefs.values[PREF_REGION_BASIC_LAYOUT];
 
+      let spocPositions = this.store
+        .getState()
+        .Prefs.values?.pocketConfig?.spocPositions?.split(`,`);
+
       // Set a hardcoded layout if one is needed.
       // Changing values in this layout in memory object is unnecessary.
-      layoutResp = getHardcodedLayout(isBasic);
+      layoutResp = getHardcodedLayout(
+        isBasic,
+        this.parseSpocPositions(spocPositions)
+      );
     }
 
     sendUpdate({
       type: at.DISCOVERY_STREAM_LAYOUT_UPDATE,
       data: layoutResp,
       meta: {
         isStartup,
       },
@@ -1530,16 +1561,24 @@ this.DiscoveryStreamFeed = class Discove
       }
     });
 
     if (changed) {
       this.writeDataPref(pref, impressions);
     }
   }
 
+  onPocketConfigChanged() {
+    // Update layout, and reload any off screen tabs.
+    // This does not change any existing open tabs.
+    // It also doesn't update any spoc or rec data, just the layout.
+    const dispatch = action => this.store.dispatch(ac.AlsoToPreloaded(action));
+    this.loadLayout(dispatch, false);
+  }
+
   async onPrefChangedAction(action) {
     switch (action.data.name) {
       case PREF_CONFIG:
       case PREF_ENABLED:
       case PREF_HARDCODED_BASIC_LAYOUT:
       case PREF_SPOCS_ENDPOINT:
       case PREF_SPOCS_ENDPOINT_QUERY:
         // This is a config reset directly related to Discovery Stream pref.
@@ -1765,28 +1804,31 @@ this.DiscoveryStreamFeed = class Discove
           if (flight_id) {
             this.recordBlockFlightId(flight_id);
           }
         });
         break;
       }
       case at.PREF_CHANGED:
         await this.onPrefChangedAction(action);
+        if (action.data.name === "pocketConfig") {
+          await this.onPocketConfigChanged(action.data.value);
+        }
         break;
     }
   }
 };
 
 // This function generates a hardcoded layout each call.
 // This is because modifying the original object would
 // persist across pref changes and system_tick updates.
 //
 // NOTE: There is some branching logic in the template based on `isBasicLayout`
 //
-getHardcodedLayout = isBasicLayout => ({
+getHardcodedLayout = (isBasicLayout, spocPositions = [2, 4, 11, 20]) => ({
   lastUpdate: Date.now(),
   spocs: {
     url: "https://spocs.getpocket.com/spocs",
   },
   layout: [
     {
       width: 12,
       components: [
@@ -1862,30 +1904,19 @@ getHardcodedLayout = isBasicLayout => ({
           },
           feed: {
             embed_reference: null,
             url:
               "https://getpocket.cdn.mozilla.net/v3/firefox/global-recs?version=3&consumer_key=$apiKey&locale_lang=$locale&region=$region&count=30",
           },
           spocs: {
             probability: 1,
-            positions: [
-              {
-                index: 2,
-              },
-              {
-                index: 4,
-              },
-              {
-                index: 11,
-              },
-              {
-                index: 20,
-              },
-            ],
+            positions: spocPositions.map(position => {
+              return { index: position };
+            }),
           },
         },
         {
           type: "Navigation",
           properties: {
             alignment: "left-align",
             links: [
               {
--- a/browser/components/newtab/lib/PrefsFeed.jsm
+++ b/browser/components/newtab/lib/PrefsFeed.jsm
@@ -21,16 +21,17 @@ XPCOMUtils.defineLazyModuleGetters(this,
   Region: "resource://gre/modules/Region.jsm",
 });
 
 this.PrefsFeed = class PrefsFeed {
   constructor(prefMap) {
     this._prefMap = prefMap;
     this._prefs = new Prefs();
     this.onExperimentUpdated = this.onExperimentUpdated.bind(this);
+    this.onPocketExperimentUpdated = this.onPocketExperimentUpdated.bind(this);
   }
 
   onPrefChanged(name, value) {
     const prefItem = this._prefMap.get(name);
     if (prefItem) {
       this.store.dispatch(
         ac[prefItem.skipBroadcast ? "OnlyToMain" : "BroadcastToContent"]({
           type: at.PREF_CHANGED,
@@ -72,19 +73,36 @@ this.PrefsFeed = class PrefsFeed {
         data: {
           name: "featureConfig",
           value,
         },
       })
     );
   }
 
+  /**
+   * Handler for Pocket specific experiment data updates.
+   */
+  onPocketExperimentUpdated(event, reason) {
+    const value = NimbusFeatures.pocketNewtab.getAllVariables() || {};
+    this.store.dispatch(
+      ac.BroadcastToContent({
+        type: at.PREF_CHANGED,
+        data: {
+          name: "pocketConfig",
+          value,
+        },
+      })
+    );
+  }
+
   init() {
     this._prefs.observeBranch(this);
     NimbusFeatures.newtab.onUpdate(this.onExperimentUpdated);
+    NimbusFeatures.pocketNewtab.onUpdate(this.onPocketExperimentUpdated);
 
     this._storage = this.store.dbStorage.getDbTable("sectionPrefs");
 
     // Get the initial value of each activity stream pref
     const values = {};
     for (const name of this._prefMap.keys()) {
       values[name] = this._prefs.get(name);
     }
@@ -150,16 +168,17 @@ this.PrefsFeed = class PrefsFeed {
     );
     values["urlbar.placeholderName"] = placeholderPrefValue;
     this._prefMap.set("urlbar.placeholderName", {
       value: placeholderPrefValue,
     });
 
     // Add experiment values and default values
     values.featureConfig = NimbusFeatures.newtab.getValue() || {};
+    values.pocketConfig = NimbusFeatures.pocketNewtab.getAllVariables() || {};
     this._setBoolPref(values, "logowordmark.alwaysVisible", false);
     this._setBoolPref(values, "feeds.section.topstories", false);
     this._setBoolPref(values, "discoverystream.enabled", false);
     this._setBoolPref(values, "discoverystream.isCollectionDismissible", false);
     this._setBoolPref(values, "discoverystream.hardcoded-basic-layout", false);
     this._setBoolPref(values, "discoverystream.recs.personalized", false);
     this._setBoolPref(values, "discoverystream.spocs.personalized", false);
     this._setStringPref(
--- a/browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
+++ b/browser/components/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
@@ -224,16 +224,28 @@ describe("DiscoveryStreamFeed", () => {
 
       const result = feed.getOrCreateImpressionId();
 
       assert.equal(result, "from get");
       assert.calledOnce(global.Services.prefs.getCharPref);
     });
   });
 
+  describe("#parseSpocPositions", () => {
+    it("should return an equivalent array for an array of non negative integers", async () => {
+      assert.deepEqual(feed.parseSpocPositions([0, 2, 3]), [0, 2, 3]);
+    });
+    it("should return undefined for an array containing negative integers", async () => {
+      assert.equal(feed.parseSpocPositions([-2, 2, 3]), undefined);
+    });
+    it("should return undefined for an undefined input", async () => {
+      assert.equal(feed.parseSpocPositions(undefined), undefined);
+    });
+  });
+
   describe("#loadLayout", () => {
     it("should fetch data and populate the cache if it is empty", async () => {
       const resp = { layout: ["foo", "bar"] };
       const fakeCache = {};
       sandbox.stub(feed.cache, "get").returns(Promise.resolve(fakeCache));
       sandbox.stub(feed.cache, "set").returns(Promise.resolve());
 
       fetchStub.resolves({ ok: true, json: () => Promise.resolve(resp) });
@@ -2077,16 +2089,26 @@ describe("DiscoveryStreamFeed", () => {
 
       await feed.onAction({
         type: at.PREF_CHANGED,
         data: { name: "showSponsored" },
       });
 
       assert.calledOnce(feed.loadSpocs);
     });
+    it("should fire onPocketConfigChanged when pocketConfig pref changes", async () => {
+      sandbox.stub(feed, "onPocketConfigChanged").returns(Promise.resolve());
+
+      await feed.onAction({
+        type: at.PREF_CHANGED,
+        data: { name: "pocketConfig", value: false },
+      });
+
+      assert.calledOnce(feed.onPocketConfigChanged);
+    });
     it("should call clearSpocs when sponsored content is turned off", async () => {
       sandbox.stub(feed, "clearSpocs").returns(Promise.resolve());
 
       await feed.onAction({
         type: at.PREF_CHANGED,
         data: { name: "showSponsored", value: false },
       });
 
@@ -2162,16 +2184,26 @@ describe("DiscoveryStreamFeed", () => {
       sandbox.stub(feed, "checkIfAnyCacheExpired").resolves(true);
       sandbox.stub(feed, "refreshAll").resolves();
 
       await feed.onAction({ type: at.SYSTEM_TICK });
       assert.calledWith(feed.refreshAll, { updateOpenTabs: false });
     });
   });
 
+  describe("#onPocketConfigChanged", () => {
+    it("should call loadLayout when Pocket config changes", async () => {
+      sandbox.stub(feed, "loadLayout").callsFake(dispatch => dispatch("foo"));
+      sandbox.stub(feed.store, "dispatch");
+      await feed.onPocketConfigChanged();
+      assert.calledOnce(feed.loadLayout);
+      assert.calledWith(feed.store.dispatch, ac.AlsoToPreloaded("foo"));
+    });
+  });
+
   describe("#onAction: PREF_SHOW_SPONSORED", () => {
     it("should call loadSpocs when preference changes", async () => {
       sandbox.stub(feed, "loadSpocs").resolves();
       sandbox.stub(feed.store, "dispatch");
 
       await feed.onAction({
         type: at.PREF_CHANGED,
         data: { name: "showSponsored" },
--- a/browser/components/newtab/test/unit/lib/PrefsFeed.test.js
+++ b/browser/components/newtab/test/unit/lib/PrefsFeed.test.js
@@ -148,16 +148,34 @@ describe("PrefsFeed", () => {
     assert.calledWith(
       feed.store.dispatch,
       ac.BroadcastToContent({
         type: at.PREF_CHANGED,
         data: { name: "foo", value: 2 },
       })
     );
   });
+  it("should send a PREF_CHANGED actions when onPocketExperimentUpdated is called", () => {
+    sandbox.stub(global.NimbusFeatures.newtab, "getAllVariables").returns({
+      prefsButtonIcon: "icon-new",
+    });
+    feed.onPocketExperimentUpdated();
+    assert.calledWith(
+      feed.store.dispatch,
+      ac.BroadcastToContent({
+        type: at.PREF_CHANGED,
+        data: {
+          name: "pocketConfig",
+          value: {
+            prefsButtonIcon: "icon-new",
+          },
+        },
+      })
+    );
+  });
   it("should send a PREF_CHANGED actions when onExperimentUpdated is called", () => {
     sandbox.stub(global.NimbusFeatures.newtab, "getValue").returns({
       prefsButtonIcon: "icon-new",
     });
     feed.onExperimentUpdated();
     assert.calledWith(
       feed.store.dispatch,
       ac.BroadcastToContent({
--- a/browser/components/newtab/test/unit/unit-entry.js
+++ b/browser/components/newtab/test/unit/unit-entry.js
@@ -461,16 +461,17 @@ const TEST_GLOBAL = {
     getExperiment() {},
     on: () => {},
     off: () => {},
   },
   NimbusFeatures: {
     newtab: {
       isEnabled() {},
       getValue() {},
+      getAllVariables() {},
       onUpdate() {},
       off() {},
     },
   },
   TelemetryEnvironment: {
     setExperimentActive() {},
     currentEnvironment: {
       profile: {
@@ -504,14 +505,15 @@ const TEST_GLOBAL = {
   PageThumbs: {
     addExpirationFilter() {},
     removeExpirationFilter() {},
   },
   gUUIDGenerator: {
     generateUUID: () => "{foo-123-foo}",
   },
 };
+TEST_GLOBAL.NimbusFeatures.pocketNewtab = TEST_GLOBAL.NimbusFeatures.newtab;
 overrider.set(TEST_GLOBAL);
 
 describe("activity-stream", () => {
   after(() => overrider.restore());
   files.forEach(file => req(file));
 });
--- a/toolkit/components/nimbus/FeatureManifest.js
+++ b/toolkit/components/nimbus/FeatureManifest.js
@@ -108,16 +108,28 @@ const FeatureManifest = {
         description: "Enable the customization panel inside of the newtab",
       },
       prefsButtonIcon: {
         type: "string",
         description: "Icon url to use for the preferences button",
       },
     },
   },
+  pocketNewtab: {
+    description: "The Pocket section in newtab",
+    isEarlyStartup: true,
+    variables: {
+      spocPositions: {
+        type: "string",
+        fallbackPref:
+          "browser.newtabpage.activity-stream.discoverystream.spoc-positions",
+        description: "CSV string of spoc position indexes on newtab grid",
+      },
+    },
+  },
   "password-autocomplete": {
     description: "A special autocomplete UI for password fields.",
     variables: {
       directMigrateSingleProfile: {
         type: "boolean",
         description: "Enable direct migration?",
       },
     },