Closed
Bug 226005
Opened 21 years ago
Closed 19 years ago
MailNews should use the newer nsIPrefService APIs instead of nsIPref
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mikael, Assigned: mikael)
References
Details
Attachments
(8 files, 24 obsolete files)
31.38 KB,
patch
|
Bienvenu
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
36.87 KB,
patch
|
Bienvenu
:
review+
|
Details | Diff | Splinter Review |
142.47 KB,
patch
|
neil
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
34.17 KB,
patch
|
neil
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
34.18 KB,
patch
|
Details | Diff | Splinter Review | |
35.42 KB,
patch
|
standard8
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
44.50 KB,
patch
|
Details | Diff | Splinter Review | |
12.92 KB,
patch
|
standard8
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
addressbook should not use the deprecated nsIPref
Assignee | ||
Comment 1•21 years ago
|
||
patch coming in a few days
Status: NEW → ASSIGNED
Component: Address Book → Mail Back End
Summary: Address Book should not use nsIPref → Messenger should not use nsIPref
Assignee | ||
Updated•21 years ago
|
Summary: Messenger should not use nsIPref → MailNews should not use nsIPref
Assignee | ||
Comment 2•21 years ago
|
||
Assignee | ||
Comment 3•21 years ago
|
||
Comment on attachment 137803 [details]
[part 1]Simple changes
oops wrong file
Attachment #137803 -
Attachment is obsolete: true
Attachment #137803 -
Attachment is patch: false
Assignee | ||
Comment 4•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #137805 -
Flags: superreview?(bienvenu)
Attachment #137805 -
Flags: review?(mscott)
Comment 5•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #137805 -
Flags: review?(mscott) → review?(sspitzer)
Assignee | ||
Updated•21 years ago
|
Attachment #137805 -
Attachment is obsolete: true
Attachment #137805 -
Flags: superreview?(bienvenu)
Attachment #137805 -
Flags: review?(sspitzer)
Assignee | ||
Comment 6•21 years ago
|
||
new patch, fixed from issues in comment #5
Assignee | ||
Updated•21 years ago
|
Attachment #137808 -
Flags: superreview?(sspitzer)
Attachment #137808 -
Flags: review?(bienvenu)
Updated•21 years ago
|
Attachment #137808 -
Flags: review?(bienvenu) → review+
Assignee | ||
Updated•21 years ago
|
Attachment #137808 -
Flags: superreview?(sspitzer) → superreview?(peterv)
Assignee | ||
Updated•21 years ago
|
Attachment #137808 -
Flags: superreview?(peterv) → superreview?(dbaron)
Attachment #137808 -
Flags: superreview?(dbaron) → superreview?(mscott)
Updated•21 years ago
|
Attachment #137808 -
Flags: superreview?(mscott) → superreview+
Assignee | ||
Comment 7•21 years ago
|
||
beeing new to mozilla code I try not to make to large patches. bienvenu - could you help with checkin of patch 1?
Assignee | ||
Updated•21 years ago
|
Attachment #138371 -
Flags: superreview?(mscott)
Attachment #138371 -
Flags: review?(bienvenu)
Comment 8•21 years ago
|
||
sure, I'll check in the first patch.
Comment 9•21 years ago
|
||
patch 1 checked in.
Assignee | ||
Updated•21 years ago
|
Attachment #137808 -
Attachment description: [patch 1] new version → [patch 1] new version (checked in 20040104)
Comment 10•21 years ago
|
||
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 :-(
Assignee | ||
Comment 11•21 years ago
|
||
is DeleteBranch what you're missing? if so - New patch coming up...
Comment 12•21 years ago
|
||
Actually no... I'll explain in a sec...
Assignee | ||
Comment 13•21 years ago
|
||
the usage of getPrefString().. Maybe I should make a member var to point at branch "mail.smtpserver." + mKey instead?
Comment 14•21 years ago
|
||
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.
Assignee | ||
Updated•21 years ago
|
Attachment #138371 -
Attachment description: [patch 2] more changes → [patch 2] more changes - working on a new
Attachment #138371 -
Attachment is obsolete: true
Attachment #138371 -
Flags: superreview?(mscott)
Attachment #138371 -
Flags: review?(bienvenu)
Assignee | ||
Comment 15•21 years ago
|
||
Neil, is this better?
Assignee | ||
Comment 16•21 years ago
|
||
This patch also removes some unneeded calls to get PrefBranch or PrefBranchInternal from PrefService
Assignee | ||
Updated•21 years ago
|
Attachment #138632 -
Flags: superreview?(mscott)
Attachment #138632 -
Flags: review?(bienvenu)
Comment 17•21 years ago
|
||
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.
Assignee | ||
Comment 18•21 years ago
|
||
Attachment #138617 -
Attachment is obsolete: true
Comment 19•21 years ago
|
||
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 :-(
Assignee | ||
Comment 20•21 years ago
|
||
Attachment #138641 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #138653 -
Flags: superreview?(mscott)
Attachment #138653 -
Flags: review?(bienvenu)
Comment 21•21 years ago
|
||
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
Attachment #138653 -
Flags: review?(bienvenu) → review+
Comment 22•21 years ago
|
||
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)
Attachment #138632 -
Flags: review?(bienvenu) → review+
Comment 23•21 years ago
|
||
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•21 years ago
|
||
> 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.
Assignee | ||
Comment 25•21 years ago
|
||
Attachment #138653 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #138653 -
Flags: superreview?(mscott)
Assignee | ||
Updated•21 years ago
|
Attachment #138710 -
Flags: superreview?(mscott)
Assignee | ||
Comment 26•21 years ago
|
||
Attachment #138632 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #138711 -
Flags: superreview?(mscott)
Assignee | ||
Updated•21 years ago
|
Attachment #138632 -
Flags: superreview?(mscott)
Assignee | ||
Updated•21 years ago
|
Attachment #138710 -
Flags: superreview?(mscott) → superreview?(bz-vacation)
Assignee | ||
Updated•21 years ago
|
Attachment #138711 -
Flags: superreview?(mscott) → superreview?(bz-vacation)
Comment 27•21 years ago
|
||
I'm not likely to be able to get to this before 1.7a freezes...
Assignee | ||
Updated•21 years ago
|
Attachment #138710 -
Flags: superreview?(bz-vacation) → superreview?(dbaron)
Assignee | ||
Updated•21 years ago
|
Attachment #138711 -
Flags: superreview?(bz-vacation) → superreview?(dbaron)
Assignee | ||
Updated•21 years ago
|
Attachment #138710 -
Flags: superreview?(dbaron) → superreview?(alecf)
Assignee | ||
Updated•21 years ago
|
Attachment #138711 -
Flags: superreview?(dbaron) → superreview?(darin)
Assignee | ||
Comment 28•21 years ago
|
||
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
Attachment #138710 -
Attachment is obsolete: true
Attachment #138710 -
Flags: superreview?(alecf)
Comment 29•21 years ago
|
||
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
Attachment #138711 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 30•21 years ago
|
||
Assignee | ||
Updated•21 years ago
|
Attachment #141240 -
Flags: superreview?(mscott)
Attachment #141240 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 31•21 years ago
|
||
Attachment #138711 -
Attachment is obsolete: true
Comment 32•21 years ago
|
||
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.
Attachment #141240 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Comment 33•21 years ago
|
||
> mEmailPrefix.Trunate(0);
err, don't use Truncate(0), use Truncate().
Updated•20 years ago
|
Summary: MailNews should not use nsIPref → MailNews should use the newer nsIPrefService APIs instead of nsIPref
Comment 34•20 years ago
|
||
Mikael, any news on this? Shouldn't your last patch ([patch 3] adressing darins comments) be reviewed?
Assignee | ||
Updated•20 years ago
|
Attachment #141240 -
Flags: superreview?(mscott)
Assignee | ||
Comment 35•20 years ago
|
||
Attachment #141419 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #154148 -
Flags: review?(bienvenu)
Comment 36•20 years ago
|
||
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...
Attachment #154148 -
Flags: review?(bienvenu) → review+
Assignee | ||
Comment 37•20 years ago
|
||
(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•20 years ago
|
||
+ prefName = NS_LITERAL_CSTRING("ldap_2.servers.pab.description"); this could use .AssignLiteral
Comment 39•20 years ago
|
||
Comment on attachment 154148 [details] [diff] [review] [patch 3] adressing darins comments, re-diffed (checked in) patch 3 checked in
Attachment #154148 -
Attachment description: [patch 3] adressing darins comments, re-diffed → [patch 3] adressing darins comments, re-diffed (checked in)
Comment 40•20 years ago
|
||
(with the change from comment 38 made)
Assignee | ||
Comment 41•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #141240 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #156909 -
Flags: superreview?(bienvenu)
Attachment #156909 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Updated•20 years ago
|
Attachment #156909 -
Flags: review?(neil.parkwaycc.co.uk) → review?(mscott)
Assignee | ||
Updated•20 years ago
|
Attachment #156909 -
Flags: superreview?(bienvenu)
Attachment #156909 -
Flags: review?(mscott)
Assignee | ||
Comment 42•20 years ago
|
||
Attachment #156909 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #163241 -
Flags: superreview?(bienvenu)
Attachment #163241 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 43•20 years ago
|
||
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
Attachment #163241 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Assignee | ||
Updated•20 years ago
|
Attachment #163241 -
Flags: superreview?(bienvenu)
Comment 44•20 years ago
|
||
I'm told that that part of the makefile doesn't actually require tabs but you still need them for consistency.
Updated•20 years ago
|
Product: MailNews → Core
Comment 45•20 years ago
|
||
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.
Assignee | ||
Comment 46•20 years ago
|
||
Attachment #163241 -
Attachment is obsolete: true
Attachment #169382 -
Flags: superreview?(roc)
Attachment #169382 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 47•20 years ago
|
||
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.
Assignee | ||
Comment 48•20 years ago
|
||
(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
Assignee | ||
Updated•20 years ago
|
Attachment #169382 -
Attachment is obsolete: true
Attachment #170023 -
Flags: superreview?(darin)
Attachment #170023 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 49•20 years ago
|
||
(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•20 years ago
|
||
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.
Attachment #170023 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Updated•20 years ago
|
Attachment #169382 -
Flags: superreview?(roc)
Attachment #169382 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 51•20 years ago
|
||
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
Attachment #170023 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 52•20 years ago
|
||
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
Attachment #170023 -
Attachment is obsolete: true
Attachment #170756 -
Flags: superreview?(darin)
Attachment #170756 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 53•20 years ago
|
||
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 ;-)
Attachment #170756 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Updated•20 years ago
|
Attachment #170756 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 54•20 years ago
|
||
Comment on attachment 170756 [details] [diff] [review] [patch 4] updated (2) check ed in Checked in 050115 by Daniel Brooks, db48x@yahoo.com
Attachment #170756 -
Attachment description: [patch 4] updated (2) → [patch 4] updated (2) check ed in
Comment 55•20 years ago
|
||
"View > Message Body As >" switching seems broken in current trunk, I suspect it regressed with this patch somehow...
Assignee | ||
Comment 56•20 years ago
|
||
(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
Assignee | ||
Comment 57•20 years ago
|
||
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.
Assignee | ||
Comment 58•20 years ago
|
||
Comment on attachment 171359 [details] [diff] [review] regression fix I filed bug 278596 for the regression
Attachment #171359 -
Attachment is obsolete: true
Assignee | ||
Comment 59•20 years ago
|
||
Also includes a few lines string-cleanup
Attachment #172107 -
Flags: superreview?(darin)
Attachment #172107 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 60•20 years ago
|
||
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
Attachment #172107 -
Flags: superreview?(darin) → superreview+
Updated•20 years ago
|
Attachment #172107 -
Flags: review?(neil.parkwaycc.co.uk) → review+
Assignee | ||
Comment 61•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #172202 -
Attachment description: [patch 5] updated with review comment, for check-in → [patch 5] updated with review comment, for check-in [checked in 2005-01-26]
Assignee | ||
Comment 62•19 years ago
|
||
Last files in /mailnews. This patch removes nsIPref from nsDirPrefs. I have also filed bug 287832 for some clean up of nsDirPrefs
Attachment #179306 -
Flags: superreview?(darin)
Attachment #179306 -
Flags: review?(neil.parkwaycc.co.uk)
Assignee | ||
Comment 63•19 years ago
|
||
Comment 64•19 years ago
|
||
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•19 years ago
|
||
(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•19 years ago
|
||
wow.. isn't that slick? thanks robert!
Comment 67•19 years ago
|
||
(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•19 years ago
|
||
if only that worked more reliable...
Comment 69•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #179306 -
Flags: review?(neil.parkwaycc.co.uk) → review-
Updated•19 years ago
|
Attachment #179306 -
Flags: superreview?(darin) → superreview-
Comment 71•19 years ago
|
||
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.
Attachment #179306 -
Attachment is obsolete: true
Attachment #179307 -
Attachment is obsolete: true
Attachment #201162 -
Flags: review?(darin)
Comment 72•19 years ago
|
||
Comment 73•19 years ago
|
||
Comment on attachment 201162 [details] [diff] [review] [patch 6] updated (diff -w) redirecting review request to mailnews peer.
Attachment #201162 -
Flags: review?(darin) → review?(bienvenu)
Comment 74•19 years ago
|
||
+ 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•19 years ago
|
||
(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•19 years ago
|
||
Update patch base on Davids comments.
Attachment #201162 -
Attachment is obsolete: true
Attachment #201163 -
Attachment is obsolete: true
Attachment #201911 -
Flags: review?(bienvenu)
Attachment #201162 -
Flags: review?(bienvenu)
Comment 77•19 years ago
|
||
Updated•19 years ago
|
Attachment #201911 -
Flags: review?(bienvenu) → review+
Updated•19 years ago
|
Attachment #201911 -
Flags: superreview?(neil.parkwaycc.co.uk)
Comment 78•19 years ago
|
||
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 ;-)
Attachment #201911 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
Comment 79•19 years ago
|
||
Revised addressing Neil's comments.
Attachment #201911 -
Attachment is obsolete: true
Attachment #201912 -
Attachment is obsolete: true
Attachment #202298 -
Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #202298 -
Flags: review+
Comment 80•19 years ago
|
||
Comment 81•19 years ago
|
||
Comment on attachment 202298 [details] [diff] [review] [patch 6] updated 3 (diff -w) OK this should be good to go now.
Attachment #202298 -
Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Comment 82•19 years ago
|
||
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
Attachment #202299 -
Attachment description: [patch 6] updated 3 (normal patch) → [patch 6] updated 3 (normal patch) [checked in 09/11/05]
Comment 83•19 years ago
|
||
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...).
Attachment #202529 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 84•19 years ago
|
||
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.
Attachment #202529 -
Flags: review?(neil.parkwaycc.co.uk)
Comment 85•19 years ago
|
||
Updated addressing Neil's nits and requesting r from thunderbird team.
Attachment #202529 -
Attachment is obsolete: true
Attachment #202666 -
Flags: review?(bienvenu)
Comment 86•19 years ago
|
||
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]));
Attachment #202666 -
Flags: review?(bienvenu) → review+
Comment 87•19 years ago
|
||
Updated patch per David's nits, requesting sr.
Attachment #202666 -
Attachment is obsolete: true
Attachment #202695 -
Flags: superreview?(mscott)
Attachment #202695 -
Flags: review+
Updated•19 years ago
|
Attachment #202695 -
Flags: superreview?(mscott) → superreview+
Updated•19 years ago
|
Attachment #202695 -
Attachment description: [patch 7] updated 2 → [patch 7] updated 2 [checked in 18/11/05]
Comment 88•19 years ago
|
||
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 :-)
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•