commit 0bd70563fbc9583f94b6e9d75370fbedfb059d66 parent 776693692df8f2cc1f69be77d5beabb6b10e2305 Author: Drew Willcoxon <adw@mozilla.com> Date: Tue, 18 Nov 2025 00:23:38 +0000 Bug 2000362 - Make L10nCache.setElementL10n() cache all l10n strings, and implement a cache eviction mechanism. r=daisuke,urlbar-reviewers This makes `L10nCache.setElementL10n()` cache all l10n strings. It also implements a cache eviction mechanism: Up to 5 translated strings are cached per l10n ID. For l10n strings that don't have any args, only one translated string is possible, but for strings that do have args, any number of translated strings are possible, depending on the args. The cache will store at most 5 per ID. Cached strings are evicted in least recently used order. This also makes some other subtle changes to `setElementL10n`: If the message is cached, then it will set all cached attributes and text content, so even if the caller passed in an `attribute`, it will set all attributes and text content, not just the passed-in one. I think that makes sense and is easier to understand. Also, `setElementL10n` will now always set the l10n attributes (`data-l10n-id`, etc.) even if the message is cached. There are some tests that depend on that. It wasn't a problem for them before because the functionality being tested by these tests didn't pass in `cacheable: true` for their strings, so they weren't being cached before, but now they are. Differential Revision: https://phabricator.services.mozilla.com/D272767 Diffstat:
21 files changed, 544 insertions(+), 584 deletions(-)
diff --git a/browser/components/urlbar/UrlbarProviderActionsSearchMode.sys.mjs b/browser/components/urlbar/UrlbarProviderActionsSearchMode.sys.mjs @@ -109,7 +109,7 @@ export class UrlbarProviderActionsSearchMode extends UrlbarProvider { return { label: { - l10n: { id: action.label, cacheable: true }, + l10n: { id: action.label }, }, }; } diff --git a/browser/components/urlbar/UrlbarProviderGlobalActions.sys.mjs b/browser/components/urlbar/UrlbarProviderGlobalActions.sys.mjs @@ -220,12 +220,12 @@ export class UrlbarProviderGlobalActions extends UrlbarProvider { let viewUpdate = {}; if (result.payload.showOnboardingLabel) { viewUpdate["press-tab-label"] = { - l10n: { id: "press-tab-label", cacheable: true }, + l10n: { id: "press-tab-label" }, }; } result.payload.actionsResults.forEach((action, i) => { viewUpdate[`label-${i}`] = { - l10n: { id: action.l10nId, args: action.l10nArgs, cacheable: true }, + l10n: { id: action.l10nId, args: action.l10nArgs }, }; }); return viewUpdate; diff --git a/browser/components/urlbar/UrlbarUtils.sys.mjs b/browser/components/urlbar/UrlbarUtils.sys.mjs @@ -1833,12 +1833,6 @@ const L10N_SCHEMA = { parseMarkup: { type: "boolean", }, - cacheable: { - type: "boolean", - }, - excludeArgsFromCacheKey: { - type: "boolean", - }, }, }; @@ -3190,8 +3184,46 @@ export class SkippableTimer { * `L10nMessage` objects, not bare strings. This allows the cache to store not * only l10n strings with bare values but also strings that define attributes * (e.g., ".label = My label value"). See `get` for details. + * + * The cache stores up to `MAX_ENTRIES_PER_ID` entries per l10n ID, and entries + * are sorted from least recently cached to most recently cached. This only + * matters for strings that have arguments. For strings that don't have + * arguments, there can be only one cached value, so there can be only one cache + * entry. But for strings that do have arguments, their cached values depend on + * the arguments they were cached with. The cache will store up to + * `MAX_ENTRIES_PER_ID` of the most recently cached values for a given l10n ID. + * + * For example, given the following string from an ftl file: + * + * foo = My arg value is { $bar } + * + * And the following cache calls: + * + * cache.add({ id: "foo", args: { bar: "aaa" }}); + * cache.add({ id: "foo", args: { bar: "bbb" }}); + * cache.add({ id: "foo", args: { bar: "ccc" }}); + * + * Then three different versions of the "foo" string will be cached, from least + * recently cached to most recently cached: + * + * "My arg value is aaa" + * "My arg value is bbb" + * "My arg value is ccc" + * + * If `MAX_ENTRIES_PER_ID` is 3 and we cache a fourth version like this: + * + * cache.add({ id: "foo", args: { bar: "zzz" }}); + * + * Then the least recently cached version -- the "aaa" one -- will be evicted + * from the cache, and the remaining cached versions will be: + * + * "My arg value is bbb" + * "My arg value is ccc" + * "My arg value is zzz" */ export class L10nCache { + static MAX_ENTRIES_PER_ID = 5; + /** * @param {Localization} l10n * A `Localization` object like `document.l10n`. This class keeps a weak @@ -3215,14 +3247,13 @@ export class L10nCache { * @param {string} options.id * The string's Fluent ID. * @param {object} [options.args] - * The Fluent arguments as passed to `l10n.setAttributes`. - * @param {boolean} [options.excludeArgsFromCacheKey] - * Pass true if the string was cached using a key that excluded arguments. + * The Fluent arguments as passed to `l10n.setAttributes`. Required if the + * l10n string has arguments. + * @returns {L10nCachedMessage|null} + * The cached message or null if it's not cached. */ - get({ id, args = undefined, excludeArgsFromCacheKey = false }) { - return this.#messagesByKey.get( - this.#key({ id, args, excludeArgsFromCacheKey }) - ); + get({ id, args = undefined }) { + return this.#messagesByArgsById.get(id)?.get(this.#argsKey(args)) ?? null; } /** @@ -3233,19 +3264,15 @@ export class L10nCache { * @param {string} options.id * The string's Fluent ID. * @param {object} [options.args] - * The Fluent arguments as passed to `l10n.setAttributes`. - * @param {boolean} [options.excludeArgsFromCacheKey] - * Pass true to cache the string using a key that excludes the arguments. - * The string will be cached only by its ID. This is useful if the string is - * used only once in the UI, its arguments vary, and it's acceptable to - * fetch and show a cached value with old arguments until the string is - * relocalized with new arguments. - */ - async add({ id, args = undefined, excludeArgsFromCacheKey = false }) { + * The Fluent arguments as passed to `l10n.setAttributes`. Required if the + * l10n string has arguments. + */ + async add({ id, args = undefined }) { let l10n = this.l10n.get(); if (!l10n) { return; } + let messages = await l10n.formatMessages([{ id, args }]); if (!messages?.length) { console.error( @@ -3268,37 +3295,34 @@ export class L10nCache { {} ); } - this.#messagesByKey.set( - this.#key({ id, args, excludeArgsFromCacheKey }), - message - ); + + this.#update({ id, args, message }); } /** - * Fetches and caches a string if it's not already cached. This is just a - * slight optimization over `add` that avoids calling into Fluent - * unnecessarily. + * Ensures that a string is the most recently cached for its ID. If the string + * is not already cached, then it's fetched from Fluent. This is just a slight + * optimization over `add` that avoids calling into Fluent unnecessarily. * * @param {object} options * Options * @param {string} options.id * The string's Fluent ID. * @param {object} [options.args] - * The Fluent arguments as passed to `l10n.setAttributes`. - * @param {boolean} [options.excludeArgsFromCacheKey] - * Pass true to cache the string using a key that excludes the arguments. - * The string will be cached only by its ID. See `add()` for more. + * The Fluent arguments as passed to `l10n.setAttributes`. Required if the + * l10n string has arguments. */ - async ensure({ id, args = undefined, excludeArgsFromCacheKey = false }) { - // Always re-cache if `excludeArgsFromCacheKey` is true. The values in - // `args` may be different from the values in the cached string. - if (excludeArgsFromCacheKey || !this.get({ id, args })) { - await this.add({ id, args, excludeArgsFromCacheKey }); + async ensure({ id, args = undefined }) { + let message = this.get({ id, args }); + if (message) { + await this.#update({ id, args, message }); + } else { + await this.add({ id, args }); } } /** - * Fetches and caches strings that aren't already cached. + * A version of `ensure` that ensures multiple strings are cached at once. * * @param {object[]} objects * An array of objects as passed to `ensure()`. @@ -3319,29 +3343,33 @@ export class L10nCache { * @param {string} options.id * The string's Fluent ID. * @param {object} [options.args] - * The Fluent arguments as passed to `l10n.setAttributes`. - * @param {boolean} [options.excludeArgsFromCacheKey] - * Pass true if the string was cached using a key that excludes the - * arguments. If true, `args` is ignored. - */ - delete({ id, args = undefined, excludeArgsFromCacheKey = false }) { - this.#messagesByKey.delete( - this.#key({ id, args, excludeArgsFromCacheKey }) - ); + * The Fluent arguments as passed to `l10n.setAttributes`. Required if the + * l10n string has arguments. + */ + delete({ id, args = undefined }) { + let messagesByArgs = this.#messagesByArgsById.get(id); + if (messagesByArgs) { + messagesByArgs.delete(this.#argsKey(args)); + if (!messagesByArgs.size) { + this.#messagesByArgsById.delete(id); + } + } } /** * Removes all cached strings. */ clear() { - this.#messagesByKey.clear(); + this.#messagesByArgsById.clear(); } /** * Returns the number of cached messages. */ size() { - return this.#messagesByKey.size; + return this.#messagesByArgsById + .values() + .reduce((total, messagesByArg) => total + messagesByArg.size, 0); } /** @@ -3350,10 +3378,10 @@ export class L10nCache { * `document.l10n.setAttributes()` using the given l10n ID and args, which * means the string will pop in on a later animation frame. * - * This also optionally caches the string so that it will be ready the next - * time this is called for it. The function returns a promise that will be - * resolved when the string has been cached. Typically there's no need to - * await it unless you want to be sure the string is cached before continuing. + * This also caches the string so that it will be ready the next time. It + * returns a promise that will be resolved when the string has been cached. + * Typically there's no need to await it unless you want to be sure the string + * is cached before continuing. * * @param {Element} element * The l10n string will be applied to this element. @@ -3380,27 +3408,10 @@ export class L10nCache { * string is expected to contain markup. When true, the cached string is * essentially assigned to the element's `innerHTML`. When false, it's * assigned to the element's `textContent`. - * @param {boolean} [options.cacheable] - * Whether the string should be cached in addition to applying it to the - * given element. - * @param {boolean} [options.excludeArgsFromCacheKey] - * This affects how the string is stored in and fetched from the cache and - * is only relevant if the string has arguments. When true, all formatted - * values of the string share the same cache entry regardless of the - * arguments they were formatted with. In other words, only the ID matters. - * When false, formatted values with different arguments have separate cache - * entries. Typically it should be true when the number of possible argument - * values is unbounded and false otherwise. For example, it should be true - * if the argument is a user search string since that could be anything. It - * should be false if the argument is the name of an installed search engine - * since there's a relatively small number of those. - * - * If `cacheable` is false but you previously cached the string using - * another function, you should pass the same value you passed for - * `excludeArgsFromCacheKey` when you cached it. - * @returns {Promise|null} - * If `cacheable` is true, this returns a promise that's resolved when the - * string has been cached. Otherwise it returns null. + * @returns {Promise} + * A promise that's resolved when the string has been cached. You can ignore + * it and do not need to await it unless you want to make sure the string is + * cached before continuing. */ setElementL10n( element, @@ -3410,80 +3421,75 @@ export class L10nCache { argsHighlights = undefined, attribute = undefined, parseMarkup = false, - cacheable = false, - excludeArgsFromCacheKey = false, } ) { - let message = this.get({ id, args, excludeArgsFromCacheKey }); + // If the message is cached, apply it to the element. + let message = this.get({ id, args }); if (message) { - element.removeAttribute("data-l10n-id"); - element.removeAttribute("data-l10n-attrs"); - element.removeAttribute("data-l10n-args"); - if (attribute) { - element.setAttribute(attribute, message.attributes[attribute]); - } else if (!parseMarkup) { - element.textContent = message.value; - } else { - element.innerHTML = ""; - element.append( - lazy.parserUtils.parseFragment( - message.value, - Ci.nsIParserUtils.SanitizerDropNonCSSPresentation | - Ci.nsIParserUtils.SanitizerDropForms | - Ci.nsIParserUtils.SanitizerDropMedia, - false, - Services.io.newURI(element.ownerDocument.documentURI), - element - ) - ); + if (message.attributes) { + for (let [name, value] of Object.entries(message.attributes)) { + element.setAttribute(name, value); + } + } + if (typeof message.value == "string") { + if (!parseMarkup) { + element.textContent = message.value; + } else { + element.innerHTML = ""; + element.append( + lazy.parserUtils.parseFragment( + message.value, + Ci.nsIParserUtils.SanitizerDropNonCSSPresentation | + Ci.nsIParserUtils.SanitizerDropForms | + Ci.nsIParserUtils.SanitizerDropMedia, + false, + Services.io.newURI(element.ownerDocument.documentURI), + element + ) + ); + } } } - // If the message wasn't cached, set the element's l10n attributes and let - // `DOMLocalization` do its asynchronous translation. The element's content - // will pop in when translation finishes. - // - // Also do this if the message was cached but its args aren't part of the - // cache key because in that case the cached message may contain outdated - // arg values. We just set the element's content to the old message above, - // and when `DOMLocalization` finishes translating the new message, it will - // set the element's content again. If the old and new args are different, - // the new content will pop in. If they're the same, nothing will appear to - // change. - if (!message || (cacheable && excludeArgsFromCacheKey)) { - if (attribute) { - element.setAttribute("data-l10n-attrs", attribute); - } else { - element.removeAttribute("data-l10n-attrs"); - - if (argsHighlights) { - // To avoid contamination args because we cache it, create a new - // instance. - args = { ...args }; - - let span = element.ownerDocument.createElement("span"); - for (let key in argsHighlights) { - UrlbarUtils.addTextContentWithHighlights( - span, - args[key], - argsHighlights[key] - ); - args[key] = span.innerHTML; - } - } + // If the message isn't cached and args highlights were specified, apply + // them now. + if (!message && !attribute && argsHighlights) { + // To avoid contaminated args because we cache it, create a new instance. + args = { ...args }; + + let span = element.ownerDocument.createElement("span"); + for (let key in argsHighlights) { + UrlbarUtils.addTextContentWithHighlights( + span, + args[key], + argsHighlights[key] + ); + args[key] = span.innerHTML; } - element.ownerDocument.l10n.setAttributes(element, id, args); } - if (cacheable) { - // Cache the string. We specifically do not do this first and await it - // because the whole point of the l10n cache is to synchronously update - // the element's content when possible. Here, we return a promise rather - // than making this function async and awaiting so it's clearer to callers - // that they probably don't need to wait for caching to finish. - return this.ensure({ id, args, excludeArgsFromCacheKey }); + // If an attribute was passed in, make sure it's allowed to be localized by + // setting `data-l10n-attrs`. This isn't required for attrbutes already in + // the Fluent allowlist but it doesn't hurt. + if (attribute) { + element.setAttribute("data-l10n-attrs", attribute); + } else { + element.removeAttribute("data-l10n-attrs"); } - return null; + + // Set the l10n attributes. If the message wasn't cached, `DOMLocalization` + // will do its asynchronous translation and the text content will pop in. If + // the message was cached, then we already set the cached attributes and + // text content above, but we set the l10n attributes anyway because some + // tests rely on them being set. It shouldn't hurt anyway. + element.ownerDocument.l10n.setAttributes(element, id, args); + + // Cache the string. We specifically do not do this first and await it + // because the whole point of the l10n cache is to synchronously update the + // element's content when possible. Here, we return a promise rather than + // making this function async and awaiting so it's clearer to callers that + // they probably don't need to wait for caching to finish. + return this.ensure({ id, args }); } /** @@ -3525,14 +3531,26 @@ export class L10nCache { } /** - * Cache keys => cached message objects + * L10n ID => l10n args cache key => cached message object * - * @type {Map<string, L10nCachedMessage>} + * We rely on the fact that `Map` remembers insertion order to keep track of + * which cache entries are least recent, per l10n ID. The inner `Map`s will + * iterate their entries in order from least recently inserted to most + * recently inserted, i.e., least recently cached to most recently cached. + * + * @type {Map<string, Map<string, L10nCachedMessage>>} */ - #messagesByKey = new Map(); + #messagesByArgsById = new Map(); /** - * Returns a cache key for a string in `#messagesByKey`. + * Max entries per l10n ID for this cache. + * + * @type {number} + */ + #maxEntriesPerId = L10nCache.MAX_ENTRIES_PER_ID; + + /** + * Inserts a message into the cache and makes it most recently cached. * * @param {object} options * Options @@ -3540,23 +3558,54 @@ export class L10nCache { * The string's Fluent ID. * @param {object} options.args * The Fluent arguments as passed to `l10n.setAttributes`. - * @param {boolean} options.excludeArgsFromCacheKey - * Pass true to exclude the arguments from the key and include only the ID. + * @param {L10nCachedMessage} options.message + * The message to cache. + */ + #update({ id, args, message }) { + let messagesByArgs = this.#messagesByArgsById.get(id); + if (!messagesByArgs) { + messagesByArgs = new Map(); + this.#messagesByArgsById.set(id, messagesByArgs); + } + + // We rely on the fact that `Map` remembers insertion order to keep track of + // which cache entries are least recent. To make `message` the most recent + // for its ID, delete it from `messagesByArgs` (step 1) and then reinsert it + // (step 2). That way it will move to the end of iteration. + let argsKey = this.#argsKey(args); + + // step 1 + messagesByArgs.delete(argsKey); + + if (messagesByArgs.size == this.#maxEntriesPerId) { + // The cache entries are full for this ID. Remove the least recently + // cached entry, which will be the first entry returned by the map's + // iterator. + messagesByArgs.delete(messagesByArgs.keys().next().value); + } + + // step 2 + messagesByArgs.set(argsKey, message); + } + + /** + * Returns a cache key for the inner `Maps` inside `#messagesByArgsById`. + * These `Map`s are keyed on l10n args. + * + * @param {object} args + * The Fluent arguments as passed to `l10n.setAttributes`. * @returns {string} - * The cache key. + * The args cache key. */ - #key({ id, args, excludeArgsFromCacheKey }) { - // Keys are `id` plus JSON'ed `args` values. `JSON.stringify` doesn't - // guarantee a particular ordering of object properties, so instead of - // stringifying `args` as is, sort its entries by key and then pull out the - // values. The final key is a JSON'ed array of `id` concatenated with the + #argsKey(args) { + // `JSON.stringify` doesn't guarantee a particular ordering of object + // properties, so instead of stringifying `args` as is, sort its entries by + // key and then pull out the values. The final key is a JSON'ed array of // sorted-by-key `args` values. - args = (!excludeArgsFromCacheKey && args) || []; - let argValues = Object.entries(args) + let argValues = Object.entries(args ?? []) .sort(([key1], [key2]) => key1.localeCompare(key2)) .map(([_, value]) => value); - let parts = [id].concat(argValues); - return JSON.stringify(parts); + return JSON.stringify(argValues); } } diff --git a/browser/components/urlbar/UrlbarView.sys.mjs b/browser/components/urlbar/UrlbarView.sys.mjs @@ -1783,13 +1783,8 @@ export class UrlbarView { name: "result-menu", classList: ["urlbarView-button-menu"], l10n: result.showFeedbackMenu - ? { - id: "urlbar-result-menu-button-feedback", - cacheable: true, - } - : { - id: "urlbar-result-menu-button", - }, + ? { id: "urlbar-result-menu-button-feedback" } + : { id: "urlbar-result-menu-button" }, attributes: lazy.UrlbarPrefs.get("resultMenu.keyboardAccessible") ? null : { diff --git a/browser/components/urlbar/private/AddonSuggestions.sys.mjs b/browser/components/urlbar/private/AddonSuggestions.sys.mjs @@ -97,7 +97,6 @@ export class AddonSuggestions extends SuggestProvider { description: suggestion.description, bottomTextL10n: { id: "firefox-suggest-addons-recommended", - cacheable: true, }, helpUrl: lazy.QuickSuggest.HELP_URL, }; diff --git a/browser/components/urlbar/private/FlightStatusSuggestions.sys.mjs b/browser/components/urlbar/private/FlightStatusSuggestions.sys.mjs @@ -227,8 +227,6 @@ export class FlightStatusSuggestions extends RealtimeSuggestProvider { city: item.origin.city, code: item.origin.code, }, - cacheable: true, - excludeArgsFromCacheKey: true, }, }, [`destination_airport_${index}`]: { @@ -238,8 +236,6 @@ export class FlightStatusSuggestions extends RealtimeSuggestProvider { city: item.destination.city, code: item.destination.code, }, - cacheable: true, - excludeArgsFromCacheKey: true, }, }, [`flight_number_${index}`]: item.airline.name @@ -250,8 +246,6 @@ export class FlightStatusSuggestions extends RealtimeSuggestProvider { flightNumber: item.flight_number, airlineName: item.airline.name, }, - cacheable: true, - excludeArgsFromCacheKey: !!statusL10nArgs, }, } : { @@ -261,8 +255,6 @@ export class FlightStatusSuggestions extends RealtimeSuggestProvider { l10n: { id: statusL10nId, args: statusL10nArgs, - cacheable: true, - excludeArgsFromCacheKey: !!statusL10nArgs, }, }, [`time_left_${index}`]: timeLeft @@ -272,8 +264,6 @@ export class FlightStatusSuggestions extends RealtimeSuggestProvider { args: { timeLeft, }, - cacheable: true, - excludeArgsFromCacheKey: !!statusL10nArgs, }, } : null, diff --git a/browser/components/urlbar/private/ImportantDatesSuggestions.sys.mjs b/browser/components/urlbar/private/ImportantDatesSuggestions.sys.mjs @@ -207,8 +207,6 @@ export class ImportantDatesSuggestions extends SuggestProvider { } else { descriptionL10n = { ...this.#formatDateCountdown(eventDateOrRange, payload.name), - cacheable: true, - excludeArgsFromCacheKey: true, }; } diff --git a/browser/components/urlbar/private/MDNSuggestions.sys.mjs b/browser/components/urlbar/private/MDNSuggestions.sys.mjs @@ -64,7 +64,6 @@ export class MDNSuggestions extends SuggestProvider { shouldShowUrl: true, bottomTextL10n: { id: "firefox-suggest-mdn-bottom-text", - cacheable: true, }, }; diff --git a/browser/components/urlbar/private/RealtimeSuggestProvider.sys.mjs b/browser/components/urlbar/private/RealtimeSuggestProvider.sys.mjs @@ -101,14 +101,12 @@ export class RealtimeSuggestProvider extends SuggestProvider { get optInTitleL10n() { return { id: `urlbar-result-${this.realtimeTypeForFtl}-opt-in-title`, - cacheable: true, }; } get optInDescriptionL10n() { return { id: `urlbar-result-${this.realtimeTypeForFtl}-opt-in-description`, - cacheable: true, parseMarkup: true, }; } @@ -382,14 +380,12 @@ export class RealtimeSuggestProvider extends SuggestProvider { command: "dismiss", l10n: { id: "urlbar-result-realtime-opt-in-dismiss", - cacheable: true, }, } : { command: "not_now", l10n: { id: "urlbar-result-realtime-opt-in-not-now", - cacheable: true, }, }; @@ -410,7 +406,6 @@ export class RealtimeSuggestProvider extends SuggestProvider { command: "opt_in", l10n: { id: "urlbar-result-realtime-opt-in-allow", - cacheable: true, }, input: queryContext.searchString, attributes: { diff --git a/browser/components/urlbar/private/WeatherSuggestions.sys.mjs b/browser/components/urlbar/private/WeatherSuggestions.sys.mjs @@ -252,13 +252,10 @@ export class WeatherSuggestions extends SuggestProvider { ...titleL10n.args, }, parseMarkup: true, - cacheable: true, - excludeArgsFromCacheKey: true, }, bottomTextL10n: { id: "urlbar-result-weather-provider-sponsored", args: { provider: WEATHER_PROVIDER_DISPLAY_NAME }, - cacheable: true, }, helpUrl: lazy.QuickSuggest.HELP_URL, }, @@ -297,7 +294,6 @@ export class WeatherSuggestions extends SuggestProvider { currently: { l10n: { id: "firefox-suggest-weather-currently", - cacheable: true, }, }, temperature: { @@ -307,8 +303,6 @@ export class WeatherSuggestions extends SuggestProvider { value: result.payload.temperature, unit: uppercaseUnit, }, - cacheable: true, - excludeArgsFromCacheKey: true, }, }, weatherIcon: { @@ -318,8 +312,6 @@ export class WeatherSuggestions extends SuggestProvider { l10n: { id: "firefox-suggest-weather-title", args: { city: result.payload.city, region: result.payload.region }, - cacheable: true, - excludeArgsFromCacheKey: true, }, }, url: { @@ -334,8 +326,6 @@ export class WeatherSuggestions extends SuggestProvider { currentConditions: result.payload.currentConditions, forecast: result.payload.forecast, }, - cacheable: true, - excludeArgsFromCacheKey: true, }, }, highLow: { @@ -346,8 +336,6 @@ export class WeatherSuggestions extends SuggestProvider { low: result.payload.low, unit: uppercaseUnit, }, - cacheable: true, - excludeArgsFromCacheKey: true, }, }, highLowWrap: { @@ -364,7 +352,6 @@ export class WeatherSuggestions extends SuggestProvider { l10n: { id: "urlbar-result-weather-provider-sponsored", args: { provider: WEATHER_PROVIDER_DISPLAY_NAME }, - cacheable: true, }, }, }; diff --git a/browser/components/urlbar/private/YelpRealtimeSuggestions.sys.mjs b/browser/components/urlbar/private/YelpRealtimeSuggestions.sys.mjs @@ -117,8 +117,6 @@ export class YelpRealtimeSuggestions extends RealtimeSuggestProvider { }).format(new Date()), }, parseMarkup: true, - cacheable: true, - excludeArgsFromCacheKey: true, }, }, [`popularity_${index}`]: { @@ -128,8 +126,6 @@ export class YelpRealtimeSuggestions extends RealtimeSuggestProvider { rating: item.rating, review_count: item.review_count, }, - cacheable: true, - excludeArgsFromCacheKey: true, }, }, }; diff --git a/browser/components/urlbar/private/YelpSuggestions.sys.mjs b/browser/components/urlbar/private/YelpSuggestions.sys.mjs @@ -204,7 +204,6 @@ export class YelpSuggestions extends SuggestProvider { originalUrl: suggestion.url, bottomTextL10n: { id: "firefox-suggest-yelp-bottom-text", - cacheable: true, }, iconBlob: suggestion.icon_blob, }; diff --git a/browser/components/urlbar/tests/browser-tips/browser_selection.js b/browser/components/urlbar/tests/browser-tips/browser_selection.js @@ -28,7 +28,6 @@ add_task(async function tipIsSecondResult() { helpUrl: HELP_URL, descriptionL10n: { id: "urlbar-result-market-opt-in-description", - cacheable: true, parseMarkup: true, }, descriptionLearnMoreTopic: LEARN_MORE_TOPIC, @@ -146,7 +145,6 @@ add_task(async function tipIsOnlyResult() { helpUrl: HELP_URL, descriptionL10n: { id: "urlbar-result-market-opt-in-description", - cacheable: true, parseMarkup: true, }, descriptionLearnMoreTopic: LEARN_MORE_TOPIC, @@ -257,7 +255,6 @@ add_task(async function tipHasNoResultMenuButton() { buttonUrl: TIP_URL, descriptionL10n: { id: "urlbar-result-market-opt-in-description", - cacheable: true, parseMarkup: true, }, descriptionLearnMoreTopic: LEARN_MORE_TOPIC, diff --git a/browser/components/urlbar/tests/browser-tips/browser_tip_richSuggestion.js b/browser/components/urlbar/tests/browser-tips/browser_tip_richSuggestion.js @@ -134,6 +134,7 @@ add_task(async function learn_more() { titleL10n: { id: "urlbar-search-tips-confirm" }, descriptionL10n: { id: "firefox-suggest-onboarding-main-accept-option-label", + parseMarkup: true, }, descriptionLearnMoreTopic: topic, }, @@ -153,7 +154,10 @@ add_task(async function learn_more() { let learnMoreLink = row.querySelector( ".urlbarView-row-body-description > a" ); - Assert.equal(!!learnMoreLink, !!topic); + Assert.ok( + learnMoreLink, + "The descriptionL10n contains a learn-more link, so the element should have a learn-more link" + ); if (topic) { info("Activate learn more link and check"); diff --git a/browser/components/urlbar/tests/browser/browser_l10nCache.js b/browser/components/urlbar/tests/browser/browser_l10nCache.js @@ -37,7 +37,7 @@ add_task(async function comprehensive() { // // {object} l10n // An l10n object to pass to `L10nCache.setElementL10n()` and other - // `L10nCache` methods. It should never contain `cacheable`. + // `L10nCache` methods. // {Function} assert // This function is called as `await assert(span)` at the end of each // subtest and should check final state of the span after the cached string @@ -174,6 +174,7 @@ add_task(async function comprehensive() { assert: span => { checkAttributes(span, { label: "attrs1 label has zero args", + tooltiptext: "attrs1 tooltiptext arg value is foo", }); }, }, @@ -186,6 +187,7 @@ add_task(async function comprehensive() { }, assert: span => { checkAttributes(span, { + label: "attrs1 label has zero args", tooltiptext: "attrs1 tooltiptext arg value is foo", }); }, @@ -199,44 +201,35 @@ add_task(async function comprehensive() { let span = document.createElement("span"); - // Set the string without caching it. (We assume none of the `l10n` objects - // in `tests` have `cacheable` set, which they shouldn't.) + // Set the span's l10n. await cache.setElementL10n(span, l10n); - Assert.ok(!cache.get(l10n), "String should not be cached"); - Assert.equal( - span.dataset.l10nId, - l10n.id, - "span.dataset.l10nId should be set" + + Assert.ok( + cache.get(l10n), + "String should be cached after awaiting setElementL10n" ); - if (l10n.attribute) { - Assert.equal( - span.dataset.l10nAttrs, - l10n.attribute, - "span.dataset.l10nAttrs should be set" - ); + + // The cache should have set the l10n attributes. + checkL10n(span, l10n); + + // Clear the text content and attributes so we can verify they're set by the + // next `setElementL10n()`. + span.textContent = ""; + for (let name of span.getAttributeNames()) { + span.removeAttribute(name); } - // Set the string again but cache it this time. `setElementL10n()` will - // cache the string but not wait for it to finish being cached and then - // immediately look up the string in the cache, so it may or may not use the - // cached value at this point. - await cache.setElementL10n(span, { - ...l10n, - cacheable: true, - }); - Assert.ok(cache.get(l10n), "String should be cached"); - - // Set the string again. It's definitely cached now, so `setElementL10n()` - // should use the cached value. - let cachePromise = cache.setElementL10n(span, { - ...l10n, - cacheable: true, - }); + // Set the span l10n again but don't await the call. Since the string is now + // cached, `setElementL10n()` should synchronously update the span by + // setting its text content and attributes directly. + let cachePromise = cache.setElementL10n(span, l10n); + Assert.ok(cache.get(l10n), "String should still be cached"); - for (let a of ["data-l10n-id", "data-l10n-attrs", "data-l10n-args"]) { - Assert.ok(!span.hasAttribute(a), "Attribute should be unset: " + a); - } await assert(span); + + // The l10n attributes should still be set. + checkL10n(span, l10n); + await cachePromise; cache.clear(); @@ -256,21 +249,8 @@ add_task(async function removeElementL10n() { }; await cache.setElementL10n(span, l10n); - Assert.equal( - span.dataset.l10nId, - l10n.id, - "span.dataset.l10nId should be set" - ); - Assert.equal( - span.dataset.l10nAttrs, - l10n.attribute, - "span.dataset.l10nAttrs should be set" - ); - Assert.equal( - span.dataset.l10nArgs, - JSON.stringify(l10n.args), - "span.dataset.l10nArgs should be set" - ); + // The cache should have set the l10n attributes. + checkL10n(span, l10n); // Call `removeElementL10n()`. It should remove the l10n attributes. cache.removeElementL10n(span, l10n); @@ -280,8 +260,8 @@ add_task(async function removeElementL10n() { } }); -// Tests arg updates w/r/t `excludeArgsFromCacheKey`. -add_task(async function excludeArgsFromCacheKey() { +// A more real-world test that triggers `DOMLocalization` in a document. +add_task(async function inDocument() { // This task is more of a real-world test than the others. It relies on the // test element being present in a document so that the automatic translation // behavior of `DOMLocalization` is triggered. We'll use the current browser @@ -292,30 +272,22 @@ add_task(async function excludeArgsFromCacheKey() { registerCleanupFunction(() => span.remove()); // We'll also use a real string since that's easier than trying to inject mock - // strings. We need a string with an argument since we're specifically testing - // arg updates and `excludeArgsFromCacheKey`. + // strings. Use a string with an argument to test that too. let id = "urlbar-result-action-search-w-engine"; let arg = "engine"; let value = a => `Search with ${a}`; // Call `setElementL10n()` with an initial arg value for the string. - // `setElementL10n()` should set l10n attributes on the span since the string - // isn't cached. - await cache.setElementL10n(span, { + // `setElementL10n()` should set l10n attributes on the span. + let l10n1 = { id, args: { [arg]: "aaa" }, - cacheable: true, - excludeArgsFromCacheKey: true, - }); + }; + await cache.setElementL10n(span, l10n1); + checkL10n(span, l10n1); - Assert.equal(span.dataset.l10nId, id, "span.dataset.l10nId should be set"); - Assert.equal( - span.dataset.l10nArgs, - JSON.stringify({ [arg]: "aaa" }), - "span.dataset.l10nArgs should be set" - ); Assert.deepEqual( - cache.get({ id }), + cache.get(l10n1), { attributes: null, value: value("aaa"), @@ -323,35 +295,36 @@ add_task(async function excludeArgsFromCacheKey() { "String should be cached with 'aaa' arg" ); + // Wait for `DOMLocalization` to update the `textContent`. + await TestUtils.waitForCondition(() => { + info("Waiting for new textContent, current is: " + span.textContent); + return span.textContent == value("aaa"); + }, "Waiting for new textContent with 'aaa' arg value"); + // Call `setElementL10n()` again but with a different arg value. It should do // three things: (1) Cache the new value of the string but not wait for that // to finish, (2) immediately set the span's `textContent` to the currently // cached string, which remains the old value, and (3) set the span's l10n // attributes so that `DOMLocalization` will generate the string's new value // and assign it to the span's `textContent` when translation is done. - let cachePromise = cache.setElementL10n(span, { + let l10n2 = { id, args: { [arg]: "bbb" }, - cacheable: true, - excludeArgsFromCacheKey: true, - }); + }; + let cachePromise = cache.setElementL10n(span, l10n2); Assert.equal( span.textContent, value("aaa"), "span.textContent should be the old cached value with 'aaa'" ); - Assert.equal(span.dataset.l10nId, id, "span.dataset.l10nId should be set"); - Assert.equal( - span.dataset.l10nArgs, - JSON.stringify({ [arg]: "bbb" }), - "span.dataset.l10nArgs should be set with the new 'bbb' arg value" - ); + + checkL10n(span, l10n2); // The new string value should be cached. await cachePromise; Assert.deepEqual( - cache.get({ id }), + cache.get(l10n2), { attributes: null, value: value("bbb"), @@ -428,7 +401,33 @@ function checkAttributes(element, expected) { let attrs = {}; for (let i = 0; i < element.attributes.length; i++) { let a = element.attributes.item(i); - attrs[a.name] = a.value; + + // Ignore l10n-related attributes by skipping data attributes. The main + // `comprehensive` task checks these attributes. + if (!a.name.startsWith("data-")) { + attrs[a.name] = a.value; + } } Assert.deepEqual(attrs, expected, "Attributes should be correct"); } + +function checkL10n(element, expectedL10n) { + // `document.l10n.getAttributes` always returns an object with only `id` and + // `args` entries, and `args` will be null if there are no args. + // `element.dataset.l10nAttrs` is not included in this object. + Assert.deepEqual( + document.l10n.getAttributes(element), + { + id: expectedL10n.id, + args: expectedL10n.args ?? null, + }, + "The element should have the expected l10n attributes" + ); + + // Now check `dataset.l10nAttrs`. + Assert.equal( + element.dataset.l10nAttrs, + expectedL10n.attribute, + "element.dataset.l10nAttrs should be set or not as expected" + ); +} diff --git a/browser/components/urlbar/tests/engagementTelemetry/browser/browser_glean_telemetry_realtime_optin_engagement.js b/browser/components/urlbar/tests/engagementTelemetry/browser/browser_glean_telemetry_realtime_optin_engagement.js @@ -23,11 +23,9 @@ add_setup(async function () { icon: "chrome://browser/skin/illustrations/market-opt-in.svg", titleL10n: { id: "urlbar-result-market-opt-in-title", - cacheable: true, }, descriptionL10n: { id: "urlbar-result-market-opt-in-description", - cacheable: true, parseMarkup: true, }, }, diff --git a/browser/components/urlbar/tests/quicksuggest/QuickSuggestTestUtils.sys.mjs b/browser/components/urlbar/tests/quicksuggest/QuickSuggestTestUtils.sys.mjs @@ -942,7 +942,6 @@ class _QuickSuggestTestUtils { shouldShowUrl: true, bottomTextL10n: { id: "firefox-suggest-addons-recommended", - cacheable: true, }, helpUrl: lazy.QuickSuggest.HELP_URL, telemetryType: "amo", @@ -1000,7 +999,6 @@ class _QuickSuggestTestUtils { shouldShowUrl: true, bottomTextL10n: { id: "firefox-suggest-mdn-bottom-text", - cacheable: true, }, source: "rust", provider: "Mdn", @@ -1064,7 +1062,6 @@ class _QuickSuggestTestUtils { telemetryType: "yelp", bottomTextL10n: { id: "firefox-suggest-yelp-bottom-text", - cacheable: true, }, url, originalUrl, @@ -1135,8 +1132,6 @@ class _QuickSuggestTestUtils { unit: temperatureUnit.toUpperCase(), }, parseMarkup: true, - cacheable: true, - excludeArgsFromCacheKey: true, }; return { @@ -1152,7 +1147,6 @@ class _QuickSuggestTestUtils { bottomTextL10n: { id: "urlbar-result-weather-provider-sponsored", args: { provider: "AccuWeather®" }, - cacheable: true, }, source, provider, diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_importantDatesSuggestions.js b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_importantDatesSuggestions.js @@ -334,8 +334,6 @@ add_task(async function testTwoSuggestions() { descriptionL10n: { id: "urlbar-result-dates-countdown", args: { daysUntilStart: 4, name: "Event 1" }, - cacheable: true, - excludeArgsFromCacheKey: true, }, }); @@ -533,9 +531,7 @@ function makeExpectedResult({ query: name, lowerCaseSuggestion: name.toLocaleLowerCase(), description, - descriptionL10n: descriptionL10n - ? { cacheable: true, excludeArgsFromCacheKey: true, ...descriptionL10n } - : undefined, + descriptionL10n, isSponsored, telemetryType: "important_dates", source: "rust", diff --git a/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_yelp_realtime.js b/browser/components/urlbar/tests/quicksuggest/unit/test_quicksuggest_yelp_realtime.js @@ -415,14 +415,12 @@ function yelpOptInResult({ dismissButton = false } = {}) { command: "dismiss", l10n: { id: "urlbar-result-realtime-opt-in-dismiss", - cacheable: true, }, } : { command: "not_now", l10n: { id: "urlbar-result-realtime-opt-in-not-now", - cacheable: true, }, }; return { @@ -440,11 +438,9 @@ function yelpOptInResult({ dismissButton = false } = {}) { icon: "chrome://browser/skin/illustrations/yelpRealtime-opt-in.svg", titleL10n: { id: "urlbar-result-yelp-realtime-opt-in-title", - cacheable: true, }, descriptionL10n: { id: "urlbar-result-yelp-realtime-opt-in-description", - cacheable: true, parseMarkup: true, }, descriptionLearnMoreTopic: "firefox-suggest", @@ -453,7 +449,6 @@ function yelpOptInResult({ dismissButton = false } = {}) { command: "opt_in", l10n: { id: "urlbar-result-realtime-opt-in-allow", - cacheable: true, }, input: "coffee", attributes: { diff --git a/browser/components/urlbar/tests/quicksuggest/unit/xpcshell.toml b/browser/components/urlbar/tests/quicksuggest/unit/xpcshell.toml @@ -1,5 +1,7 @@ [DEFAULT] -run-if = ["os != 'android'"] +run-if = [ + "os != 'android'", +] head = "../../unit/head.js head.js" firefox-appdir = "browser" prefs = [ @@ -31,9 +33,12 @@ requesttimeoutfactor = 2 # Slow on Mac in verify mode ["test_quicksuggest_flight_status.js"] ["test_quicksuggest_importantDatesSuggestions.js"] +requesttimeoutfactor = 2 # Slow on Mac in verify mode ["test_quicksuggest_impressionCaps.js"] -skip-if = ["true"] # Bug 1880214 +skip-if = [ + "true", # Bug 1880214 +] ["test_quicksuggest_market.js"] diff --git a/browser/components/urlbar/tests/unit/test_l10nCache.js b/browser/components/urlbar/tests/unit/test_l10nCache.js @@ -337,320 +337,285 @@ add_task(async function comprehensive() { Assert.equal(cache.size(), 0, "The cache is empty on app locale change"); }); -// Tests the `excludeArgsFromCacheKey` option. -add_task(async function excludeArgsFromCacheKey() { +// Tests cache eviction. +add_task(async function eviction() { // Set up a mock localization. let l10n = initL10n({ args0: "Zero args value", args1: "One arg value is { $arg1 }", + args2: "Two arg values are { $arg1 } and { $arg2 }", + args3: "Three arg values are { $arg1 }, { $arg2 }, and { $arg3 }", + attrs0: [".label = attrs0 label has zero args"], attrs1: [ ".label = attrs1 label has zero args", ".tooltiptext = attrs1 tooltiptext arg value is { $arg1 }", ], + attrs2: [ + ".label = attrs2 label has zero args", + ".tooltiptext = attrs2 tooltiptext arg value is { $arg1 }", + ".alt = attrs2 alt arg values are { $arg1 } and { $arg2 }", + ], }); let cache = new L10nCache(l10n); - // Test cases. For each test case, we cache a string using one or more - // methods, `cache.add({ excludeArgsFromCacheKey: true })` and/or - // `cache.ensure({ excludeArgsFromCacheKey: true })`. After calling each - // method, we call `cache.get()` to get the cached string. - // - // Test cases are cumulative, so when `cache.add()` is called for a string and - // then `cache.ensure()` is called for the same string but with different l10n - // argument values, the string should be re-cached with the new values. - // - // Each item in the tests array is: `{ methods, obj, gets }` - // - // {array} methods - // Array of cache method names, one or more of: "add", "ensure" - // Methods are called in the order they are listed. - // {object} obj - // An l10n object that will be passed to the cache methods: - // `{ id, args, excludeArgsFromCacheKey }` - // {array} gets - // An array of objects that describes a series of calls to `cache.get()` and - // the expected return values: `{ obj, expected }` - // - // {object} obj - // An l10n object that will be passed to `cache.get():` - // `{ id, args, excludeArgsFromCacheKey }` - // {object} expected - // The expected return value from `get()`. - let tests = [ - // args0: string with no args and no attributes - { - methods: ["add", "ensure"], - obj: { - id: "args0", - excludeArgsFromCacheKey: true, + // Get the max cache entries per l10n ID. + let maxEntriesPerId = L10nCache.MAX_ENTRIES_PER_ID; + Assert.equal( + typeof maxEntriesPerId, + "number", + "MAX_ENTRIES_PER_ID should be a number" + ); + Assert.greater(maxEntriesPerId, 0, "MAX_ENTRIES_PER_ID should be > 0"); + + // Cache enough l10n objects with the same ID but different args to fill up + // the ID's cache entries. The args will be "aaa-0", "aaa-1", etc. + for (let i = 0; i < maxEntriesPerId; i++) { + let arg1 = "aaa-" + i; + let l10nObj = { + id: "args1", + args: { arg1 }, + }; + await cache.add(l10nObj); + + // The message should be cached. + Assert.deepEqual( + cache.get(l10nObj), + { + value: `One arg value is ${arg1}`, + attributes: null, }, - gets: [ - { - obj: { id: "args0" }, - expected: { - value: "Zero args value", - attributes: null, - }, - }, - { - obj: { id: "args0", excludeArgsFromCacheKey: true }, - expected: { - value: "Zero args value", - attributes: null, - }, - }, - ], - }, + "Message should be cached: " + JSON.stringify(l10nObj) + ); - // args1: string with one arg and no attributes - { - methods: ["add"], - obj: { - id: "args1", - args: { arg1: "ADD" }, - excludeArgsFromCacheKey: true, + // The cache size should be incremented. + Assert.equal( + cache.size(), + i + 1, + "Expected cache size after adding l10n obj: " + JSON.stringify(l10nObj) + ); + } + + // Check some l10n objects we did not cache. + for (let arg1 of [`aaa-${maxEntriesPerId}`, "some other value"]) { + let l10nObj = { + id: "args1", + args: { arg1 }, + }; + Assert.ok( + !cache.get(l10nObj), + "Message should not be cached since it wasn't added: " + + JSON.stringify(l10nObj) + ); + } + + // Now cache more l10n objects with the same ID as before but with new args: + // "bbb-0", "bbb-1", etc. Each time we cache a new object, the oldest "aaa" + // entry should be evicted since the ID's cache entries are filled up. + for (let i = 0; i < maxEntriesPerId; i++) { + let arg1 = "bbb-" + i; + let l10nObj = { + id: "args1", + args: { arg1 }, + }; + await cache.add(l10nObj); + + // The message should be cached. + Assert.deepEqual( + cache.get(l10nObj), + { + value: `One arg value is ${arg1}`, + attributes: null, }, - gets: [ - { - obj: { id: "args1" }, - expected: { - value: "One arg value is ADD", - attributes: null, - }, - }, - { - obj: { id: "args1", excludeArgsFromCacheKey: true }, - expected: { - value: "One arg value is ADD", - attributes: null, - }, - }, - { - obj: { - id: "args1", - args: { arg1: "some other value" }, - excludeArgsFromCacheKey: true, - }, - expected: { - value: "One arg value is ADD", - attributes: null, - }, - }, - { - obj: { - id: "args1", - args: { arg1: "some other value" }, - }, - expected: undefined, - }, - ], - }, - { - methods: ["ensure"], - obj: { + "Message should be cached: " + JSON.stringify(l10nObj) + ); + + // The cache size should remain maxed out. + Assert.equal( + cache.size(), + maxEntriesPerId, + "Cache size should remain maxed out after caching l10n obj: " + + JSON.stringify(l10nObj) + ); + + // The oldest "aaa" entry should have been evicted, and all previous oldest + // entries in prior iterations of this loop should remain evicted. + for (let j = 0; j < maxEntriesPerId; j++) { + let oldArg1 = "aaa-" + j; + let oldL10nObj = { id: "args1", - args: { arg1: "ENSURE" }, - excludeArgsFromCacheKey: true, - }, - gets: [ - { - obj: { id: "args1" }, - expected: { - value: "One arg value is ENSURE", - attributes: null, - }, - }, - { - obj: { id: "args1", excludeArgsFromCacheKey: true }, - expected: { - value: "One arg value is ENSURE", - attributes: null, - }, - }, - { - obj: { - id: "args1", - args: { arg1: "some other value" }, - excludeArgsFromCacheKey: true, - }, - expected: { - value: "One arg value is ENSURE", + args: { arg1: oldArg1 }, + }; + if (j <= i) { + Assert.deepEqual( + cache.get(oldL10nObj), + null, + "Message should be evicted for old l10n obj: " + + JSON.stringify(oldL10nObj) + ); + } else { + Assert.deepEqual( + cache.get(oldL10nObj), + { + value: `One arg value is ${oldArg1}`, attributes: null, }, - }, - { - obj: { - id: "args1", - args: { arg1: "some other value" }, - }, - expected: undefined, - }, - ], - }, + "Message should not yet be evicted for old l10n obj: " + + JSON.stringify(oldL10nObj) + ); + } + } + } - // attrs0: string with no args and one attribute - { - methods: ["add", "ensure"], - obj: { - id: "attrs0", - excludeArgsFromCacheKey: true, - }, - gets: [ - { - obj: { id: "attrs0" }, - expected: { - value: null, - attributes: { - label: "attrs0 label has zero args", - }, - }, - }, - { - obj: { id: "attrs0", excludeArgsFromCacheKey: true }, - expected: { - value: null, - attributes: { - label: "attrs0 label has zero args", - }, - }, - }, - ], - }, + // Now cache more l10n objects just like before but with a different ID. Since + // the ID is new, we should be able to fill up its cache entries. + for (let i = 0; i < maxEntriesPerId; i++) { + let arg1 = "yyy-" + i; + let arg2 = "zzz-" + i; + let l10nObj = { + id: "args2", + args: { arg1, arg2 }, + }; + await cache.add(l10nObj); - // attrs1: string with one arg and two attributes - { - methods: ["add"], - obj: { - id: "attrs1", - args: { arg1: "ADD" }, - excludeArgsFromCacheKey: true, - }, - gets: [ - { - obj: { id: "attrs1" }, - expected: { - value: null, - attributes: { - label: "attrs1 label has zero args", - tooltiptext: "attrs1 tooltiptext arg value is ADD", - }, - }, - }, - { - obj: { id: "attrs1", excludeArgsFromCacheKey: true }, - expected: { - value: null, - attributes: { - label: "attrs1 label has zero args", - tooltiptext: "attrs1 tooltiptext arg value is ADD", - }, - }, - }, - { - obj: { - id: "attrs1", - args: { arg1: "some other value" }, - excludeArgsFromCacheKey: true, - }, - expected: { - value: null, - attributes: { - label: "attrs1 label has zero args", - tooltiptext: "attrs1 tooltiptext arg value is ADD", - }, - }, - }, - { - obj: { - id: "attrs1", - args: { arg1: "some other value" }, - }, - expected: undefined, - }, - ], - }, - { - methods: ["ensure"], - obj: { - id: "attrs1", - args: { arg1: "ENSURE" }, - excludeArgsFromCacheKey: true, + // The message should be cached. + Assert.deepEqual( + cache.get(l10nObj), + { + value: `Two arg values are ${arg1} and ${arg2}`, + attributes: null, }, - gets: [ - { - obj: { id: "attrs1" }, - expected: { - value: null, - attributes: { - label: "attrs1 label has zero args", - tooltiptext: "attrs1 tooltiptext arg value is ENSURE", - }, - }, - }, - { - obj: { id: "attrs1", excludeArgsFromCacheKey: true }, - expected: { - value: null, - attributes: { - label: "attrs1 label has zero args", - tooltiptext: "attrs1 tooltiptext arg value is ENSURE", - }, - }, - }, - { - obj: { - id: "attrs1", - args: { arg1: "some other value" }, - excludeArgsFromCacheKey: true, - }, - expected: { - value: null, - attributes: { - label: "attrs1 label has zero args", - tooltiptext: "attrs1 tooltiptext arg value is ENSURE", - }, - }, - }, - { - obj: { - id: "attrs1", - args: { arg1: "some other value" }, - }, - expected: undefined, - }, - ], - }, - ]; + "Message should be cached: " + JSON.stringify(l10nObj) + ); - let sandbox = sinon.createSandbox(); - let spy = sandbox.spy(cache, "add"); + // The cache size should start increasing again since we're caching l10n + // objects with a different ID from before. + Assert.equal( + cache.size(), + maxEntriesPerId + i + 1, + "Cache size should start increasing again: " + JSON.stringify(l10nObj) + ); - for (let { methods, obj, gets } of tests) { - for (let method of methods) { - info(`Calling method '${method}' with l10n obj: ` + JSON.stringify(obj)); - await cache[method](obj); + // All the messages with the "args1" ID from above should remain cached. + for (let j = 0; j < maxEntriesPerId; j++) { + let prevArg1 = "bbb-" + j; + let prevL10nObj = { + id: "args1", + args: { arg1: prevArg1 }, + }; + Assert.deepEqual( + cache.get(prevL10nObj), + { + value: `One arg value is ${prevArg1}`, + attributes: null, + }, + "Previous message should remain cached: " + JSON.stringify(prevL10nObj) + ); + } + } - // `add()` should always be called: We either just called it directly, or - // `ensure({ excludeArgsFromCacheKey: true })` called it. + // Now re-cache some of the previously cached "args1" messages. This should + // reorder the "args1" cache entries so that these re-cached messages are most + // recently used. We'll re-cache messages with even-numbered args values. + for (let i = 0; i < maxEntriesPerId; i++) { + if (i % 2 == 0) { + let arg1 = "bbb-" + i; + let l10nObj = { + id: "args1", + args: { arg1 }, + }; Assert.ok( - spy.calledOnce, - "add() should have been called once: " + JSON.stringify(obj) + await cache.get(l10nObj), + "Sanity check: Message should still be cached: " + + JSON.stringify(l10nObj) + ); + await cache.add(l10nObj); + + // The cache size should remain maxed out. + Assert.equal( + cache.size(), + 2 * maxEntriesPerId, + "Cache size should remain maxed out after caching l10n obj: " + + JSON.stringify(l10nObj) ); - spy.resetHistory(); + } + } - for (let { obj: getObj, expected } of gets) { + // Build a list of args in the expected cached "args1" entries sorted from + // least recently used to most recently used. Since we just re-cached messages + // with even-numbered args, they should be at the end of this list, and + // messages with odd-numbered args should be at the front. + let expected = []; + for (let i = 0; i < maxEntriesPerId; i++) { + if (i % 2) { + // odd + expected.push("bbb-" + i); + } + } + for (let i = 0; i < maxEntriesPerId; i++) { + if (i % 2 == 0) { + // even + expected.push("bbb-" + i); + } + } + + // Now cache more l10n objects with the same "args1" ID but with new args. + // The old "bbb" entries should be evicted in the expected order. + for (let i = 0; i < maxEntriesPerId; i++) { + let arg1 = "ccc-" + i; + let l10nObj = { + id: "args1", + args: { arg1 }, + }; + await cache.add(l10nObj); + + // The message should be cached. + Assert.deepEqual( + cache.get(l10nObj), + { + value: `One arg value is ${arg1}`, + attributes: null, + }, + "Message should be cached: " + JSON.stringify(l10nObj) + ); + + // The cache size should remain maxed out. + Assert.equal( + cache.size(), + 2 * maxEntriesPerId, + "Cache size should remain maxed out after caching l10n obj: " + + JSON.stringify(l10nObj) + ); + + // The oldest entry should have been evicted, and all previous oldest + // entries in prior iterations of this loop should remain evicted. + for (let j = 0; j < expected.length; j++) { + let oldArg1 = expected[j]; + let oldL10nObj = { + id: "args1", + args: { arg1: oldArg1 }, + }; + if (j <= i) { Assert.deepEqual( - cache.get(getObj), - expected, - "Expected message for get: " + JSON.stringify(getObj) + cache.get(oldL10nObj), + null, + "Message should be evicted for old l10n obj: " + + JSON.stringify(oldL10nObj) + ); + } else { + Assert.deepEqual( + cache.get(oldL10nObj), + { + value: `One arg value is ${oldArg1}`, + attributes: null, + }, + "Message should not yet be evicted for old l10n obj: " + + JSON.stringify(oldL10nObj) ); } } } - - sandbox.restore(); }); /**