323211 - convert mailnews/import to nsIFile

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[Mark Banner (:standard8)]

To save bug 33451 getting full up with patches, I've created this one to start looking at removing nsFileSpec & co from mailnews/import. Depending on how it goes, I'm mainly going to look at removing it from the address book code first and following it with the email code later.

Comment 1

[Mark Banner (:standard8)]

Attachment: Part 1 (-w)

This patch starts to remove nsFileSpec & co from mailnews/import/text. It also removes a parameter from elsewhere that isn't used.

Comment 2

[Mark Banner (:standard8)]

Attachment: Part 1 (normal patch)

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 208572 [details] [diff] [review]
Part 1 (-w)

There seems to be a prevailing bracing style of inner blocks in these files that look like

if (foo) {
  bar
}

This patch leaves the files with a mix of that style and

if (foo)
{
  bar
}

Matching the prevailing style is good for readability, I think.

>-nsresult nsTextAddress::ImportAddresses( PRBool *pAbort, const PRUnichar *pName, nsIFileSpec *pSrc, nsIAddrDatabase *pDb, nsIImportFieldMap *fieldMap, nsString& errors, PRUint32 *pProgress)
>+nsresult nsTextAddress::ImportAddresses(PRBool *pAbort, const PRUnichar *pName, nsIFile *aSrc, nsIAddrDatabase *pDb, nsIImportFieldMap *fieldMap, nsString& errors, PRUint32 *pProgress)

I'd suggest you leave the name as pSrc.  Even though this style of Hungarian isn't used elsewhere in the tree, I think trying to convert this file gradually from old to new style has the problem of leaving it in mixed style, which is even harder to read, probably indefinitely.

