Last Comment Bug 729676 - mail import assumes berkeley mailbox stores
: mail import assumes berkeley mailbox stores
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Thunderbird 15.0
Assigned To: David :Bienvenu
:
:
Mentors:
Depends on: 786683
Blocks: 561562 700920
  Show dependency treegraph
 
Reported: 2012-02-22 13:12 PST by David :Bienvenu
Modified: 2012-08-29 08:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wip (36.15 KB, patch)
2012-02-22 14:26 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
patch that builds on windows (56.13 KB, patch)
2012-02-22 19:49 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
patch that runs on windows (56.65 KB, patch)
2012-02-23 09:47 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
get building on mac (43.80 KB, patch)
2012-02-23 11:44 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
fix tabs (43.77 KB, patch)
2012-02-23 12:28 PST, David :Bienvenu
hiikezoe: review-
Details | Diff | Splinter Review
fix addressing comments (46.87 KB, patch)
2012-02-23 17:16 PST, David :Bienvenu
hiikezoe: review+
Details | Diff | Splinter Review
de-bitrotted patch (45.21 KB, patch)
2012-05-03 13:50 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
fix typo in de-bitrotted patch (45.21 KB, patch)
2012-05-03 15:51 PDT, David :Bienvenu
standard8: superreview+
Details | Diff | Splinter Review

Description David :Bienvenu 2012-02-22 13:12:36 PST
The import code needs to be adapted to use pluggable stores.
Comment 1 David :Bienvenu 2012-02-22 14:26:16 PST
Created attachment 599774 [details] [diff] [review]
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 2012-02-22 19:49:00 PST
Created attachment 599849 [details] [diff] [review]
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 2012-02-23 09:47:36 PST
Created attachment 600064 [details] [diff] [review]
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 2012-02-23 11:44:54 PST
Created attachment 600117 [details] [diff] [review]
get building on mac

this builds on the mac
Comment 5 David :Bienvenu 2012-02-23 12:28:42 PST
Created attachment 600138 [details] [diff] [review]
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) 2012-02-23 15:46:37 PST
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) 2012-02-23 15:58:39 PST
(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 2012-02-23 17:16:34 PST
Created attachment 600255 [details] [diff] [review]
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) 2012-02-23 18:45:00 PST
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 2012-02-24 10:53:32 PST
(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) 2012-02-24 16:50:47 PST
(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) 2012-04-23 06:15:22 PDT
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 2012-05-03 10:20:50 PDT
yeah, bug 730147 essentially bit-rotted my whole patch, so I'm going to have to do it again.
Comment 14 David :Bienvenu 2012-05-03 13:50:45 PDT
Created attachment 620839 [details] [diff] [review]
de-bitrotted patch

haven't been able to build this yet, due to the vc 10 switch over. Trying now.
Comment 15 David :Bienvenu 2012-05-03 15:51:35 PDT
Created attachment 620873 [details] [diff] [review]
fix typo in de-bitrotted patch

this one should build...
Comment 16 David :Bienvenu 2012-05-09 09:55:13 PDT
fixed on trunk -http://hg.mozilla.org/comm-central/rev/c752678985e4

Note You need to log in before you can comment on or make changes to this bug.