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

Status: RESOLVED INCOMPLETE

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

Description

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

I'm gonna try to write some tests, but I would appreciate some feedback in the mean time.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

[Blocking Requested - why for this release]: Blocks a 2.5+ (bug 1216301)

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Now with tests

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

... and now with the correct patch

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e27963e5dd6

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v2

Now avoiding to read the settings.json file twice on the very first run and when the OTA includes a settings DB version bump.

Comment 7

[[:fabrice] Fabrice Desré]

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.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

I updated the patch to remove the cache for isFirstRun.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f67dd62304d4

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v3

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.

Comment 11

[Gregor Wagner [:gwagner]]

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?

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Thank you Gregor. That's a question for Dale and/or Michael.

Comment 13

[Michael Henretty [:mikehenrty][:mhenretty]]

(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.

Comment 14

[Gregor Wagner [:gwagner]]

(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.

Comment 15

[Dale Harvey (:daleharvey)]

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 :)

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

(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

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/338e1e550393

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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?)

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like the test started passing again after this was backed out.

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f8e98d50064

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d006ef2e79e

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ff1dd81eab2

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Fix FilePicker tests race condition

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ec6ddae7e15

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 27

[Phil Ringnalda (:philor)]

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

Comment 28

[Phil Ringnalda (:philor)]

Also, frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17953122&repo=mozilla-inbound

Comment 29

[Fernando Jiménez Moreno [:ferjm]]

2.5- because we did the version bump

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33bde8380947

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2496c0c9bfa8

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec1b344a594c

Comment 33

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

FxOS no longer in tree. Marking old FxOS Device Interfaces bugs as INCOMPLETE.