335692 - Remove nsIPref from profile/pref-migrator

Status: RESOLVED FIXED

(Core Graveyard :: Profile: Migration, defect)

Description

[Alfred Peng]

Replace nsIPref with nsIPrefBranch and nsIPrefService.

Comment 1

[Alfred Peng]

Attachment: Patch v1

Some questions for the patch:

1. For the method GetComplexValue, nsIFileSpec has been marked as a deprecated argument. Do we need to remove it from nsPrefMigration::GetPremigratedFilePref?

2. nsIPref has a method named EnumerateChildren. I replace it with GetChildList in my patch. Don't know whether it's appropriate.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm not a great reviewer for this patch.  I don't know much about this code, so it would take me a while to get to it.  You might want to try someone who's reviewed nsIPref/nsIPrefBranch stuff before.

> Do we need to remove it from nsPrefMigration::GetPremigratedFilePref?

I doubt it...

Comment 3

[Alfred Peng]

Comment on attachment 220018 [details] [diff] [review]
Patch v1

caillon, roc, I checked on bugzilla and found out that  you've helped review nsIPref/nsIPrefBranch related bugs. Hope you can help me review this one.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sorry about the delay Alfred. This patch needs to be updated to trunk. I promise I'll review it quickly next time.

Comment 5

[Alfred Peng]

Attachment: Patch v2

Update the patch based on the change of the trunk.

roc, it's fine. Anyway, thanks for your review :-)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You need to call NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY after each successful GetChildList. Also, the GetChildList parameters should have a '.' after the preference so you don't match things like "intl.fontstuff".

Comment 7

[Alfred Peng]

Attachment: Patch v3

Update the patch based on roc's comment.

Two problems:
1. There are three chunk of similar code in the patch. Do I need to make a helper function to handle them?
2. I check the definition of nsPref::EnumerateChildren. It also calls GetChildList to get the array. If we add "." after the preferences, the meaning of the function will be changed I think.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #7)
> Two problems:
> 1. There are three chunk of similar code in the patch. Do I need to make a
> helper function to handle them?

No, I'll let that go :-)

> 2. I check the definition of nsPref::EnumerateChildren. It also calls
> GetChildList to get the array. If we add "." after the preferences, the
> meaning of the function will be changed I think.

True. The old code was buggy. I think this is the right thing to do.

+  if (prefs->GetChildList("intl.font.", &count, &childArray)) {

You should be calling NS_SUCCEEDED here, not just testing the result.

Almost done now :-)

Comment 9

[Alfred Peng]

Attachment: Patch v4

Updated patch.

Comment 10

[Leon Sha]

Checking in nsPrefMigration.cpp;
/cvsroot/mozilla/profile/pref-migrator/src/nsPrefMigration.cpp,v  <--  nsPrefMigration.cpp
new revision: 1.206; previous revision: 1.205
done
Checking in nsPrefMigration.h;
/cvsroot/mozilla/profile/pref-migrator/src/nsPrefMigration.h,v  <--  nsPrefMigration.h
new revision: 1.54; previous revision: 1.53
done

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This broke the Windows SeaMonkey build (which uses the old-style for loop scoping rules):
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey

Comment 12

[Alfred Peng]

Has been fixed by timeless. Thanks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

For future reference, see http://www.mozilla.org/hacking/portable-cpp.html#variables_in_for