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

RESOLVED INCOMPLETE

Status

()

P2
normal
RESOLVED INCOMPLETE
3 years ago
9 months ago

People

(Reporter: ferjm, Unassigned)

Tracking

unspecified
2.6 S6 - 1/29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Updated

3 years ago
Assignee: nobody → ferjmoreno
(Reporter)

Comment 1

3 years ago
Created attachment 8686118 [details] [diff] [review]
v1

I'm gonna try to write some tests, but I would appreciate some feedback in the mean time.
Attachment #8686118 - Flags: feedback?(anygregor)
(Reporter)

Updated

3 years ago
Blocks: 1216301
(Reporter)

Comment 2

3 years ago
[Blocking Requested - why for this release]: Blocks a 2.5+ (bug 1216301)
blocking-b2g: --- → 2.5?
blocking-b2g: 2.5? → 2.5+
(Reporter)

Comment 3

3 years ago
Created attachment 8686523 [details] [diff] [review]
v1

Now with tests
Attachment #8686118 - Attachment is obsolete: true
Attachment #8686118 - Flags: feedback?(anygregor)
Attachment #8686523 - Flags: review?(anygregor)
(Reporter)

Comment 4

3 years ago
Created attachment 8686524 [details] [diff] [review]
v1

... and now with the correct patch
Attachment #8686523 - Attachment is obsolete: true
Attachment #8686523 - Flags: review?(anygregor)
Attachment #8686524 - Flags: review?(anygregor)
(Reporter)

Comment 6

3 years ago
Created attachment 8686714 [details] [diff] [review]
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.
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-
(Reporter)

Comment 8

3 years ago
Created attachment 8687146 [details] [diff] [review]
v3

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

Updated

3 years ago
Depends on: 1225157
(Reporter)

Comment 10

3 years ago
Created attachment 8687970 [details] [diff] [review]
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.
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?
(Reporter)

Comment 12

3 years ago
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)
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
(Reporter)

Comment 16

3 years ago
(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
(Reporter)

Updated

3 years ago
No longer blocks: 1216301
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
Flags: needinfo?(ferjmoreno)
(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.
(Reporter)

Comment 24

3 years ago
Created attachment 8692602 [details] [diff] [review]
Fix FilePicker tests race condition
Flags: needinfo?(ferjmoreno)
Attachment #8692602 - Flags: review?(fabrice)
(Reporter)

Updated

3 years ago
Attachment #8692602 - Attachment is obsolete: true
Attachment #8692602 - Flags: review?(fabrice)
(Reporter)

Comment 26

3 years ago
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
(Reporter)

Updated

3 years ago
Blocks: 824026
Priority: -- → P2
Target Milestone: --- → 2.6 S6 - 1/29
(Reporter)

Comment 29

3 years ago
2.5- because we did the version bump
blocking-b2g: 2.5+ → ---
(Reporter)

Updated

3 years ago
Assignee: ferjmoreno → nobody
FxOS no longer in tree. Marking old FxOS Device Interfaces bugs as INCOMPLETE.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.