Open Bug 131040 Opened 22 years ago Updated 2 years ago

Reduce duplicated import code (string bundle utility functions)

Categories

(MailNews Core :: Import, defect)

defect

Tracking

(Not tracked)

People

(Reporter: sspitzer, Unassigned)

References

Details

Attachments

(3 files, 6 obsolete files)

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.
Severity: normal → minor
Target Milestone: --- → Future
Product: MailNews → Core
OS: Windows 2000 → All
Hardware: PC → All
Attached patch Part 1 v1 (-w) (obsolete) — Splinter Review
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.
Assignee: cavin → bugzilla
Status: NEW → ASSIGNED
Attachment #204908 - Flags: review?(neil.parkwaycc.co.uk)
Component: MailNews: Backend → MailNews: Import
Target Milestone: Future → ---
Attached patch Part 1 v1 (non -w) (obsolete) — Splinter Review
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.
Attachment #204908 - Flags: review?(neil.parkwaycc.co.uk) → review?(bienvenu)
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
Attachment #204908 - Flags: review?(bienvenu) → review+
Addressed David's review comments. Requesting sr.
Attachment #204908 - Attachment is obsolete: true
Attachment #216926 - Flags: superreview?(neil)
Attachment #216926 - Flags: review+
Attachment #204909 - Attachment is obsolete: true
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.
Attachment #216926 - Flags: superreview?(neil) → superreview+
Attachment #216926 - Attachment description: Part 1 v2 → Part 1 v2 (checked in)
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?
Depends on: 368103
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.
Attachment #255722 - Attachment is obsolete: true
Depends on: 371542
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.
Attachment #259608 - Flags: review?(neil)
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.
Attachment #259608 - Flags: review?(neil) → review+
(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.
Attachment #260453 - Flags: superreview?(bienvenu)
Attachment #260453 - Flags: review+
Attachment #259608 - Attachment is obsolete: true
Attachment #259609 - Attachment is obsolete: true
Comment on attachment 260453 [details] [diff] [review]
Part 2 v3 Remove nsTextStringBundle.h/.cpp (diff -w)

great, thx, Mark.
Attachment #260453 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 260454 [details] [diff] [review]
Part 2 v3 Remove nsTextStringBundle.h/.cpp (normal, checked in)

Part 2 v3 now checked into trunk.
Attachment #260454 - Attachment description: Part 2 v3 Remove nsTextStringBundle.h/.cpp (normal) → Part 2 v3 Remove nsTextStringBundle.h/.cpp (normal, checked in)
Attachment #260453 - Attachment is obsolete: true
Priority: -- → P4
QA Contact: esther → import
Product: Core → MailNews Core
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.
Assignee: bugzilla → nobody
Status: ASSIGNED → NEW
Priority: P4 → --
Summary: reduce duplicated import code → Reduce duplicated import code (string bundle utility functions)
Severity: minor → S4
You need to log in before you can comment on or make changes to this bug.