Closed
Bug 1101006
Opened 10 years ago
Closed 10 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)
Hello (Loop)
Client
Tracking
(firefox35 fixed, firefox36 fixed)
People
(Reporter: jaws, Assigned: jaws)
References
Details
Attachments
(1 file)
46.27 KB,
patch
|
mikedeboer
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
Thanks! Landed as https://hg.mozilla.org/integration/fx-team/rev/e8f199ee233d
Whiteboard: [fixed-in-fx-team]
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/e8f199ee233d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla36
Comment 7•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8524906 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 8•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/e44e811d5206
status-firefox35:
--- → fixed
status-firefox36:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•