reduce duplicated import code this bug could cover a number of things, but for now, just the string bundle code. see C:\builds\bridge\mozilla\mailnews\import\eudora\src\nsEudoraStringBundle.cpp C:\builds\bridge\mozilla\mailnews\import\oexpress\nsOEStringBundle.cpp C:\builds\bridge\mozilla\mailnews\import\outlook\src\nsOutlookStringBundle.cpp C:\builds\bridge\mozilla\mailnews\import\src\nsImportStringBundle.cpp C:\builds\bridge\mozilla\mailnews\import\text\src\nsTextStringBundle.cpp I smell copy-and-paste.

this is minor, future. I'm sure we've got better things to fix. this doesn't hurt us at run time, if the user doesn't import. maybe one day someone will want to teach the import code a lesson (and have the time to fix all the issues), and they'll fix all this bug. future for now.

First patch of several. Here's the general plan for this tidy up: 1) Make the nsImportStringBundle class a bit more generic so that its basically a set of utility functions. 2) Make the various import modules responsible for their own bundles using nsImportStringBundle as opposed to ns*StringBundle that they use at the moment. I expect the ns*StringBundle.h files will probably stay to define the various string ids, though I'm open to debate on that... Feel free to suggest improvements, I won't be surprised if nsImportStringBundle needs to morph a bit more when it comes to part 2, but this step is hopefully an improvement on what is there at the moment.

Comment on attachment 204908 [details] [diff] [review] Part 1 v1 (-w) Neil's busy at the moment, seeing if David has more time ;-) David: please note my above comments as well. Thanks.

This looks fairly reasonable but thread proxies scare me :-(

Comment on attachment 204908 [details] [diff] [review] Part 1 v1 (-w) looks ok - just some nits: + m_pThreadData->stringBundle = m_stringBundle; + NS_IF_ADDREF(m_pThreadData->stringBundle); + you can do the assignment inside the NS_IF_ADDREF... + if (NS_SUCCEEDED(rv)) { + nsImportStringBundle::GetStringByID( i, *pStr, theBundle); are there tabs here? @@ -551,6 +558,9 @@ NS_IMETHODIMP nsImportGenericMail::Begin m_pThreadData->performingMigration = m_performingMigration; NS_IF_ADDREF( m_pDestFolder); + m_pThreadData->stringBundle = m_stringBundle; + NS_IF_ADDREF(m_pThreadData->stringBundle); + again, assignment can be in side NS_IF_ADDREF

Addressed David's review comments. Requesting sr.

Comment on attachment 216926 [details] [diff] [review] Part 1 v2 (checked in) Only one of each nit was reported. >+nsImportFieldMap::nsImportFieldMap(nsIStringBundle *pBundle) Should be aBundle. >- nsIStringBundle *pBundle = nsImportStringBundle::GetStringBundleProxy(); >+ nsCOMPtr<nsIStringBundle> theBundle; Should be pBundle. >- if (pBundle) { Should not need to change.

will attachment 75306 [details] [diff] [review] from bug 57658 affect your patch and is it beyond scope of this bug to incorporate said attachment in your patch?

The framework is now in place so that we can remove the ns*StringBundle.{h,cpp} files. This patch does the text string bundle as that's easiest for me to test ;-)

Comment on attachment 255722 [details] [diff] [review] Part 2 v1 - Remove nsTextStringBundle.h/.cpp This nees some formatting work and I've decided to incorporate some other changes into it as well.

This patch removes the need for nsTextStringBundle by reusing the nsImportStringBundle I reworked earlier in this bug. Note that I've also included various tidy ups to make the whole thing flow better and look better.

Comment on attachment 259608 [details] [diff] [review] Part 2 v2 Remove nsTextStringBundle.h/.cpp (diff -w) >+#include "nsISupportsPrimitives.h" Not used. >+class nsISupportsString; Not used. >+ static void GetStringByID(PRInt32 aStringID, >+ nsIStringBundle *aBundle, >+ nsString &aResult); Nit: indentation. >+ if (!m_notProxyBundle) >+ return NS_ERROR_FAILURE; >+ >+ nsCOMPtr<nsIStringBundle> proxy; >+ nsresult rv = >+ nsImportStringBundle::GetStringBundleProxy(m_notProxyBundle, >+ getter_AddRefs(proxy)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsImportStringBundle::GetStringByID(TEXTIMPORT_ADDRESS_NAME, proxy, str); I'm not sure this is correct, maybe biesi would know. >+ nsCRT::free(pFmt); I think these should probably be NS_Free or nsMemory::Free >+ PRUnichar *pFmt = nsImportStringBundle::GetStringByID(errorNum, pBundle); > PRUnichar *pText = nsTextFormatter::smprintf( pFmt, name.get()); A pity there's no nsImportStringBundle::FormatStringById

(In reply to comment #15) > (From update of attachment 259608 [details] [diff] [review]) > >+ if (!m_notProxyBundle) > >+ return NS_ERROR_FAILURE; > >+ > >+ nsCOMPtr<nsIStringBundle> proxy; > >+ nsresult rv = > >+ nsImportStringBundle::GetStringBundleProxy(m_notProxyBundle, > >+ getter_AddRefs(proxy)); > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ > >+ nsImportStringBundle::GetStringByID(TEXTIMPORT_ADDRESS_NAME, proxy, str); > I'm not sure this is correct, maybe biesi would know. biesi, do you know if this is correct?

Comment on attachment 259608 [details] [diff] [review] Part 2 v2 Remove nsTextStringBundle.h/.cpp (diff -w) OK, so biesi said on irc that it's acceptable because although it's not thread safe, it uses thread safe addref and release. r=me with the nits fixed.

(In reply to comment #15) > >+ PRUnichar *pFmt = nsImportStringBundle::GetStringByID(errorNum, pBundle); > > PRUnichar *pText = nsTextFormatter::smprintf( pFmt, name.get()); > A pity there's no nsImportStringBundle::FormatStringById I'll deal with that in a later patch as there's a few areas where that is duplicated so we can tidy it all up at once.

Addressed Neil's nits, carrying forward his r. Requesting sr.

Comment on attachment 260453 [details] [diff] [review] Part 2 v3 Remove nsTextStringBundle.h/.cpp (diff -w) great, thx, Mark.

Comment on attachment 260454 [details] [diff] [review] Part 2 v3 Remove nsTextStringBundle.h/.cpp (normal, checked in) Part 2 v3 now checked into trunk.

Is there a logical next step for this bug that perhaps someone could take on?

(In reply to comment #23) > Is there a logical next step for this bug that perhaps someone could take on? This should be relatively easy to finish off. Basically the import-type-specific string bundles here: http://mxr.mozilla.org/comm-central/find?string=import.*StringBundle&tree=comm-central&hint= should be removed and use the nsImportStringBundle instead (also shown in that list). The patches already on this bug should give a good idea for how to do this.