1497705 - Errors in common/, hidden by ESLint config file

Status: RESOLVED FIXED

Description

[Geoff Lankow (:darktrojan)]

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.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1497705-eslint-common-1.diff

Comment 2

[Jorg K (GMT+2)]

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?

Comment 3

[Geoff Lankow (:darktrojan)]

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.

Comment 4

[Jorg K (GMT+2)]

You can ship it if there's a merge in ~5 hours.

Comment 5

[Jorg K (GMT+2)]

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?

Comment 6

[Geoff Lankow (:darktrojan)]

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.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: 1497705-eslint-common-2.diff

I could go step further and fix the name of extensionSupport.jsm, but I'll save that for another day. ;-)

Comment 8

[Jorg K (GMT+2)]

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?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fe10a3a8d48e
Remove ESLint rule overrides in common/ and broken code they were hiding. r=jorgk