369913 - Remove Duplicated code in nsIImportService and nsMsgI18N.h/cpp

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

The import service defines SystemStringToUnicode and SystemStringFromUnicode:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/import/src/nsImportService.cpp&rev=1.53&mark=141-205,207-269#140

The nsMsgI18N.h/cpp implementations of these are very similar:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/util/nsMsgI18N.cpp&rev=1.96&mark=71-131,133-197#70

and there's a couple of helper functions that'll add the missing parts.

If we change the code that uses the import service to use the nsMsgI18N code, then we reduce duplication, and stop mailnews/addrbook/src and mailnews/base/search/src needing to require mailnews/import (not anything major, but still nice tidy up ;-) )

Comment 1

[Mark Banner (:standard8)]

Attachment: Removes duplication of SystemStringFromUnicode v1 (checked in)

This removes the "duplication" of SystemStringFromUnicode and uses the msg base util implementation.

Note that SystemStringFromUnicode uses kPlatformCharsetSel_FileName for getting the file name which is why I've used nsMsgI18NFileSystemCharset() for getting the system charset to supply to the msg base util implementation.

Comment 2

[David :Bienvenu]

Comment on attachment 254671 [details] [diff] [review]
Removes duplication of SystemStringFromUnicode v1 (checked in)

thx, Mark!

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 254671 [details] [diff] [review]
Removes duplication of SystemStringFromUnicode v1 (checked in)

I checked this patch in with an additional uuid update in nsIImportService that we'd both missed.

Comment 4

[Mark Banner (:standard8)]

Attachment: Part 2 - use the nsMsgI18n versions in mailnews/base/search (checked in)

Part 2 swaps the use of the functions in mailnews/base/search from the import versions to the nsMsgI18n versions. The added benefit is we remove the mailnews/import dependency that mailnews/base/search has.

Comment 5

[David :Bienvenu]

Comment on attachment 257142 [details] [diff] [review]
Part 2 - use the nsMsgI18n versions in mailnews/base/search (checked in)

thx, Mark

Comment 6

[Mark Banner (:standard8)]

Attachment: Final Part - rest of duplicated code in mailnews/import (diff -w)

David, non -w patch coming up in a mo. Could you test this on windows for me please?

Comment 7

[Mark Banner (:standard8)]

Attachment: Final Part - rest of duplicated code in mailnews/import (full patch)

Comment 8

[David :Bienvenu]

Attachment: fix nsOESettings.cpp

Mark, I had to wrap a few char * params with nsDependentCString to get nsOESettings.cpp to compile

If you generate a new patch with those changes, I can review that...

Comment 9

[David :Bienvenu]

Comment on attachment 258012 [details] [diff] [review]
Final Part - rest of duplicated code in mailnews/import (diff -w)

nsOutlookSettings.cpp didn't have the corresponding changes - I'll try to apply the same pattern as your other changes to that file.

Comment 10

[David :Bienvenu]

Attachment: patch that compiles for me.

Mark, I had to make a few more changes - this patch compiles for me. Do you want to make your own diff from this, or just have me land this (after I review it)?

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> Created an attachment (id=258055) [details]
> patch that compiles for me.
> 
> Mark, I had to make a few more changes - this patch compiles for me. Do you
> want to make your own diff from this, or just have me land this (after I review
> it)?
> 

David, I've had a look at your changes and I'm happy with it as it is, so feel free to review & land. Thanks.

Comment 12

[David :Bienvenu]

fix landed on trunk, Thx, Mark!

Comment 13

[David :Bienvenu]

re-opening to fix assignment.

Comment 14

[Mark Banner (:standard8)]

Thanks David.