commit dc73d30c7a031a4734fafe7da32325cd921feb27 parent 2ff5a963f6bf2766ec04778adb367fc5e5d7b9ad Author: scottdowne <sdowne@mozilla.com> Date: Fri, 3 Oct 2025 20:08:38 +0000 Bug 1989148 - Newtab spocs cache time updates r=home-newtab-reviewers,nbarrett Differential Revision: https://phabricator.services.mozilla.com/D265207 Diffstat:
14 files changed, 482 insertions(+), 40 deletions(-)
diff --git a/browser/extensions/newtab/common/Actions.mjs b/browser/extensions/newtab/common/Actions.mjs @@ -77,6 +77,9 @@ for (const type of [ "DISCOVERY_STREAM_RETRY_FEED", "DISCOVERY_STREAM_SPOCS_CAPS", "DISCOVERY_STREAM_SPOCS_ENDPOINT", + "DISCOVERY_STREAM_SPOCS_ONDEMAND_LOAD", + "DISCOVERY_STREAM_SPOCS_ONDEMAND_RESET", + "DISCOVERY_STREAM_SPOCS_ONDEMAND_UPDATE", "DISCOVERY_STREAM_SPOCS_PLACEMENTS", "DISCOVERY_STREAM_SPOCS_UPDATE", "DISCOVERY_STREAM_SPOC_BLOCKED", diff --git a/browser/extensions/newtab/common/Reducers.sys.mjs b/browser/extensions/newtab/common/Reducers.sys.mjs @@ -87,6 +87,11 @@ export const INITIAL_STATE = { spocs: { spocs_endpoint: "", lastUpdated: null, + cacheUpdateTime: null, + onDemand: { + enabled: false, + loaded: false, + }, data: { // "spocs": {title: "", context: "", items: [], personalized: false}, // "placement1": {title: "", context: "", items: [], personalized: false}, @@ -821,17 +826,53 @@ function DiscoveryStream(prevState = INITIAL_STATE.DiscoveryStream, action) { }; case at.DISCOVERY_STREAM_SPOCS_UPDATE: if (action.data) { + // If spocs have been loaded on this tab, we can ignore future updates. + // This should never be true on the main store, only content pages. + if (prevState.spocs.onDemand.loaded) { + return prevState; + } return { ...prevState, spocs: { ...prevState.spocs, lastUpdated: action.data.lastUpdated, data: action.data.spocs, + cacheUpdateTime: action.data.spocsCacheUpdateTime, + onDemand: { + enabled: action.data.spocsOnDemand, + loaded: false, + }, loaded: true, }, }; } return prevState; + case at.DISCOVERY_STREAM_SPOCS_ONDEMAND_LOAD: + return { + ...prevState, + spocs: { + ...prevState.spocs, + onDemand: { + ...prevState.spocs.onDemand, + loaded: true, + }, + }, + }; + case at.DISCOVERY_STREAM_SPOCS_ONDEMAND_RESET: + if (action.data) { + return { + ...prevState, + spocs: { + ...prevState.spocs, + cacheUpdateTime: action.data.spocsCacheUpdateTime, + onDemand: { + ...prevState.spocs.onDemand, + enabled: action.data.spocsOnDemand, + }, + }, + }; + } + return prevState; case at.DISCOVERY_STREAM_SPOC_BLOCKED: return { ...prevState, diff --git a/browser/extensions/newtab/content-src/components/Base/Base.jsx b/browser/extensions/newtab/content-src/components/Base/Base.jsx @@ -119,6 +119,7 @@ export class BaseContent extends React.PureComponent { fixedNavStyle: {}, wallpaperTheme: "", showDownloadHighlightOverride: null, + visible: false, }; } @@ -131,8 +132,70 @@ export class BaseContent extends React.PureComponent { } onVisible() { + this.setState({ + visible: true, + }); this.setFirstVisibleTimestamp(); this.shouldDisplayTopicSelectionModal(); + this.onVisibilityDispatch(); + } + + onVisibilityDispatch() { + const { onDemand = {} } = this.props.DiscoveryStream.spocs; + + // We only need to dispatch this if: + // 1. onDemand is enabled, + // 2. onDemand spocs have not been loaded on this tab. + // 3. Spocs are expired. + if (onDemand.enabled && !onDemand.loaded && this.isSpocsOnDemandExpired) { + // This dispatches that spocs are expired and we need to update them. + this.props.dispatch( + ac.OnlyToMain({ + type: at.DISCOVERY_STREAM_SPOCS_ONDEMAND_UPDATE, + }) + ); + } + } + + get isSpocsOnDemandExpired() { + const { + onDemand = {}, + cacheUpdateTime, + lastUpdated, + } = this.props.DiscoveryStream.spocs; + + // We can bail early if: + // 1. onDemand is off, + // 2. onDemand spocs have been loaded on this tab. + if (!onDemand.enabled || onDemand.loaded) { + return false; + } + + return Date.now() - lastUpdated >= cacheUpdateTime; + } + + spocsOnDemandUpdated() { + const { onDemand = {}, loaded } = this.props.DiscoveryStream.spocs; + + // We only need to fire this if: + // 1. Spoc data is loaded. + // 2. onDemand is enabled. + // 3. The component is visible (not preloaded tab). + // 4. onDemand spocs have not been loaded on this tab. + // 5. Spocs are not expired. + if ( + loaded && + onDemand.enabled && + this.state.visible && + !onDemand.loaded && + !this.isSpocsOnDemandExpired + ) { + // This dispatches that spocs have been loaded on this tab + // and we don't need to update them again for this tab. + this.props.dispatch( + ac.BroadcastToContent({ type: at.DISCOVERY_STREAM_SPOCS_ONDEMAND_LOAD }) + ); + } } componentDidMount() { @@ -222,6 +285,8 @@ export class BaseContent extends React.PureComponent { this.updateWallpaper(); } } + + this.spocsOnDemandUpdated(); } handleColorModeChange() { @@ -800,6 +865,7 @@ export class BaseContent extends React.PureComponent { <DiscoveryStreamBase locale={props.App.locale} firstVisibleTimestamp={this.state.firstVisibleTimestamp} + placeholder={this.isSpocsOnDemandExpired} /> </ErrorBoundary> ) : ( diff --git a/browser/extensions/newtab/content-src/components/DiscoveryStreamBase/DiscoveryStreamBase.jsx b/browser/extensions/newtab/content-src/components/DiscoveryStreamBase/DiscoveryStreamBase.jsx @@ -176,6 +176,7 @@ export class _DiscoveryStreamBase extends React.PureComponent { firstVisibleTimestamp={this.props.firstVisibleTimestamp} ctaButtonSponsors={component.properties.ctaButtonSponsors} ctaButtonVariant={component.properties.ctaButtonVariant} + placeholder={this.props.placeholder} /> ); } @@ -197,6 +198,7 @@ export class _DiscoveryStreamBase extends React.PureComponent { hideDescriptions={this.props.DiscoveryStream.hideDescriptions} firstVisibleTimestamp={this.props.firstVisibleTimestamp} spocPositions={component.spocs?.positions} + placeholder={this.props.placeholder} /> ); } diff --git a/browser/extensions/newtab/content-src/components/DiscoveryStreamComponents/CardGrid/CardGrid.jsx b/browser/extensions/newtab/content-src/components/DiscoveryStreamComponents/CardGrid/CardGrid.jsx @@ -114,6 +114,7 @@ export class _CardGrid extends React.PureComponent { const rec = recs[index]; cards.push( topicsLoading || + this.props.placeholder || !rec || rec.placeholder || (rec.flight_id && diff --git a/browser/extensions/newtab/content-src/components/DiscoveryStreamComponents/CardSections/CardSections.jsx b/browser/extensions/newtab/content-src/components/DiscoveryStreamComponents/CardSections/CardSections.jsx @@ -150,6 +150,7 @@ function CardSection({ ctaButtonSponsors, anySectionsFollowed, showWeather, + placeholder, }) { const prefs = useSelector(state => state.Prefs.values); @@ -258,7 +259,13 @@ function CardSection({ ); }, [dispatch, sectionPersonalization, sectionKey, sectionPosition]); - const { maxTile } = getMaxTiles(responsiveLayouts); + let { maxTile } = getMaxTiles(responsiveLayouts); + if (placeholder) { + // We need a number that divides evenly by 2, 3, and 4. + // So it can be displayed without orphans in grids with 2, 3, and 4 columns. + maxTile = 12; + } + const displaySections = section.data.slice(0, maxTile); const isSectionEmpty = !displaySections?.length; const shouldShowLabels = sectionKey === "top_stories_section" && showTopics; @@ -369,6 +376,7 @@ function CardSection({ if ( !rec || rec.placeholder || + placeholder || (rec.flight_id && !spocsStartupCacheEnabled && isForStartupCache.DiscoveryStream) @@ -447,6 +455,7 @@ function CardSections({ firstVisibleTimestamp, ctaButtonVariant, ctaButtonSponsors, + placeholder, }) { const prefs = useSelector(state => state.Prefs.values); const { spocs, sectionPersonalization } = useSelector( @@ -474,7 +483,25 @@ function CardSections({ sectionPersonalization && Object.values(sectionPersonalization).some(section => section?.isFollowed); - let filteredSections = data.sections.filter( + let sectionsData = data.sections; + + if (placeholder) { + // To clean up the placeholder state for sections if the whole section is loading still. + sectionsData = [ + { + ...sectionsData[0], + title: "", + subtitle: "", + }, + { + ...sectionsData[1], + title: "", + subtitle: "", + }, + ]; + } + + let filteredSections = sectionsData.filter( section => !sectionPersonalization[section.sectionKey]?.isBlocked ); @@ -501,6 +528,7 @@ function CardSections({ ctaButtonVariant={ctaButtonVariant} ctaButtonSponsors={ctaButtonSponsors} anySectionsFollowed={anySectionsFollowed} + placeholder={placeholder} showWeather={ weatherEnabled && weatherPlacement === "section" && diff --git a/browser/extensions/newtab/data/content/activity-stream.bundle.js b/browser/extensions/newtab/data/content/activity-stream.bundle.js @@ -150,6 +150,9 @@ for (const type of [ "DISCOVERY_STREAM_RETRY_FEED", "DISCOVERY_STREAM_SPOCS_CAPS", "DISCOVERY_STREAM_SPOCS_ENDPOINT", + "DISCOVERY_STREAM_SPOCS_ONDEMAND_LOAD", + "DISCOVERY_STREAM_SPOCS_ONDEMAND_RESET", + "DISCOVERY_STREAM_SPOCS_ONDEMAND_UPDATE", "DISCOVERY_STREAM_SPOCS_PLACEMENTS", "DISCOVERY_STREAM_SPOCS_UPDATE", "DISCOVERY_STREAM_SPOC_BLOCKED", @@ -5382,7 +5385,7 @@ class _CardGrid extends (external_React_default()).PureComponent { const cards = []; for (let index = 0; index < items; index++) { const rec = recs[index]; - cards.push(topicsLoading || !rec || rec.placeholder || rec.flight_id && !spocsStartupCacheEnabled && this.props.App.isForStartupCache.DiscoveryStream ? /*#__PURE__*/external_React_default().createElement(PlaceholderDSCard, { + cards.push(topicsLoading || this.props.placeholder || !rec || rec.placeholder || rec.flight_id && !spocsStartupCacheEnabled && this.props.App.isForStartupCache.DiscoveryStream ? /*#__PURE__*/external_React_default().createElement(PlaceholderDSCard, { key: `dscard-${index}` }) : /*#__PURE__*/external_React_default().createElement(DSCard, { key: `dscard-${rec.id}`, @@ -7406,6 +7409,11 @@ const INITIAL_STATE = { spocs: { spocs_endpoint: "", lastUpdated: null, + cacheUpdateTime: null, + onDemand: { + enabled: false, + loaded: false, + }, data: { // "spocs": {title: "", context: "", items: [], personalized: false}, // "placement1": {title: "", context: "", items: [], personalized: false}, @@ -8140,17 +8148,53 @@ function DiscoveryStream(prevState = INITIAL_STATE.DiscoveryStream, action) { }; case actionTypes.DISCOVERY_STREAM_SPOCS_UPDATE: if (action.data) { + // If spocs have been loaded on this tab, we can ignore future updates. + // This should never be true on the main store, only content pages. + if (prevState.spocs.onDemand.loaded) { + return prevState; + } return { ...prevState, spocs: { ...prevState.spocs, lastUpdated: action.data.lastUpdated, data: action.data.spocs, + cacheUpdateTime: action.data.spocsCacheUpdateTime, + onDemand: { + enabled: action.data.spocsOnDemand, + loaded: false, + }, loaded: true, }, }; } return prevState; + case actionTypes.DISCOVERY_STREAM_SPOCS_ONDEMAND_LOAD: + return { + ...prevState, + spocs: { + ...prevState.spocs, + onDemand: { + ...prevState.spocs.onDemand, + loaded: true, + }, + }, + }; + case actionTypes.DISCOVERY_STREAM_SPOCS_ONDEMAND_RESET: + if (action.data) { + return { + ...prevState, + spocs: { + ...prevState.spocs, + cacheUpdateTime: action.data.spocsCacheUpdateTime, + onDemand: { + ...prevState.spocs.onDemand, + enabled: action.data.spocsOnDemand, + }, + }, + }; + } + return prevState; case actionTypes.DISCOVERY_STREAM_SPOC_BLOCKED: return { ...prevState, @@ -12135,7 +12179,8 @@ function CardSection({ ctaButtonVariant, ctaButtonSponsors, anySectionsFollowed, - showWeather + showWeather, + placeholder }) { const prefs = (0,external_ReactRedux_namespaceObject.useSelector)(state => state.Prefs.values); const { @@ -12229,9 +12274,14 @@ function CardSection({ } })); }, [dispatch, sectionPersonalization, sectionKey, sectionPosition]); - const { + let { maxTile } = getMaxTiles(responsiveLayouts); + if (placeholder) { + // We need a number that divides evenly by 2, 3, and 4. + // So it can be displayed without orphans in grids with 2, 3, and 4 columns. + maxTile = 12; + } const displaySections = section.data.slice(0, maxTile); const isSectionEmpty = !displaySections?.length; const shouldShowLabels = sectionKey === "top_stories_section" && showTopics; @@ -12310,7 +12360,7 @@ function CardSection({ // 1. No recommendation is available. // 2. The item is flagged as a placeholder. // 3. Spocs are loading for with spocs startup cache disabled. - if (!rec || rec.placeholder || rec.flight_id && !spocsStartupCacheEnabled && isForStartupCache.DiscoveryStream) { + if (!rec || rec.placeholder || placeholder || rec.flight_id && !spocsStartupCacheEnabled && isForStartupCache.DiscoveryStream) { return /*#__PURE__*/external_React_default().createElement(PlaceholderDSCard, { key: `dscard-${index}` }); @@ -12377,7 +12427,8 @@ function CardSections({ type, firstVisibleTimestamp, ctaButtonVariant, - ctaButtonSponsors + ctaButtonSponsors, + placeholder }) { const prefs = (0,external_ReactRedux_namespaceObject.useSelector)(state => state.Prefs.values); const { @@ -12404,7 +12455,20 @@ function CardSections({ // Used to determine if we should show FollowSectionButtonHighlight const anySectionsFollowed = sectionPersonalization && Object.values(sectionPersonalization).some(section => section?.isFollowed); - let filteredSections = data.sections.filter(section => !sectionPersonalization[section.sectionKey]?.isBlocked); + let sectionsData = data.sections; + if (placeholder) { + // To clean up the placeholder state for sections if the whole section is loading still. + sectionsData = [{ + ...sectionsData[0], + title: "", + subtitle: "" + }, { + ...sectionsData[1], + title: "", + subtitle: "" + }]; + } + let filteredSections = sectionsData.filter(section => !sectionPersonalization[section.sectionKey]?.isBlocked); if (interestPickerEnabled && visibleSections.length) { filteredSections = visibleSections.reduce((acc, visibleSection) => { const found = filteredSections.find(({ @@ -12426,6 +12490,7 @@ function CardSections({ ctaButtonVariant: ctaButtonVariant, ctaButtonSponsors: ctaButtonSponsors, anySectionsFollowed: anySectionsFollowed, + placeholder: placeholder, showWeather: weatherEnabled && weatherPlacement === "section" && sectionPosition === 0 && section.sectionKey === dailyBriefSectionId })); @@ -14064,7 +14129,8 @@ class _DiscoveryStreamBase extends (external_React_default()).PureComponent { type: component.type, firstVisibleTimestamp: this.props.firstVisibleTimestamp, ctaButtonSponsors: component.properties.ctaButtonSponsors, - ctaButtonVariant: component.properties.ctaButtonVariant + ctaButtonVariant: component.properties.ctaButtonVariant, + placeholder: this.props.placeholder }); } return /*#__PURE__*/external_React_default().createElement(CardGrid, { @@ -14083,7 +14149,8 @@ class _DiscoveryStreamBase extends (external_React_default()).PureComponent { ctaButtonVariant: component.properties.ctaButtonVariant, hideDescriptions: this.props.DiscoveryStream.hideDescriptions, firstVisibleTimestamp: this.props.firstVisibleTimestamp, - spocPositions: component.spocs?.positions + spocPositions: component.spocs?.positions, + placeholder: this.props.placeholder }); } case "HorizontalRule": @@ -16384,7 +16451,8 @@ class BaseContent extends (external_React_default()).PureComponent { colorMode: "", fixedNavStyle: {}, wallpaperTheme: "", - showDownloadHighlightOverride: null + showDownloadHighlightOverride: null, + visible: false }; } setFirstVisibleTimestamp() { @@ -16395,8 +16463,63 @@ class BaseContent extends (external_React_default()).PureComponent { } } onVisible() { + this.setState({ + visible: true + }); this.setFirstVisibleTimestamp(); this.shouldDisplayTopicSelectionModal(); + this.onVisibilityDispatch(); + } + onVisibilityDispatch() { + const { + onDemand = {} + } = this.props.DiscoveryStream.spocs; + + // We only need to dispatch this if: + // 1. onDemand is enabled, + // 2. onDemand spocs have not been loaded on this tab. + // 3. Spocs are expired. + if (onDemand.enabled && !onDemand.loaded && this.isSpocsOnDemandExpired) { + // This dispatches that spocs are expired and we need to update them. + this.props.dispatch(actionCreators.OnlyToMain({ + type: actionTypes.DISCOVERY_STREAM_SPOCS_ONDEMAND_UPDATE + })); + } + } + get isSpocsOnDemandExpired() { + const { + onDemand = {}, + cacheUpdateTime, + lastUpdated + } = this.props.DiscoveryStream.spocs; + + // We can bail early if: + // 1. onDemand is off, + // 2. onDemand spocs have been loaded on this tab. + if (!onDemand.enabled || onDemand.loaded) { + return false; + } + return Date.now() - lastUpdated >= cacheUpdateTime; + } + spocsOnDemandUpdated() { + const { + onDemand = {}, + loaded + } = this.props.DiscoveryStream.spocs; + + // We only need to fire this if: + // 1. Spoc data is loaded. + // 2. onDemand is enabled. + // 3. The component is visible (not preloaded tab). + // 4. onDemand spocs have not been loaded on this tab. + // 5. Spocs are not expired. + if (loaded && onDemand.enabled && this.state.visible && !onDemand.loaded && !this.isSpocsOnDemandExpired) { + // This dispatches that spocs have been loaded on this tab + // and we don't need to update them again for this tab. + this.props.dispatch(actionCreators.BroadcastToContent({ + type: actionTypes.DISCOVERY_STREAM_SPOCS_ONDEMAND_LOAD + })); + } } componentDidMount() { this.applyBodyClasses(); @@ -16472,6 +16595,7 @@ class BaseContent extends (external_React_default()).PureComponent { this.updateWallpaper(); } } + this.spocsOnDemandUpdated(); } handleColorModeChange() { const colorMode = this.prefersDarkQuery?.matches ? "dark" : "light"; @@ -16903,7 +17027,8 @@ class BaseContent extends (external_React_default()).PureComponent { className: "borderless-error" }, /*#__PURE__*/external_React_default().createElement(DiscoveryStreamBase, { locale: props.App.locale, - firstVisibleTimestamp: this.state.firstVisibleTimestamp + firstVisibleTimestamp: this.state.firstVisibleTimestamp, + placeholder: this.isSpocsOnDemandExpired })) : /*#__PURE__*/external_React_default().createElement(Sections_Sections, null)), /*#__PURE__*/external_React_default().createElement(ConfirmDialog, null), wallpapersEnabled && this.renderWallpaperAttribution()), /*#__PURE__*/external_React_default().createElement("aside", null, this.props.Notifications?.showNotifications && /*#__PURE__*/external_React_default().createElement(ErrorBoundary, null, /*#__PURE__*/external_React_default().createElement(Notifications_Notifications, { dispatch: this.props.dispatch }))), mayShowTopicSelection && pocketEnabled && /*#__PURE__*/external_React_default().createElement(TopicSelection, { diff --git a/browser/extensions/newtab/lib/ActivityStream.sys.mjs b/browser/extensions/newtab/lib/ActivityStream.sys.mjs @@ -1266,6 +1266,13 @@ export const PREFS_CONFIG = new Map([ }, ], [ + "discoverystream.spocs.onDemand", + { + title: "Set sponsored content to only update cache when requested.", + value: false, + }, + ], + [ "discoverystream.spocs.cacheTimeout", { title: "Set sponsored content cache timeout in minutes.", diff --git a/browser/extensions/newtab/lib/DiscoveryStreamFeed.sys.mjs b/browser/extensions/newtab/lib/DiscoveryStreamFeed.sys.mjs @@ -92,6 +92,7 @@ const PREF_SELECTED_TOPICS = "discoverystream.topicSelection.selectedTopics"; const PREF_TOPIC_SELECTION_ENABLED = "discoverystream.topicSelection.enabled"; const PREF_TOPIC_SELECTION_PREVIOUS_SELECTED = "discoverystream.topicSelection.hasBeenUpdatedPreviously"; +const PREF_SPOCS_CACHE_ONDEMAND = "discoverystream.spocs.onDemand"; const PREF_SPOCS_CACHE_TIMEOUT = "discoverystream.spocs.cacheTimeout"; const PREF_SPOCS_STARTUP_CACHE_ENABLED = "discoverystream.spocs.startupCache.enabled"; @@ -525,20 +526,37 @@ export class DiscoveryStreamFeed { } return null; } + get spocsOnDemand() { + if (this._spocsOnDemand === undefined) { + const { values } = this.store.getState().Prefs; + const spocsOnDemandConfig = values.trainhopConfig?.spocsOnDemand || {}; + const spocsOnDemand = + spocsOnDemandConfig.enabled || values[PREF_SPOCS_CACHE_ONDEMAND]; + this._spocsOnDemand = spocsOnDemand; + } + + return this._spocsOnDemand; + } get spocsCacheUpdateTime() { if (this._spocsCacheUpdateTime === undefined) { + const { values } = this.store.getState().Prefs; + const spocsOnDemandConfig = values.trainhopConfig?.spocsOnDemand || {}; const spocsCacheTimeout = - this.store.getState().Prefs.values[PREF_SPOCS_CACHE_TIMEOUT]; + spocsOnDemandConfig.timeout || 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 ( + + // We have some guard rails against misconfigured values. + // Ignore 0: a zero-minute timeout would cause constant fetches. + // Check min max times, or ensure we don't make requests on a timer. + const guardRailed = spocsCacheTimeout && - spocsCacheTimeout <= MAX_TIMEOUT && - spocsCacheTimeout >= MIN_TIMEOUT - ) { + (this.spocsOnDemand || + (spocsCacheTimeout <= MAX_TIMEOUT && + spocsCacheTimeout >= MIN_TIMEOUT)); + + if (guardRailed) { // This value is in minutes, but we want ms. this._spocsCacheUpdateTime = spocsCacheTimeout * 60 * 1000; } else { @@ -1496,6 +1514,8 @@ export class DiscoveryStreamFeed { await this.cache.set("spocs", { lastUpdated: spocsState.lastUpdated, spocs: spocsState.spocs, + spocsOnDemand: this.spocsOnDemand, + spocsCacheUpdateTime: this.spocsCacheUpdateTime, }); sendUpdate({ @@ -1503,6 +1523,8 @@ export class DiscoveryStreamFeed { data: { lastUpdated: spocsState.lastUpdated, spocs: spocsState.spocs, + spocsOnDemand: this.spocsOnDemand, + spocsCacheUpdateTime: this.spocsCacheUpdateTime, }, meta: { isStartup, @@ -2238,6 +2260,8 @@ export class DiscoveryStreamFeed { await this.cache.set("spocs", { lastUpdated: spocsState.lastUpdated, spocs: spocsState.data, + spocsOnDemand: this.spocsOnDemand, + spocsCacheUpdateTime: this.spocsCacheUpdateTime, }); this.store.dispatch( ac.AlsoToPreloaded({ @@ -2245,6 +2269,8 @@ export class DiscoveryStreamFeed { data: { lastUpdated: spocsState.lastUpdated, spocs: spocsState.data, + spocsOnDemand: this.spocsOnDemand, + spocsCacheUpdateTime: this.spocsCacheUpdateTime, }, }) ); @@ -2260,7 +2286,7 @@ export class DiscoveryStreamFeed { * @param {RefreshAll} options */ async refreshAll(options = {}) { - const { updateOpenTabs, isStartup } = options; + const { updateOpenTabs, isStartup, isSystemTick } = options; const dispatch = updateOpenTabs ? action => this.store.dispatch(ac.BroadcastToContent(action)) @@ -2272,16 +2298,19 @@ export class DiscoveryStreamFeed { this.store.getState().Prefs.values[PREF_SPOCS_STARTUP_CACHE_ENABLED]; const promises = []; - // We could potentially have either or both sponsored topsites or stories. - // We only make one fetch, and control which to request when we fetch. - // So for now we only care if we need to make this request at all. - const spocsPromise = this.loadSpocs( - dispatch, - isStartup && spocsStartupCacheEnabled - ).catch(error => - console.error("Error trying to load spocs feeds:", error) - ); - promises.push(spocsPromise); + // We don't want to make spoc requests during system tick if on demand is on. + if (!(this.spocsOnDemand && isSystemTick)) { + // We could potentially have either or both sponsored topsites or stories. + // We only make one fetch, and control which to request when we fetch. + // So for now we only care if we need to make this request at all. + const spocsPromise = this.loadSpocs( + dispatch, + isStartup && spocsStartupCacheEnabled + ).catch(error => + console.error("Error trying to load spocs feeds:", error) + ); + promises.push(spocsPromise); + } if (this.showStories) { const storiesPromise = this.loadComponentFeeds( dispatch, @@ -2292,6 +2321,9 @@ export class DiscoveryStreamFeed { promises.push(storiesPromise); } await Promise.all(promises); + // We don't need to check onDemand here, + // even though _maybeUpdateCachedData fetches spocs. + // This is because isStartup and isSystemTick can never both be true. if (isStartup) { // We don't pass isStartup in _maybeUpdateCachedData on purpose, // because startup loads have a longer cache timer, @@ -2383,6 +2415,7 @@ export class DiscoveryStreamFeed { // Reset in-memory caches. this._isContextualAds = undefined; this._spocsCacheUpdateTime = undefined; + this._spocsOnDemand = undefined; } resetDataPrefs() { @@ -2603,6 +2636,17 @@ export class DiscoveryStreamFeed { ); } + async onSpocsOnDemandUpdate() { + if (this.spocsOnDemand) { + const expirationPerComponent = await this._checkExpirationPerComponent(); + if (expirationPerComponent.spocs) { + await this.loadSpocs(action => + this.store.dispatch(ac.BroadcastToContent(action)) + ); + } + } + } + async onSystemTick() { // Only refresh when enabled and after initial load has completed. if (!this.config.enabled || !this.loaded) { @@ -2610,12 +2654,25 @@ export class DiscoveryStreamFeed { } const expirationPerComponent = await this._checkExpirationPerComponent(); - if (expirationPerComponent.feeds || expirationPerComponent.spocs) { - await this.refreshAll({ updateOpenTabs: false }); + let expired = false; + + if (this.spocsOnDemand) { + // With on-demand only feeds can trigger a refresh. + expired = expirationPerComponent.feeds; + } else { + // Without on-demand both feeds or spocs can trigger a refresh. + expired = expirationPerComponent.feeds || expirationPerComponent.spocs; + } + + if (expired) { + // We use isSystemTick so refreshAll can know to check onDemand + await this.refreshAll({ updateOpenTabs: false, isSystemTick: true }); } } - async onTrainhopConfigChanged() {} + async onTrainhopConfigChanged() { + this.resetSpocsOnDemand(); + } async onPrefChangedAction(action) { switch (action.data.name) { @@ -2704,6 +2761,11 @@ export class DiscoveryStreamFeed { await this.updateOrRemoveSpocs(); break; } + case PREF_SPOCS_CACHE_ONDEMAND: + case PREF_SPOCS_CACHE_TIMEOUT: { + this.resetSpocsOnDemand(); + break; + } } if (action.data.name === "pocketConfig") { @@ -2715,6 +2777,20 @@ export class DiscoveryStreamFeed { } } + resetSpocsOnDemand() { + // This is all we have to do, because we're just changing how often caches update. + // No need to reset what is already fetched, we just care about the next check. + this._spocsCacheUpdateTime = undefined; + this._spocsOnDemand = undefined; + this.store.dispatch({ + type: at.DISCOVERY_STREAM_SPOCS_ONDEMAND_RESET, + data: { + spocsOnDemand: this.spocsOnDemand, + spocsCacheUpdateTime: this.spocsCacheUpdateTime, + }, + }); + } + async onAction(action) { switch (action.type) { case at.INIT: @@ -2740,6 +2816,10 @@ export class DiscoveryStreamFeed { case at.SYSTEM_TICK: await this.onSystemTick(); break; + case at.DISCOVERY_STREAM_SPOCS_ONDEMAND_UPDATE: { + await this.onSpocsOnDemandUpdate(); + break; + } case at.DISCOVERY_STREAM_DEV_SYNC_RS: lazy.RemoteSettings.pollChanges(); break; @@ -2844,6 +2924,8 @@ export class DiscoveryStreamFeed { await this.cache.set("spocs", { lastUpdated: spocsState.lastUpdated, spocs: spocsState.data, + spocsOnDemand: this.spocsOnDemand, + spocsCacheUpdateTime: this.spocsCacheUpdateTime, }); this.store.dispatch( @@ -2852,6 +2934,8 @@ export class DiscoveryStreamFeed { data: { lastUpdated: spocsState.lastUpdated, spocs: spocsState.data, + spocsOnDemand: this.spocsOnDemand, + spocsCacheUpdateTime: this.spocsCacheUpdateTime, }, }) ); @@ -2919,6 +3003,8 @@ export class DiscoveryStreamFeed { await this.cache.set("spocs", { lastUpdated: spocsState.lastUpdated, spocs: spocsState.data, + spocsOnDemand: this.spocsOnDemand, + spocsCacheUpdateTime: this.spocsCacheUpdateTime, }); // If we're blocking a spoc, we want open tabs to have diff --git a/browser/extensions/newtab/lib/StartupCacheInit.sys.mjs b/browser/extensions/newtab/lib/StartupCacheInit.sys.mjs @@ -71,6 +71,8 @@ export class StartupCacheInit { data: { lastUpdated: spocsState.lastUpdated, spocs: spocsState.data, + spocsOnDemand: spocsState.onDemand.enabled, + spocsCacheUpdateTime: spocsState.cacheUpdateTime, }, }; this.store.dispatch(ac.OnlyToOneContent(action, target)); diff --git a/browser/extensions/newtab/test/unit/common/Reducers.test.js b/browser/extensions/newtab/test/unit/common/Reducers.test.js @@ -819,6 +819,8 @@ describe("Reducers", () => { const data = { lastUpdated: 123, spocs: [1, 2, 3], + spocsCacheUpdateTime: 10 * 60 * 1000, + spocsOnDemand: true, }; const state = DiscoveryStream(undefined, { type: at.DISCOVERY_STREAM_SPOCS_UPDATE, @@ -826,12 +828,17 @@ describe("Reducers", () => { }); assert.deepEqual(state.spocs, { spocs_endpoint: "", - data: [1, 2, 3], - lastUpdated: 123, + data: data.spocs, + lastUpdated: data.lastUpdated, loaded: true, frequency_caps: [], blocked: [], placements: [], + cacheUpdateTime: data.spocsCacheUpdateTime, + onDemand: { + enabled: data.spocsOnDemand, + loaded: false, + }, }); }); it("should default to a single spoc placement", () => { diff --git a/browser/extensions/newtab/test/unit/content-src/components/Base.test.jsx b/browser/extensions/newtab/test/unit/content-src/components/Base.test.jsx @@ -79,7 +79,7 @@ describe("<BaseContent>", () => { App: { initialized: true }, Prefs: { values: {} }, Sections: [], - DiscoveryStream: { config: { enabled: false } }, + DiscoveryStream: { config: { enabled: false }, spocs: {} }, Weather: {}, dispatch: () => {}, document: { diff --git a/browser/extensions/newtab/test/unit/lib/DiscoveryStreamFeed.test.js b/browser/extensions/newtab/test/unit/lib/DiscoveryStreamFeed.test.js @@ -821,7 +821,12 @@ describe("DiscoveryStreamFeed", () => { await feed.loadSpocs(feed.store.dispatch); assert.notCalled(global.fetch); - assert.calledWith(feed.cache.set, "spocs", { lastUpdated: 0, spocs: {} }); + assert.calledWith(feed.cache.set, "spocs", { + lastUpdated: 0, + spocs: {}, + spocsOnDemand: undefined, + spocsCacheUpdateTime: 30 * 60 * 1000, + }); }); it("should fetch fresh spocs data if cache is empty", async () => { sandbox.stub(feed.cache, "get").returns(Promise.resolve()); @@ -833,6 +838,8 @@ describe("DiscoveryStreamFeed", () => { assert.calledWith(feed.cache.set, "spocs", { spocs: { placement: "data" }, lastUpdated: 0, + spocsOnDemand: undefined, + spocsCacheUpdateTime: 30 * 60 * 1000, }); assert.equal( feed.store.getState().DiscoveryStream.spocs.data.placement, @@ -898,6 +905,8 @@ describe("DiscoveryStreamFeed", () => { }, }, lastUpdated: loadTimestamp, + spocsOnDemand: undefined, + spocsCacheUpdateTime: 30 * 60 * 1000, }); assert.deepEqual( @@ -2231,6 +2240,11 @@ describe("DiscoveryStreamFeed", () => { data, }, }, + Prefs: { + values: { + trainhopConfig: {}, + }, + }, }); }); @@ -2364,6 +2378,10 @@ describe("DiscoveryStreamFeed", () => { }); it("should call dispatch if found a blocked spoc", async () => { Object.defineProperty(feed, "showSpocs", { get: () => true }); + Object.defineProperty(feed, "spocsOnDemand", { get: () => false }); + Object.defineProperty(feed, "spocsCacheUpdateTime", { + get: () => 30 * 60 * 1000, + }); sandbox.spy(feed.store, "dispatch"); @@ -2394,6 +2412,10 @@ describe("DiscoveryStreamFeed", () => { }); it("should dispatch a DISCOVERY_STREAM_SPOC_BLOCKED for a blocked spoc", async () => { Object.defineProperty(feed, "showSpocs", { get: () => true }); + Object.defineProperty(feed, "spocsOnDemand", { get: () => false }); + Object.defineProperty(feed, "spocsCacheUpdateTime", { + get: () => 30 * 60 * 1000, + }); sandbox.spy(feed.store, "dispatch"); await feed.onAction({ @@ -2679,7 +2701,10 @@ describe("DiscoveryStreamFeed", () => { sandbox.stub(feed, "refreshAll").resolves(); await feed.onAction({ type: at.SYSTEM_TICK }); - assert.calledWith(feed.refreshAll, { updateOpenTabs: false }); + assert.calledWith(feed.refreshAll, { + updateOpenTabs: false, + isSystemTick: true, + }); }); }); @@ -2811,6 +2836,7 @@ describe("DiscoveryStreamFeed", () => { assert.equal(cacheTime, defaultCacheTime); }); it("should set _spocsCacheUpdateTime with spocsCacheTimeout", () => { + const defaultCacheTime = 20 * 60 * 1000; feed.store.getState = () => ({ Prefs: { values: { @@ -2819,8 +2845,50 @@ describe("DiscoveryStreamFeed", () => { }, }); const cacheTime = feed.spocsCacheUpdateTime; - assert.equal(feed._spocsCacheUpdateTime, 20 * 60 * 1000); - assert.equal(cacheTime, 20 * 60 * 1000); + assert.equal(feed._spocsCacheUpdateTime, defaultCacheTime); + assert.equal(cacheTime, defaultCacheTime); + }); + it("should set _spocsCacheUpdateTime with spocsCacheTimeout and onDemand", () => { + const defaultCacheTime = 4 * 60 * 1000; + feed.store.getState = () => ({ + Prefs: { + values: { + "discoverystream.spocs.onDemand": true, + "discoverystream.spocs.cacheTimeout": 4, + }, + }, + }); + const cacheTime = feed.spocsCacheUpdateTime; + assert.equal(feed._spocsCacheUpdateTime, defaultCacheTime); + assert.equal(cacheTime, defaultCacheTime); + }); + it("should set _spocsCacheUpdateTime with spocsCacheTimeout without max", () => { + const defaultCacheTime = 31 * 60 * 1000; + feed.store.getState = () => ({ + Prefs: { + values: { + "discoverystream.spocs.onDemand": true, + "discoverystream.spocs.cacheTimeout": 31, + }, + }, + }); + const cacheTime = feed.spocsCacheUpdateTime; + assert.equal(feed._spocsCacheUpdateTime, defaultCacheTime); + assert.equal(cacheTime, defaultCacheTime); + }); + it("should set _spocsCacheUpdateTime with spocsCacheTimeout without min", () => { + const defaultCacheTime = 1 * 60 * 1000; + feed.store.getState = () => ({ + Prefs: { + values: { + "discoverystream.spocs.onDemand": true, + "discoverystream.spocs.cacheTimeout": 1, + }, + }, + }); + const cacheTime = feed.spocsCacheUpdateTime; + assert.equal(feed._spocsCacheUpdateTime, defaultCacheTime); + assert.equal(cacheTime, defaultCacheTime); }); }); @@ -3273,6 +3341,8 @@ describe("DiscoveryStreamFeed", () => { const spocsTestResult = { lastUpdated: 1234, + spocsCacheUpdateTime: 1800000, + spocsOnDemand: undefined, spocs: { placement1: { personalized: true, diff --git a/browser/extensions/newtab/test/xpcshell/test_StartupCacheInit.js b/browser/extensions/newtab/test/xpcshell/test_StartupCacheInit.js @@ -173,6 +173,8 @@ add_task(async function test_onAction_DISCOVERY_STREAM_SPOCS_UPDATE() { spocs: { data: ["spoc1", "spoc2"], lastUpdated: "lastUpdated", + onDemand: { enabled: false }, + cacheUpdateTime: 30 * 60 * 1000, }, }, Prefs: { @@ -207,6 +209,8 @@ add_task(async function test_onAction_DISCOVERY_STREAM_SPOCS_UPDATE() { data: { spocs: ["spoc1", "spoc2"], lastUpdated: "lastUpdated", + spocsOnDemand: false, + spocsCacheUpdateTime: 30 * 60 * 1000, }, }, "fromTarget"