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)

x86
macOS
defect
Not set
normal

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)
Sorry, wrong bug. Latest DB version bump was bug 1061902, happens days after bug 1055424.
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)
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)
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 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 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 on attachment 8536937 [details] [PullReq] gasolin:issue-1109451 to mozilla-b2g:master Redirect to reporter :)
Attachment #8536937 - Flags: review?(alive) → review?(timdream)
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-
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)
Discussed offline. We will put off the effort until v2.2 branches. Fred can fill in the rest of the details.
Flags: needinfo?(timdream)
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.

Attachment

General

Created:
Updated:
Size: