729676 - mail import assumes berkeley mailbox stores

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[David :Bienvenu]

The import code needs to be adapted to use pluggable stores.

Comment 1

[David :Bienvenu]

Attachment: wip

this does Eudora import using pluggable stores, just to give you an idea (it only compiles; haven't tried linking or running yet)

Comment 2

[David :Bienvenu]

Attachment: patch that builds on windows

this patch builds on windows, but I haven't tried it on mac yet, or tried running it yet.

Comment 3

[David :Bienvenu]

Attachment: patch that runs on windows

this works with Eudora and Outlook on Windows - I don't have OE installed, and might need some help testing that.

I need to go fix this so it builds on the mac first, though.

Comment 4

[David :Bienvenu]

Attachment: get building on mac

this builds on the mac

Comment 5

[David :Bienvenu]

Attachment: fix tabs

Hiro, do you think you could review this? You know the import code as well as anyone, and I think you're in a position to test things like Outlook Express. I'll ask Standard8 for sr since I'm changing an interface method, and he can catch any style issues.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 600138 [details] [diff] [review]
fix tabs

Review of attachment 600138 [details] [diff] [review]:
-----------------------------------------------------------------

Basically it looks good to me and works well with the patch for Windows Live Mail importer too.

Actually there a lots of space after the open bracket but I do not care about it here. I will open a new bug for those style fixes in import codes.

I am just concerned about inconsistency of the pattern of GetNewMsgOutputStream .. FinishNewMessage .. DiscardNewMessage.
Some modules are

  GetNewMsgOutputStream();
  ..
  if (NS_SUCCEEDED(rv))
    FinishNewMessage();
  else
    DiscardNewMessage();

but some are

  if (NS_FAILED(rv)) {
    DiscardNewMessage();
    break;
  }
  FinishNewMessage();

Those should be consistent. I'd prefer the pattern in nsOutlookMail.cpp:

  GetNewMsgOutputStream();
  ..
  if (NS_SUCCEEDED(rv)) {
    FinishNewMessage();
  } else {
    output some log;
    DiscardNewMessage();
  }

It's not related to this issue, nsOE5File::ImportMailbox should be clean up. The function is too long... I will work on it later.

::: mailnews/import/applemail/src/nsAppleMailImport.cpp
@@ +565,5 @@
>      nsCOMPtr<nsIOutputStream> outStream;
> +    nsCOMPtr<nsIMsgPluggableStore> msgStore;
> +    nsCOMPtr<nsIMsgDBHdr> msgHdr;
> +
> +    bool reusable;

I'd prefer msgHdr and reusable are positioned nearby GetNewMsgOutputStream.

@@ +599,4 @@
>        if (!StringEndsWith(leafName, NS_LITERAL_STRING(".emlx")))
>          continue;
>  
> +       rv = msgStore->GetNewMsgOutputStream(aDstFolder, getter_AddRefs(msgHdr), &reusable,

Indentation is slipped here.

@@ +602,5 @@
> +       rv = msgStore->GetNewMsgOutputStream(aDstFolder, getter_AddRefs(msgHdr), &reusable,
> +                                            getter_AddRefs(outStream));
> +      if (NS_FAILED(rv))
> +        break;
> +      

Remove these spaces on this blank line.

::: mailnews/import/eudora/src/nsEudoraImport.cpp
@@ +529,3 @@
>  {
>    NS_PRECONDITION(pSource != nsnull, "null ptr");
> +  NS_PRECONDITION(pDstFolder, "null ptr");

It's better to remove all '!= nsnull'? Or use NS_ENSURE_ARG_POINTER?

::: mailnews/import/eudora/src/nsEudoraMailbox.cpp
@@ +323,2 @@
>  
> +      bool reusable;

msgHdr and reusable should move nearby GetNewMsgOutputStream.

@@ +347,5 @@
>  
>          if (!readBuffer.m_bytesInBuf && (state.offset >= state.size))
>            break;
>        }
> +      outputStream->Close();

Need if (outputStream) statement.

@@ +408,5 @@
> +  nsCOMPtr<nsIOutputStream> outputStream;
> +  nsCOMPtr<nsIMsgPluggableStore> msgStore;
> +  nsCOMPtr<nsIMsgDBHdr> msgHdr;
> +
> +  bool reusable;

These msgHdr and reusable should move too.

@@ +466,2 @@
>    }
>  

Should close outputStream after the while loop.

::: mailnews/import/oexpress/nsOE5File.cpp
@@ +282,5 @@
>  
> +  nsCOMPtr<nsIInputStream> inputStream;
> +  nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), inFile);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsCOMPtr<nsIOutputStream> outputStream;

This outputStream should move before 'for' loop.

