Open
Bug 131040
Opened 23 years ago
Updated 2 years ago
Reduce duplicated import code (string bundle utility functions)
Categories
(MailNews Core :: Import, defect)
MailNews Core
Import
Tracking
(Not tracked)
NEW
People
(Reporter: sspitzer, Unassigned)
References
Details
Attachments
(3 files, 6 obsolete files)
24.96 KB,
patch
|
standard8
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
26.58 KB,
patch
|
Details | Diff | Splinter Review | |
30.34 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
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
Updated•20 years ago
|
Product: MailNews → Core
Updated•19 years ago
|
OS: Windows 2000 → All
Hardware: PC → All
Comment 2•19 years ago
|
||
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)
Updated•19 years ago
|
Component: MailNews: Backend → MailNews: Import
Target Milestone: Future → ---
Comment 3•19 years ago
|
||
Comment 4•19 years ago
|
||
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)
Comment 5•19 years ago
|
||
This looks fairly reasonable but thread proxies scare me :-(
Comment 6•19 years ago
|
||
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+
Comment 7•19 years ago
|
||
Addressed David's review comments. Requesting sr.
Attachment #204908 -
Attachment is obsolete: true
Attachment #216926 -
Flags: superreview?(neil)
Attachment #216926 -
Flags: review+
Comment 8•19 years ago
|
||
Attachment #204909 -
Attachment is obsolete: true
Comment 9•19 years ago
|
||
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+
Updated•19 years ago
|
Attachment #216926 -
Attachment description: Part 1 v2 → Part 1 v2 (checked in)
Comment 10•18 years ago
|
||
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?
Comment 11•18 years ago
|
||
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 12•18 years ago
|
||
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
Comment 13•18 years ago
|
||
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 14•18 years ago
|
||
Comment 15•18 years ago
|
||
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
Comment 16•18 years ago
|
||
(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 17•18 years ago
|
||
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+
Comment 18•18 years ago
|
||
(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.
Comment 19•18 years ago
|
||
Addressed Neil's nits, carrying forward his r. Requesting sr.
Attachment #260453 -
Flags: superreview?(bienvenu)
Attachment #260453 -
Flags: review+
Comment 20•18 years ago
|
||
Attachment #259608 -
Attachment is obsolete: true
Attachment #259609 -
Attachment is obsolete: true
Comment 21•18 years ago
|
||
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 22•18 years ago
|
||
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)
Updated•18 years ago
|
Attachment #260453 -
Attachment is obsolete: true
Updated•16 years ago
|
Priority: -- → P4
Updated•16 years ago
|
QA Contact: esther → import
Assignee | ||
Updated•16 years ago
|
Product: Core → MailNews Core
Comment 23•16 years ago
|
||
Is there a logical next step for this bug that perhaps someone could take on?
Comment 24•15 years ago
|
||
(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)
Updated•2 years ago
|
Severity: minor → S4
You need to log in
before you can comment on or make changes to this bug.
Description
•