Refactor mozLoop.{get, set}LoopCharPref and mozLoop.{get, set}LoopBoolPref to mozLoop.{get, set}Pref that uses getPrefType

RESOLVED FIXED in Firefox 35

Status

Hello (Loop)
Client
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
mozilla36
Points:
3
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

Attachments

(1 attachment)

We can refactor mozLoop.{get, set}LoopBoolPref and mozLoop.{get, set}LoopCharPref to mozLoop.{get, set}Pref.

From bug 1083466 comment 18:
> I already dislike the fact that we don't have a `MozLoopService.getPref()` 
> function that uses `getPrefType`, but we can have that here:

> `setPref: function(prefName, value, [prefType]){}` with an optional preftype 
> for when you know that the pref isn't set in firefox.js (rare)

We will also need to handle the preference storage for standalone clients which is currently planned to use localStorage (bug 1084362).
Flags: qe-verify-
Flags: firefox-backlog+
Created attachment 8524906 [details] [diff] [review]
Patch

Well, that patch ended up being way bigger than I thought it was going to be. /run_all_tests.sh passes. (built on top of bug 1084362)
Attachment #8524906 - Flags: review?(mdeboer)
I kept "Loop" in the function name since we are placing "loop." in front of the preference name that is passed in. Just "getPref" or "setPref" may lead to people thinking that they can call this for arbitrary preferences.
Results will be displayed on Treeherder as they come in:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=48857d34fb79

Alternatively, view them on TBPL (soon to be deprecated):
https://tbpl.mozilla.org/?tree=Try&rev=48857d34fb79
Comment on attachment 8524906 [details] [diff] [review]
Patch

Review of attachment 8524906 [details] [diff] [review]:
-----------------------------------------------------------------

Never be afraid of big patches, this is awesome! ;-)

::: browser/components/loop/content/js/panel.jsx
@@ +193,5 @@
>    });
>  
>    var ToSView = React.createClass({
>      getInitialState: function() {
> +      return {seenToS: navigator.mozLoop.getLoopPref('seenToS')};

nit: while you're here, can you make these double quotes?

::: browser/components/loop/content/shared/js/utils.js
@@ +48,5 @@
>     */
>    function getBoolPreference(prefName) {
>      if (navigator.mozLoop) {
> +      debugger;
> +      return !!navigator.mozLoop.getLoopPref(prefName, /* Ci.nsIPrefBranch.PREF_BOOL = */ 128);

you can make this simply `!!navigator.mozLoop.getLoopPref(prefName)`
Attachment #8524906 - Flags: review?(mdeboer) → review+
Thanks!

Landed as https://hg.mozilla.org/integration/fx-team/rev/e8f199ee233d
Whiteboard: [fixed-in-fx-team]
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/e8f199ee233d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Comment on attachment 8524906 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: N/A
Major renaming of prefs access functions - low risk, avoids conflicts on other uplifts of bug fixes, cleans up code.
[Describe test coverage new/current, TBPL]: on m-c
[String/UUID change made/needed]: none
Attachment #8524906 - Flags: approval-mozilla-aurora?
Attachment #8524906 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e44e811d5206
status-firefox35: --- → fixed
status-firefox36: --- → fixed
Depends on: 1127557
You need to log in before you can comment on or make changes to this bug.