From 1b8f43abb6b3a7adaccc18eab89f4e3fb7a22371 Mon Sep 17 00:00:00 2001 From: Gijs Kruitbosch Date: Wed, 2 Apr 2025 18:41:13 +0000 Subject: [PATCH] Bug 1951773 - add some documentation for callModulesFromCategory, r=mconley,frontend-codestyle-reviewers,Standard8 Differential Revision: https://phabricator.services.mozilla.com/D244049 --- .eslintrc-rollouts.js | 5 +- browser/docs/BrowserStartup.md | 36 ++++++ browser/docs/CategoryManagerIndirection.md | 138 +++++++++++++++++++++ browser/docs/index.rst | 1 + docs/conf.py | 1 + toolkit/modules/BinarySearch.sys.mjs | 12 +- toolkit/modules/BrowserUtils.sys.mjs | 57 +++++---- 7 files changed, 214 insertions(+), 36 deletions(-) create mode 100644 browser/docs/CategoryManagerIndirection.md diff --git a/.eslintrc-rollouts.js b/.eslintrc-rollouts.js index d3bbae68d687..bec812f909a4 100644 --- a/.eslintrc-rollouts.js +++ b/.eslintrc-rollouts.js @@ -391,7 +391,10 @@ const rollouts = [ "toolkit/components/workerloader/require.js", "toolkit/content/**", "toolkit/crashreporter/**", - "toolkit/modules/**", + "toolkit/modules/{A,C,D,E,F,G,I,J,K,L,N,O,P,R,S,U,W}*.sys.mjs", + "toolkit/modules/sessionstore/**", + "toolkit/modules/subprocess/**", + "toolkit/modules/tests/**", "toolkit/mozapps/downloads/**", "toolkit/mozapps/extensions/**", "toolkit/mozapps/handling/**", diff --git a/browser/docs/BrowserStartup.md b/browser/docs/BrowserStartup.md index 02d9f03f98c4..3338ba759d97 100644 --- a/browser/docs/BrowserStartup.md +++ b/browser/docs/BrowserStartup.md @@ -1,5 +1,41 @@ # Browser Startup +## Invoking your code on browser startup + +The first rule of running code during startup is: don't. + +We take performance very seriously and ideally your component/feature should +initialize only when needed. + +If you have established that you really must run code during startup, +available entrypoints are: + +- registering a `browser-idle-startup` category entry for your JS module (or + even a "best effort" user idle task, see `BrowserGlue.sys.mjs`) +- registering a `browser-window-delayed-startup` category entry for your JS + module. **Note that this is invoked for each browser window.** +- registering a `browser-before-ui-startup` category entry if you really really + need to. This will run code before the first browser window appears on the + screen and make Firefox seem slow, so please don't do it unless absolutely + necessary. + +See [the category manager indirection docs](./CategoryManagerIndirection.md) for +more details on this. + +Other useful points in startup are: +- `BrowserGlue`'s `_onFirstWindowLoaded` (which should be converted to use a + category manager call instead), which fires after the first browser window's + `browser-window-delayed-startup` call (see above). +- `BrowserGlue`'s `_scheduleBestEffortUserIdleTasks` as mentioned above. + Note that in this case, your code **may not run at all** if the browser is + shut down quickly. +- `BrowserGlue`'s `_onWindowsRestored`, and/or the observer service's + `sessionstore-windows-restored` topic, and/or a category manager call that + should replace the `BrowserGlue` list of direct calls. This fires after + session restore has completed restoring all windows (but before all pages + that may have been restored have necessarily loaded). Note that this is + guaranteed to fire even if automatic session restore is not enabled. + ## How do first run/first startup experiments work? Why does synchronously reading Nimbus feature values work for diff --git a/browser/docs/CategoryManagerIndirection.md b/browser/docs/CategoryManagerIndirection.md new file mode 100644 index 000000000000..77ee9051ca5e --- /dev/null +++ b/browser/docs/CategoryManagerIndirection.md @@ -0,0 +1,138 @@ +# Category manager indirection (callModulesFromCategory) + +Firefox front-end code uses the category manager as a publish/subscribe +mechanism for dependency injection, so consumers can be notified of interesting +things happening without having to directly talk to the publisher/actor who +decides the interesting thing is happening. + +There are 2 parts to this: + +1. Consumers registering with the category manager +2. Publishers/actors invoking consumers via the category manager. + +## Consumer registration with the category manager. + +The category manager is used for various purposes within Firefox; it is more or +less an arbitrary double string-keyed data store. + +For this particular usecase, the publisher/consumer have to use the same +primary key (i.e. category name), such as `browser-idle-startup`. + +The secondary key is a full URL to a `sys.mjs` module. Note that because this +is a key, **only one consumer per module & category combination is possible**. + +The "value" part is an `Object.method` notation, where the expectation is that +`Object` is an exported symbol from the module identified as the secondary key, +and `method` is some method on that object. + +At compile-time, registration can happen with an entry in a `.manifest` file +like [BrowserComponents.manifest](https://searchfox.org/mozilla-central/source/browser/components/BrowserComponents.manifest). +Note that any manifest successfully processed by the build system would do, +we don't need to use `BrowserComponents.manifest` specifically. In fact, it +would be preferable if components used their own manifest files. + +An example registration looks like: + +``` +category browser-idle-startup moz-src://browser/components/tabbrowser/TabUnloader.sys.mjs TabUnloader.init +``` + +This will ensure that when the `browser-idle-startup` publisher is invoked, +the `TabUnloader.sys.mjs` module is loaded and the `init` method on the exported +`TabUnloader` object is invoked. + +### Runtime registration + +Runtime registration is less-often used, but can be done using the category +manager's XPCOM API: + +```js +Services.catMan.addCategoryEntry( + "browser-idle-startup", + "moz-src://browser/components/tabbrowser/TabUnloader.sys.mjs", + "TabUnloader.init" +) +``` + +## Publishers/actors invoking consumers + +Publishers call `BrowserUtils.callModulesFromCategory` with a dictionary of +options as the first argument. If provided, any other arguments are passed +straight through to any consumers. + +```{js:autofunction} BrowserUtils.callModulesFromCategory +``` + +Example: + +```js +BrowserUtils.callModulesFromCategory({ + categoryName: "my-fancy-category-name", + profilerMarker: "markMyCategories", + idleDispatch: true, + someArgument +}); +``` + +This will pass `someArgument` to each consumer registered for +`my-fancy-category-name`. Each consumer will be invoked via an idle task, and +each task will get a profiler marker (labelled `"markMyCategories"`) in the +[Firefox Profiler](https://profiler.firefox.com/) so it's easy to find in +performance profiles. + +You should consider using `idleDispatch: true` if invocation of the consumers +does not need to happen synchronously. + +If you need to care about errors produced by consumers, you can specify +a function for `failureHandler` and handle any exceptions/errors using your own +logic. Note that it may be invoked asynchronously if the consumers are async. + +## Caveats + +Any errors thrown by consumers are automatically caught and reported via the +[Browser Console](/devtools-user/browser_console/index.rst). + +Async functions are not awaited before invoking other consumers. Note that +rejections (exceptions from async code) are still caught and reported to the +console, and that the async duration of a given consumer will be what +determines the length of the profiler marker, if the publisher asks for profiler +markers. + +## Why not just call consumers directly? + +There are a number of benefits over direct method calls and module imports. + +### Reducing direct dependencies between different parts of the code-base + +Code that looks like this: + +``` +Foo.thingHappened(arg); +Bar.thingHappened(arg); +Baz.thingHappened(arg); +``` + +is not only repetitive, it also means that the code in question has to directly +import all the modules that provide `Foo`, `Bar` and `Baz`. It means that if +those modules change or move or are refactored, the "publisher" code has to +be updated, with all the added burdens that comes with (potential for merge +conflicts, more automatically added reviewer groups for trivial changes, easy +to miss if dependencies are widespread). + +### Avoiding a bootstrap problem in favour of a "just in time" approach + +To make sure code is invoked later, when using the observer service, DOM event +listeners, or other mechanisms, it usually needs to add a listener +before the event of interest happens. If not managed carefully, this often leads +to component initialization being front-loaded to make sure not to "miss" it +later. This in turn makes browser startup more heavyweight rather than it needs +to be, because we set up listeners for _everything_, potentially loading entire +JS modules just to do that. + +### Unified error-handling, performance inspection, and scheduling + +Using the `BrowserUtils.callModulesFromCategory` API allows specifying error +handling, performance profiler markers, and scheduling (use of idle tasks) in +one place. This abstracts away the fact that we never want observers, event +listeners or other mechanisms like this to break and stop notifying (or worse, +propagate an exception themselves) when one of the consumers breaks. diff --git a/browser/docs/index.rst b/browser/docs/index.rst index 1736bb4ebd13..d7f1f4547548 100644 --- a/browser/docs/index.rst +++ b/browser/docs/index.rst @@ -12,6 +12,7 @@ This is the nascent documentation of the Firefox front-end code. FrontendCodeReviewBestPractices CommandLineParameters BrowserStartup + CategoryManagerIndirection components/customizableui/docs/index components/enterprisepolicies/docs/index extensions/formautofill/docs/index diff --git a/docs/conf.py b/docs/conf.py index 31e0811813ec..f3684c780a92 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -83,6 +83,7 @@ js_source_path = [ "../toolkit/components/extensions", "../toolkit/components/extensions/parent", "../toolkit/components/ml/content/backends/ONNXPipeline.mjs", + "../toolkit/modules/BrowserUtils.sys.mjs", "../toolkit/mozapps/extensions", "../toolkit/components/prompts/src", "../toolkit/components/pictureinpicture", diff --git a/toolkit/modules/BinarySearch.sys.mjs b/toolkit/modules/BinarySearch.sys.mjs index 7697b7704442..4615c5726eb6 100644 --- a/toolkit/modules/BinarySearch.sys.mjs +++ b/toolkit/modules/BinarySearch.sys.mjs @@ -9,7 +9,7 @@ export var BinarySearch = Object.freeze({ * * See search() for a description of this function's parameters. * - * @return The index of `target` in `array` or -1 if `target` is not found. + * @returns {integer} The index of `target` in `array` or -1 if `target` is not found. */ indexOf(comparator, array, target) { let [found, idx] = this.search(comparator, array, target); @@ -22,7 +22,7 @@ export var BinarySearch = Object.freeze({ * * See search() for a description of this function's parameters. * - * @return The index in `array` where `target` may be inserted to keep `array` + * @returns {integer} The index in `array` where `target` may be inserted to keep `array` * ordered. */ insertionIndexOf(comparator, array, target) { @@ -32,18 +32,18 @@ export var BinarySearch = Object.freeze({ /** * Searches for the given target in the given array. * - * @param comparator + * @param {(a: any, b: any) => number} comparator * A function that takes two arguments and compares them, returning a * negative number if the first should be ordered before the second, * zero if the first and second have the same ordering, or a positive * number if the second should be ordered before the first. The first * argument is always `target`, and the second argument is a value * from the array. - * @param array + * @param {Array} array * An array whose elements are ordered by `comparator`. - * @param target + * @param {any} target * The value to search for. - * @return An array with two elements. If `target` is found, the first + * @returns {Array} An array with two elements. If `target` is found, the first * element is true, and the second element is its index in the array. * If `target` is not found, the first element is false, and the * second element is the index where it may be inserted to keep the diff --git a/toolkit/modules/BrowserUtils.sys.mjs b/toolkit/modules/BrowserUtils.sys.mjs index 257d72c6dcf7..3f147f9aa369 100644 --- a/toolkit/modules/BrowserUtils.sys.mjs +++ b/toolkit/modules/BrowserUtils.sys.mjs @@ -103,6 +103,13 @@ function stringPrefToSet(prefVal) { ); } +/** + * @class BrowserUtils + * + * A motley collection of utilities (also known as a dumping ground). + * + * Please avoid expanding this if possible. + */ export var BrowserUtils = { /** * Return or create a principal with the content of one, and the originAttributes @@ -110,12 +117,12 @@ export var BrowserUtils = { * not to change, that is, we should keep the userContextId, privateBrowsingId, * etc. the same when changing the principal). * - * @param principal + * @param {nsIPrincipal} principal * The principal whose content/null/system-ness we want. - * @param existingPrincipal + * @param {nsIPrincipal} existingPrincipal * The principal whose originAttributes we want, usually the current * principal of a docshell. - * @return an nsIPrincipal that matches the content/null/system-ness of the first + * @returns {nsIPrincipal} an nsIPrincipal that matches the content/null/system-ness of the first * param, and the originAttributes of the second. */ principalWithMatchingOA(principal, existingPrincipal) { @@ -166,7 +173,7 @@ export var BrowserUtils = { /** * Returns true if |mimeType| is text-based, or false otherwise. * - * @param mimeType + * @param {string} mimeType * The MIME type to check. */ mimeTypeIsTextBased(mimeType) { @@ -213,7 +220,7 @@ export var BrowserUtils = { * * @param {string} topic * The topic to observe. - * @param {function(nsISupports, string)} [test] + * @param {(subject: nsISupports, data: string) => boolean} [test] * An optional test function which, when called with the * observer's subject and data, should return true if this is the * expected notification, false otherwise. @@ -221,7 +228,7 @@ export var BrowserUtils = { */ promiseObserved(topic, test = () => true) { return new Promise(resolve => { - let observer = (subject, topic, data) => { + let observer = (subject, _topic, data) => { if (test(subject, data)) { Services.obs.removeObserver(observer, topic); resolve({ subject, data }); @@ -337,11 +344,11 @@ export var BrowserUtils = { /** * Extracts linkNode and href for a click event. * - * @param event + * @param {UIEvent} event * The click event. - * @return [href, linkNode, linkPrincipal]. + * @returns {Array} [href, linkNode, linkPrincipal]. * - * @note linkNode will be null if the click wasn't on an anchor + * Note that linkNode will be null if the click wasn't on an anchor * element. This includes SVG links, because callers expect |node| * to behave like an element, which SVG links (XLink) don't. */ @@ -419,9 +426,10 @@ export var BrowserUtils = { * - Alt can't be used on the bookmarks toolbar because Alt is used for "treat this as something draggable". * - The button is ignored for the middle-click-paste-URL feature, since it's always a middle-click. * - * @param e {Event|Object} Event or JSON Object - * @param ignoreButton {Boolean} - * @param ignoreAlt {Boolean} + * @param {Event|object} e + * Event or JSON Object + * @param {boolean} ignoreButton + * @param {boolean} ignoreAlt * @returns {"current" | "tabshifted" | "tab" | "save" | "window"} */ whereToOpenLink(e, ignoreButton, ignoreAlt) { @@ -493,32 +501,23 @@ export var BrowserUtils = { /** * Invoke all the category manager consumers of a given JS consumer. - * Similar to the (C++-only) NS_CreateServicesFromCategory in that it'll + * Similar to the (C++-only) ``NS_CreateServicesFromCategory`` in that it'll * abstract away the actual work of invoking the modules/services. * Different in that it's JS-only and will invoke methods in modules * instead of using XPCOM services. * - * The main benefits of using this over direct calls are: - * - error handling (one consumer throwing an exception doesn't stop the - * others being called) - * - dependency injection (callsite doesn't have to [lazy] import half - * the world to call all the methods) - * - performance/bootstrapping using build-time registration, when - * compared to nsIObserver or events: with nsIObserver/handleEvent, - * you'd have to call addObserver or addEventListener somewhere, which - * means either loading your code early (bad for performance) or burdening - * other code that already runs early with adding your handlers (not great - * for code cleanliness). + * More context is available in + * https://firefox-source-docs.mozilla.org/browser/CategoryManagerIndirection.html * - * @param {Object} options + * @param {object} options * @param {string} options.categoryName - * What category's consumers to call + * What category's consumers to call. * @param {boolean} [options.idleDispatch=false] * If set to true, call each consumer in an idle task. * @param {string} [options.profilerMarker=""] * If specified, will create a profiler marker with the provided * identifier for each consumer. - * @param {function} [options.failureHandler] + * @param {Function} [options.failureHandler] * If specified, will be called for any exceptions raised, in * order to do custom failure handling. * @param {...any} args @@ -580,7 +579,7 @@ export var BrowserUtils = { /** * Returns whether the build is a China repack. * - * @return {boolean} True if the distribution ID is 'MozillaOnline', + * @returns {boolean} True if the distribution ID is 'MozillaOnline', * otherwise false. */ isChinaRepack() { @@ -616,7 +615,7 @@ export var BrowserUtils = { * * @param {BrowserUtils.PromoType} promoType - What promo are we checking on? * - * @return {boolean} - should we display this promo now or not? + * @returns {boolean} - should we display this promo now or not? */ shouldShowPromo(promoType) { switch (promoType) {