Calls to common service methods shouldn't supply additional parameters

RESOLVED FIXED in Firefox 53

Status

()

Firefox
General
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: florian, Assigned: florian)

Tracking

53 Branch
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

5 months ago
Created attachment 8825993 [details]
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.
(Assignee)

Comment 1

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

Comment 2

5 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)
(Assignee)

Comment 3

5 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.
(Assignee)

Comment 5

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

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

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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9d508a7deecf
https://hg.mozilla.org/mozilla-central/rev/100507e98842
https://hg.mozilla.org/mozilla-central/rev/d94f6ca9e752
Status: NEW → RESOLVED
Last Resolved: 5 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.