1330464 - Calls to common service methods shouldn't supply additional parameters

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Florian Quèze [:florian]]

Attachment: removeNonExistentParameters.js

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.

Comment 1

[Florian Quèze [:florian]]

Attachment: script-generated patch

Comment 2

[Florian Quèze [:florian]]

Attachment: Changes to the no-useless-parameters

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

Comment 3

[Florian Quèze [:florian]]

Attachment: comm-central script-generated patch

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Very cool to see the capabilities being used to clean up our codebase in such a smart way.

Comment 5

[Florian Quèze [:florian]]

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.

Comment 6

[Florian Quèze [:florian]]

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

Comment 7

[Florian Quèze [:florian]]

(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 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9d508a7deecf
https://hg.mozilla.org/mozilla-central/rev/100507e98842
https://hg.mozilla.org/mozilla-central/rev/d94f6ca9e752

Comment 9

[Philipp Kewisch [:Fallen][☁️📆]]

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.