@@ +348,5 @@
> +    if (NS_FAILED(rv))
> +    {
> +      IMPORT_LOG1( "Mbx getting outputstream error: 0x%lx\n", rv);
> +      return false;
> +    }

I' prefer break here instead of return for closing outputStream.

::: mailnews/import/oexpress/nsOEImport.cpp
@@ +427,5 @@
>                                                PRUnichar **pSuccessLog,
>                                                bool *fatalError)
>  {
>    NS_PRECONDITION(pSource != nsnull, "null ptr");
> +  NS_PRECONDITION(dstFolder, "null ptr");

Same as I mentioning about ImportEudoraMailImpl::ImportMailbox.

::: mailnews/import/outlook/src/nsOutlookImport.cpp
@@ +430,3 @@
>  {
>    NS_PRECONDITION(pSource != nsnull, "null ptr");
> +  NS_PRECONDITION(dstFolder, "null ptr");

Same as I mentioning about ImportEudoraMailImpl::ImportMailbox.

::: mailnews/import/outlook/src/nsOutlookMail.cpp
@@ +418,3 @@
>        return( NS_ERROR_FAILURE);
>      }
>  

I' prefer break here instead of return for closing outputStream.

And m_mapi.OpenMdbEntry failure case (the case is not in this patch) too.

@@ +462,4 @@
>      }
>    }
>  
> +  outputStream->Close();

Need if (outputStream) statement here.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Comment on attachment 600138 [details] [diff] [review]
> fix tabs
> 
> Review of attachment 600138 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Basically it looks good to me and works well with the patch for Windows Live
> Mail importer too.
> 
> Actually there a lots of space after the open bracket but I do not care
> about it here. I will open a new bug for those style fixes in import codes.

Opened the bug. bug 730147

Comment 8

[David :Bienvenu]

Attachment: fix addressing comments

thx much for the speedy review. This addresses your comments, I think. I decided to go with NS_ENSURE_ARG_POINTER, since it's really a programming error, and shouldn't be expected at runtime.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 600255 [details] [diff] [review]
fix addressing comments

Review of attachment 600255 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. r=me but a nit and inconsistency fix.

There are still inconsistency about DiscardNewMessage case, those should be fixed.
I said that I'd prefer no break in the case (I mean importing continues), but I changed my mind, it is better to notice the failure to user.
Because most of failure cases are possibly I/O errors, I suppose it is disk full. So user can clean up disk, and retry importing.
What do you think about it?

::: mailnews/import/oexpress/nsOEMailbox.cpp
@@ +327,4 @@
>      IMPORT_LOG1( "Mbx seek error: 0x%lx\n", offset);
>      return( false);
>    }
> +   rv = m_mbxFileInputStream->Read( (char *) pChar, cnt, &cntRead);

nit: Indentation broken.

Comment 10

[David :Bienvenu]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> 
> There are still inconsistency about DiscardNewMessage case, those should be
> fixed.
> I said that I'd prefer no break in the case (I mean importing continues),
> but I changed my mind, it is better to notice the failure to user.
> Because most of failure cases are possibly I/O errors, I suppose it is disk
> full. 
I've tried to keep doing what the existing code did in the face of failure. Sometimes the failure might be on the reading/converting end, in which case we should continue importing, in sometimes the failure will be in writing the imported messages, and that failure is unlikely to be recoverable. I think changing how the various importers handle failure is beyond the scope of this bug.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to David :Bienvenu from comment #10)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> > 
> > There are still inconsistency about DiscardNewMessage case, those should be
> > fixed.
> > I said that I'd prefer no break in the case (I mean importing continues),
> > but I changed my mind, it is better to notice the failure to user.
> > Because most of failure cases are possibly I/O errors, I suppose it is disk
> > full. 
> I've tried to keep doing what the existing code did in the face of failure.
> Sometimes the failure might be on the reading/converting end, in which case
> we should continue importing, in sometimes the failure will be in writing
> the imported messages, and that failure is unlikely to be recoverable. I
> think changing how the various importers handle failure is beyond the scope
> of this bug.

OK. I understand.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 600255 [details] [diff] [review]
fix addressing comments

There's some significant bitrotting here (which I'm surprised about), can you take a look and I'll review right away?

Comment 13

[David :Bienvenu]

yeah, bug 730147 essentially bit-rotted my whole patch, so I'm going to have to do it again.

Comment 14

[David :Bienvenu]

Attachment: de-bitrotted patch

haven't been able to build this yet, due to the vc 10 switch over. Trying now.

Comment 15

[David :Bienvenu]

Attachment: fix typo in de-bitrotted patch

this one should build...

Comment 16

[David :Bienvenu]

fixed on trunk -http://hg.mozilla.org/comm-central/rev/c752678985e4