226005 - MailNews should use the newer nsIPrefService APIs instead of nsIPref

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mikael Parknert]

addressbook should not use the deprecated nsIPref

Comment 1

[Mikael Parknert]

patch coming in a few days

Comment 2

[Mikael Parknert]

Attachment: [part 1]Simple changes

Comment 3

[Mikael Parknert]

Comment on attachment 137803 [details]
[part 1]Simple changes

oops wrong file

Comment 4

[Mikael Parknert]

Attachment: [patch 1]Simple changes

Comment 5

[David :Bienvenu]

thx for doing this.

+
+			    nsCOMPtr<nsIPrefBranch>
prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
+			    if (NS_FAILED(rv))
+			        return rv;
+
+			    if (prefBranch)
+			    {
+                   prefBranch->GetBoolPref("mail.mdn.report.enabled",
+                                           &bVal);
+                   m_mdnEnabled = bVal;
+                   prefBranch->GetIntPref("mail.mdn.report.not_in_to_cc",
+                                          &m_notInToCcOp);
+                   prefBranch->GetIntPref("mail.mdn.report.outside_domain",
+                                           &m_outsideDomainOp);
+                   prefBranch->GetIntPref("mail.mdn.report.other",
+                                          &m_otherOp);
+                }

looks like there are tabs in here.

Here, you don't need to check both rv and the return pointer, so just check one
of them (null pointer check is probably preferred). This occurs twice in
nsStreamConverter.cpp and once in nsMsgDatabase.cpp, nsMsgSendLater.cpp,
nsMsgSend.cpp, nsMsgAttachmentHandler.cpp, nsMimeHtmlEmitter.cpp and
nsMimeBaseEmitter.cpp(I know it was this way before, but while you're here, you
should fix it)

-    nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv));
+    nsCOMPtr<nsIPrefBranch>
pPrefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
     if (NS_SUCCEEDED(rv))
-       prefs->GetBoolPref("mailnews.confirm.moveFoldersToTrash", &confirmDeletion);
+       pPrefBranch->GetBoolPref("mailnews.confirm.moveFoldersToTrash",
&confirmDeletion);

I'm happy to check this in for you once you get an sr, if you don't have checkin
rights.

Comment 6

[Mikael Parknert]

Attachment: [patch 1] new version (checked in 20040104)

new patch, fixed from issues in comment #5

Comment 7

[Mikael Parknert]

Attachment: [patch 2] more changes - working on a new

beeing new to mozilla code I try not to make to large patches. bienvenu - could
you help with checkin of patch 1?

Comment 8

[David :Bienvenu]

sure, I'll check in the first patch.

Comment 9

[David :Bienvenu]

patch 1 checked in.

Comment 10

[[e-mail]]

Comment on attachment 138371 [details] [diff] [review]
[patch 2] more changes - working on a new

The changes to nsSmtpServer.cpp don't use the full power of nsIPrefBranch :-(

Comment 11

[Mikael Parknert]

is DeleteBranch what you're missing? if so - New patch coming up...

Comment 12

[[e-mail]]

Actually no... I'll explain in a sec...

Comment 13

[Mikael Parknert]

the usage of getPrefString().. Maybe I should make a member var to point at
branch 
"mail.smtpserver." + mKey instead?

Comment 14

[[e-mail]]

Quite right, in fact you might not need mKey as a member if you do that. Also a
static for mail.smtpserver.default would be nice - allocate the branch when you
create the first instance and deallocate it when you delete the last instance.

Comment 15

[Mikael Parknert]

Attachment: [patch 2] trying to address Neil's comments

Neil, is this better?

Comment 16

[Mikael Parknert]

Attachment: [patch 3] some more nsIPref uses

This patch also removes some unneeded calls to get PrefBranch or
PrefBranchInternal from PrefService

Comment 17

[[e-mail]]

I'm not 100% sure so I'm cc'ing caillon for a definitive statement but I seem to
remember him stating that getBranch() is the preferred way to access preferences
(rather than [ab]using the pref service as the null branch) which would imply
that removing getBranch(null) is incorrect.

Comment 18

[Mikael Parknert]

Attachment: [patch 2] forgot the static

Comment 19

[[e-mail]]

Comment on attachment 138641 [details] [diff] [review]
[patch 2] forgot the static

>Index: mailnews/compose/src/nsSmtpServer.cpp
I'm just looking at this file, because there is some lovely cleanup here, but I
have a few nits...

>+#include "nsIPrefBranch.h"
> #include "nsEscape.h"
> #include "nsSmtpServer.h"
nsSmtpServer.h already includes nsIPrefBranch.h - I'm not sure what the correct
style is here.

