448105 - Provide a pref transform method to migrate a string to a URL spec

Status: RESOLVED FIXED

(SeaMonkey :: Startup & Profiles, defect)

Description

[Ian Neal]

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

Comment 1

[Ian Neal]

Attachment: patch for nsNetscapeProfileMigratorBase v0.1

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 2

[[e-mail]]

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.

Comment 3

[Ian Neal]

Attachment: patch for nsNetscapeProfileMigratorBase v0.1a

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 4

[[e-mail]]

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]

Comment 5

[Ian Neal]

Attachment: patch for nsNetscapeProfileMigratorBase v0.1b

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 6

[Mark Banner (:standard8)]

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.

Comment 7

[Ian Neal]

Attachment: patch for nsNetscapeProfileMigratorBase v0.1c (Checkin: Comment 8)

Changes since v0.1b:
* Made changes as per review comments

Carrying forward r/sr

Comment 8

[Ian Neal]

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