Closed
Bug 1330147
Opened 8 years ago
Closed 8 years ago
removeObserver calls should not supply a third parameter
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: florian, Assigned: florian)
References
Details
Attachments
(3 files, 1 obsolete file)
56.77 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
6.40 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
8.55 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
No hand editing in this patch.
Attachment #8825569 -
Flags: review?(jaws)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
eslint rule to enforce this, and the changes made in bug 1329182.
Attachment #8825571 -
Flags: review?(jaws)
Assignee | ||
Comment 3•8 years ago
|
||
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•8 years ago
|
||
Comment 5•8 years 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•8 years 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 7•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Attachment #8825571 -
Attachment is obsolete: true
Attachment #8825571 -
Flags: review?(jaws)
Updated•8 years ago
|
Attachment #8825569 -
Flags: review?(jaws) → review+
Comment 9•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8825574 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 10•8 years 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•8 years 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
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment 12•8 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•