>-    rv = pSrc->Eof( &eof);
>-    if (NS_FAILED( rv)) {
>+  PRUint32 bytesLeft = 0;
>+  rv = inputStream->Available(&bytesLeft);
>+  if (NS_FAILED(rv))

It's not clear to me why we want to track bytes consumed at all.  Why not just read records until one of the reading functions either reaches the end of file or an error is thrown?

>+nsresult nsTextAddress::ReadRecord(nsILineInputStream *aLineStream, char *pLine, PRInt32 bufferSz, char delim, PRInt32 *pLineLen, PRBool *aMore)

For some reason, I find it really difficult to follow the code flow in this function, simple as it is.  Can you add some comments here?  Among other things, it'd be helpful to know exactly how the pLine parameter is intended to work and what the ownership model is.

>+        if (line.Length() >= (PRUint32)(bufferSz - lineLen))

C++ style casts (NS_STATIC_CAST in this particular case) have the advantage of expressing intent more precisely to the compiler, which allows it to catch more errors.

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> >-    rv = pSrc->Eof( &eof);
> >-    if (NS_FAILED( rv)) {
> >+  PRUint32 bytesLeft = 0;
> >+  rv = inputStream->Available(&bytesLeft);
> >+  if (NS_FAILED(rv))
> 
> It's not clear to me why we want to track bytes consumed at all.  Why not just
> read records until one of the reading functions either reaches the end of file
> or an error is thrown?

We track the bytes consumed because of the pProgress pointer - it allows a progress bar to be updated as we progress through the file for UI feedback.

We actually use the "more" argument from the nsILineInputStream read line argument to tell us when we have got to the end of the file.

Comment 5

[Mark Banner (:standard8)]

Attachment: Part 1 v2 (-w) (normal patch checked in)

This patch addresses dmose's comments from the first one, but instead of commenting ReadRecord, I have reworked it so that it is nsCString based rather than char* based - it is now a much simpler function. The rework also allows removal of the IsLineComplete function as I've integrated that into the much simpler ReadRecord.

I've also added a change to the uuid in the idl that I'd previously forgotten.

Comment 6

[Mark Banner (:standard8)]

Attachment: Part 1 v2 (normal patch - checked in)

Comment 7

[David :Bienvenu]

+      if (NS_SUCCEEDED(rv)) {
+        aLine += line;
+        numQuotes += line.CountChar('"');

question about this code: Can quotes be escaped, e.g., \" ? If so, counting quotes like this won't work. Otherwise, the patch looks ok.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #7)
> +      if (NS_SUCCEEDED(rv)) {
> +        aLine += line;
> +        numQuotes += line.CountChar('"');
> 
> question about this code: Can quotes be escaped, e.g., \" ? If so, counting
> quotes like this won't work. Otherwise, the patch looks ok.
> 
Good question and one I'd forgotten. I've just checked and both Outlook and our Address book use double quotes ("") to escape quotes. Therefore the algorithm will actually work as it stands, but I it's probably worth a comment in the code along the lines of:

// Note that quotes are escaped using double quotes, therefore we can still
// use the count of the number of quotes as single escaped quotes won't change
// the odd/even state of the total count.

I'll add that in pre-checkin (assuming reviews are ok etc).

Comment 9

[David :Bienvenu]

Comment on attachment 209994 [details] [diff] [review]
Part 1 v2 (-w) (normal patch checked in)

ok, thx, Mark, r=bienvenu with that comment added.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 209994 [details] [diff] [review]
Part 1 v2 (-w) (normal patch checked in)

Changing sr request from Dan to Scott as Dan's busy at the moment.

Comment 11

[Scott MacGregor]

I'm kind of torn about what to do here. Maybe you guys have thoughts. 

One of the goals for managing the trunk and the 1.8 branch together intially was to make it easy to merge changes from the trunk to the branch while minimizing API changes on the branch. I don't think we want to change all of the mail interfaces on the branch to use nsIFile for 1.8.1. And if we start doing it on the trunk, we're going to have lots of unpleasant merge work ahead of us when we try to merge trunk bug fixes to the branch. 

The firefox team has been getting around this merge issue by wrapping trunk only interface changes in mozilla_1_8_branch ifdefs on the trunk and the branch, thus making it easier to merge patches back and forth without changing the apis on the branch since the same code is in both places. I'd rather us not have to do that for all of the interfaces that use nsIFile. The alternative is to hold off on wide scale nsIFile API changes until we are closer to releasing something from the 1.8.1 branch so we have fewer merge issues as we move bugs to the branch. 

That's what I've been wrestling with and why I haven't reviewed this yet. Thoughts? This change itself is pretty well isolated, but I'm trying to figure out what we want to do in general about this issue for nsIFile conversion.

Comment 12

[David :Bienvenu]

I agree that we're not going to get rid of nsIFile from all of mailnews in 1.8.1, and keeping the trunk and branch in sync saves us a lot of trouble. On the other hand, the import code isn't likely to see a lot of changes on the 1.8.1 branch, so this patch didn't bother me so much for the trunk. 

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #11)
> That's what I've been wrestling with and why I haven't reviewed this yet.
> Thoughts? This change itself is pretty well isolated, but I'm trying to figure
> out what we want to do in general about this issue for nsIFile conversion.
> 

Given that the import code is isolated and receives very few changes normally, I don't see there being too much of a problem with merging to the trunk wrt fixing import.

I do understand that in the heart of mailnews there are some quite complex interfaces etc. What I think also has to be considered is how long will it take to remove nsIFile from mailnews completely and if its only started just before 2.0 is released, would there be enough time before 3.0 to complete it, or would we again be stuck in the same situation? This is obsolete code that we really shouldn't be using.

Considering that this is also now blocking other issues (see bug 33451), then removing nsFileSpec needs to be higher on the 3.0 priority list.

Additionally from a personal perspective - I use these patches as "fillers" between working on big Address Book patches & other fixes. The "fillers" allow me to put effort into tidying up the code base (definitely required) and obviously fill in time whilst waiting for reviews.

In about 2-3 months time, I'm going to a lot less time to work on the Mozilla codebase - what patches I do (if any) will probably focus on Address Book. It may be that I do have a similar amount of time - its unknown at the moment. So delaying this now means that I possibly won't be able to help with it for if we push for it towards 3.0 to fix some of those bugs its blocking.

Comment 14

[Håkan Waara]

I really think mailnews should consider taking this, at the very least on the trunk.  When developing new import code, it doesn't feel good to add lots of new usage of these year-old interfaces.

Comment 15

[Håkan Waara]

Also, does TB 2.0 really want to ship with all of this nsIFileSpec usage (which will probably mean support it all the way to 3.0, no?) ?  

I thought the point of big-number versioning was to take patches such as this.

Comment 16

[Mark Banner (:standard8)]

Ok there is no point in me doing anything if this is just going to be blocked by the fact that its going to cause a little extra work getting patches into the branch. Therefore I'm reassigning to default owner. I'm tempted to close it as wont-ever-get-round-to-fixing based on some comments.

Its a real shame that a program such as Thunderbird which is striving to be as good as Firefox isn't committed to taking opportunities to improve the codebase and remove long-obsoleted and unsupported code especially with such a long trunk cycle between 1.5 and 3.0. Its an even bigger shame that its not committed to producing good quality code which doesn't rely on assertions and other things to work correctly.

Comment 17

[Scott MacGregor]

Comment on attachment 209994 [details] [diff] [review]
Part 1 v2 (-w) (normal patch checked in)

I forgot about this conversation. As I said in my original comments, I had no problem with taking this well isolated change to the import module. I'm still not sure what the right way to handle this is in the coming months without leaving too large of a discrepenancy between trunk and branch APIs to make moving patches to the branch unhappy. Unless folks are willing to do moz_1_8_ifdefs for all of the interface changes like the firefox folks are doing? Are willing to do that?

Comment 18

[Scott MacGregor]

Mark, I'm not sure I understand the anger you are seeing by me asking about how we want to deal with this with the trunk and the branch. I was just asking for input and ideas for how to deal with this. The official policy for all projects including thunderbird is that changes like this would require MOZILLA_1_8_BRANCH ifdefs to preserve API compatibility and support ease of patch merging between the two branches. If you are interested in doing large scale nsIFileSpec (which is awesome), that that's great. I was suggesting we might also have to go down this road and wanted to hear thoughts on it.

"Its an even bigger shame that its not
committed to producing good quality code which doesn't rely on assertions and
other things to work correctly."

I'm not sure I understand what you mean by this, can you elaborate? Are there assertions that are firing in nsFileSpec which lead to behavior we dependon? Thanks. 

Comment 19

[Håkan Waara]

The reason I'm interested in this, and other import cleanup is that I've started work on what might become (if I succeed) a Mail.app importer/migration module.

As I've spent lots of time reading and understanding this code, I'd very much like to help out do similar cleanup work, as I go along.

Is this desirable?

Is it really necessary to preserve API compatibility (for import modules) also between the 1.x and 2.0 versions?

Comment 20

[David :Bienvenu]

Mark, sorry, I've been meaning to respond to your concern about how long it will take to remove nsIFile and whether we can do it in the 3.0 time frame even if we wait to do the core stuff until 2.0 development ramps down. My thought is that since we're planning on supporting maildir for 3.0, we're going to be touching all that code anyway and I was going to take that opportunity to get rid of nsIFileSpec from the mailbox code then.

If our priorities change and we decide to do maildir for 2.0, then the nsIFileSpec removal would happen that much sooner. That is unlikely, however.

There is also work that can be done getting rid of nsIFileSpec internally by calling nsMsgDBFolder::GetFilePath().

I really appreciate all you've done to get rid of nsIFileSpec - without your work, the task would be a lot more daunting.

Comment 21

[Mark Banner (:standard8)]

(In reply to comment #18)
> Mark, I'm not sure I understand the anger you are seeing by me asking about how
> we want to deal with this with the trunk and the branch. 
>
> "Its an even bigger shame that its not
> committed to producing good quality code which doesn't rely on assertions and
> other things to work correctly."
> 
> I'm not sure I understand what you mean by this, can you elaborate? Are there
> assertions that are firing in nsFileSpec which lead to behavior we dependon?
> Thanks. 

Maybe my anger is misplaced, but I do have various issues that I'm not happy with, maybe I'm wrong to bring them up here - I'll respond via email in a few days.

> The official policy for all projects including thunderbird is that changes like this would require
> MOZILLA_1_8_BRANCH ifdefs to preserve API compatibility and support ease of
> patch merging between the two branches.

Ermm, are you sure you're right here? The preservation of API compatibility was for 2.0 to be compatible with 1.5. Trunk is the development ground so that major changes can go into 3.0 (see all the address book reorganisation that has happened). The only reason for the ifdefs was to test stuff out or implement stuff that was only ever going to be on the 1.8 branch.

Comment 22

[Scott MacGregor]

(In reply to comment #21)
> 
> Ermm, are you sure you're right here? 

Hey Mark,

From:
http://wiki.mozilla.org/Global:1.9_Trunk_1.8_Branch_Plan
in particular the 2nd bullet item

All Firefox 2 changes will land on the trunk as well as the 1.8 branch

    * Some Firefox 2 code will need to be #ifdef MOZILLA_1_8_BRANCH to cope with API or bug-compatibility skew between trunk and branch
    * Some Firefox 3 changes will be needed by Gecko 1.9 back end changes (e.g. for XUL box layout standardization) -- these must be #ifndef MOZILLA_1_8_BRANCH
    * The MOZILLA_1_8_BRANCH macro will be defined for C++ and XUL on the branch
    * After Firefox 2 ships, #ifdefs testing this macro will be purged from the trunk
    * We'll use an automatic unifdef program to avoid fat-finger errors 

Also later on in the FAQ on that same page, there is more detailed talk about the merging implications and why the moz1.8 branch ifdefs are appropriate.

That's where I was getting my information from anyway.

Comment 23

[Håkan Waara]

Attachment: Change EscapeFromSpaceLine() away from filespec (checked in)

Simple fix so I can reuse this utility method in my own code. 

Uses the fact that nsIFileSpec exposes its nsIOutputStream in the interface.

Comment 24

[Håkan Waara]

Comment on attachment 220697 [details] [diff] [review]
Change EscapeFromSpaceLine() away from filespec (checked in)

Request r/sr per Standard8's suggestion.

Comment 25

[Håkan Waara]

Mcsmurf helped me test-compile on windows for the win-only importers.

Comment 26

[David :Bienvenu]

Comment on attachment 220697 [details] [diff] [review]
Change EscapeFromSpaceLine() away from filespec (checked in)

this looks fine, except, can you name the parameter outputStream, not anOutStream, to be more consistent with our naming conventions elsewhere? thx!

Comment 27

[Håkan Waara]

Comment on attachment 220697 [details] [diff] [review]
Change EscapeFromSpaceLine() away from filespec (checked in)

Thanks for the review. Checked this in with the param name change.

Comment 28

[David :Bienvenu]

I'm doing the remaining work on this right now.

Comment 29

[Masatoshi Kimura [:emk]]

finxed by bug 33451