1240576 - Use Preferences.jsm module over Services.pref

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Andreas Tolfsen ❲:ato❳]

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.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: MozReview Request: Bug 1240576 - Use Preferences.jsm throughout Marionette; r?automatedtester

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/

Comment 2

[David Burns :automatedtester]

Comment on attachment 8709183 [details]
MozReview Request: Bug 1240576 - Use Preferences.jsm throughout Marionette; r?automatedtester

https://reviewboard.mozilla.org/r/31319/#review28201

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3011bfcf124

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for b2g xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=20081356&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8be7ffe2df

Comment 5

[Andreas Tolfsen ❲:ato❳]

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/

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/939930dcd555

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/939930dcd555

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 9

[Andreas Tolfsen ❲: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.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 11

[David Burns :automatedtester]

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.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok filed as bug 1243358.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Moving bugs for Marionette client due to component changes.