Calls to common service methods shouldn't supply additional parameters

RESOLVED FIXED in Firefox 53



7 months ago
7 months ago


(Reporter: florian, Assigned: florian)


53 Branch
Firefox 53

Firefox Tracking Flags

(firefox53 fixed)



(4 attachments)



7 months ago
Created attachment 8825993 [details]

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

7 months ago
Created attachment 8825994 [details] [diff] [review]
script-generated patch
Attachment #8825994 - Flags: review?(jaws)

Comment 2

7 months ago
Created attachment 8825996 [details] [diff] [review]
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).
Attachment #8825996 - Flags: review?(jaws)

Comment 3

7 months ago
Created attachment 8826004 [details] [diff] [review]
comm-central script-generated patch
Attachment #8826004 - Flags: review?(philipp)
Attachment #8825996 - Flags: review?(jaws) → review+
Attachment #8825994 - Flags: review?(jaws) → review+
Very cool to see the capabilities being used to clean up our codebase in such a smart way.

Comment 5

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

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

Comment 7

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

7 months ago
Last Resolved: 7 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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.