>+    if(!mDefPrefBranch) {
>+        nsCOMPtr<nsIPrefService> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
>+        prefs->GetBranch("mail.smtpserver.default", getter_AddRefs(mDefPrefBranch));
>+    }
Hmmm... this is never released...

>+    
Adding a line of whitespace is very naughty ;-)

> nsSmtpServer::SetKey(const char * aKey)
Seeing this amused me because the caller casts away const :-)

>+    nsCAutoString  branchName;
>+    branchName = "mail.smtpserver.";
>+    branchName += mKey;
>+
>+    rv = prefs->GetBranch(branchName.get(), getter_AddRefs(mPrefBranch));
Hmmm... I feel sure there must be a better way to do this... nsCAutoString
branchName = NS_LITERAL_STRING("mail.smtpserver.") + mKey; perhaps?

>+        getDefaultIntPref(0, "try_ssl", trySSL);
I think getDefaultIntPref is probably small enough to inline.

> NS_IMETHODIMP
> nsSmtpServer::ClearAllValues()
> {
>-    nsresult rv = NS_OK;
>-    nsCOMPtr<nsIPref> prefs(do_GetService(NS_PREF_CONTRACTID, &rv));
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    nsresult rv;
>+    PRUint32 numChildren;
>+    char     **childArray;
> 
>-    nsCAutoString rootPref("mail.smtpserver.");
>-    rootPref += mKey;
>+    rv = mPrefBranch->GetChildList("", &numChildren, &childArray);
>+    if (NS_SUCCEEDED(rv))
>+    {
>+        PRUint32  i;
>+        for (i = 0; i < numChildren; i++) 
>+            mPrefBranch->ClearUserPref(childArray[i]);
> 
>-    rv = prefs->EnumerateChildren(rootPref.get(), clearPrefEnum, (void *)prefs.get());
>+        NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(numChildren, childArray);
>+    }
> 
>     return rv;
It's a pity that mPrefBranch->resetBranch(null); doesn't work AFAIK :-(

Comment 20

[Mikael Parknert]

Attachment: [patch 2] new, with comments from irc

Comment 21

[David :Bienvenu]

Comment on attachment 138653 [details] [diff] [review]
[patch 2] new, with comments from irc

you don't need to init comptrs to null. It happens automatically:

+    mPrefBranch(nsnull),
+    mDefPrefBranch(nsnull)

so those lines aren't needed. other than that, r=bienvenu

Comment 22

[David :Bienvenu]

Comment on attachment 138632 [details] [diff] [review]
[patch 3] some more nsIPref uses

as before, no need to init comptrs: nsMsgAccount::nsMsgAccount():
-  m_prefs(0),
+  m_prefs(nsnull),
   m_incomingServer(nsnull),
   m_defaultIdentity(nsnull)

all of these can be cleaned up.

this too:
-  m_prefs(0)
+  m_prefs(nsnull)

Comment 23

[[e-mail]]

I think I spotted a bug in the latest patch... I'm not convinced that you always
initialize mDefaultPrefBranch although it occurs to me that it's not used as
much as I had originally thought (especially given how difficult it is to share
the default branch between servers) so perhaps there won't be a problem just
getting it when it is needed after all.

Comment 24

[Christopher Aillon (sabbatical, not receiving bugmail)]

> I'm not 100% sure so I'm cc'ing caillon for a definitive statement but I seem to
remember him stating that getBranch() is the preferred way to access preferences
(rather than [ab]using the pref service as the null branch) which would imply
that removing getBranch(null) is incorrect.

It is preferred in that it uses the code the way it was designed.  QI'ing works,
but that is just a "coincidence" which is not guaranteed by the interface
contracts, so getting the null branch that way is technically "impure".  That
said, I don't really care.  It would be measurably easier to fix the QI'ing at
some future date than it will be to change nsIPref usage to the frozen
interfaces, but the less future fixing needed the better.

Comment 25

[Mikael Parknert]

Attachment: [patch 2], w/ changes from comment 21

Comment 26

[Mikael Parknert]

Attachment: [patch 3] w/ changes from comment 22

Comment 27

[Boris Zbarsky [:bzbarsky]]

I'm not likely to be able to get to this before 1.7a freezes...

Comment 28

[Mikael Parknert]

Comment on attachment 138710 [details] [diff] [review]
[patch 2], w/ changes from comment 21

I've found out a problem with this patch.. Be back later

Comment 29

[Darin Fisher]

Comment on attachment 138711 [details] [diff] [review]
[patch 3] w/ changes from comment 22

>Index: mailnews/base/src/nsMsgAccount.cpp

>+nsMsgAccount::nsMsgAccount()
> {
> }
> 
> nsMsgAccount::~nsMsgAccount()
> {
> }

nit: i would look at moving these into the header file, thus inlining them,
and i might even just eliminate them altogether ;-)


> {
>   if (m_prefs) 
>     return NS_OK;
...
>+  m_prefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  if(NS_FAILED(rv))
>+    m_prefs = nsnull;

style nit: keep style consistent... add space between "if" and opening "("


>+    PRUint32 cntChild, i;
>+    char     **childArray;

nit: do away with the extra spaces in |char	**childArray;|


>+    m_prefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
> 
>+  if (NS_FAILED(rv)) return rv;
>   /* m_prefs is good now */
>   return NS_OK;
> }

hmm... can you you just do |return rv;| instead?
is there too much space before |m_prefs|?


>Index: mailnews/base/src/nsMsgAccountManagerDS.cpp

>-     if (prefBranch) {
>-       prefBranchInternal = do_QueryInterface(prefBranch);
>+   nsCOMPtr<nsIPrefBranchInternal> prefBranchInternal = do_GetService(NS_PREFSERVICE_CONTRACTID);
>+   if (prefBranchInternal)
>        prefBranchInternal->AddObserver(PREF_SHOW_FAKE_ACCOUNT, this, PR_FALSE);

nit: looks like old indentation style was two spaces... keep it consistent! :)


sr=darin

Comment 30

[Mikael Parknert]

Attachment: [patch 4]

Comment 31

[Mikael Parknert]

Attachment: [patch 3] adressing darins comments

Comment 32

[[e-mail]]

Comment on attachment 141240 [details] [diff] [review]
[patch 4]

>-  nsCOMPtr<nsIPrefBranch> prefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  nsCOMPtr<nsIPrefBranch> prefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
Not sure what house style is here...

>-  nsCOMPtr<nsIPrefService> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>-  NS_ENSURE_SUCCESS(rv,rv);
>-
>-  nsCOMPtr<nsIPrefBranch> prefBranch;
>-  rv = prefs->GetBranch(nsnull, getter_AddRefs(prefBranch));
There was no reason to change this code, it is 110% correct.

>+      if (NS_SUCCEEDED(rv))
>+        tmp->ToString(getter_Copies(prefixValue));
>+
>       if (prefixValue.get())
>         mEmailPrefix.Assign(prefixValue);
>       else
>         mEmailPrefix.Trunate(0);
Am I overlooking something?
if (NS_SUCCEEDED(rv))
  tmp->GetData(mEmailPrefix);
else
  mEmailPrefix.Trunate(0);

>+NS_MSG_BASE nsresult NS_GetUnicharPreferenceWithDefault(nsIPrefBranch *prefBranch,
>+                                                        const char *prefName,
>+                                                        const nsAString& defValue,
>+                                                        PRUnichar **prefValue)
I would like to suggest that this return via an nsAString&, and use
nsAutoString in your callers instead of nsXPIDLString.

>+        nsCOMPtr<nsIPrefLocalizedString> str;
>+        nsresult rv = prefBranch->GetComplexValue(prefName, NS_GET_IID(nsIPrefLocalizedString), getter_AddRefs(str));
>+        if (NS_SUCCEEDED(rv)) 
>+            return str->ToString(prefValue);
I would prefer it if you used GetData instead please.

>+  m_DefaultCharacterSet.AssignWithConversion(charset);
CopyUTF16toUTF8(charset, m_DefaultCharacterSet);

>Index: mailnews/compose/src/nsSmtpServer.cpp
What happened to all the lovely cleanup in your earlier patch?

>+        gDefaultCharacterSet.AssignWithConversion(ucsval.get());
CopyUTF16toUTF8 again please.

>+        m_defCharset.Assign(defaultCharset.get());
Actually I think this can be simplified to m_defCharset.Assign(defaultCharset);
- the old code used .get() to work with the ?: operator.

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

>  mEmailPrefix.Trunate(0);

err, don't use Truncate(0), use Truncate().

Comment 34

[Simon Paquet [:sipaq]]

Mikael, any news on this? Shouldn't your last patch ([patch 3] adressing darins
comments) be reviewed?

