commit e1735fa63df22d4963ccce6e1ec3f65111aae303
parent 9dd79a9a56ba1447c463a429e4e835a5c9cd345e
Author: scottdowne <sdowne@mozilla.com>
Date: Thu, 2 Oct 2025 20:20:02 +0000
Bug 1990585 - Newtab refactor discovery stream feed setupSpocsCacheUpdateTime r=nbarrett
Differential Revision: https://phabricator.services.mozilla.com/D266087
Diffstat:
2 files changed, 81 insertions(+), 82 deletions(-)
diff --git a/browser/extensions/newtab/lib/DiscoveryStreamFeed.sys.mjs b/browser/extensions/newtab/lib/DiscoveryStreamFeed.sys.mjs
@@ -358,8 +358,6 @@ export class DiscoveryStreamFeed {
const nimbusConfig = this.store.getState().Prefs.values?.pocketConfig || {};
const { region } = this.store.getState().Prefs.values;
- this.setupSpocsCacheUpdateTime();
-
const hideDescriptionsRegions = nimbusConfig.hideDescriptionsRegions
?.split(",")
.map(s => s.trim());
@@ -529,31 +527,27 @@ export class DiscoveryStreamFeed {
}
get spocsCacheUpdateTime() {
- if (this._spocsCacheUpdateTime) {
- return this._spocsCacheUpdateTime;
+ if (this._spocsCacheUpdateTime === undefined) {
+ const spocsCacheTimeout =
+ this.store.getState().Prefs.values[PREF_SPOCS_CACHE_TIMEOUT];
+ const MAX_TIMEOUT = 30;
+ const MIN_TIMEOUT = 5;
+ // We do a bit of min max checking the configured value is between
+ // 5 and 30 minutes, to protect against unreasonable values.
+ if (
+ spocsCacheTimeout &&
+ spocsCacheTimeout <= MAX_TIMEOUT &&
+ spocsCacheTimeout >= MIN_TIMEOUT
+ ) {
+ // This value is in minutes, but we want ms.
+ this._spocsCacheUpdateTime = spocsCacheTimeout * 60 * 1000;
+ } else {
+ // The const is already in ms.
+ this._spocsCacheUpdateTime = SPOCS_FEEDS_UPDATE_TIME;
+ }
}
- this.setupSpocsCacheUpdateTime();
- return this._spocsCacheUpdateTime;
- }
- setupSpocsCacheUpdateTime() {
- const spocsCacheTimeout =
- this.store.getState().Prefs.values[PREF_SPOCS_CACHE_TIMEOUT];
- const MAX_TIMEOUT = 30;
- const MIN_TIMEOUT = 5;
- // We do a bit of min max checking the configured value is between
- // 5 and 30 minutes, to protect against unreasonable values.
- if (
- spocsCacheTimeout &&
- spocsCacheTimeout <= MAX_TIMEOUT &&
- spocsCacheTimeout >= MIN_TIMEOUT
- ) {
- // This value is in minutes, but we want ms.
- this._spocsCacheUpdateTime = spocsCacheTimeout * 60 * 1000;
- } else {
- // The const is already in ms.
- this._spocsCacheUpdateTime = SPOCS_FEEDS_UPDATE_TIME;
- }
+ return this._spocsCacheUpdateTime;
}
/**
@@ -572,6 +566,7 @@ export class DiscoveryStreamFeed {
const EXPIRATION_TIME = isStartup
? STARTUP_CACHE_EXPIRE_TIME
: updateTimePerComponent[key];
+
switch (key) {
case "spocs":
return !spocs || !(Date.now() - spocs.lastUpdated < EXPIRATION_TIME);
@@ -590,6 +585,7 @@ export class DiscoveryStreamFeed {
async _checkExpirationPerComponent() {
const cachedData = (await this.cache.get()) || {};
const { feeds } = cachedData;
+
return {
spocs: this.showSpocs && this.isExpired({ cachedData, key: "spocs" }),
feeds:
@@ -601,14 +597,6 @@ export class DiscoveryStreamFeed {
};
}
- /**
- * Returns true if any data for the cached endpoints has expired or is missing.
- */
- async checkIfAnyCacheExpired() {
- const expirationPerComponent = await this._checkExpirationPerComponent();
- return expirationPerComponent.spocs || expirationPerComponent.feeds;
- }
-
updatePlacements(sendUpdate, layout, isStartup = false) {
const placements = [];
const placementsMap = {};
@@ -2615,6 +2603,20 @@ export class DiscoveryStreamFeed {
);
}
+ async onSystemTick() {
+ // Only refresh when enabled and after initial load has completed.
+ if (!this.config.enabled || !this.loaded) {
+ return;
+ }
+
+ const expirationPerComponent = await this._checkExpirationPerComponent();
+ if (expirationPerComponent.feeds || expirationPerComponent.spocs) {
+ await this.refreshAll({ updateOpenTabs: false });
+ }
+ }
+
+ async onTrainhopConfigChanged() {}
+
async onPrefChangedAction(action) {
switch (action.data.name) {
case PREF_CONFIG:
@@ -2703,6 +2705,14 @@ export class DiscoveryStreamFeed {
break;
}
}
+
+ if (action.data.name === "pocketConfig") {
+ await this.onPrefChange();
+ this.setupPrefs(false /* isStartup */);
+ }
+ if (action.data.name === "trainhopConfig") {
+ await this.onTrainhopConfigChanged(action);
+ }
}
async onAction(action) {
@@ -2728,14 +2738,7 @@ export class DiscoveryStreamFeed {
break;
case at.DISCOVERY_STREAM_DEV_SYSTEM_TICK:
case at.SYSTEM_TICK:
- // Only refresh if we loaded once in .enable()
- if (
- this.config.enabled &&
- this.loaded &&
- (await this.checkIfAnyCacheExpired())
- ) {
- await this.refreshAll({ updateOpenTabs: false });
- }
+ await this.onSystemTick();
break;
case at.DISCOVERY_STREAM_DEV_SYNC_RS:
lazy.RemoteSettings.pollChanges();
@@ -2968,10 +2971,6 @@ export class DiscoveryStreamFeed {
}
case at.PREF_CHANGED:
await this.onPrefChangedAction(action);
- if (action.data.name === "pocketConfig") {
- await this.onPrefChange();
- this.setupPrefs(false /* isStartup */);
- }
break;
case at.TOPIC_SELECTION_IMPRESSION:
this.topicSelectionImpressionEvent();
diff --git a/browser/extensions/newtab/test/unit/lib/DiscoveryStreamFeed.test.js b/browser/extensions/newtab/test/unit/lib/DiscoveryStreamFeed.test.js
@@ -2651,7 +2651,7 @@ describe("DiscoveryStreamFeed", () => {
await feed.onAction({ type: at.INIT });
- sandbox.stub(feed, "checkIfAnyCacheExpired").resolves(false);
+ sandbox.stub(feed, "onSystemTick").resolves();
sandbox.stub(feed, "refreshAll").resolves();
await feed.onAction({ type: at.SYSTEM_TICK });
@@ -2664,7 +2664,6 @@ describe("DiscoveryStreamFeed", () => {
await feed.onAction({ type: at.INIT });
- sandbox.stub(feed, "checkIfAnyCacheExpired").resolves(true);
sandbox.stub(feed, "refreshAll").resolves();
await feed.onAction({ type: at.SYSTEM_TICK });
@@ -2677,7 +2676,6 @@ describe("DiscoveryStreamFeed", () => {
await feed.onAction({ type: at.INIT });
- sandbox.stub(feed, "checkIfAnyCacheExpired").resolves(true);
sandbox.stub(feed, "refreshAll").resolves();
await feed.onAction({ type: at.SYSTEM_TICK });
@@ -2755,7 +2753,6 @@ describe("DiscoveryStreamFeed", () => {
await feed.onAction({ type: at.INIT });
- sandbox.stub(feed, "checkIfAnyCacheExpired").resolves(true);
sandbox.stub(feed, "refreshAll").resolves();
await feed.onAction({ type: at.DISCOVERY_STREAM_DEV_SYSTEM_TICK });
@@ -2774,31 +2771,18 @@ describe("DiscoveryStreamFeed", () => {
});
describe("#spocsCacheUpdateTime", () => {
- it("should call setupSpocsCacheUpdateTime", () => {
+ it("should return default cache time", () => {
const defaultCacheTime = 30 * 60 * 1000;
- sandbox.spy(feed, "setupSpocsCacheUpdateTime");
const cacheTime = feed.spocsCacheUpdateTime;
assert.equal(feed._spocsCacheUpdateTime, defaultCacheTime);
assert.equal(cacheTime, defaultCacheTime);
- assert.calledOnce(feed.setupSpocsCacheUpdateTime);
});
it("should return _spocsCacheUpdateTime", () => {
- sandbox.spy(feed, "setupSpocsCacheUpdateTime");
const testCacheTime = 123;
feed._spocsCacheUpdateTime = testCacheTime;
const cacheTime = feed.spocsCacheUpdateTime;
- // Ensure _spocsCacheUpdateTime was not changed.
assert.equal(feed._spocsCacheUpdateTime, testCacheTime);
assert.equal(cacheTime, testCacheTime);
- assert.notCalled(feed.setupSpocsCacheUpdateTime);
- });
- });
-
- describe("#setupSpocsCacheUpdateTime", () => {
- it("should set _spocsCacheUpdateTime with default value", () => {
- const defaultCacheTime = 30 * 60 * 1000;
- feed.setupSpocsCacheUpdateTime();
- assert.equal(feed._spocsCacheUpdateTime, defaultCacheTime);
});
it("should set _spocsCacheUpdateTime with min", () => {
const defaultCacheTime = 30 * 60 * 1000;
@@ -2809,8 +2793,9 @@ describe("DiscoveryStreamFeed", () => {
},
},
});
- feed.setupSpocsCacheUpdateTime();
+ const cacheTime = feed.spocsCacheUpdateTime;
assert.equal(feed._spocsCacheUpdateTime, defaultCacheTime);
+ assert.equal(cacheTime, defaultCacheTime);
});
it("should set _spocsCacheUpdateTime with max", () => {
const defaultCacheTime = 30 * 60 * 1000;
@@ -2821,8 +2806,9 @@ describe("DiscoveryStreamFeed", () => {
},
},
});
- feed.setupSpocsCacheUpdateTime();
+ const cacheTime = feed.spocsCacheUpdateTime;
assert.equal(feed._spocsCacheUpdateTime, defaultCacheTime);
+ assert.equal(cacheTime, defaultCacheTime);
});
it("should set _spocsCacheUpdateTime with spocsCacheTimeout", () => {
feed.store.getState = () => ({
@@ -2832,8 +2818,9 @@ describe("DiscoveryStreamFeed", () => {
},
},
});
- feed.setupSpocsCacheUpdateTime();
+ const cacheTime = feed.spocsCacheUpdateTime;
assert.equal(feed._spocsCacheUpdateTime, 20 * 60 * 1000);
+ assert.equal(cacheTime, 20 * 60 * 1000);
});
});
@@ -2873,7 +2860,7 @@ describe("DiscoveryStreamFeed", () => {
});
});
- describe("#checkIfAnyCacheExpired", () => {
+ describe("#_checkExpirationPerComponent", () => {
let cache;
beforeEach(() => {
cache = {
@@ -2885,35 +2872,48 @@ describe("DiscoveryStreamFeed", () => {
});
it("should return false if nothing in the cache is expired", async () => {
- const result = await feed.checkIfAnyCacheExpired();
- assert.isFalse(result);
+ const results = await feed._checkExpirationPerComponent();
+ assert.isFalse(results.spocs);
+ assert.isFalse(results.feeds);
});
it("should return true if .spocs is missing", async () => {
delete cache.spocs;
- assert.isTrue(await feed.checkIfAnyCacheExpired());
- });
- it("should return true if .spocs is expired", async () => {
- clock.tick(THIRTY_MINUTES + 1);
- // Update other caches we aren't testing
- cache.spocs.lastUpdated = Date.now();
- cache.feeds["foo.com"].lastUpdate = Date.now();
- assert.isTrue(await feed.checkIfAnyCacheExpired());
+ const results = await feed._checkExpirationPerComponent();
+ assert.isTrue(results.spocs);
+ assert.isFalse(results.feeds);
});
-
it("should return true if .feeds is missing", async () => {
delete cache.feeds;
- assert.isTrue(await feed.checkIfAnyCacheExpired());
+
+ const results = await feed._checkExpirationPerComponent();
+ assert.isFalse(results.spocs);
+ assert.isTrue(results.feeds);
+ });
+ it("should return true if spocs are expired", async () => {
+ clock.tick(THIRTY_MINUTES + 1);
+ // Update other caches we aren't testing
+ cache.feeds["foo.com"].lastUpdated = Date.now();
+
+ const results = await feed._checkExpirationPerComponent();
+ assert.isTrue(results.spocs);
+ assert.isFalse(results.feeds);
});
it("should return true if data for .feeds[url] is missing", async () => {
cache.feeds["foo.com"] = null;
- assert.isTrue(await feed.checkIfAnyCacheExpired());
+
+ const results = await feed._checkExpirationPerComponent();
+ assert.isFalse(results.spocs);
+ assert.isTrue(results.feeds);
});
it("should return true if data for .feeds[url] is expired", async () => {
clock.tick(THIRTY_MINUTES + 1);
// Update other caches we aren't testing
- cache.spocs.lastUpdate = Date.now();
- assert.isTrue(await feed.checkIfAnyCacheExpired());
+ cache.spocs.lastUpdated = Date.now();
+
+ const results = await feed._checkExpirationPerComponent();
+ assert.isFalse(results.spocs);
+ assert.isTrue(results.feeds);
});
});
