Closed Bug 1223781 Opened 5 years ago Closed 3 years ago

New settings added to settings.json should be applied after OTA without requiring to bump settings DB version

Categories

(Core :: DOM: Device Interfaces, defect, P2)

defect

Tracking

()

RESOLVED INCOMPLETE
2.6 S6 - 1/29

People

(Reporter: ferjm, Unassigned)

References

Details

Attachments

(1 file, 6 obsolete files)

1:47 PM <daleharvey> mhenretty: if we put a new setting in common settings will it not get picked up in OTA's ?
1:57 PM <gwagner> daleharvey: no
1:58 PM <gwagner> you have to bump the settings DB version
Assignee: nobody → ferjmoreno
Attached patch v1 (obsolete) — Splinter Review
I'm gonna try to write some tests, but I would appreciate some feedback in the mean time.
Attachment #8686118 - Flags: feedback?(anygregor)
Blocks: 1216301
[Blocking Requested - why for this release]: Blocks a 2.5+ (bug 1216301)
blocking-b2g: --- → 2.5?
blocking-b2g: 2.5? → 2.5+
Attached patch v1 (obsolete) — Splinter Review
Now with tests
Attachment #8686118 - Attachment is obsolete: true
Attachment #8686118 - Flags: feedback?(anygregor)
Attachment #8686523 - Flags: review?(anygregor)
Attached patch v1 (obsolete) — Splinter Review
... and now with the correct patch
Attachment #8686523 - Attachment is obsolete: true
Attachment #8686523 - Flags: review?(anygregor)
Attachment #8686524 - Flags: review?(anygregor)
Attached patch v2 (obsolete) — Splinter Review
Now avoiding to read the settings.json file twice on the very first run and when the OTA includes a settings DB version bump.
Attachment #8686524 - Attachment is obsolete: true
Attachment #8686524 - Flags: review?(anygregor)
Attachment #8686714 - Flags: review?(anygregor)
Comment on attachment 8686714 [details] [diff] [review]
v2

Review of attachment 8686714 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/apps/AppsUtils.jsm
@@ +635,5 @@
>     */
>    isFirstRun: function isFirstRun(aPrefBranch) {
> +    if (this._isFirstRun !== undefined) {
> +      return this._isFirstRun;
> +    }

isFirstRun() works on a per pref branch basis. So you can use it in several places and still get the right result if you use a different branch. For instance b2gdroid uses it with a non default branch at https://mxr.mozilla.org/mozilla-central/source/mobile/android/b2gdroid/components/Setup.js#249
Your usage of _isFirstRun here will break that.
Attachment #8686714 - Flags: review-
Attached patch v3 (obsolete) — Splinter Review
I updated the patch to remove the cache for isFirstRun.
Attachment #8686714 - Attachment is obsolete: true
Attachment #8686714 - Flags: review?(anygregor)
Attachment #8687146 - Flags: review?(anygregor)
Depends on: 1225157
Attached patch v3Splinter Review
I had to disable the test for the emulator because of bug 1225157. It seems that we initialize the Settings DB from Marionette while launching the emulator. That means that we need to mock the first run by setting a preference, but because of bug 1225157 we can't do that. I am disabling the test on the emulator until bug is fixed. The test is green on Mulet.
Attachment #8687146 - Attachment is obsolete: true
Attachment #8687146 - Flags: review?(anygregor)
Attachment #8687970 - Flags: review?(anygregor)
Attachment #8687970 - Flags: review?(anygregor) → review+
Not sure this has to block 2.5. Can't we just bump the DB version for 2.5 to make 1216301 work and land this on master?
Thank you Gregor. That's a question for Dale and/or Michael.
Flags: needinfo?(mhenretty)
Flags: needinfo?(dale)
(In reply to Gregor Wagner [:gwagner] from comment #11)
> Not sure this has to block 2.5. Can't we just bump the DB version for 2.5 to
> make 1216301 work and land this on master?

That works for me.
Flags: needinfo?(mhenretty)
(In reply to Michael Henretty [:mhenretty] from comment #13)
> (In reply to Gregor Wagner [:gwagner] from comment #11)
> > Not sure this has to block 2.5. Can't we just bump the DB version for 2.5 to
> > make 1216301 work and land this on master?
> 
> That works for me.

Yeah lets do a simple version number bump on master and uplift the patch once the add-on code has landed.
The add-on code has landed, not particularly fussed whether we dont uplift this and do a settings version bump or uplift this, just happy it got fixed, cheers Fernando :)
Flags: needinfo?(dale)
Keywords: checkin-needed
(In reply to Gregor Wagner [:gwagner] from comment #14)
> (In reply to Michael Henretty [:mhenretty] from comment #13)
> > (In reply to Gregor Wagner [:gwagner] from comment #11)
> > > Not sure this has to block 2.5. Can't we just bump the DB version for 2.5 to
> > > make 1216301 work and land this on master?
> > 
> > That works for me.
> 
> Yeah lets do a simple version number bump on master and uplift the patch
> once the add-on code has landed.

Bug 1226906
No longer blocks: 1216301
(In reply to Wes Kocher (:KWierso) from comment #18)
> I backed this out to see if it fixes the mochitest(1) permafail on b2g
> emulator:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/596c14e58f1d
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=17701622&repo=mozilla-
> inbound

(I'm not seeing how it could, but this seems like the only patch that landed recently that was even remotely related to the failing test. Maybe the added test pushed something else into a new mochitest chunk where it decided to start misbehaving?)
Looks like the test started passing again after this was backed out.
Flags: needinfo?(ferjmoreno)
Attachment #8692602 - Flags: review?(fabrice)
Attachment #8692602 - Attachment is obsolete: true
Attachment #8692602 - Flags: review?(fabrice)
https://hg.mozilla.org/integration/mozilla-inbound/rev/350ecdaedef88b5007e0d47002ac6a36c951dc45
Bug 1223781 - New settings added to settings.json should be applied after OTA without requiring to bump settings DB version. r=gwagner
Well, you might have hypothetically helped out the person hypothetically working on fixing bug 1205191, because you made it easier to catch by making it permanently fail.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d2d607e83200 for xpcshell permaorange on opt emulator a la https://treeherder.mozilla.org/logviewer.html#?job_id=17928194&repo=mozilla-inbound
Blocks: fxos-sync
Priority: -- → P2
Target Milestone: --- → 2.6 S6 - 1/29
2.5- because we did the version bump
blocking-b2g: 2.5+ → ---
Assignee: ferjmoreno → nobody
FxOS no longer in tree. Marking old FxOS Device Interfaces bugs as INCOMPLETE.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.