Going from SM1.x to SM2.x there are certain strings we want to make sure are URL specs e.g. privacy.popups.sound_url browser.download.finished_sound_url and possibly more

This patch: * Adds a new transform method called SetFile (maybe could be called SetURLSpec?) this patch is based on part of a patch I have for bug 441403 but makes it more generic so that it can be used for multiple preferences by making use of both sourcePrefName and targetPrefName

Comment on attachment 331409 [details] [diff] [review] patch for nsNetscapeProfileMigratorBase v0.1 >+ if (aTransform->targetPrefName) { I would do this at the end, just after setting the source pref. (I think I overlooked this in the other bug because I thought that the source pref was going to be written anyway.) You then wouldn't have to bother with separate rv and res variables. >+ printf("targetPrefName hit with %s\n", aTransform->targetPrefName); ;-) >+ nsCString fileURL = nsDependentCString(aTransform->stringValue); nsDependentCString fileURL(aTransform->stringValue); Or possibly use nsCString fileURL(aTransform->stringValue); and reuse it in place of fileURLSpec. >+ // Start off by assuming fileURL is a URL spec and >+ // try and get a File from it Apparently if you want to start your comment with a capital letter you should end with a full stop. >+ // Okay it wasn't a URL spec so assume it is a localfile, >+ // if this fails then just return result of last pref setting But you don't, do you? You just don't set anything at all.

Changes since v0.1 (as per Neil's comments) * Removed printf * Moved targetPrefName code to end of method * Used rv throughout * Reused fileURL * Ended comments with a full stop * Adjusted wording of the one comment

Comment on attachment 331421 [details] [diff] [review] patch for nsNetscapeProfileMigratorBase v0.1a >+ rv = aBranch->SetCharPref(aTransform->sourcePrefName, >+ fileURL.get()); >+ } >+ if (aTransform->targetPrefName) { >+ rv |= aBranch->SetIntPref(aTransform->targetPrefName, >+ aTransform->stringValue == "" ? 0 : 1); >+ } Actually I was thinking that you could just set the int pref to 1 at the end of the previous block, since that's when we know we have a file URL. [The comparison with "" is erroneous anyway, you would use *aTransform->stringValue ? 1 : 0]

Changes since v0.1a: * Moved setting of targetPrefName preference into previous if statement as per comment and made it conditional on sourcePrefName preference being set successfully.

Comment on attachment 331429 [details] [diff] [review] patch for nsNetscapeProfileMigratorBase v0.1b + if (NS_FAILED(rv)) return NS_OK; nit: please put the return on the next line (two places). + if (NS_SUCCEEDED(rv) && exists) { + rv = aFile->IsFile(&exists); + } nit: no need for the braces, but please ensure there is a blank line after the rv = aFile... (two places). r=me with those fixed.

Changes since v0.1b: * Made changes as per review comments Carrying forward r/sr

Comment on attachment 331648 [details] [diff] [review] patch for nsNetscapeProfileMigratorBase v0.1c (Checkin: Comment 8) http://hg.mozilla.org/comm-central/index.cgi/rev/c295f23e75c6