Closed
Bug 1109451
Opened 10 years ago
Closed 9 years ago
Remove system/js/migrators/settings_migrator.js
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: timdream, Unassigned)
References
Details
Attachments
(1 file)
The correct way to have the existing profile to pick up new value in settings.json is to bump the DB Version in Gecko [1], instead of filing the default in the System app. Therefore, SettingsMigrator is not a correct approach and should be removed.
The DB version was previously bumped in bug 1021351. So in this bug we can safely remove the script without doing anything, since it will be a no-op anyway.
[1] https://mxr.mozilla.org/mozilla-central/source/dom/settings/SettingsDB.jsm#43
Flags: needinfo?(gasolin)
Flags: needinfo?(arthur.chen)
Flags: needinfo?(alive)
Reporter | ||
Comment 1•10 years ago
|
||
Sorry, wrong bug. Latest DB version bump was bug 1061902, happens days after bug 1055424.
Comment 2•10 years ago
|
||
I think Fred/Arthur is on it. If not I will spend time after current system 2 stuff finished (statusbar & screen clean-up)
Flags: needinfo?(alive)
Comment 3•10 years ago
|
||
We have conversation offline with Tim. Basic idea is we'll try to identify what gecko settingsDB.jsm already has and move current System/Settings migration related code into it.
Flags: needinfo?(gasolin)
Flags: needinfo?(arthur.chen)
Comment 4•10 years ago
|
||
Current settings_migrator handle very special case because hour12 needs locale info, which can't be done in gecko. But we may move it into mozHour12 shim in shared/date_time_helper.
Comment 5•10 years ago
|
||
Comment 6•10 years ago
|
||
Comment on attachment 8536937 [details]
[PullReq] gasolin:issue-1109451 to mozilla-b2g:master
delegate settings migrator's duty to mozHour12 shim, and we'll lost unit test..
Attachment #8536937 -
Flags: review?(arthur.chen)
Attachment #8536937 -
Flags: review?(alive)
Comment 7•10 years ago
|
||
Comment on attachment 8536937 [details]
[PullReq] gasolin:issue-1109451 to mozilla-b2g:master
The code looks good to me. Could you also migrate the existing tests to apps/sharedtests where the unit tests for shared scripts are placed. Thanks.
Attachment #8536937 -
Flags: review?(arthur.chen)
Comment 8•10 years ago
|
||
Comment on attachment 8536937 [details]
[PullReq] gasolin:issue-1109451 to mozilla-b2g:master
Redirect to reporter :)
Attachment #8536937 -
Flags: review?(alive) → review?(timdream)
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8536937 [details]
[PullReq] gasolin:issue-1109451 to mozilla-b2g:master
That's not what I asked. Your patch attempt to write the setting in every app from date_time_helper.js, which is a worse migration way than what we previously have.
What I say offline was:
1. The settings key should be a tri-state value: locale-dependent, user-on, user-off. We can internally represent it as -1, 0, 1, and default to "locale-dependent".
2. When the DateTimeHelper encounters "locale-dependent", it will attempt to resolve the shim API value to true/false, based on the current locale. And you would need to handle locale change as well.
3. We will *NEVER* encounter "undefined". If so feel free to |console.error()|.
4. We do, however, need to migrate the two state |locale.hour12| to a new value somewhere, since we made a bad choice back then.
Attachment #8536937 -
Flags: review?(timdream) → review-
Comment 10•10 years ago
|
||
Thanks for feedback. I think it's a right way to make mozHour12 less dependent on mozSettings stat.
Though I take the patch's approach because it will run only once when system clock (the very first one) calls mozHour12 API. And 'settings write' is only called when settings key is undefined (edge case in fota bug 1055424), so it still make no impact to other apps.
Instead to change settings key to the new format will impact 18+ files including marionette test, I am not sure if this edge case worth the effort?
Flags: needinfo?(timdream)
Reporter | ||
Comment 11•10 years ago
|
||
Discussed offline. We will put off the effort until v2.2 branches. Fred can fill in the rest of the details.
Flags: needinfo?(timdream)
Comment 12•9 years ago
|
||
according to bug 1112092, we'd not moving the migrator from gaia to gecko.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•