1109451 - Remove system/js/migrators/settings_migrator.js

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sorry, wrong bug. Latest DB version bump was bug 1061902, happens days after bug 1055424.

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

I think Fred/Arthur is on it. If not I will spend time after current system 2 stuff finished (statusbar & screen clean-up)

Comment 3

[[e-mail]]

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.

Comment 4

[[e-mail]]

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

[GitHub Autolander Bot]

Attachment: [PullReq] gasolin:issue-1109451 to mozilla-b2g:master

Comment 6

[[e-mail]]

Comment on attachment 8536937 [details] [review]
[PullReq] gasolin:issue-1109451 to mozilla-b2g:master

delegate settings migrator's duty to mozHour12 shim, and we'll lost unit test..

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8536937 [details] [review]
[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.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8536937 [details] [review]
[PullReq] gasolin:issue-1109451 to mozilla-b2g:master

Redirect to reporter :)

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8536937 [details] [review]
[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.

Comment 10

[[e-mail]]

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?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Discussed offline. We will put off the effort until v2.2 branches. Fred can fill in the rest of the details.

Comment 12

[[e-mail]]

according to bug 1112092, we'd not moving the migrator from gaia to gecko.