Errors in common/, hidden by ESLint config file

RESOLVED FIXED in Thunderbird 64.0

Status

enhancement
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

unspecified
Thunderbird 64.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I've just uncovered a bug in ChromeManifest.jsm that was hidden by the "no-undef" rule being turned off in common/.eslintrc.js. That file is unnecessary, so I removed it and uncovered another bug.

Not really that happy about that.
Posted patch 1497705-eslint-common-1.diff (obsolete) — Splinter Review
Assignee: nobody → geoff
Status: NEW → ASSIGNED
Attachment #9015728 - Flags: review?(jorgk)
Comment on attachment 9015728 [details] [diff] [review]
1497705-eslint-common-1.diff

Review of attachment 9015728 [details] [diff] [review]:
-----------------------------------------------------------------

Does a quick review make you happier?

::: common/src/ChromeManifest.jsm
@@ +94,5 @@
>     * @param {String[]} flags        An array of raw flag values in the form key=value.
>     * @return {Boolean}              True, if the flags match the options provided in the constructor
>     */
>    _parseFlags(flags) {
> +    if (!flags.length) {

Do you prefer this or flags.length == 0?
Attachment #9015728 - Flags: review?(jorgk) → review+
Actually I do prefer == 0, but it's all the same :) Ship it!

This really should go on beta too, I don't know if it applies cleanly there.
Keywords: checkin-needed
You can ship it if there's a merge in ~5 hours.
Comment on attachment 9015728 [details] [diff] [review]
1497705-eslint-common-1.diff

Review of attachment 9015728 [details] [diff] [review]:
-----------------------------------------------------------------

::: common/src/extensionSupport.jsm
@@ +26,5 @@
>  function extensionDefaults() {
>    // Fetch enabled non-bootstrapped add-ons.
>    let enabledAddons = Services.dirsvc.get("XREExtDL", Ci.nsISimpleEnumerator);
>    for (let addonFile of fixIterator(enabledAddons, Ci.nsIFile)) {
> +    ExtensionSupport.loadAddonPrefs(addonFile);

Does that mean it didn't work?
It didn't work, but it doesn't throw an exception, because that code never runs, because enabledAddons is always empty. Fortunately we call ExtensionSupport.loadAddonPrefs from the legacy extension handler anyway, so even if it did work, it's redundant.

I'm going to remove the whole thing.
I could go step further and fix the name of extensionSupport.jsm, but I'll save that for another day. ;-)
Attachment #9015728 - Attachment is obsolete: true
Attachment #9015778 - Flags: review?(jorgk)
Keywords: checkin-needed
Comment on attachment 9015778 [details] [diff] [review]
1497705-eslint-common-2.diff

(In reply to Geoff Lankow (:darktrojan) from comment #6)
> It didn't work, but it doesn't throw an exception, because that code never
> runs, because enabledAddons is always empty. Fortunately we call
> ExtensionSupport.loadAddonPrefs from the legacy extension handler anyway, so
> even if it did work, it's redundant.
Can you enlighten me here a little bit. This was implemented in the mozilla59/60 days when M-C scrapped the code that read the defaults. It's still actively working here:
https://dxr.mozilla.org/comm-esr60/source/common/src/extensionSupport.jsm#

So when did Services.dirsvc.get("XREExtDL", Ci.nsISimpleEnumerator); start returning nothing? Most likely when the non-bootstrapped add-ons disappeared, right, during mozilla 61?
Attachment #9015778 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fe10a3a8d48e
Remove ESLint rule overrides in common/ and broken code they were hiding. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
You need to log in before you can comment on or make changes to this bug.