Comment 35

[Mikael Parknert]

Attachment: [patch 3] adressing darins comments, re-diffed (checked in)

Comment 36

[David :Bienvenu]

Comment on attachment 154148 [details] [diff] [review]
[patch 3] adressing darins comments, re-diffed (checked in)

thx. Let me know if you want me to check this in...

Comment 37

[Mikael Parknert]

(In reply to comment #36)
> (From update of attachment 154148 [details] [diff] [review])
> thx. Let me know if you want me to check this in...
> 
David, I'd like check-in help please!

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

+        prefName = NS_LITERAL_CSTRING("ldap_2.servers.pab.description");

this could use .AssignLiteral

Comment 39

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 154148 [details] [diff] [review]
[patch 3] adressing darins comments, re-diffed (checked in)

patch 3 checked in

Comment 40

[Christian :Biesinger (don't email me, ping me on IRC)]

(with the change from comment 38 made)

Comment 41

[Mikael Parknert]

Attachment: [patch] adressing Neil's comment 32

Comment 42

[Mikael Parknert]

Attachment: [patch 4] updated

Comment 43

[[e-mail]]

Comment on attachment 163241 [details] [diff] [review]
[patch 4] updated

Sorry, but I've got too many issues to give this even a conditional +
The -p flag to diff would have been nice in places.

>+  rv = pPrefBranchInt->AddObserver(PREF_MAIL_COLLECT_ADDRESSBOOK, this, PR_TRUE);
I don't think you need a weak observer here, the address book collector service
isn't going to go away in a hurry...

>+    nsresult NeedToSearchReplicatedLDAPDirectories(nsIPrefBranch *aPrefs, PRBool *aNeedToSearch);
>+    nsresult NeedToSearchLocalDirectories(nsIPrefBranch *aPrefs, PRBool *aNeedToSearch);
I don't see these using members so it could be possible to make these static?

>+        nsXPIDLString thestr;
>+        str->ToString(getter_Copies(thestr));
>+        prefValue = thestr;
Would it be possible to make prefValue an nsXPIDLString& instead? Most of the
callers wouldn't notice the difference between an nsXPIDLString and an
nsString.

>+    NS_GetUnicharPreferenceWithDefault(prefBranch, "mailnews.reply_header_locale", EmptyString(),
>+                                       reply_header_locale);
>+    NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "mailnews.reply_header_authorwrote", NS_LITERAL_STRING("%s wrote"),
>+                                                reply_header_authorwrote);
>+    NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "mailnews.reply_header_ondate", NS_LITERAL_STRING("On %s"),
>+                                                reply_header_ondate);
>+    NS_GetUnicharPreferenceWithDefault(prefBranch, "mailnews.reply_header_separator", NS_LITERAL_STRING(", "),
>+                                       reply_header_separator);
>+    NS_GetUnicharPreferenceWithDefault(prefBranch, "mailnews.reply_header_colon", NS_LITERAL_STRING(":"),
>+                                       reply_header_colon);
>+    NS_GetLocalizedUnicharPreferenceWithDefault(prefBranch, "mailnews.reply_header_originalmessage",
>+                                                NS_LITERAL_STRING("--- Original Message ---"),
>+                                                reply_header_originalmessage);
Strange wrapping here... you should wrap at 80 cols or not bother.

>         nsCAutoString combined;
>         combined = hostname.get();
>         combined += ":";
>         combined.AppendInt(port);
Gosh, our strings were so awful back then, we couldn't even AppendInt on an
nsXPIDLCString :-(

>+    rv = mPrefBranch->GetIntPref("try_ssl", trySSL);
>     if (NS_FAILED(rv))
>+        getDefaultIntPref(0, "try_ssl", trySSL);
Hmm... is this sequence common enough to fold it into PRInt32
getIntPrefWithDefault(char *, PRInt32)? I'm just trying for even more - lines
in the patch ;-)

>+        mServerKeyList += ",";
You were probably just reindenting this but .Append(',') might be better.

> REQUIRES	= xpcom \
> 		  xpcom_obsolete \
> 		  string \
> 		  import \
> 		  addrbook \
> 		  mork \
> 		  intl \
> 		  msgbase \
>+      pref \
> 		  mailnews \
> 		  necko \
> 		  msgdb \
> 		  msgbaseutil \
> 		  msgcompose \
> 		  msglocal \
> 		  unicharutil \
> 		  $(NULL)
This is a makefile, so you do have to use tabs...

>+        prefBranch->GetBoolPref(PREF_MAPI_WARN_PRIOR_TO_BLIND_SEND,&warn);
>+        prefBranch->GetBoolPref(PREF_MAPI_BLIND_SEND_ENABLED,&enabled);
>+        prefBranch->SetBoolPref(PREF_MAPI_WARN_PRIOR_TO_BLIND_SEND,PR_FALSE);
Please fix these to have space after comma, thanks.

>-      AppendUTF16toUTF8(detector_name, detector_contractid);
>-    }
>-  }
>+  NS_GetLocalizedUnicharPreferenceWithDefault(nsnull, "intl.charset.detector", EmptyString(), detector_name);
>+  detector_contractid.Append(NS_ConvertUCS2toUTF8(detector_name));
The "original" code that I quoted above was better. Maybe you missed out on a
sweep of UCS2 changes?

