The default bug view has changed. See this FAQ.

removeObserver calls should not supply a third parameter

RESOLVED FIXED in Firefox 53

Status

()

Firefox
General
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

52 Branch
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

2 months ago
Services.prefs.removeObserver and Services.obs.removeObserver both take only 2 parameters, but there are lots of calls in our codebase that supply a third boolean parameter. My guess is it's either copy/paste mistakes from addObserver calls, or habit from removeEventListener calls that often had a third 'false' parameter before it became optional.
(Assignee)

Comment 1

2 months ago
Created attachment 8825569 [details] [diff] [review]
Script generated patch

No hand editing in this patch.
Attachment #8825569 - Flags: review?(jaws)
(Assignee)

Updated

2 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 months ago
Created attachment 8825571 [details] [diff] [review]
eslint rule

eslint rule to enforce this, and the changes made in bug 1329182.
Attachment #8825571 - Flags: review?(jaws)
(Assignee)

Comment 3

2 months ago
Created attachment 8825574 [details] [diff] [review]
Separate patch for hand fixes

eslint tells me I missed a couple newURI calls in bug 1329182, and the script I used to generate attachment 8825569 [details] [diff] [review] didn't run on xml files, so it missed tabbrowser.xml.
Attachment #8825574 - Flags: review?(jaws)
(Assignee)

Comment 4

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35952d301e26

Comment 5

2 months ago
Comment on attachment 8825571 [details] [diff] [review]
eslint rule

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

Would be nice if eslint autofixing worked with the rule as well.

Comment 6

2 months ago
Comment on attachment 8825574 [details] [diff] [review]
Separate patch for hand fixes

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

::: toolkit/components/remotebrowserutils/tests/browser/browser_RemoteWebNavigation.js
@@ +23,5 @@
>  
>  function makeURI(url) {
>    return Cc["@mozilla.org/network/io-service;1"].
>           getService(Ci.nsIIOService).
> +         newURI(url);

The two last lines should fit into one.
Comment on attachment 8825571 [details] [diff] [review]
eslint rule

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

Now bug 1330071 has landed in mozilla-central, it'd be nice to add a simple test or two for this. Should be quite quick to write.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/index.js
@@ +25,5 @@
>      "no-aArgs": require("../lib/rules/no-aArgs"),
>      "no-cpows-in-tests": require("../lib/rules/no-cpows-in-tests"),
>      "no-single-arg-cu-import": require("../lib/rules/no-single-arg-cu-import"),
>      "no-import-into-var-and-global": require("../lib/rules/no-import-into-var-and-global.js"),
> +    "no-useless-parameters": require("../lib/rules/no-useless-parameters"),

Please can you add this to the rulesConfig as well. It probably doesn't do much, but is good to keep them in sync and clear as to what the defaults are.

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-parameters.js
@@ +29,5 @@
> +        return;
> +      }
> +
> +      if (callee.property.name === "removeObserver" &&
> +          node.arguments.length === 3) {

I almost wonder if we should make this node.arguments.length > 2. Not sure if it is worth it or not.

::: tools/lint/eslint/eslint-plugin-mozilla/package.json
@@ +1,3 @@
>  {
>    "name": "eslint-plugin-mozilla",
> +  "version": "0.2.7",

Bug 1329614 just landed, so you'll need to bump this again.
(Assignee)

Comment 8

2 months ago
Created attachment 8825889 [details] [diff] [review]
eslint rule v2

Addressed comment 7.
Attachment #8825889 - Flags: review?(jaws)
(Assignee)

Updated

2 months ago
Attachment #8825571 - Attachment is obsolete: true
Attachment #8825571 - Flags: review?(jaws)
Attachment #8825569 - Flags: review?(jaws) → review+
Comment on attachment 8825889 [details] [diff] [review]
eslint rule v2

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

::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/no-useless-parameters.js
@@ +33,5 @@
> +          node.arguments.length === 3) {
> +        let arg = node.arguments[2];
> +        if (arg.type === "Literal" && (arg.value === false ||
> +                                       arg.value === true)) {
> +          context.report(node, "removeObserver only takes 2 parameters.");

Implementing --fix support should be as simple as supplying the fix function like is done here, https://github.com/eslint/eslint/blob/master/lib/rules/object-shorthand.js#L415

Note that in the above linked source, they use the object argument to context.report instead of the style that you're using here.

@@ +41,5 @@
> +      if (callee.property.name === "newURI" &&
> +          node.arguments.length === 3) {
> +        let arg = node.arguments[2];
> +        if (arg.type === "Literal" && arg.value === null) {
> +          context.report(node, "newURI's optional parameters passed as null.");

We could easily implement --fix support here too.
Attachment #8825889 - Flags: review?(jaws) → review+
Attachment #8825574 - Flags: review?(jaws) → review+
(Assignee)

Comment 10

2 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0d90af0af192e2278393292e25df2b6710b4c8
Bug 1330147 - add eslint rule to reject newURI(uri, null, null) and removeObserver(notificationName, observer, false), r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0b7bc5fd7164d083261a709bdf3dc0266365ca
Bug 1330147 - automatic removal of the third boolean parameter in removeObserver calls, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/224c1524464dd3d5311fce2c403b36f902ea9a6e
Bug 1330147 - fix by hand the issues raised by the new no-useless-parameters eslint rule, r=jaws.

Comment 11

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ac0d90af0af1
https://hg.mozilla.org/mozilla-central/rev/8e0b7bc5fd71
https://hg.mozilla.org/mozilla-central/rev/224c1524464d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
I've just submitted https://github.com/mozilla/pdf.js/pull/7956 to get the relevant changes upstreamed to the PDF.js repo.

Similar to bug 1329182 comment 20: For future reference, please remember to notify upstream of any PDF.js changes made directly in mozilla-central.
Depends on: 1332910
You need to log in before you can comment on or make changes to this bug.