Migration for old profiles #2990
Conversation
system-addon/lib/ManualMigration.jsm
Outdated
| if (migrationLastShownDate < today) { | ||
| let profileAge = new ProfileAge(null, null); |
There was a problem hiding this comment.
This constructor works with no args. Remove the double null
system-addon/lib/ManualMigration.jsm
Outdated
| if (migrationLastShownDate < today) { | ||
| let profileAge = new ProfileAge(null, null); | ||
| let profileCreationDate = await profileAge.created; | ||
| let daysSinceProfileCreation = (Date.now() - profileCreationDate) / MS_PER_DAY; |
There was a problem hiding this comment.
Any reason to do this only inside if (migrationLastShownDate < today) ? It seems like all the profile age stuff can be done at the beginning of isMigrationMessageExpired and just short circuit the rest of the method? It would also avoid setting the remaining/lastshown prefs below… unless that's desired to set?
There was a problem hiding this comment.
The reason I had it like was to reduce the number of returns in the function. Makes it easier to reason (at least for me). But I don't feel strongly about it.
system-addon/lib/ManualMigration.jsm
Outdated
| if (!alreadyExpired && this.isMigrationMessageExpired()) { | ||
| async expireIfNecessary(alreadyExpired) { | ||
| const isMigrationExpired = await this.isMigrationMessageExpired(); | ||
| if (!alreadyExpired && isMigrationExpired) { |
There was a problem hiding this comment.
adding await in the same original line is well within 80 characters on the line:
if (!alreadyExpired && await this.isMigrationMessageExpired()) {
|
@Mardak updated & answered your question. Let me know if you still want me to make the change. |
|
The code is doing unnecessary things and will be confusing to others why those changes are being done. E.g., "why are we checking and updating last shown date if profile age is independent of that?" Move the profile age check and add a comment above that section to make it easier for you to reason. |
Closes #2967