Closed Bug 1497705 Opened 2 years ago Closed 2 years ago

Errors in common/, hidden by ESLint config file

Categories

(Thunderbird :: Add-Ons: Extensions API, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached 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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.