Calls to common service methods shouldn't supply additional parameters

RESOLVED FIXED in Firefox 53



2 years ago
2 years ago


(Reporter: florian, Assigned: florian)


53 Branch
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)



(4 attachments)



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 call checks that the argument count doesn't exceed

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.

Comment 1

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

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)

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.

Comment 5

2 years ago
Bug 1330464 - script-generated removal of additional parameters that don't exist in the interface, r=jaws.
Bug 1330464 - make the no-useless-parameters eslint rule detect misuses of the Services.prefs APIs, r=jaws.

Comment 6

2 years ago
Bug 1330464 - make no-useless-parameters.js not use array.includes, r=jaws over IRC.

Comment 7

2 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> 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.