Closed Bug 1101006 Opened 5 years ago Closed 5 years ago

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

Categories

(Hello (Loop) :: Client, defect)

defect
Not set
Points:
3

Tracking

(firefox35 fixed, firefox36 fixed)

RESOLVED FIXED
mozilla36
Iteration:
36.3
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file)

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+
Attached patch PatchSplinter Review
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
Closed: 5 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+
Depends on: 1127557
You need to log in before you can comment on or make changes to this bug.