Bug 1717891 - Spoc position pref r=thecount
☠☠ backed out by bb240664f5cb ☠ ☠
authorGavin Lazar Suntop <gavin@gsuntop.com>
Fri, 23 Jul 2021 01:07:38 +0000
changeset 586512 cfd1a5096364a4a03e0dc04277f27cff357ea432
parent 586511 9bbaeeac02061196dd56b394843e05897ead1e2b
child 586513 42fcc35abbd8a2caddd5b8d3932501e30d977669
push id146778
push usergsuntop@getpocket.com
push dateFri, 23 Jul 2021 03:41:45 +0000
treeherderautoland@cfd1a5096364 [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,27 @@ 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",
+      },
+    },
+  },
   "password-autocomplete": {
     description: "A special autocomplete UI for password fields.",
     variables: {
       directMigrateSingleProfile: {
         type: "boolean",
         description: "Enable direct migration?",
       },
     },