>   if (detector_contractid.Length() > sizeof(NS_STRCDETECTOR_CONTRACTID_BASE)) {
Although, the surrounding code is worse, there's even an nsFixedCString!

> #include "mimemoz2.h"
>+#include "nsIPrefBranch.h"
Hmm... mimemoz2.h already includes this

Comment 44

[[e-mail]]

I'm told that that part of the makefile doesn't actually require tabs but you
still need them for consistency.

Comment 45

[[e-mail]]

As per bug 273311 comment 2 (which is about to bitrot this patch) it would be
nice if you converted the clear user pref code to delete the branch instead.

Comment 46

[Mikael Parknert]

Attachment: [patch 4] updated adressing comments 43,45

Comment 47

[[e-mail]]

Comment on attachment 169382 [details] [diff] [review]
[patch 4] updated adressing comments 43,45

I did spot a few nits:

>+    nsresult NeedToSearchReplicatedLDAPDirectories(nsIPrefBranch *aPrefs, PRBool *aNeedToSearch);
Having made it static in the .cpp you don't need it here. (Or alternatively
make it static here, not in the .cpp).

>+  *srcCharset = ToNewUnicode(m_defaultCharset.IsEmpty() ? 
>+    NS_LITERAL_STRING("ISO-8859-1") : m_defaultCharset);
I'm not sure all compilers can handle that. Perhaps you should put ToNewUnicode
in each case of the ?: instead.

