1366593 - Don't run old migration code every time at startup, only when suite.migration.version doesn't exist yet

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #1366496 +++

(Quoting Frank-Rainer Grahl from bug 1366496 comment #2)
> I added a _migrateUI to nsSuiteGlue.js in bug 1331477 to avoid running
> migrations every time on startup.

(Quoting rsx11m from bug 1366496 comment #3)
> The other migrations should go there as well, let's do this in a follow-up[...]

Comment 1

[rsx11m]

Attachment: Direct fix

So, this is the easiest I can come up with - just moving this._updatePrefs() and this._migrateDownloadPrefs() into _migrateUI() and executing those if there is no user_pref suite.migration.version in the current profile.

I don't see the point in actually moving all the old migrations from the separate functions into _migrateUI(), makes it messier and loses per-line change history.
 
Note that migrateMailnews() is still executed on every startup, that's what Thunderbird does as well, so apparently there are use cases for that.

This patch goes on top of attachment 8869825 [details] [diff] [review] to account for UI_VERSION being bumped in bug 1366496.

Comment 2

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8869837 [details] [diff] [review]
Direct fix

Yes keeping the old functions is preferred.

You could move the code in the else inside the initial if: 

> if (currentUIVersion < 1) {

Code in it is run the same time as the else because the version is 0 then. Just needs an adjustment of the comment and a new one for the if. Let me know if this is ok for you too.

f+ for now.

Comment 3

[rsx11m]

(In reply to Frank-Rainer Grahl from comment #2)
> You could move the code in the else inside the initial if: 

I was thinking of putting them there first as well, but then decided to make it part of the very first "check if that preference exists" construct. It is more explicit there and thus not hidden in what is essentially the first "real" (currentUIVersion < 1) migration within that function.

Meaning: I'd prefer as is, but it's obviously straight-forward to change the patch as you suggested.

Comment 4

[Frank-Rainer Grahl (:frg)]

While performance is a non issue for both choices my reasoning was/is that all migration code is below:

> if (currentUIVersion >= UI_VERSION)
>       return;

Also one "else" less which reduces complexity by 0.0001% :)

Just let me know your final verdict. We could ask IanN but he would probably call us crazy discussing 2 lines of code. :D

Comment 5

[rsx11m]

Attachment: Use a single if() only

(In reply to Frank-Rainer Grahl from comment #4)
> > if (currentUIVersion >= UI_VERSION)
> >       return;

Admittedly, this would make it consistent with the other applications, which in turn had the UI_VERSION from the beginning (thus, our "old-style" migration is unique in this sense). Also, the others may have multiple migrations in a single version step, thus one step isn't necessarily just one migration.

> Also one "else" less which reduces complexity by 0.0001% :)

I'd think that a single "branch" instruction over the "else" part is really cheap... ;-)

Anyway, as long as we move the comment inside the "if (currentUIVersion < 1)" clause this still looks reasonably distinguishable. So, let's just go with this and save a few picoseconds.

I've also adjusted capitalization and punctuation in the new comment.

Comment 6

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8870409 [details] [diff] [review]
Use a single if() only

Looks good. Just as I wanted it :) I think this is very clean looking now.

> So, let's just go with this and save a few picoseconds.

I wouldn't mind to save the whole bunch of picoseconds for the 2 migration routines in 2.49 esr. If you think so too please ask IanN for approval.

Comment 7

[rsx11m]

Thanks, pushed as https://hg.mozilla.org/comm-central/rev/849228c56d57

Comment 8

[rsx11m]

Comment on attachment 8870409 [details] [diff] [review]
Use a single if() only

(In reply to Frank-Rainer Grahl from comment #6)
> I wouldn't mind to save the whole bunch of picoseconds for the 2 migration
> routines in 2.49 esr. If you think so too please ask IanN for approval.

Should be simple enough.

[Approval Request Comment]
Regression caused by (bug #): No regression, follow-up to bug 1331477
User impact if declined: startup time is a few picoseconds longer
String changes made by this patch: none

Comment 9

[rsx11m]

Comment on attachment 8870409 [details] [diff] [review]
Use a single if() only

[Approval Request Comment (cont.)]
Testing completed (on m-c, etc.): SM 2.49 (c-r build just before the last merge)
Risk to taking this patch (and alternatives if risky): very low.

This time I've actually retested the patch on a branch build /before/ claiming that it works there! ;-)

Comment 10

[Ian Neal]

Comment on attachment 8870409 [details] [diff] [review]
Use a single if() only

[Triage Comment]
a=me for all repos that need it

Comment 11

[rsx11m]

Thanks, pushed as
https://hg.mozilla.org/releases/comm-beta/rev/ac2c15271d47
https://hg.mozilla.org/releases/comm-esr52/rev/82ad24f24c2c