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.

No hand editing in this patch.

eslint rule to enforce this, and the changes made in bug 1329182.

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.

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 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.

Addressed comment 7.

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.

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.

https://hg.mozilla.org/mozilla-central/rev/ac0d90af0af1 https://hg.mozilla.org/mozilla-central/rev/8e0b7bc5fd71 https://hg.mozilla.org/mozilla-central/rev/224c1524464d

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.