>+    destination = *srcCharset;
I'm not sure if .Assign is preferred here...

>       nsXPIDLString prefixValue;
>-      prefService->CopyUnicharPref(providerPrefixPref.get(), getter_Copies(prefixValue));
>-      if (prefixValue.get())
>-        mEmailPrefix.Assign(prefixValue);
I don't think you're using prefixValue any more here.

>+    if (NS_SUCCEEDED(rv)) {
>+        str->ToString(getter_Copies(prefValue));
>+    }
>+    else
>+        prefValue = defValue;
Inconsistent bracing.

>+#include "nsIPrefBranch.h"
You didn't want to use class nsIPrefBranch; this time?

>-        prefs->ClearUserPref(pref.get());
>+        mPrefBranch->DeleteBranch("hostname");
Sorry, I didn't mean you to change these one to delete branch, just the
DeleteBranch("") at the end, speaking of which, you could write
DeleteBranch(nsnull), I'm not sure which is preferred here.

>+  if (!detector_name.IsEmpty()) {
>+    nsCOMPtr<nsIStringCharsetDetector> detector = do_CreateInstance(detector_contractid.get(), &res);
Consider moving the contract id string concatenation inside this block.

Comment 48

[Mikael Parknert]

Attachment: [patck 4] updated

(In reply to comment #47)
> >+	nsresult NeedToSearchReplicatedLDAPDirectories(nsIPrefBranch *aPrefs,
PRBool *aNeedToSearch);
> Having made it static in the .cpp you don't need it here. (Or alternatively
> make it static here, not in the .cpp).

sorry, missed that
 
> >+  *srcCharset = ToNewUnicode(m_defaultCharset.IsEmpty() ? 
> >+	NS_LITERAL_STRING("ISO-8859-1") : m_defaultCharset);
> I'm not sure all compilers can handle that. Perhaps you should put
ToNewUnicode
> in each case of the ?: instead.
done

> >+	destination = *srcCharset;
> I'm not sure if .Assign is preferred here...

I don't know :-) changed it to .Assign anyway 

> >	  nsXPIDLString prefixValue;
> >-	  prefService->CopyUnicharPref(providerPrefixPref.get(),
getter_Copies(prefixValue));
> >-	  if (prefixValue.get())
> >-	    mEmailPrefix.Assign(prefixValue);
> I don't think you're using prefixValue any more here.

removed

> >+	if (NS_SUCCEEDED(rv)) {
> >+	    str->ToString(getter_Copies(prefValue));
> >+	}
> >+	else
> >+	    prefValue = defValue;
> Inconsistent bracing.

fixed

> >+#include "nsIPrefBranch.h"
> You didn't want to use class nsIPrefBranch; this time?
You are right, more consistent that way. done

> >-	    prefs->ClearUserPref(pref.get());
> >+	    mPrefBranch->DeleteBranch("hostname");
> Sorry, I didn't mean you to change these one to delete branch, just the
> DeleteBranch("") at the end, speaking of which, you could write
> DeleteBranch(nsnull), I'm not sure which is preferred here.
ok, I was not sure; should have asked. Replaced with ClearUserPref!

I think DeleteBranch("") is preferred since there is an NS_ENSURE_ARG_POINTER
on
the argument in the prefbranch-code


> >+  if (!detector_name.IsEmpty()) {
> >+	nsCOMPtr<nsIStringCharsetDetector> detector =
do_CreateInstance(detector_contractid.get(), &res);
> Consider moving the contract id string concatenation inside this block.
> 
done

Comment 49

[[e-mail]]

(In reply to comment #48)
>I think DeleteBranch("") is preferred since there is an NS_ENSURE_ARG_POINTER
>on the argument in the prefbranch-code
Silly me, I looked at getPrefName instead of getValidatedPrefName

Comment 50

[[e-mail]]

Comment on attachment 170023 [details] [diff] [review]
[patck 4] updated

In reference to my previous comment I'd put the declaration and initialization
of detector_contractid in the if block too.

Comment 51

[Darin Fisher]

Comment on attachment 170023 [details] [diff] [review]
[patck 4] updated

>Index: mailnews/base/search/src/nsMsgSearchAdapter.cpp

>+  } else
>+    destination.Assign(*srcCharset);
>+

maybe write the braces like this instead:

    }
    else
    {
      destination.Assign(*srcCharset);
    }

that would seem to be more consistent with the existing coding style.


sr=darin

Comment 52

[Mikael Parknert]

Attachment: [patch 4] updated (2) check ed in

It seems that the patch you r/sr:ed missed some files (those were
present in earlier versions)

these files were not present
Index: mailnews/addrbook/src/nsAbAddressCollecter.cpp
Index: mailnews/addrbook/src/nsAbAutoCompleteSession.cpp
Index: mailnews/compose/src/nsMsgCompose.cpp

sorry about this, re-requesting reviews

Comment 53

[[e-mail]]

Comment on attachment 170756 [details] [diff] [review]
[patch 4] updated (2) check ed in

Assuming those three files were unchanged so they don't need new reviews ;-)

Comment 54

[Mikael Parknert]

Comment on attachment 170756 [details] [diff] [review]
[patch 4] updated (2) check ed in

Checked in 050115 by Daniel Brooks, [e-mail]

Comment 55

[Robert Kaiser]

"View > Message Body As >" switching seems broken in current trunk, I suspect it
regressed with this patch somehow...

Comment 56

[Mikael Parknert]

(In reply to comment #55)
> "View > Message Body As >" switching seems broken in current trunk, I suspect it
> regressed with this patch somehow...

I've started looking at this. It seems like the prefBranch "goes away" after a
while. Looking more 

Comment 57

[Mikael Parknert]

Attachment: regression fix

The original mime-code is using the prefs after Release(), but since
the nsIPref* was never zero:ed out - the code could access the
prefs anyway. My patch used an COMPtr, which i clear:ed out,
resulting in failure to get the preferences.

This patch uses a nsIPrefBranch* insted of a COMPtr. I 
will file a new bug on the fact that mime-code releases the
preference but want to use it more. 

My debug-printf:s showed a lot of getting and releaseing of 
the prefs.

Comment 58

[Mikael Parknert]

Comment on attachment 171359 [details] [diff] [review]
regression fix

I filed bug 278596 for the regression

Comment 59

[Mikael Parknert]

Attachment: [patch 5]

Also includes a few lines string-cleanup

Comment 60

[Darin Fisher]

Comment on attachment 172107 [details] [diff] [review]
[patch 5]

>Index: mailnews/base/src/nsMessengerMigrator.cpp

>+  PRUint32 num, i;
>+  char **arr;
>+
>+  rv = m_prefs->GetChildList(ADDRESSBOOK_PREF_NAME_ROOT, &num, &arr);
>+  if (NS_SUCCEEDED(rv)) {
>+    for (i = 0; i < num; i++)
>+      migrateAddressBookPrefEnum(arr[i]);
>+
>+    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(num, arr);
>+  }
>+
>   return rv;
> }

nit: move the declaration of |i| into the for loop initializer
since it is not needed outside of the for loop.


sr=darin with that change

Comment 61

[Mikael Parknert]

Attachment: [patch 5] updated with review comment, for check-in [checked in 2005-01-26]

Comment 62

[Mikael Parknert]

Attachment: [patch 6] diff -w

Last files in /mailnews. This patch removes nsIPref from nsDirPrefs. I have
also filed bug 287832 for some clean up of nsDirPrefs

Comment 63

[Mikael Parknert]

Attachment: [patch 6] for check in

Comment 64

[Darin Fisher]

Can you post an interdiff (using the program interdiff, usually included on most
linux distros and cygwin) between patch 5 and 6?  That would make this much
easier to review.  Thanks!

Comment 65

[Robert Kaiser]

(In reply to comment #64)
> Can you post an interdiff (using the program interdiff, usually included on most
> linux distros and cygwin) between patch 5 and 6?  That would make this much
> easier to review.  Thanks!

darin, shouldn't you be able to view an interdiff with bugzilla?
Say, something like
https://bugzilla.mozilla.org/attachment.cgi?oldid=172202&action=interdiff&newid=179307&headers=0
should already help a bit, I guess...

(If you wonder how I got it: edited patch6, clicked "view as diff" and selected
"differences between patch5 and this patch")

Comment 66

[Darin Fisher]

wow.. isn't that slick?  thanks robert!

Comment 67

[Robert Kaiser]

(In reply to comment #66)
> wow.. isn't that slick?  thanks robert!

Thank Gerv for doing that stuff in Bugzilla and bringing it to our attention on
FOSDEM this year :)

Comment 68

[Christian :Biesinger (don't email me, ping me on IRC)]

if only that worked more reliable...

Comment 69

[Darin Fisher]

Yeah, bugzilla's interdiff seems to have a lot of problems :-(


>+  rv = m_prefs->GetChildList(ADDRESSBOOK_PREF_NAME_ROOT, &num, &arr);
>+  if (NS_SUCCEEDED(rv)) {
>+    for (PRUint32 i = 0; i < num; i++)
>+      migrateAddressBookPrefEnum(arr[i]);
>+
>+    NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(num, arr);
>+  }
>   return rv;

nit: For code like this, an early return if GetChildList fails might be better.


>+  nsCOMPtr <nsIPrefBranch> m_prefs;

nit: Avoid spaces between nsCOMPtr and opening angle bracket.  (Although, if you
can argue that this is the normal style for this code then that's fine.)


nit: DirPrefObserver doesn't need to have a virtual destructor, and moreover it
doesn't need to have a destructor at all.  Better to just not define it.


nit: A simpler way to get the default pref branch is to simply QI nsIPrefService
to nsIPrefBranch, or more compactly:

  nsCOMPtr<nsIPrefBranch> defaultPrefBranch =
    do_GetService(NS_PREFSERVICE_CONTRACTID);

It looks like you use this trick in some places, but not in others.


nit: In DIR_GetStringPref, it looks like there are some tabstops mixed with
whitespace indentation that is broken when the tabstops are varied.  I think you
should either always use tabstops or always use spaces.  Be consistent :)


Patch looks good otherwise.

Comment 70

[[e-mail]]

Comment on attachment 179306 [details] [diff] [review]
[patch 6] diff -w

>+DirPrefObserver::DirPrefObserver(nsIPrefBranch2 *pbi)
>+{
>+  pbi->AddObserver(PREF_LDAP_SERVER_TREE_NAME, this, PR_FALSE);
>+}
>+
>+DirPrefObserver::~DirPrefObserver()
>+{
>+}
>+ 
>+void DirPrefObserver::Shutdown(nsIPrefBranch2 *pbi)
>+{
>+  pbi->RemoveObserver(PREF_LDAP_SERVER_TREE_NAME, this);
>+}
I don't like this. Why can't you just inline it? Or see my comment blow.

>+    delete prefObserver;
>+    prefObserver = nsnull;
You're not allowed to delete XPCOM objects. You must NS_ADDREF the object if
you succesfully create it and NS_RELEASE it when you've finished with it. Note
that if you make the pref observer a weak reference then the release will
unregister the observer.

>+    if (NS_FAILED(rv))
> 		return NS_ERROR_FAILURE;
Might as well return rv in places like this.

> static void DIR_ClearIntPref (const char *pref)
This code is horrible. Please replace the entire call with
pPref->ClearUserPref(pref);

>-		if (PREF_NOERROR == pPref->CopyCharPref (scratch, &userPref))
>+		if (PREF_NOERROR == pPref->GetCharPref (scratch, &userPref))
You're missing a few NS_SUCCEEDED()s around here.

Comment 71

[Mark Banner (:standard8)]

Attachment: [patch 6] updated (diff -w)

I emailed Mikael direct and he's busy with work and other things at the moment, but that I'm welcome to finish his patches for him - hence this is a revised patch  (-w) that addresses Darin's and Neil's comments on the first version. I'll attach the non -w in a moment.

Comment 72

[Mark Banner (:standard8)]

Attachment: [patch 6] updated (normal patch)

Comment 73

[Darin Fisher]

Comment on attachment 201162 [details] [diff] [review]
[patch 6] updated (diff -w)

redirecting review request to mailnews peer.

Comment 74

[David :Bienvenu]

+  else
+  {
+    if (id == idPosition || id == idType || id == idServerName || id == idDescription)
+      dir_ValidateAndAddNewServer(dir_ServerList, prefname);
+  }
+

this can just be else if...

+  if (prefObserver) {
+    nsCOMPtr<nsIPrefBranch2> pbi(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
+    if (NS_FAILED(rv))
+      return rv;
+    
+    NS_RELEASE(prefObserver);
   }

Why are we just not simply calling NS_RELEASE(prefObserver)? Why get the pref service?

+  PR_ASSERT(NS_SUCCEEDED(prefErr));

I much prefer NS_ASSERTION here - PR_ASSERT aborts in debug mode.

Comment 75

[Mark Banner (:standard8)]

(In reply to comment #74)
> +  if (prefObserver) {
> +    nsCOMPtr<nsIPrefBranch2> pbi(do_GetService(NS_PREFSERVICE_CONTRACTID,
> &rv));
> +    if (NS_FAILED(rv))
> +      return rv;
> +    
> +    NS_RELEASE(prefObserver);
>    }
> Why are we just not simply calling NS_RELEASE(prefObserver)? Why get the pref
> service?

The previous version needed to get the pref service there - I forgot with this version to remove it.

New patch coming up later.

Comment 76

[Mark Banner (:standard8)]

Attachment: [patch 6] updated 2 (diff -w)

Update patch base on Davids comments.

Comment 77

[Mark Banner (:standard8)]

Attachment: [patch 6] updated 2 (normal patch)

Comment 78

[[e-mail]]

Comment on attachment 201911 [details] [diff] [review]
[patch 6] updated 2 (diff -w)

>+class DirPrefObserver : public nsIObserver
>+{
>+public:
>+  DirPrefObserver(nsIPrefBranch2 *pbi)
>+  {
>+    pbi->AddObserver(PREF_LDAP_SERVER_TREE_NAME, this, PR_TRUE);
>+  }
Never, ever, call XPCOM methods from constructors. What would happen if pbi decided to AddRef and Release you? Also, PR_TRUE presupposes that your object supports weak reference. To achieve this, you need to base your class on nsSupportsWeakReference, nsIObserver and NS_IMPL_ISUPPORTS2(DirPrefObserver, nsIWeakReferece, nsIObserver). I think I also saw a stray if( without a space so double-check those too ;-)

Comment 79

[Mark Banner (:standard8)]

Attachment: [patch 6] updated 3 (diff -w)

Revised addressing Neil's comments.

Comment 80

[Mark Banner (:standard8)]

Attachment: [patch 6] updated 3 (normal patch) [checked in 09/11/05]

Comment 81

[[e-mail]]

Comment on attachment 202298 [details] [diff] [review]
[patch 6] updated 3 (diff -w)

OK this should be good to go now.

Comment 82

[Mark Banner (:standard8)]

Comment on attachment 202299 [details] [diff] [review]
[patch 6] updated 3 (normal patch) [checked in 09/11/05]

I checked this patch in earlier.
/cvsroot/mozilla/mailnews/addrbook/src/nsDirPrefs.cpp,v  <--  nsDirPrefs.cpp
new revision: 1.100; previous revision: 1.99
done
Checking in mailnews/addrbook/src/nsDirPrefs.h;
/cvsroot/mozilla/mailnews/addrbook/src/nsDirPrefs.h,v  <--  nsDirPrefs.h
new revision: 1.31; previous revision: 1.30

Comment 83

[Mark Banner (:standard8)]

Attachment: [patch 7]

This is the last patch for this bug sorting out a couple of files in the mail directory (not strictly mailnews I know, but whilst we're here...).

Comment 84

[[e-mail]]

Comment on attachment 202529 [details] [diff] [review]
[patch 7]

I can't review this properly.

>+  nsCOMPtr<nsIPrefService> prefService(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
Could use do_QueryInterface(mPrefs) here.

>-  rv = mPrefs->CopyCharPref(pref,getter_Copies(oldPrefPathStr));
>+  rv = mPrefs->GetCharPref(pref,getter_Copies(oldPrefPathStr));
Nit: could insert space before comma

>+      nsCString str(childPrefs[i]);
>+      data->AppendCString(str);
AppendCString copies, so you can use an nsDependentCString here.

Comment 85

[Mark Banner (:standard8)]

Attachment: [patch 7] updated

Updated addressing Neil's nits and requesting r from thunderbird team.

Comment 86

[David :Bienvenu]

Comment on attachment 202666 [details] [diff] [review]
[patch 7] updated

some nits:

don't need extra braces here:

+    if (charEndsWith(childPrefs[i], ".fixed_font") || charEndsWith(childPrefs[i], ".prop_font"))
+    {
+      data->AppendCString(nsDependentCString(childPrefs[i]));
+    }

does it make sense to use nsCAutoString here? Or even better, an nsDependentCString? So that code and the code a few lines later could be

+    if (charEndsWith(childPrefs[i], ".description"))
+      data->AppendCString(nsDependentCString(childPrefs[i]));

Comment 87

[Mark Banner (:standard8)]

Attachment: [patch 7] updated 2 [checked in 18/11/05]

Updated patch per David's nits, requesting sr.

Comment 88

[Mark Banner (:standard8)]

Ok, the last patch in so /mailnews and /mail are nsIPref free :-)

Although I did the last patch & updated the penultimate one - I'm leaving this assigned to Mikael as he did majority of the work - Thanks Mikael :-)