Closed
Bug 1363925
Opened 7 years ago
Closed 6 years ago
Move add-on installation code to a separate JSM
Categories
(Toolkit :: Add-ons Manager, enhancement, P3)
Toolkit
Add-ons Manager
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: kmag, Assigned: aswan)
References
(Blocks 1 open bug)
Details
(Whiteboard: [triaged][fxperf:p3])
Attachments
(14 files)
59 bytes,
text/x-review-board-request
|
kmag
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mccr8
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
zombie
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
aswan
:
review+
|
Details |
We currently load a lot of code in XPIProvider.jsm that isn't needed at all in the vast majority of sessions, which adds a lot of unnecessary overhead to startup. This in particular applies to the code only needed during add-on installation, including code for parsing install and update manifests. There's quite a lot of code in XPIProviderUtils which is likewise only needed when installing new add-ons, but not when simply loading the database to read metadata. All of this code, and anything else that isn't needed during startup, should be moved to a separate JSM.
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [triaged]
Assignee | ||
Comment 1•7 years ago
|
||
I got part way through moving the install and update code into its own jsm. There are of course dependencies between that code and the code currently in XPIProvider.jsm but also between that code and the code in XPIProviderUtils.js. Is there a good reason to have 3 separate files here? I'm thinking the current (intended) distinction between stuff needed at startup in XPIProvider.jsm and everything else in XPIProviderUtils.js should be sufficient and I can just move a bunch more stuff into XPIProviderUtils.js. Am I overlooking something?
Flags: needinfo?(kmaglione+bmo)
Reporter | ||
Comment 2•7 years ago
|
||
Yes. Having a separate JSM will allow us to delay loading the installation code and update code later than the database code, since it's needed much less often, and allow it to be unloaded when idle after bug 1354820 lands.
Flags: needinfo?(kmaglione+bmo)
Comment 3•7 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #1) > I got part way through moving the install and update code into its own jsm. > There are of course dependencies between that code and the code currently in > XPIProvider.jsm but also between that code and the code in > XPIProviderUtils.js. Is there a good reason to have 3 separate files here? > I'm thinking the current (intended) distinction between stuff needed at > startup in XPIProvider.jsm and everything else in XPIProviderUtils.js should > be sufficient and I can just move a bunch more stuff into > XPIProviderUtils.js. Am I overlooking something? It's worth considering at this point giving these more descriptive names - I don't find "Utils" to be particularly helpful in identifying how the code is split up.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Updated•7 years ago
|
Attachment #8870912 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 5•7 years ago
|
||
There's much more to do here but it looks like this buys us about 10ms on ts_paint: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=96e18bec9fc8&newProject=try&newRevision=cacc940116a0fda039cb716858baa48f34dedada&framework=1&filter=ts_paint&showOnlyImportant=0
Reporter | ||
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8870912 [details] Bug 1363925 Split some install code out of XPIProvider.jsm https://reviewboard.mozilla.org/r/142478/#review146248 \o/ ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1105 (Diff revision 1) > - * the file when it is no longer needed. > * > - * @return an nsIFile that points to a randomly named, initially empty file in > - * the OS temporary files directory > + * @param aFile > + * The nsIFile to remove > */ > -function getTemporaryFile() { > +function recursiveRemove(aFile) { Any reason this can't be moved to XPIInstall.jsm? ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6893 (Diff revision 1) > +Object.defineProperty(this.XPIInternal, "XPIDatabase", { > + get: () => gGlobalScope.XPIDatabase > +}); Just add `get XPIDatabase() { return gGlobalScope.XPIDatabase; }` to `XPIInternal`.
Attachment #8870912 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8870912 [details] Bug 1363925 Split some install code out of XPIProvider.jsm https://reviewboard.mozilla.org/r/142478/#review146248 > Any reason this can't be moved to XPIInstall.jsm? that's on my hit list for round 2
Comment 9•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s f3839056eabb -d f8948f4cb4ac: rebasing 398219:f3839056eabb "Bug 1363925 Split some install code out of XPIProvider.jsm r=kmag" (tip) merging toolkit/mozapps/extensions/internal/XPIProvider.jsm and toolkit/mozapps/extensions/internal/XPIInstall.jsm to toolkit/mozapps/extensions/internal/XPIInstall.jsm merging toolkit/mozapps/extensions/internal/XPIProvider.jsm warning: conflicts while merging toolkit/mozapps/extensions/internal/XPIInstall.jsm! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5ae2cc64391c Split some install code out of XPIProvider.jsm r=kmag
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5ae2cc64391c
Updated•6 years ago
|
Whiteboard: [triaged] → [triaged][fxperf]
Updated•6 years ago
|
Whiteboard: [triaged][fxperf] → [triaged][fxperf:p3]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Updated•6 years ago
|
Attachment #8969963 -
Flags: review?(aswan) → review?(tomica)
Attachment #8969964 -
Flags: review?(aswan) → review?(tomica)
Attachment #8969965 -
Flags: review?(aswan) → review?(tomica)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8969959 [details] Bug 1363925: Part 2 - Support inferring array length from typed arrays. https://reviewboard.mozilla.org/r/238740/#review244648 I'm not really familiar with this API, but it looks okay to me...
Attachment #8969959 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8969960 [details] Bug 1363925: Part 3 - Move more install logic from XPIProvider to XPIInstall. https://reviewboard.mozilla.org/r/238742/#review244682 ::: xpcom/io/nsIBinaryOutputStream.idl:58 (Diff revision 2) > - void writeBytes([size_is(aLength)] in string aString, in uint32_t aLength); > + void writeBytes([size_is(aLength)] in string aString, > + [optional] in uint32_t aLength); was this meant to be in the previous patch?
Reporter | ||
Comment 37•6 years ago
|
||
(In reply to Andrew Swan [:aswan] from comment #36) > Comment on attachment 8969960 [details] > Bug 1363925: Part 3 - Move more install logic from XPIProvider to XPIInstall. > > https://reviewboard.mozilla.org/r/238742/#review244682 > > ::: xpcom/io/nsIBinaryOutputStream.idl:58 > (Diff revision 2) > > - void writeBytes([size_is(aLength)] in string aString, in uint32_t aLength); > > + void writeBytes([size_is(aLength)] in string aString, > > + [optional] in uint32_t aLength); > > was this meant to be in the previous patch? Nope. It more or less depends on the previous patch, but it's needed for this patch, and seemed like a trivial enough change not to need its own.
Assignee | ||
Comment 38•6 years ago
|
||
mozreview-review |
Comment on attachment 8969960 [details] Bug 1363925: Part 3 - Move more install logic from XPIProvider to XPIInstall. https://reviewboard.mozilla.org/r/238742/#review244724
Attachment #8969960 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8969961 [details] Bug 1363925: Part 4 - Move XPIProvider install methods to XPIInstall. https://reviewboard.mozilla.org/r/238744/#review244730
Attachment #8969961 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 40•6 years ago
|
||
mozreview-review |
Comment on attachment 8969962 [details] Bug 1363925: Part 5 - Move startup update check logic to XPIInstall. https://reviewboard.mozilla.org/r/238746/#review244732 Hm, see also bug 1433574
Assignee | ||
Comment 41•6 years ago
|
||
mozreview-review |
Comment on attachment 8969962 [details] Bug 1363925: Part 5 - Move startup update check logic to XPIInstall. https://reviewboard.mozilla.org/r/238746/#review244742 If we need to get an ack from product before doing bug 1433574 then this is good in the mean time
Attachment #8969962 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 42•6 years ago
|
||
mozreview-review |
Comment on attachment 8969963 [details] Bug 1363925: Part 6 - Move staged add-on install logic to XPIInstall. https://reviewboard.mozilla.org/r/238748/#review244746
Attachment #8969963 -
Flags: review+
Assignee | ||
Comment 43•6 years ago
|
||
mozreview-review |
Comment on attachment 8969963 [details] Bug 1363925: Part 6 - Move staged add-on install logic to XPIInstall. https://reviewboard.mozilla.org/r/238748/#review244748
Assignee | ||
Comment 44•6 years ago
|
||
mozreview-review |
Comment on attachment 8970041 [details] Bug 1363925: Part 8a - Migrate XPIProviderUtils.js to XPIDatabase.jsm. https://reviewboard.mozilla.org/r/238794/#review244752
Attachment #8970041 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 45•6 years ago
|
||
mozreview-review |
Comment on attachment 8970042 [details] Bug 1363925: Part 8b - Move AddonInternal to XPIDatabase.jsm. https://reviewboard.mozilla.org/r/238796/#review244756
Attachment #8970042 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8970043 [details] Bug 1363925: Part 8c - Move isUsableAddon to XPIDatabase.jsm. https://reviewboard.mozilla.org/r/238798/#review244758
Attachment #8970043 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 47•6 years ago
|
||
mozreview-review |
Comment on attachment 8970044 [details] Bug 1363925: Part 8d - Move updateAddonDisabledState to XPIDatabase. https://reviewboard.mozilla.org/r/238800/#review244764 This is a general comment, not specific to this (sub-)part. I think it would be helpful to add comments somewhere explaining the purposes of the various XPIfoo.jsm files, explaining the goal of loading just the bare minimum amount of code for "normal case" startup, and explaining how somebody adding something new should decide in which file it should live. Near the top of XPIProvider.jsm would be as good a place as any I guess.
Attachment #8970044 -
Flags: review?(aswan) → review+
Assignee | ||
Comment 48•6 years ago
|
||
mozreview-review |
Comment on attachment 8970045 [details] Bug 1363925: Part 8e - Convert AddonInternal classes to ES6 classes. https://reviewboard.mozilla.org/r/238802/#review244766
Attachment #8970045 -
Flags: review?(aswan) → review+
Reporter | ||
Comment 49•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/af2ff7d301a7b481423a8ec172f92c8bbacaf285 Bug 1363925: Part 2 - Support inferring array length from typed arrays. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/cffb4b6e971a38219fab7fa49fd8d6565e986394 Bug 1363925: Part 3 - Move more install logic from XPIProvider to XPIInstall. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/801a6d33fa88b3030a3989d602a34237fb467e5c Bug 1363925: Part 4 - Move XPIProvider install methods to XPIInstall. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/afd6f3e6e6af154925a2c13bd363d38187cecf2a Bug 1363925: Part 6 - Move staged add-on install logic to XPIInstall. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/ee1bdb001079770b20899df675e6cd0640a2d5fe Bug 1363925: Part 7a - Turn on valid-jsdoc rule for XPIInstall.jsm. r=me,npotb https://hg.mozilla.org/integration/mozilla-inbound/rev/2132b4cf18db3a8c0ab9ffc4977be05ec58748b1 Bug 1363925: Part 7b - Turn on valid-jsdoc rule for XPIProvider.jsm. r=me,npotb https://hg.mozilla.org/integration/mozilla-inbound/rev/fde77a548f547094e5c06a30f927175c03ad23ae Bug 1363925: Part 7c - Turn on valid-jsdoc rule for XPIProviderUtils.js. r=me,npotb https://hg.mozilla.org/integration/mozilla-inbound/rev/20d1663e4a8cd7c1aa2bdef66c6b0e92bf8b81f5 Bug 1363925: Part 8a - Migrate XPIProviderUtils.js to XPIDatabase.jsm. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/8adbe703aeba227b6ec186c306a42b291b0b627f Bug 1363925: Part 8b - Move AddonInternal to XPIDatabase.jsm. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/1646b51d323f8d98211fb48436d6204b22c6044e Bug 1363925: Part 8c - Move isUsableAddon to XPIDatabase.jsm. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/c948486ee8e1faed6cd9f6200771ae40a9232e6a Bug 1363925: Part 8d - Move updateAddonDisabledState to XPIDatabase. r=aswan https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad2e050c66a73c748761d451d2c7a5339501773 Bug 1363925: Part 8e - Convert AddonInternal classes to ES6 classes. r=aswan
Reporter | ||
Comment 50•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e46a44d9b62e3fd555885c6b5dffde236397e67f Bug 1363925: Follow-up: Fix eslint bustage after rebase. r=bustage DONTBUILD
Reporter | ||
Comment 51•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fff626057b893d01b60b3b24b528258c9ab9fcbf Bug 1363925: Follow-up: Fix more rebase bustage. r=bustage
Comment 52•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/af2ff7d301a7 https://hg.mozilla.org/mozilla-central/rev/cffb4b6e971a https://hg.mozilla.org/mozilla-central/rev/801a6d33fa88 https://hg.mozilla.org/mozilla-central/rev/afd6f3e6e6af https://hg.mozilla.org/mozilla-central/rev/ee1bdb001079 https://hg.mozilla.org/mozilla-central/rev/2132b4cf18db https://hg.mozilla.org/mozilla-central/rev/fde77a548f54 https://hg.mozilla.org/mozilla-central/rev/20d1663e4a8c https://hg.mozilla.org/mozilla-central/rev/8adbe703aeba https://hg.mozilla.org/mozilla-central/rev/1646b51d323f https://hg.mozilla.org/mozilla-central/rev/c948486ee8e1 https://hg.mozilla.org/mozilla-central/rev/3ad2e050c66a https://hg.mozilla.org/mozilla-central/rev/e46a44d9b62e https://hg.mozilla.org/mozilla-central/rev/fff626057b89
Comment 53•6 years ago
|
||
mozreview-review |
Comment on attachment 8969964 [details] Bug 1363925: Part 7a - Turn on valid-jsdoc rule for XPIInstall.jsm. https://reviewboard.mozilla.org/r/238750/#review244960 ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:427 (Diff revision 2) > * switch between versions. > * > * @param {string} oldVersion The version of the existing extension instance. > * @param {string} newVersion The version of the extension being installed. > * > - * @return {BOOSTRAP_REASONS.ADDON_UPGRADE|BOOSTRAP_REASONS.ADDON_DOWNGRADE} > + * @returns {integer} nit: JSDocs (and typescript, as I'm finding out these days) only know about `{number}`. ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:653 (Diff revision 2) > - * The nsIRDFDatasource to read from > - * @param aSource > - * The nsIRDFResource to read the properties from > - * @param isDefault > - * True if the locale is to be read from the main install manifest > - * root > + * The datasource to read from. > + * @param {nsIRDFResource} aSource > + * The resource to read the properties from. > + * @param {boolean} isDefault > + * True if the locale is to be read from the main install manifest > + * root punctuation consistency! ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1089 (Diff revision 2) > > /** > * Verifies that a bundle's contents are all correctly signed by an > * AMO-issued certificate > * > - * @param aBundle > + * @param {nsIFile}aBundle nit: space ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1093 (Diff revision 2) > * > - * @param aBundle > - * the nsIFile for the bundle to check, either a directory or zip file > - * @param aAddon > - * the add-on object to verify > - * @return a Promise that resolves to an AddonManager.SIGNEDSTATE_* constant. > + * @param {nsIFile}aBundle > + * The nsIFile for the bundle to check, either a directory or zip file. > + * @param {AddonInternal} aAddon > + * The add-on object to verify. > + * @returns {Prommise<number>} is that a British spelling? ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1152 (Diff revision 2) > } > > /** > * Converts an iterable of addon objects into a map with the add-on's ID as key. > + * > + * @param {sequence<AddonInternal>} addons Not sure what a `sequence` is exactly, but this would probably just be an `Array<>` or `ArrayLike<>` in TS (or `{{map(callback: (a: AddonInternal) => any)}}` if you want to be exact). ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1479 (Diff revision 2) > - * @param aDir > - * Directory to look at > - * @param aSortEntries > - * True to sort entries by filename > - * @return An array of nsIFile, or an empty array if aDir is not a readable directory > + * @param {nsIFile} aDir > + * Directory to look at > + * @param {boolean} aSortEntries > + * True to sort entries by filename > + * @returns {nsIFile[]} > + * An array of nsIFile, or an empty array if aDir is not a readable directory this is now restating the type instead of explaining the semantics. ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:1541 (Diff revision 2) > - * The add-on this install will update if known > - * @param options.name > - * An optional name for the add-on > - * @param options.type > - * An optional type for the add-on > - * @param options.icons > + * The add-on this install will update if known > + * @param {string} [options.name] > + * An optional name for the add-on > + * @param {string} [options.type] > + * An optional type for the add-on > + * @param {object} [options.icons] nit: consistency, should probably prefer `object`, like `string`, `number` or `function` (you don't actually mean `instanceof Object`). ::: toolkit/mozapps/extensions/internal/XPIInstall.jsm:3131 (Diff revision 2) > - * The ID of the add-on to install > - * @param source > - * The source nsIFile to install from > - * @param existingAddonID > - * The ID of an existing add-on to uninstall at the same time > - * @param action > + * Installation options. > + * @param {string} options.id > + * The ID of the add-on to install > + * @param {nsIFile} options.source > + * The source nsIFile to install from > + * @param {string?} [options.existingAddonID] Double optional? Does that make it forbidden?
Attachment #8969964 -
Flags: review?(tomica) → review+
Updated•6 years ago
|
Attachment #8969963 -
Flags: review?(tomica)
Reporter | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: qe-verify-
Keywords: leave-open
Resolution: --- → FIXED
Comment 54•6 years ago
|
||
mozreview-review |
Comment on attachment 8969965 [details] Bug 1363925: Part 7b - Turn on valid-jsdoc rule for XPIProvider.jsm. https://reviewboard.mozilla.org/r/238752/#review245110 ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:708 (Diff revision 2) > + * Yes, this description is extremely helpful. How dare you question its > + * utility? O_O ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2147 (Diff revision 2) > * distributed add-on. > * > - * @param aManifests > - * A dictionary to add new install manifests to to save having to > - * reload them later > - * @param aAppChanged > + * @param {Object} aManifests > + * A dictionary to add new install manifests to to save having to > + * reload them later > + * @param {string} [aAppChanged] boolean? ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2394 (Diff revision 2) > * mimetype. > * > - * @param aMimetype > - * The mimetype to check for > - * @return true if the mimetype is application/x-xpinstall > + * @param {string} aMimetype > + * The mimetype to check for > + * @returns {boolean} > + * True if the mimetype is application/x-xpinstall hmm, comment managed to be longer than the code ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2425 (Diff revision 2) > }, > > /** > * Returns an Addon corresponding to an instance ID. > - * @param aInstanceID > + * > + * @param {Symbol} aInstanceID huh, are we really using `Symbols` for this? ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4173 (Diff revision 2) > /** > * Reads a directory linked to in a file. > * > - * @param file > - * The file containing the directory path > - * @return An nsIFile object representing the linked directory. > + * @param {nsIFile} aFile > + * The file containing the directory path > + * @returns {nsIFile} can return null ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4305 (Diff revision 2) > + * > + * @param {boolean} [rescan = false] > + * True if the directory should be re-scanned, even if it has > + * already been initialized. > + * > + * @returns {nsIFile[]} I guess Map is kinda like an array
Attachment #8969965 -
Flags: review?(tomica) → review+
Reporter | ||
Comment 55•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8969964 [details] Bug 1363925: Part 7a - Turn on valid-jsdoc rule for XPIInstall.jsm. https://reviewboard.mozilla.org/r/238750/#review244960 > nit: JSDocs (and typescript, as I'm finding out these days) only know about `{number}`. Hm. I guess I'll make it a typedef. I'd rather keep the assertion that this should only be an integer. > punctuation consistency! Meh. It annoys me too, but I mostly decided not to fix the lack of punctuation in the existing docs. > Not sure what a `sequence` is exactly, but this would probably just be an `Array<>` or `ArrayLike<>` in TS (or `{{map(callback: (a: AddonInternal) => any)}}` if you want to be exact). It's WebIDL notation. It essentially means any iterable object. If TS doesn't understand it, maybe I can typedef it to ArrayLike. I'd rather keep our type names closer to our WebIDL definitions where possible... > nit: consistency, should probably prefer `object`, like `string`, `number` or `function` (you don't actually mean `instanceof Object`). Not sure what you mean. I *did* use `object` here. That said, I tend to use `Object` in cases where I mean a plain object (like an object literal), and `object` where I mean, more or less, `typeof thing === "object"` > Double optional? Does that make it forbidden? They mean different things. `string?` means the value can be null. Braces mean the property can be omitted. Optional `string?` means it can be present and null.
Reporter | ||
Comment 56•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8ebb6476c98519589404ab0793afc969c49216 Bug 1363925: Follow-up: Cleanup merge artifacts from bug 1433574. r=trivial https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef195c68eed3a1875e9ee63ff556b7875c14f9b Bug 1363925: Follow-up: Fix JSDoc issues. r=zombie DONTBUILD
Comment 57•6 years ago
|
||
mozreview-review |
Comment on attachment 8970040 [details] Bug 1363925: Part 7c - Turn on valid-jsdoc rule for XPIProviderUtils.js. https://reviewboard.mozilla.org/r/238792/#review245138 ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:78 (Diff revision 1) > + * @param {AddonInternal} aAddon > + * The add-on to annotate. > + * @returns {AddonInternal} > + * The annotated add-on. > */ > async function getRepositoryAddon(aAddon) { great method name! ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:188 (Diff revision 1) > + * The filter predecate. The first add-on for which it returns > + * true will be returned. could be worded better ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:490 (Diff revision 1) > * Reconstruct when the DB file exists but is unreadable > * (for example because read permission is denied) > + * > + * @param {Error} aError > + * The error that triggered the rebuild. > + * @param {boolean} aRebuildOnError Rebuild the db. On error, rebuild the db? ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:512 (Diff revision 1) > * necessary. If any DB load operation fails, we need to > * synchronously rebuild the DB from the installed extensions. > * > - * @return Promise<Map> resolves to the Map of loaded JSON data stored > - * in this.addonDB; never rejects. > + * @returns {Promise<AddonDB>} > + * Resolves to the Map of loaded JSON data stored in > + * this.addonDB; never rejects. A bold claim. ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:748 (Diff revision 1) > }, > > /** > * Synchronously gets all add-ons of a particular type(s). > * > - * @param aType, aType2, ... > + * @param {Array<string>} aTypes I think you can keep `{...string}`, at least TS understands it. ::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1193 (Diff revision 1) > - * The new state of the add-on > - * @param aNewAddon > - * The manifest for the new add-on if it has already been loaded > - * @return a boolean indicating if flushing caches is required to complete > - * changing this add-on > + * The new state of the add-on > + * @param {AddonInternal?} [aNewAddon] > + * The manifest for the new add-on if it has already been loaded > + * @returns {boolean?} > + * A boolean indicating if flushing caches is required to complete > + * changing this add-on An optional boolean return type? It's either a mistake or needs to be documented what undefined means.
Attachment #8970040 -
Flags: review?(tomica) → review+
Comment 58•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d8ebb6476c9 https://hg.mozilla.org/mozilla-central/rev/3ef195c68eed
Updated•6 years ago
|
status-firefox61:
--- → fixed
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•