334354 - Remove nsIImportService

Status: NEW

(MailNews Core :: Import, task)

Description

[Håkan Waara]

Currently, nsIImportService (http://lxr.mozilla.org/mozilla/source/mailnews/import/public/nsIImportService.idl) does the follow things:

1. Converts native system strings to/from unicode
2. Has methods for creating new instances of nsIImportFieldMap, nsIImportMailboxDescriptor, etc.
3. Does generic module bookkeeping such as GetModule(), GetModuleCount() etc.

#1 simply doesn't belong in the import service.

#2: We should make those of these components that are actually worth keeping independent and accessible via do_CreateInstance.  For example, every import module passes around an import service as parameter in a lot of internal functions, just to create a nsIImportMailboxDescriptor at the end.

#3: I'm sure we can solve this in a more XPCOM-friendly way.

Thoughts?

Comment 1

[Håkan Waara]

It also seems like we should be able to remove nsIImportModule without too much effort, since the primary use of that is by nsIImportService.

Most of nsIImportModule's functions also remain unused.

Comment 2

[David :Bienvenu]

I think if you actually started to do this work, you'd run into problems, and the solutions might look a lot like what we have now :-) For example, we have one string bundle that all the import modules can share, which is why the factory methods end up going through the import service, so they can pass the string bundle into the constructors. If you use normal xpcom create instance calls directly, you'll need to initialize the string bundle pointers separately, and probably end up duplicating code in a fair number of cases. The GetModuleCount code is a wrapper around categories (I was going to suggest using categories, but nsIIMportService already does). That code could be rewritten in js and used in http://lxr.mozilla.org/seamonkey/source/mailnews/import/resources/content/importDialog.js#330

but I don't think you can get rid of the import service completely. It also handles setting up PR_Logging, which is shared across import modules.

I think we have a lot higher bang for the buck things we can do...

Comment 3

[Håkan Waara]

(In reply to comment #2)
> I think if you actually started to do this work, you'd run into problems, and
> the solutions might look a lot like what we have now :-) For example, we have
> one string bundle that all the import modules can share, which is why the
> factory methods end up going through the import service, so they can pass the
> string bundle into the constructors. If you use normal xpcom create instance
> calls directly, you'll need to initialize the string bundle pointers
> separately, and probably end up duplicating code in a fair number of cases. 

In nsImportService.cpp, the only one of these objects that is passed the global string bundle is nsIImportFieldMap.  The other ones are passed nsnull...

If you want, for now we could just let those who want the global string bundle get it via the service manually (note though that most import modules use their own stringbundle).

> The GetModuleCount code is a wrapper around categories (I was going to 
> suggest using categories, but nsIIMportService already does). That code 
> could be rewritten in js and used in
> http://lxr.mozilla.org/seamonkey/source/mailnews/import/resources/content/importDialog.js#330
> 
> but I don't think you can get rid of the import service completely. It also
> handles setting up PR_Logging, which is shared across import modules.
> 
> I think we have a lot higher bang for the buck things we can do...
> 

It seems that the import service rather *reimplements* nsICategoryManager ;-) It does it all manually.  What are the categories for, I've not understood this 100%, I'm sure it makes sense but would like to understand.

It might be that we cannot remove the import service totally, we surely it could be slimmed down a lot.  

A big reason I'm interested is of course to make the import code more maintainable, which indirectly means much more bug-fixing (the "higher bang for the buck" I guess you're referring to).  

IMHO, it's all about making it more pleasant to work in this area. :-)

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #0)
> 1. Converts native system strings to/from unicode

Hmm, just found this bug, looks like I'm fixing this in bug 369913

Comment 5

[buovjaga]

It's still there: https://hg.mozilla.org/comm-central/file/tip/mailnews/import/public/