Closed Bug 1240576 Opened 5 years ago Closed 5 years ago

Use Preferences.jsm module over Services.pref

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

(Keywords: pi-marionette-client, pi-marionette-runner, pi-marionette-server)

Attachments

(1 file)

We rely on Services.pref.get{Char,Bool,Number} in Marionette, but the Preferences.jsm module offers a better interface that gets the preference value and automatically coerces it to the right type.

Moreover this lets us write code that does not have to rely on try…catch clauses to direct code flow.
Assignee: nobody → ato
Status: NEW → ASSIGNED
A distinct advantage is that try...catch statements are no longer used
to control the flow of code.

Review commit: https://reviewboard.mozilla.org/r/31319/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31319/
Attachment #8709183 - Flags: review?(dburns)
Comment on attachment 8709183 [details]
MozReview Request: Bug 1240576 - Use Preferences.jsm throughout Marionette; r?automatedtester

https://reviewboard.mozilla.org/r/31319/#review28201
Attachment #8709183 - Flags: review?(dburns) → review+
Comment on attachment 8709183 [details]
MozReview Request: Bug 1240576 - Use Preferences.jsm throughout Marionette; r?automatedtester

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31319/diff/1-2/
https://hg.mozilla.org/mozilla-central/rev/939930dcd555
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Andreas, at least the set_pref() method missed the docstring. Would you mind to check what else is necessary here to get the docs up to date? Thanks.
Flags: needinfo?(ato)
The set_pref() method was still missing a docstring before this change.  I think it would be reasonable to display _all exposed_ methods and functions in the API documentation, regardless whether they have a docstring or not.
Flags: needinfo?(ato)
(In reply to Andreas Tolfsen (:ato) from comment #9)
> The set_pref() method was still missing a docstring before this change.  I
> think it would be reasonable to display _all exposed_ methods and functions
> in the API documentation, regardless whether they have a docstring or not.

David, what's your standpoint here? If you agree we should get another bug filed to get the behavior changed.
Flags: needinfo?(dburns)
It is better to show all APIs that we can even if docstrings are missing. We can raise a bug to review all missing docstrings.
Flags: needinfo?(dburns)
You need to log in before you can comment on or make changes to this bug.