From a user experience point of view someone doing a reset could lose years of custom spellings they added. I've looked for serious problems that point to persdict.dat as the cause and haven't found any myself, but others can comment if they know any.

Should this copy the file, move the file, or re-write (i. e. read & write database entries directly) the file? It may make a difference if the file is corrupt.

I would suggest in this case (and any where we transfer user data) we verify and re-write the file to help avoid any issues with corruption.

(In reply to Tyler Downer [:Tyler] from comment #2) > I would suggest in this case (and any where we transfer user data) we verify > and re-write the file to help avoid any issues with corruption. If we're adding the ability to verify the integrity of a file then we should do this whenever we read the file (or on idle for a long-running verification) and do the re-write fix then. There's no need to wait for a user to reset in order to repair known data issues. We already do this type of thing with places.sqlite and sessionstore.js as an example. Reset should be saved for fixing cases where it's not straightforward to write code to identify problems. It would be fine to run such verification code before migrating the data to the new profile in case the regular checks aren't running for some reason. In this case the file format is simple and the file gets re-written on save so I think the chances of corruption are unlikely. If there are known dictionary corruption issues, file a bug to detect such issues and repair them automatically.

Just migrating the file is pretty straightforward. Looking at the personal dictionary implementation (which by its own comments is "braindead"), it seems like it'd be hard to corrupt it in a meaningful way, as it's just a UTF-8 encoded newline-separated list of words. ( http://hg.mozilla.org/mozilla-central/annotate/55f9e3e3dae7/extensions/spellcheck/src/mozPersonalDictionary.cpp#l143 ). So, not doing any verification/checking seems best.

Matt, asking feedback because I'm not entirely sure who to ask for review, and I guess that because of Fx reset, you own it now. :-)

Ahem, sorry for bugspam, but I forgot: do we need to update the UI to specifically call out that we save the user's personal dictionary? I'm not decided, but I guess at a minimum we should correct this SUMO article if/when this lands: https://support.mozilla.org/en-US/kb/reset-firefox-easily-fix-most-problems

Comment on attachment 733826 [details] [diff] [review] Migrate the file Review of attachment 733826 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Gijs Kruitbosch from comment #4) > So, not doing any verification/checking seems best. I agree in this case. If the file format wasn't so simple, I would probably lean towards WONTFIX for the bug (for the same reason as #2 below). (In reply to :Gijs Kruitbosch from comment #6) > do we need to update the UI to > specifically call out that we save the user's personal dictionary? I'm not > decided, but I guess at a minimum we should correct this SUMO article > if/when this lands: > https://support.mozilla.org/en-US/kb/reset-firefox-easily-fix-most-problems I don't think it's necessary to list it in the dialog for two reasons: 1) If there is too much to read, people won't read it 2) I think it's unlikely that someone would cancel a reset because they thought their personal dictionary wouldn't be migrated. We should update the 64_firefox string in migration.properties though. I'm tempted to change it back to "Other Data" since I don't think it was necessary to call out bookmark backups in the first place and because every change to these strings requires adding a slightly hacky mapping in MU_getLocalizedString. r=me with the string change. Updating the SUMO article makes sense to me. I'll add the user-doc-needed keyword to the bug.

(In reply to Matthew N. [:MattN] from comment #7) > r=me with the string change. Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/35179d807373

(I just noticed that the personal dictionary is now listed as something that will be saved)