Calls to common service methods shouldn't supply additional parameters

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

53 Branch
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

2 years ago
After fixing bug 1330147, I wondered if there were other APIs we use all the time that are misused in the tree.

To answer the question, I made an xpcshell script (attached) that for each Services.foo.bar() call checks that the argument count doesn't exceed Services.foo.bar.length.

Once the removeObserver calls are fixed (bug 1330147), there are surprisingly few additional issues.

Sorted by how many times there's a problem:
   5 Services.prefs.clearUserPref
   4 Services.prefs.getBoolPref
   2 Services.prefs.getCharPref
   1 Services.perms.removeAll
   1 Services.perms.testPermissionFromPrincipal

I think the Services.prefs APIs could be added to the no-useless-parameters eslint rule. For the others that are misused only once, it's probably not worth it.
Assignee

Comment 1

2 years ago
Attachment #8825994 - Flags: review?(jaws)
Assignee

Comment 2

2 years ago
This changes the no-useless-parameters eslint rule to cover the most frequently misused parts of the preferences API, and does by hand the fixes for issues eslint raises (they were not detected by the xpcshell script because it only looked at API calls using Services.jsm).
Attachment #8825996 - Flags: review?(jaws)
Assignee

Comment 3

2 years ago
Attachment #8826004 - Flags: review?(philipp)
Very cool to see the capabilities being used to clean up our codebase in such a smart way.
Assignee

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d508a7deecf6b59a7065853838625934a5db051
Bug 1330464 - script-generated removal of additional parameters that don't exist in the interface, r=jaws.

https://hg.mozilla.org/integration/mozilla-inbound/rev/100507e98842717688a28cbb5ce3decc108397ee
Bug 1330464 - make the no-useless-parameters eslint rule detect misuses of the Services.prefs APIs, r=jaws.
Assignee

Comment 6

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d94f6ca9e7521922c75c34108efcc793685e8c96
Bug 1330464 - make no-useless-parameters.js not use array.includes, r=jaws over IRC.
Assignee

Comment 7

2 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> d94f6ca9e7521922c75c34108efcc793685e8c96
> Bug 1330464 - make no-useless-parameters.js not use array.includes, r=jaws
> over IRC.

Context is: the 'ES' job was failing on treeherder with this error:

TypeError: ["getCharPref","getBoolPref","getIntPref","clearUserPref"].includes is not a function
Comment on attachment 8826004 [details] [diff] [review]
comm-central script-generated patch

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

r+ with this considered:

::: mail/test/mozmill/account/test-mail-account-setup-wizard.js
@@ +214,5 @@
>   */
>  function remember_password_test(aPrefValue) {
>    // save the pref for backup purpose
>    let rememberSignons_pref_save =
> +      Services.prefs.getBoolPref("signon.rememberSignons");

Does it work for this case? The default for this pref is true, you should check if that is still the case when removing the argument and the pref not being set.
Attachment #8826004 - Flags: review?(philipp) → review+
You need to log in before you can comment on or make changes to this bug.