684455 - Import from Becky! Internet Mail

Status: RESOLVED FIXED

(MailNews Core :: Import, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

There are large amount of companies which uses Becky! Internet Mail in Japan.

We need to import data from the mailer.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I have been developing an add-on to import data from Becky! for now. The add-on has been developed in github [1]. Actually the add-on has never been released yet!

I will post some patches split into various part(e.g. adressbooks, mail, etc.).

[1]: https://github.com/mozilla-japan/becky-import-addon

Comment 2

[Ludovic Hirlimann [:Usul]]

If I get a build add-on we can organize some crowdsource testing and files bugs. Could you provide such addon ?

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Yes, You can get it from https://github.com/mozilla-japan/becky-import-addon/wiki

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Import module for Becky! Internet mail

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Test for address book import from Becky!

This test depends on the test module in bug 700920.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Created attachment 573749 [details] [diff] [review] [diff] [details] [review]
> Test for address book import from Becky!
> 
> This test depends on the test module in bug 700920.

Ooops! The test depends on *the modification of import_helper.js* in bug 700920. Sorry for the confusion.

Comment 7

[Wayne Mery (:wsmwk)]

Comment on attachment 573748 [details] [diff] [review]
Import module for Becky! Internet mail

mconley, can you review?

note, this was flagged jp-critical some time ago.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 573748 [details] [diff] [review]
Import module for Becky! Internet mail

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

Hey hiro,

So I finally had time to take a look at this, and the more I look at it, the more I'm pretty sure I'm not qualified to review it. While I have some C++ chops, I'm nowhere near advanced enough in C++ XPCOM to do this review justice.

Standard8, irving or jcranmer might be better selections. Sorry you had to wait so long to hear that from me. :/

-Mike

::: mail/locales/en-US/chrome/messenger/beckyImportMsgs.properties
@@ +1,2 @@
> +# ***** BEGIN LICENSE BLOCK *****
> +# Version: MPL 1.1/GPL 2.0/LGPL 2.1

This should be MPL2.

@@ +59,5 @@
> +## @name BECKYIMPORT_MAILBOX_BADPARAM
> +## @loc None
> +2003=Bad parameter passed to import mailbox.
> +
> +# Error message

Why is this one commented out?

@@ +76,5 @@
> +
> +# Description
> +## @name BECKYIMPORT_ADDRESS_SUCCESS
> +## @loc None
> +# LOCALIZATION NOTE (2008): In the following sentence, do not translate the "%S". Instead,

How come the number does not match the mapping in nsBeckyStringBundle.h? Please go over nsBeckyStringBundle.h again and make sure all mappings and comments match.

::: mailnews/import/Makefile.in
@@ +50,5 @@
>  PARALLEL_DIRS	+= eudora/src applemail/src
>  endif
>  
>  ifeq ($(OS_ARCH),WINNT)
> +PARALLEL_DIRS	+= eudora/src becky/src

I believe this needs to be updated for the moz.build changes to the Mozilla build infrastructure.

::: mailnews/import/becky/src/Makefile.in
@@ +1,2 @@
> +#
> +# ***** BEGIN LICENSE BLOCK *****

MPL2.

@@ +33,5 @@
> +# the provisions above, a recipient may use your version of this file under
> +# the terms of any one of the MPL, the GPL or the LGPL.
> +#
> +# ***** END LICENSE BLOCK *****
> +

A moz.build file will be required as well.

@@ +41,5 @@
> +VPATH		= @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +MODULE		= impBecky

Nit - too much whitespace before the equals signs on lines 45 and 46.

::: mailnews/import/becky/src/nsBeckyAddressBooks.cpp
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/*
> + *
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL2.

@@ +109,5 @@
> +{
> +  return NS_OK;
> +}
> +
> +nsresult 

Nit - trailing whitespace.

@@ +169,5 @@
> +{
> +  nsresult rv;
> +  bool isDirectory = false;
> +  rv = aFile->IsDirectory(&isDirectory);
> +  if (isDirectory)

Check rv

@@ +183,5 @@
> +{
> +  nsresult rv;
> +  bool isDirectory = false;
> +  rv = aDirectory->IsDirectory(&isDirectory);
> +  if (!isDirectory)

Check rv here.

@@ +208,5 @@
> +nsBeckyAddressBooks::CountAddressBookSize(nsIFile *aDirectory)
> +{
> +  nsresult rv;
> +  bool isDirectory = false;
> +  rv = aDirectory->IsDirectory(&isDirectory);

Check rv

@@ +223,5 @@
> +  while (NS_SUCCEEDED(entries->HasMoreElements(&more)) && more) {
> +    rv = entries->GetNext(getter_AddRefs(entry));
> +    NS_ENSURE_SUCCESS(rv, 0);
> +
> +    PRInt64 size;

I believe there is an effort to avoid using NSPR types like this. Is there a reason you can't use int64_t?

@@ +267,5 @@
> +nsBeckyAddressBooks::CollectAddressBooks(nsIFile *aTarget,
> +                                         nsISupportsArray *aCollected)
> +{
> +  bool isDirectory = false;
> +  nsresult rv = aTarget->IsDirectory(&isDirectory);

Check rv here.

@@ +361,5 @@
> +    bool aborted = false;
> +    nsAutoString name;
> +    aSource->GetPreferredName(name);
> +    nsVCardAddress vcard;
> +    rv = vcard.ImportAddresses(&aborted, name.get(), entry, aDestination, error, &mReadBytes);

Check the rv value.

::: mailnews/import/becky/src/nsBeckyAddressBooks.h
@@ +1,4 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/*
> + *
> + * ***** BEGIN LICENSE BLOCK *****

MPL2

::: mailnews/import/becky/src/nsBeckyFilters.cpp
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/*
> + *
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL2.

@@ +174,5 @@
> +  nsDependentCSubstring operatorFlags(aFlags, lastTabPosition + 1);
> +  nsDependentCSubstring negativeFlag(aFlags, 0, aFlags.FindChar('\t') - 1);
> +
> +  switch (negativeFlag.CharAt(0)) {
> +  case 'X':

Please indent these cases by two spaces.

@@ +221,5 @@
> +  nsMsgSearchOpValue searchOperator;
> +  searchOperator = ConvertSearchFlagsToOperator(Substring(aLine, tabPosition + 1));
> +  if (searchOperator < 0)
> +    return NS_ERROR_FAILURE;
> +

Remove the extra newline.

@@ +457,5 @@
> +
> +  nsresult rv = NS_OK;
> +  nsCOMPtr<nsIMsgRuleAction> action;
> +  switch (aLine.CharAt(1)) {
> +  case 'R': // Reply

Please indent these cases.

@@ +554,5 @@
> +  while (NS_SUCCEEDED(rv) && more) {
> +    rv = lineStream->ReadLine(line, &more);
> +
> +    switch (line.CharAt(0)) {
> +    case ':':

Please indent these cases.

::: mailnews/import/becky/src/nsBeckyFilters.h
@@ +1,5 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/*
> + *
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

MPL2. I'll stop mentioning this now, but it applies across the whole patch.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 573749 [details] [diff] [review]
Test for address book import from Becky!

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

This test looks good to me.

::: mailnews/import/test/unit/test_becky_addressbook.js
@@ +4,5 @@
> +    return;
> +
> +  // Due to the import code using nsIAbManager off the main thread, we need
> +  // to ensure that it is initialized before we start the main test.
> +  var abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);

Use let, not var. Also, you should be able to use MailServices.ab instead of getting the service like this.

@@ +6,5 @@
> +  // Due to the import code using nsIAbManager off the main thread, we need
> +  // to ensure that it is initialized before we start the main test.
> +  var abMgr = Cc["@mozilla.org/abmanager;1"].getService(Ci.nsIAbManager);
> +
> +  var file = do_get_file("resources/becky/addressbooks");

let not var

Comment 10

[Wayne Mery (:wsmwk)]

kohei or hiro, can you revisit the reviewed patch so it can be landed?

Comment 11

[Kohei Yoshino [:kohei]]

:hiro is the developer of the add-on :)

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Current trunk on debug build causes crash due to nsISupportsArray.

I filed a new bug to remove nsISupportsArray in mailnews/import. bug 945045
I will post a new patch for this after bug 945045 is fixed.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Import module for becky internet mail

This patch contains the fixes commented in comment #8 by mconley.

This patch needs nsISupportsArray changes in mailnews/import. (bug 916095)

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Attachment: Test for address book import from Becky!

Bug 916095 has not been fixed yet, but try server results look good with the fix for bug 916095, so I will post patches here for the record.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=65860b1c00a5

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Created attachment 8342112 [details] [diff] [review]
> Import module for becky internet mail
> 
> This patch contains the fixes commented in comment #8 by mconley.
> 
> This patch needs nsISupportsArray changes in mailnews/import. (bug 916095)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14)
> Created attachment 8342113 [details] [diff] [review]
> Test for address book import from Becky!
> 
> Bug 916095 has not been fixed yet, but try server results look good with the
> fix for bug 916095, so I will post patches here for the record.

I meant bug 945045.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8342112 [details] [diff] [review]
Import module for becky internet mail

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

I think this looks OK to me, but you're going to want a mailnews reviewer on this as well - especially one better versed in C++ than I am. Tentatively requesting from NeilAway.

::: mail/locales/en-US/chrome/messenger/beckyImportMsgs.properties
@@ +6,5 @@
> +# and informational messages
> +
> +# Short name of import module
> +## @name BECKYIMPORT_NAME
> +## @loc None

What does @loc mean? Does it really have value to our localizers? Same with @name... is that valuable to them somehow?

@@ +15,5 @@
> +## @loc None
> +2001=Import Local Mail from Becky! Internet Mail
> +
> +# Success Message
> +# LOCALIZATION NOTE : Do not translate the word "%S" below.

We should indicate to the localizers what %S is.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8342113 [details] [diff] [review]
Test for address book import from Becky!

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

Again, I think this looks fine, but you'll want mailnews review as well.

Comment 18

[[e-mail]]

Comment on attachment 8342113 [details] [diff] [review]
Test for address book import from Becky!

Sorry, I don't know anything about the testing infrastructure here.

Comment 19

[[e-mail]]

Comment on attachment 8342112 [details] [diff] [review]
Import module for becky internet mail

I'm not sure what I can contribute here but I will at least do a code style review as you may have copied bad practice from other import modules which are probably one of the less well coded parts of Thunderbird, and probably not up-to-date with recommended Mozilla coding practices, possibly because of changes to the build system since the patch was created. Note that I've tried to avoid repeating myself so you may need to check for additional examples of the deprecated code patterns that I mention.

>+++ b/mailnews/import/becky/src/Makefile.in
>@@ -0,0 +1,15 @@
>+#
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+DEPTH		= @DEPTH@
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
>+VPATH		= @srcdir@
>+
>+include $(DEPTH)/config/autoconf.mk
>+
>+LOCAL_INCLUDES += -I$(srcdir)/../../src
There is an effort in progress to move LOCAL_INCLUDES to moz.build which should allow you to eliminate the Makefile.in completely.

>+if CONFIG['MOZ_INCOMPLETE_EXTERNAL_LINKAGE']:
>+    FORCE_STATIC_LIB = True
>+else:
>+    LIBXUL_LIBRARY = True
>+
>+LIBRARY_NAME = 'impBecky_s'
I think you just need to set FINAL_LIBRARY here.

>+NS_IMETHODIMP
>+nsBeckyAddressBooks::GetAutoFind(PRUnichar **aDescription,
>+                                 bool *_retval)
>+{
>+  NS_ENSURE_ARG_POINTER(aDescription);
>+  NS_ENSURE_ARG_POINTER(_retval);
>+
>+  nsString description;
>+  nsBeckyStringBundle::GetStringByID(BECKYIMPORT_DESCRIPTION, description);
>+  *aDescription = ToNewUnicode(description);
This duplicates effort. Just use
*aDescription = nsBeckyStringBundle::GetStringByID(BECKYIMPORT_DESCRIPTION);
Also, as a side note, you should look into using named strings (we're trying to switch away from numbered strings). The code would then look like
*aDescription = nsBeckyStringBundle::GetStringByName(MOZ_UTF16("BeckyImportDescription"));
Also, as a side note, you could change GetString to use a PRUnichar** out parameter, so you could just write
*_retval = false;
return nsBeckyStringBundle::GetStringByID(BECKYIMPORT_DESCRIPTION, aDescription);

>+NS_IMETHODIMP
>+nsBeckyAddressBooks::GetNeedsFieldMap(nsIFile *aLocation, bool *_retval)
>+{
>+  return NS_OK;
XPCOM requries you to write the _retval outparam before returning NS_OK.

>+  return CallQueryInterface(userDirectory, aAddressBookDirectory);
Just use userDirectory.forget(aAddressBookDirectory); return NS_OK;

>+  nsCOMPtr<nsIImportService> importService;
>+
>+  importService = do_GetService(NS_IMPORTSERVICE_CONTRACTID, &rv);
This can be written on one line.

>+  bool more;
>+  nsCOMPtr<nsIFile> entry;
>+  while (NS_SUCCEEDED(entries->HasMoreElements(&more)) && more) {
HasMoreElements is supposed to take an nsISupports**. I know that nsCOMPtr breaks type rules by allowing this but the feature is deprecated and you should not rely on it. Please enumerate into a nsCOMPtr<nsISupports> and then use do_QueryInterface to initialise your nsCOMPtr<nsIFile>.

>+  nsCOMPtr<nsISupports> interface;
>+  interface = do_QueryInterface(descriptor, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return aCollected->AppendElement(interface, false);
On the other hand you can append your nsIImportABDescriptor directly to your nsIMutableArray.

>+nsresult
>+nsBeckyAddressBooks::CollectAddressBooks(nsIFile *aTarget,
>+                                         nsIMutableArray *aCollected)
>+{
>+  bool isDirectory = false;
>+  nsresult rv = aTarget->IsDirectory(&isDirectory);
>+  if (NS_FAILED(rv) || !isDirectory)
>+    return NS_ERROR_FAILURE;
...
>+    rv = entry->IsDirectory(&isDirectory);
>+    if (NS_SUCCEEDED(rv) && isDirectory)
>+      rv = CollectAddressBooks(entry, aCollected);
You're doing two isDirectory checks on the same entry (one here and one in the recursive call), was that intentional?

>+    if (NS_FAILED(rv)) {
>+      if (!error.IsEmpty())
>+        *aErrorLog = ToNewUnicode(error);
>+      break;
>+    }
>+  }
>+
>+  if (error.IsEmpty()) {
>+    nsString successMessage;
>+    nsBeckyStringBundle::GetStringByID(BECKYIMPORT_ADDRESS_SUCCESS, successMessage);
>+    *aSuccessLog = ToNewUnicode(successMessage);
>+  }
This is a little confusing. Perhaps it should look something like this:
  if (NS_FAILED(rv))
    break;
}
if (!error.IsEmpty())
  *aErrorLog = ToNewUnicode(error);
else
  *aSuccessLog = nsBeckyStringBundle::GetStringByID(BECKY_IMPORT_ADDRESS_SUCCESS);

The next few files are quite long so I'll skip forward a bit for now.
>diff --git a/mailnews/import/becky/src/nsBeckyStringBundle.cpp b/mailnews/import/becky/src/nsBeckyStringBundle.cpp

>+  if (NS_SUCCEEDED(rv) && (nullptr != sBundleService))
No need to compare explicitly to null, automatic boolean conversion is OK.

>+    rv = sBundleService->CreateBundle(BECKY_MESSAGES_URL, &sBundle);
>+
>+  mBundle = sBundle;
Why not just mBundle throughout instead of sBundle?

>+void
>+nsBeckyStringBundle::GetStringByID(int32_t stringID, nsString& result)
>+{
>+  PRUnichar *ptrv = GetStringByID(stringID);
>+  result = ptrv;
>+  FreeString(ptrv);
If you do keep this method, use result.Adopt(ptrv); instead.

>+  rv = folder->InitWithNativePath(NS_LITERAL_CSTRING("C:\\Becky!"));
nsIFile uses Unicode internally, so InitWithPath(NS_LITERAL_STRING(...)) works better. Surely that path isn't hardcoded into Becky! though; how does it run on Windows 7?

>+#ifdef XP_WIN
Surely this file already only gets compiled on Windows?

>+#include <windows.h>
Should be near the start of the file.

>+  _retval = NS_ConvertUTF16toUTF8(unicodeString);
CopyUTF16toUTF8(unicodeString, _retval);

>+  uint32_t resultLen = 0;
>+  int n = ::MultiByteToWideChar(CP_ACP, 0, buf, inputLen, NULL, 0);
>+  if (n > 0)
>+    resultLen += n;
Why not just initialise resultLen directly?

>+  // allocate sufficient space
>+  if (!EnsureStringLength(_retval, resultLen))
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  if (resultLen > 0) {
EnsureStringLength inside the if block, perhaps?

>+nsresult
>+nsBeckyUtils::CreateLineInputStream(nsIFile *aFile,
>+                                    nsILineInputStream **_retval)
>+{
>+  NS_ENSURE_ARG_POINTER(_retval);
>+
>+  nsCOMPtr<nsIInputStream> inputStream;
>+  nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), aFile);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return CallQueryInterface(inputStream, _retval);
>+}
This achieves nothing. Callers should just call NS_NewLocalFileInputStream directly.

>+  bool exists;
>+  rv = folderListFile->Exists(&exists);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  NS_IF_ADDREF(*_retval = folderListFile);
>+
>+  return exists ? NS_OK : NS_ERROR_FILE_NOT_FOUND;
Technically XPCOM says you shouldn't write the outparam if you fail.
Also there's another opportunity to use forget() here.

>+  bool exists;
>+  rv = defaultFolder->Exists(&exists);
>+  if (NS_FAILED(rv))
>+    return rv;
You don't use the exists variable.

>+  while (more) {
>+    rv = lineStream->ReadLine(line, &more);
>+    nsBeckyUtils::ConvertNativeStringToUTF8(line, utf8String);
>+    utf8String.AppendLiteral(MSG_LINEBREAK);
>+    rv = destination->Write(utf8String.get(), utf8String.Length(), &writtenBytes);
>+  }
I suppose you can't read in (say) 4K blocks because of multibyte characters across block boundaries but maybe you could read the whole file at once?

>+EXPORTS		= \
>+                nsVCardAddress.h \
>+		$(NULL)
I think EXPORTS also belongs in moz.build these days.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Import module for becky internet mail v2

Almost addressed comment #19.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to [e-mail] from comment #19)
neil, thank you for your review.
> >+++ b/mailnews/import/becky/src/Makefile.in
> >@@ -0,0 +1,15 @@
> >+#
> >+# This Source Code Form is subject to the terms of the Mozilla Public
> >+# License, v. 2.0. If a copy of the MPL was not distributed with this
> >+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >+
> >+DEPTH		= @DEPTH@
> >+topsrcdir	= @top_srcdir@
> >+srcdir		= @srcdir@
> >+VPATH		= @srcdir@
> >+
> >+include $(DEPTH)/config/autoconf.mk
> >+
> >+LOCAL_INCLUDES += -I$(srcdir)/../../src
> There is an effort in progress to move LOCAL_INCLUDES to moz.build which
> should allow you to eliminate the Makefile.in completely.

Removed.

> >+if CONFIG['MOZ_INCOMPLETE_EXTERNAL_LINKAGE']:
> >+    FORCE_STATIC_LIB = True
> >+else:
> >+    LIBXUL_LIBRARY = True
> >+
> >+LIBRARY_NAME = 'impBecky_s'
> I think you just need to set FINAL_LIBRARY here.

Fixed.

> >+NS_IMETHODIMP
> >+nsBeckyAddressBooks::GetAutoFind(PRUnichar **aDescription,
> >+                                 bool *_retval)
> >+{
> >+  NS_ENSURE_ARG_POINTER(aDescription);
> >+  NS_ENSURE_ARG_POINTER(_retval);
> >+
> >+  nsString description;
> >+  nsBeckyStringBundle::GetStringByID(BECKYIMPORT_DESCRIPTION, description);
> >+  *aDescription = ToNewUnicode(description);
> This duplicates effort. Just use
> *aDescription = nsBeckyStringBundle::GetStringByID(BECKYIMPORT_DESCRIPTION);
> Also, as a side note, you should look into using named strings (we're trying
> to switch away from numbered strings). The code would then look like
> *aDescription =
> nsBeckyStringBundle::GetStringByName(MOZ_UTF16("BeckyImportDescription"));
> Also, as a side note, you could change GetString to use a PRUnichar** out
> parameter, so you could just write
> *_retval = false;
> return nsBeckyStringBundle::GetStringByID(BECKYIMPORT_DESCRIPTION,
> aDescription);

nsBeckyStringBundle has been changed as you suggested in the new patch.

> >+NS_IMETHODIMP
> >+nsBeckyAddressBooks::GetNeedsFieldMap(nsIFile *aLocation, bool *_retval)
> >+{
> >+  return NS_OK;
> XPCOM requries you to write the _retval outparam before returning NS_OK.

Fixed.

> >+  return CallQueryInterface(userDirectory, aAddressBookDirectory);
> Just use userDirectory.forget(aAddressBookDirectory); return NS_OK;

Fixed.

> >+  nsCOMPtr<nsIImportService> importService;
> >+
> >+  importService = do_GetService(NS_IMPORTSERVICE_CONTRACTID, &rv);
> This can be written on one line.

Fixed.

> >+  bool more;
> >+  nsCOMPtr<nsIFile> entry;
> >+  while (NS_SUCCEEDED(entries->HasMoreElements(&more)) && more) {
> HasMoreElements is supposed to take an nsISupports**. I know that nsCOMPtr
> breaks type rules by allowing this but the feature is deprecated and you
> should not rely on it. Please enumerate into a nsCOMPtr<nsISupports> and
> then use do_QueryInterface to initialise your nsCOMPtr<nsIFile>.

Fixed all over the place.
 
> >+  nsCOMPtr<nsISupports> interface;
> >+  interface = do_QueryInterface(descriptor, &rv);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  return aCollected->AppendElement(interface, false);
> On the other hand you can append your nsIImportABDescriptor directly to your
> nsIMutableArray.

Fixed.

> >+nsresult
> >+nsBeckyAddressBooks::CollectAddressBooks(nsIFile *aTarget,
> >+                                         nsIMutableArray *aCollected)
> >+{
> >+  bool isDirectory = false;
> >+  nsresult rv = aTarget->IsDirectory(&isDirectory);
> >+  if (NS_FAILED(rv) || !isDirectory)
> >+    return NS_ERROR_FAILURE;
> ...
> >+    rv = entry->IsDirectory(&isDirectory);
> >+    if (NS_SUCCEEDED(rv) && isDirectory)
> >+      rv = CollectAddressBooks(entry, aCollected);
> You're doing two isDirectory checks on the same entry (one here and one in
> the recursive call), was that intentional?

That was wrong. Fixed.

> >+    if (NS_FAILED(rv)) {
> >+      if (!error.IsEmpty())
> >+        *aErrorLog = ToNewUnicode(error);
> >+      break;
> >+    }
> >+  }
> >+
> >+  if (error.IsEmpty()) {
> >+    nsString successMessage;
> >+    nsBeckyStringBundle::GetStringByID(BECKYIMPORT_ADDRESS_SUCCESS, successMessage);
> >+    *aSuccessLog = ToNewUnicode(successMessage);
> >+  }
> This is a little confusing. Perhaps it should look something like this:
>   if (NS_FAILED(rv))
>     break;
> }
> if (!error.IsEmpty())
>   *aErrorLog = ToNewUnicode(error);
> else
>   *aSuccessLog =
> nsBeckyStringBundle::GetStringByID(BECKY_IMPORT_ADDRESS_SUCCESS);

Fixed as you suggested.

> >+  if (NS_SUCCEEDED(rv) && (nullptr != sBundleService))
> No need to compare explicitly to null, automatic boolean conversion is OK.
> 
> >+    rv = sBundleService->CreateBundle(BECKY_MESSAGES_URL, &sBundle);
> >+
> >+  mBundle = sBundle;
> Why not just mBundle throughout instead of sBundle?

Fixed. just use mBundle there.

> >+void
> >+nsBeckyStringBundle::GetStringByID(int32_t stringID, nsString& result)
> >+{
> >+  PRUnichar *ptrv = GetStringByID(stringID);
> >+  result = ptrv;
> >+  FreeString(ptrv);
> If you do keep this method, use result.Adopt(ptrv); instead.
> 
> >+  rv = folder->InitWithNativePath(NS_LITERAL_CSTRING("C:\\Becky!"));
> nsIFile uses Unicode internally, so InitWithPath(NS_LITERAL_STRING(...))
> works better. Surely that path isn't hardcoded into Becky! though; how does
> it run on Windows 7?

Thanks for the suggestion. I've added the path for Windows 7.

> >+#ifdef XP_WIN
> Surely this file already only gets compiled on Windows?
> 
> >+#include <windows.h>
> Should be near the start of the file.

I thought that this import module can be work on Linux in the future.
But I removed it for now.

> >+  _retval = NS_ConvertUTF16toUTF8(unicodeString);
> CopyUTF16toUTF8(unicodeString, _retval);
> 
> >+  uint32_t resultLen = 0;
> >+  int n = ::MultiByteToWideChar(CP_ACP, 0, buf, inputLen, NULL, 0);
> >+  if (n > 0)
> >+    resultLen += n;
> Why not just initialise resultLen directly?
>
> >+  // allocate sufficient space
> >+  if (!EnsureStringLength(_retval, resultLen))
> >+    return NS_ERROR_OUT_OF_MEMORY;
> >+  if (resultLen > 0) {
> EnsureStringLength inside the if block, perhaps?

These methods were needed in add-on codes(I had implemented becky import module as an add-on before.). These methods were removed now.

> >+nsresult
> >+nsBeckyUtils::CreateLineInputStream(nsIFile *aFile,
> >+                                    nsILineInputStream **_retval)
> >+{
> >+  NS_ENSURE_ARG_POINTER(_retval);
> >+
> >+  nsCOMPtr<nsIInputStream> inputStream;
> >+  nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), aFile);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  return CallQueryInterface(inputStream, _retval);
> >+}
> This achieves nothing. Callers should just call NS_NewLocalFileInputStream
> directly.

This method is just a utility method something like the following code does:

 rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), file);
 nsCOMPtr<nsILineInputStream> lineStream = do_QueryInterface(inputStream);

Am I missing something?

> >+  bool exists;
> >+  rv = folderListFile->Exists(&exists);
> >+  if (NS_FAILED(rv))
> >+    return rv;
> >+
> >+  NS_IF_ADDREF(*_retval = folderListFile);
> >+
> >+  return exists ? NS_OK : NS_ERROR_FILE_NOT_FOUND;
> Technically XPCOM says you shouldn't write the outparam if you fail.
> Also there's another opportunity to use forget() here.

Fixed.

> >+  bool exists;
> >+  rv = defaultFolder->Exists(&exists);
> >+  if (NS_FAILED(rv))
> >+    return rv;
> You don't use the exists variable.

Fixed.

> >+  while (more) {
> >+    rv = lineStream->ReadLine(line, &more);
> >+    nsBeckyUtils::ConvertNativeStringToUTF8(line, utf8String);
> >+    utf8String.AppendLiteral(MSG_LINEBREAK);
> >+    rv = destination->Write(utf8String.get(), utf8String.Length(), &writtenBytes);
> >+  }
> I suppose you can't read in (say) 4K blocks because of multibyte characters
> across block boundaries but maybe you could read the whole file at once?

Right.
But as far as I confirm becky's mail box, there is no such long line.

> >+EXPORTS		= \
> >+                nsVCardAddress.h \
> >+		$(NULL)
> I think EXPORTS also belongs in moz.build these days.

Fixed.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

For the record, both nsMsgBrkMBoxStore and nsMsgHDr are not thread-safe, so importing mail message data function has to be done in main thread in the new patch.

Other import modules (Outlook Express and so on) should be rewritten too. I will work on it later.

Comment 23

[[e-mail]]

(In reply to Hiroyuki Ikezoe from comment #21)
> This method is just a utility method something like the following code does:
> 
>  rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), file);
>  nsCOMPtr<nsILineInputStream> lineStream = do_QueryInterface(inputStream);
> 
> Am I missing something?

Aha, I overlooked the nsILineInputStream QI. Sorry about that.

Comment 24

[[e-mail]]

Comment on attachment 8376933 [details] [diff] [review]
Import module for becky internet mail v2

>+  *aLocation = nullptr;
>+  *_retval = false;
>+
>+  nsresult rv;
>+  nsCOMPtr<nsIFile> location;
>+  rv = GetDefaultFilterFile(getter_AddRefs(location));
>+  if (NS_FAILED(rv))
>+    location = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
>+
>+  location.forget(aLocation);
>+  *_retval = true;
>+  return NS_OK;
Just skimming the differences between this and the previous patch, I notice that something seems to have gone wrong here - you only want to set *_retval to true if you found the default file. (Same applies somewhere else too.)

>+nsIStringBundle *nsBeckyStringBundle::mBundle = nullptr;
>+
>+nsIStringBundle *
>+nsBeckyStringBundle::GetStringBundle(void)
>+{
>+  if (mBundle)
>+    return mBundle;
>+
>+  nsresult rv;
>+  nsCOMPtr<nsIStringBundleService> bundleService = do_GetService(NS_STRINGBUNDLE_CONTRACTID, &rv);
>+  if (NS_SUCCEEDED(rv) && bundleService)
>+    rv = bundleService->CreateBundle(BECKY_MESSAGES_URL, &mBundle);
>+
>+  return mBundle;
>+}
>+
>+char16_t *
>+nsBeckyStringBundle::GetStringByName(const char16_t *aName)
>+{
>+  if (!mBundle)
>+    mBundle = GetStringBundle();
>+
>+  char16_t *string = nullptr;
>+  if (mBundle)
>+    mBundle->GetStringFromName(aName, &string);
>+
>+  return string;
>+}
>+
>+nsresult
>+nsBeckyStringBundle::FormatStringFromName(const char16_t *name,
>+                                          const char16_t **params,
>+                                          uint32_t length,
>+                                          char16_t **_retval)
>+{
>+  if (!mBundle)
>+    mBundle = GetStringBundle();
I think I may have overlooked something on my previous comment - now that the code has been simplified by the removal of sBundle you can see that there's duplication of work between GetStringBundle and its callers as to whose duty it is to null-check mBundle and assign to it. (It might be worth renaming GetStringBundle to CreateStringBundle or EnsureStringBundle as well.)

Comment 25

[[e-mail]]

Comment on attachment 8376933 [details] [diff] [review]
Import module for becky internet mail v2

nsBeckyFilters:

>+  rv = GetDefaultFilterFile(getter_AddRefs(location));
To be fair I've never tried importing anything in to Thunderbird, so I don't know what the user experience is here, but maybe it would be better to always return the expected location of the filter file, so that there's something to prime the filepicker when it's not found.

>+ConvertSearchKeyToAttrib(const nsDependentCSubstring &aKey)
Nit: For string parameters we generally prefer to use nsA[C]String or ns[C]String rather than any of their subclasses (so const nsACString & here).

>+  if (aKey.Equals("From") ||
>+      aKey.Equals("Sender") ||
>+      aKey.Equals("From, Sender, X-Sender")) {
Nit: very slight benefit to using EqualsLiteral here, as the compiler will then compare the length of the string first.

>+  if (lastTabPosition < -1 ||
>+     aFlags.Length() == static_cast<uint32_t>(lastTabPosition)) {
I'm not sure what you're trying to achieve here; RFindChar will either return kNotFound (-1) or non-negative number less than the length.

>+  nsDependentCSubstring operatorFlags(aFlags, lastTabPosition + 1);
>+  nsDependentCSubstring negativeFlag(aFlags, 0, aFlags.FindChar('\t') - 1);
>+
>+  switch (negativeFlag.CharAt(0)) {
Since you only look at char 0 of negativeFlag, why not look at char 0 of aFlags directly?

>+      if (operatorFlags.FindChar('T') >= 0)
aFlags.FindChar('T', lastTabPosition + 1) perhaps?

>+    default:
>+      return -1;
>+  }
>+
>+  return -1;
Do we actually get here?

>+  aSearchKeyword.Assign(NS_ConvertUTF8toUTF16(Substring(aLine,
>+                                                        secondColonPosition + 1,
>+                                                        length)));
CopyUTF8toUTF16(Substring(...), aSearchKeyword);

>+nsBeckyFilters::SetSearchTerm(const nsCString &aLine, nsIMsgFilter *aFilter)
Is it worth making the filter a member to avoid passing it around everywhere?

>+  rv = term->SetBooleanAnd(false);
>+  NS_ENSURE_SUCCESS(rv, rv);
[I'll take your word for it that this is correct behaviour.]

>+  rv = folder->GetURI(folderURL);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  aTargetFolder.Assign(folderURL);
>+
>+  return NS_OK;
Nit: return folder->GetURI(aTargetFolder);

>+  rv = filterList->CreateFilter(NS_LITERAL_STRING(""), getter_AddRefs(filter));
Nit: EmptyString()

>+        } else {
>+          continue;
Not sure that this continue; achieves anything.

>+        if (line.Length() >= 2 &&
>+            line.CharAt(1) == 'X' &&
StringBeginsWith(line, "$X") ?

>+  nsCOMPtr<nsIArray> accounts;
>+  rv = accountManager->GetAccounts(getter_AddRefs(accounts));
[Again, not familiar with the import process, so I don't know why you're checking all the servers.]

>+  if (!found) {
>+    nsCOMPtr<nsIMsgIncomingServer> server;
>+    rv = accountManager->GetLocalFoldersServer(getter_AddRefs(server));
I think the local folders counts as an account with a server, so you've already searched it.

Comment 26

[[e-mail]]

Comment on attachment 8376933 [details] [diff] [review]
Import module for becky internet mail v2

nsBeckyImport:

>+static NS_DEFINE_IID(kISupportsIID, NS_ISUPPORTS_IID);
Not used?

>+  nsIImportMail *importer = nullptr;
Don't you want nsCOMPtr<nsIImportMail> importer; here?

>+  rv = CallQueryInterface(generic, aInterface);
>+
>+  return rv;
[You managed to simplify this below.]

>+  if (!strcmp(aImportType, "mail")) {
>+    return GetMailImportInterface(aInterface);
>+  } else if (!strcmp(aImportType, "addressbook")) {
>+    return GetAddressBookImportInterface(aInterface);
>+  } else if (!strcmp(aImportType, "settings")) {
>+    return GetSettingsImportInterface(aInterface);
>+  } else if (!strcmp(aImportType, "filters")) {
>+    return GetFiltersImportInterface(aInterface);
>+  }
Don't use else after return, just put separate if statements.

nsBeckySettings:

>+nsBeckySettings::nsBeckySettings()
>+: mLocation(nullptr),
>+  mConvertedFile(nullptr),
>+  mParser(nullptr)
These member variables already default to nullptr.

>+  rv = smtpService->FindServer(aUserName.get(),
>+                               aServerName.get(),
>+                               getter_AddRefs(server));
>+
I think you want to fail to create a server if one already exists; I think it would be a bad idea if you went around changing settings of existing servers. (The one interesting case is when you have an existing SMTP server, you might consider associating that with your new identity.)

>+    port = static_cast<int32_t>(value.ToInteger(&errorCode, 10));
ToInteger already returns an int32_t, and the radix defaults to 10.

>+    nsMsgAuthMethodValue authMethod = nsMsgAuthMethod::none;
Nit: No point initialising this if you're always going to set it anyway.

>+  if (value.Equals("0")) {
>+    protocol = NS_LITERAL_CSTRING("pop3");
>+  } else if (value.Equals("1")) {
>+    protocol = NS_LITERAL_CSTRING("imap");
>+  } else {
>+    protocol = NS_LITERAL_CSTRING("pop3");
>+  }
Duplication of logic; just test for 1 if you're going to default for everything else.

>+      server->SetBiffMinutes(minutes * 1);
*1?

>+  nsCOMPtr<nsISmtpServer> smtpServer;
>+  rv = SetupSmtpServer(getter_AddRefs(smtpServer));
>+  NS_RETURN_IF_FAILED_WITH_REMOVE_CONVERTED_FILE(rv, rv);
>+
>+  nsCOMPtr<nsIMsgIdentity> identity;
>+  rv = CreateIdentity(getter_AddRefs(identity));
>+  NS_RETURN_IF_FAILED_WITH_REMOVE_CONVERTED_FILE(rv, rv);
[Might be an idea to associate the SMTP server with the identity.]

Comment 27

[[e-mail]]

Comment on attachment 8376933 [details] [diff] [review]
Import module for becky internet mail v2

nsBeckyUtils:

>+  rv = FindUserDirectoryOnWindowsXP(getter_AddRefs(directory));
>+  if (rv == NS_ERROR_FILE_NOT_FOUND) {
>+    rv = FindUserDirectoryOnWindows7(getter_AddRefs(directory));
(I can't decide which directory is better to look for first... probably 7, then XP)

>+  directory.forget(aLocation);
(You could just pass aLocation instead of getter_AddRefs(directory) in the first place.)

>+nsresult
>+nsBeckyUtils::ConvertNativeStringToUTF8(const nsACString& aOriginal,
>+                                        nsACString& _retval)
You use this in two places, but I think there are better alternatives.
For the attachment path, you can simply use the native local file API.
For the file conversion, see below.

>+  rv = folderListFile->AppendNative(NS_LITERAL_CSTRING("Folder.lst"));
You have a literal path, so you can use Append(NS_LITERAL_STRING("Folder.lst")) instead. (Same probably applies in other parts of your patch.)

>+  nsCOMPtr<nsIINIParser> parser;
>+  rv = factory->CreateINIParser(aFile, getter_AddRefs(parser));
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  parser.forget(aParser);
>+
>+  return NS_OK;
Again, you can just write return factory->CreateINIParser(aFile, aParser);

>+  nsCOMPtr<nsILineInputStream> lineStream;
>+  rv = nsBeckyUtils::CreateLineInputStream(aSourceFile,
>+                                           getter_AddRefs(lineStream));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsAutoCString line;
>+  nsAutoCString utf8String;
>+  uint32_t writtenBytes = 0;
>+  bool more = true;
>+  while (more) {
>+    rv = lineStream->ReadLine(line, &more);
>+    nsBeckyUtils::ConvertNativeStringToUTF8(line, utf8String);
>+    utf8String.AppendLiteral(MSG_LINEBREAK);
>+    rv = destination->Write(utf8String.get(), utf8String.Length(), &writtenBytes);
>+  }
I think you should be using nsIConverterInput/OutputStream instead. (Use nsMsgI18NFileSystemCharset() to get the character set.) Note: ReadSegments is slightly more efficient than Read or ReadString as it avoids a memcpy.
Also you weren't buffering your output stream which makes writing a line at a time horribly inefficient. (The converter streams default to 8K buffers so that wouldn't be a problem.)

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 8342113 [details] [diff] [review]
Test for address book import from Becky!

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

Looks fine to me, though please make sure both patches are run through try server before landing. r=Standard8

Comment 29

[Ludovic Hirlimann [:Usul]]

:Aryx can you do the pushes and set checking needed if they pass /try ?

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed to Thunderbird-Try: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=fd7a5067d9ef

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Second patch doesn't have review?

Comment 32

[[e-mail]]

Comment on attachment 8376933 [details] [diff] [review]
Import module for becky internet mail v2

nsBeckyMail:

>+      aName = NS_ConvertUTF8toUTF16(utf8Name);
CopyUTF8toUTF16(utf8Name, aName);

>+  nsCOMPtr<nsIFile> mailboxFile;
>+  rv = descriptor->GetFile(getter_AddRefs(mailboxFile));
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  descriptor->SetDepth(aDepth);
>+
>+  mailboxFile->InitWithFile(aEntry);
[Not your fault, but this is a terrible API.]

>+  nsAutoCString value(Substring(aHeader,
>+                                valueStartPosition,
>+                                aHeader.FindChar(',', valueStartPosition) -
>+                                valueStartPosition));
How confident are you that the comma exists?

>+  nsAutoCString attachmentPath;
>+  nsBeckyUtils::ConvertNativeStringToUTF8(nativeAttachmentPath, attachmentPath);
>+
>+  nsTArray<nsCString> directoryNames;
>+  ParseString(attachmentPath, '\\', directoryNames);
>+  for (nsTArray<nsCString>::index_type i = 0; i < directoryNames.Length(); i++)
>+    rv = attachmentFile->Append(NS_ConvertUTF8toUTF16(directoryNames[i]));
attachmentFile->AppendRelativeNativePath(nativeAttachmentPath);

>+      if (StringBeginsWith(line, NS_LITERAL_CSTRING("..")))
>+        line.Cut(0, 1);
>+      else if (CheckHeaderKey(line, "From"))
>+        outputStream->Write(">", 1, &writtenBytes);
(You might have copied this pattern from one of the other importers, but I just thought it looked odd. Probably I would go for line.Insert('>', 0); instead.)

>+    rv =
>+      nsBeckyStringBundle::FormatStringFromName(MOZ_UTF16("BeckyImportMailboxSuccess"),
>+                                                &format,
>+                                                1,
>+                                                getter_Copies(successMessage));
>+    successMessage.AppendLiteral("\n");
>+    *aSuccessLog = ToNewUnicode(successMessage);
(Again, nasty API requirement making your life harder here.)

Well, I think I've covered everything now, thank you for your patience!

Comment 33

[:aceman]

What a big patch needed just to create one import module :(
But it seems contained in standalone files so there shouldn't be much bitrot. Let's try to save this nice piece of work.

Comment 34

[Wayne Mery (:wsmwk)]

aceman, if you need advice perhaps m_kato can assist. (CCed)
As noted in comment 0, Becky is very popular in Japan. Have this import would be a good win for us.

Comment 35

[:aceman]

Attachment: Test for address book import from Becky! v1.1

Refreshed the patch so that it applies to trunk.

Comment 36

[:aceman]

Attachment: Import module for becky internet mail v2.1

I have updated the patch according to Neil's comments. The attached test also passes with the new patch (locally). But the test only tests addressbook import. mkato, could you somehow check (using Becky) if this still imports mail and other settings correctly? If there is not some silent corruption due to the changes made.
Thanks.

Comment 37

[Makoto Kato [:m_kato]]

Comment on attachment 8739711 [details] [diff] [review]
Import module for becky internet mail v2.1

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

::: mailnews/import/becky/src/moz.build
@@ +10,5 @@
> +    'nsBeckyMail.cpp',
> +    'nsBeckySettings.cpp',
> +    'nsBeckyStringBundle.cpp',
> +    'nsBeckyUtils.cpp',
> +]

Cannot use UNIFIED_SOURCE?

::: mailnews/import/becky/src/nsBeckyAddressBooks.cpp
@@ +31,5 @@
> +  NS_ENSURE_ARG_POINTER(aImport);
> +
> +  *aImport = new nsBeckyAddressBooks();
> +  if (!*aImport)
> +    return NS_ERROR_OUT_OF_MEMORY;

Gecko uses infallible allocator.  So this nullptr check is unnecessary.

@@ +196,5 @@
> +
> +    int64_t size;
> +    file->GetFileSize(&size);
> +    if (total + size > UINT32_MAX)
> +      return UINT32_MAX;

nit: std::numeric_limits<uint32_t>::max() is better than UINT32_MAX.

::: mailnews/import/becky/src/nsBeckyAddressBooks.h
@@ +7,5 @@
> +#define nsBeckyAddressBooks_h___
> +
> +#include "nsIImportAddressBooks.h"
> +
> +class nsBeckyAddressBooks : public nsIImportAddressBooks

final?

::: mailnews/import/becky/src/nsBeckyFilters.cpp
@@ +32,5 @@
> +  NS_ENSURE_ARG_POINTER(aImport);
> +
> +  *aImport = new nsBeckyFilters();
> +  if (!*aImport)
> +    return NS_ERROR_OUT_OF_MEMORY;

This nullptr check is unnecessary due to infallible allocator.

@@ +463,5 @@
> +nsresult
> +nsBeckyFilters::CreateFilter(nsIMsgFilter **_retval)
> +{
> +  if (!mServer)
> +    return NS_ERROR_FAILURE;

If other codes use NS_ENSURE_SUCCESS, should you use NS_ENSURE_TURE?

@@ +483,5 @@
> +nsresult
> +nsBeckyFilters::AppendFilter(nsIMsgFilter *aFilter)
> +{
> +  if (!mServer)
> +    return NS_ERROR_FAILURE;

If other codes use NS_ENSURE_SUCCESS, should you use NS_ENSURE_TURE?

::: mailnews/import/becky/src/nsBeckyFilters.h
@@ +14,5 @@
> +class nsIMsgFilter;
> +class nsIMsgRuleAction;
> +class nsCString;
> +
> +class nsBeckyFilters : public nsIImportFilters

final?

::: mailnews/import/becky/src/nsBeckyImport.h
@@ +14,5 @@
> +  {0x9f, 0x02, 0x15, 0x0b, 0x15, 0xa0, 0xa8, 0x41}}
> +
> +#define kBeckySupportsString NS_IMPORT_MAIL_STR "," NS_IMPORT_ADDRESS_STR "," NS_IMPORT_SETTINGS_STR "," NS_IMPORT_FILTERS_STR
> +
> +class nsBeckyImport : public nsIImportModule

final?

::: mailnews/import/becky/src/nsBeckyMail.cpp
@@ +607,5 @@
> +  rv = aSource->GetFile(getter_AddRefs(mailboxFolder));
> +  if (NS_FAILED(rv))
> +    return rv;
> +
> +  rv = ImportMailFile(mailboxFolder, aDestination);

if (NS_FAILED(rv)) {
  return rv;
}

or

NS_ENSURE_SUCCESS(rv, rv);

@@ +613,5 @@
> +  uint32_t finalSize;
> +  aSource->GetSize(&finalSize);
> +  mReadBytes = finalSize;
> +
> +  if (NS_SUCCEEDED(rv)) {

remove this check.  we already add rv check.

::: mailnews/import/becky/src/nsBeckyMail.h
@@ +11,5 @@
> +class nsIFile;
> +class nsIMutableArray;
> +class nsIMsgFolder;
> +
> +class nsBeckyMail : public nsIImportMail

final?

::: mailnews/import/becky/src/nsBeckySettings.cpp
@@ +257,5 @@
> +    pop3Server->SetLeaveMessagesOnServer(true);
> +    nsresult rv = mParser->GetString(NS_LITERAL_CSTRING("Account"),
> +                                     NS_LITERAL_CSTRING("KeepDays"),
> +                                     value);
> +    if (NS_SUCCEEDED(rv)) {

if (NS_FAILED(rv)) {
  return NS_OK;
}
...

Comment 38

[:aceman]

Attachment: Import module for becky internet mail v2.2

Thanks. I hope I fixed everything.

Comment 39

[:aceman]

Thanks mkato!

rkent, can you rubber-stamp this as discussed?

Comment 40

[Kent James (:rkent)]

Has anyone investigated the threading issues of this? We've disabled import for Outlook and Eudora because of theading-related crashes. At a quick glance, it looked to me like the runnable included calls to the msgDatabase which is not thread-proof.

Comment 41

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kent James (:rkent) from comment #40)
> Has anyone investigated the threading issues of this? We've disabled import
> for Outlook and Eudora because of theading-related crashes. At a quick
> glance, it looked to me like the runnable included calls to the msgDatabase
> which is not thread-proof.

The patch handles msgDatabase in the main-thread, I really don't remember what I did in the patch, but according to comment #22, it seems to me that I wrote the patch avoid threading issue at that time.

Comment 42

[Kent James (:rkent)]

OK with Hiro last comment, and all of the work that has gone into this, I think we should go ahead and land it. rs=me if you need that.

Comment 43

[:aceman]

Yes, thanks.

Comment 44

[:aceman]

https://hg.mozilla.org/comm-central/rev/beac1b52a9251c346820d41d80a83b6037dbd1fd
https://hg.mozilla.org/comm-central/rev/53135577a75a21f8a97290a6d4669075a3037879

Comment 45

[:aceman]

Well, we have tried the patch previously and it did build on Windows (thanks to Paenglab!).

But now it fails on trunk. Maybe just some m-c identifier was renamed since then.
build/mailnews/import/becky/src/nsBeckyMail.cpp(372): error C2504: 'nsRunnable': base class undefined
build/mailnews/import/becky/src/nsBeckyMail.cpp(376): error C3668: 'ImportMessageRunnable::Run': method with override specifier 'override' did not override any base class methods
build/mailnews/import/becky/src/nsBeckyMail.cpp(570): error C2665: 'NS_DispatchToMainThread': none of the 2 overloads could convert all the argument types
build\objdir-tb\dist\include\nsThreadUtils.h(129): note: could be 'nsresult NS_DispatchToMainThread(already_AddRefed<nsIRunnable> &&,uint32_t)'
build\objdir-tb\dist\include\nsThreadUtils.h(126): note: or       'nsresult NS_DispatchToMainThread(nsIRunnable *,uint32_t)'
build/mailnews/import/becky/src/nsBeckyMail.cpp(570): note: while trying to match the argument list '(RefPtr<ImportMessageRunnable>, )'
build/mozilla/config/rules.mk:934: recipe for target 'Unified_cpp_import_becky_src0.obj' failed

Comment 46

[Jorg K (CEST = GMT+2)]

Is this some fresh bustage? Or is there an easy fix? It might be best to back this out for now.

Comment 47

[:aceman]

I think the bustage is caused by this patch. I do not see nsRunnable defined in the tree (nor m-c).
Yes, if nobody finds a fix quickly we can safely back this it out.

Comment 48

[Jorg K (CEST = GMT+2)]

Should be nsIRunnable?
https://dxr.mozilla.org/comm-central/source/mozilla/xpcom/threads/nsIRunnable.idl#13
I just don't understand how that compiled on the 2016-04-09.

Comment 49

[:aceman]

Quite possible. Can you please try that change on try?

Comment 50

[Richard Marti (:Paenglab)]

Aceman, can you backout before the Daily begins to build?

Comment 51

[:aceman]

Sorry, I can't do anything for the next 24hours therefore I ask anybody to try the fix (nsIRunnable) or back the patch out.

Comment 52

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/653792abce5917dc041546cf53afb869237f11a5
Backed out changeset beac1b52a925 for build failures (Bug 684455).

https://hg.mozilla.org/comm-central/rev/1a8b3bd56c135170ce82385d19542f85b555e653
Backed out changeset 53135577a75a for build failures (Bug 684455).

Comment 53

[aleth [:aleth]]

Please also fix the commit message typo when you reland this ;)

Comment 54

[Philip Chee]

Maybe?
Bug 1270631 - Port bug 1268313 part 7 to c-c [Move NS_NewRunnableMethod to mozilla::NewRunnableMethod]
Bug 1268313 - Consolidate on one set of runnable method/function macros

Comment 55

[Philip Chee]

Aha!
> Bug 1265927: Move nsRunnable to mozilla::Runnable, CancelableRunnable to mozilla::CancelableRunnable
> https://hg.mozilla.org/mozilla-central/rev/fcc0936b576d

s/public nsRunnable/public Runnable/g
You might need mozilla::Runnable if namespace...

Comment 56

[Jorg K (CEST = GMT+2)]

The Stainless Steel Rat Saves the World! Thanks.

Comment 57

[Jorg K (CEST = GMT+2)]

I'll take care of it after lunch.

Comment 58

[Jorg K (CEST = GMT+2)]

Relanded using mozilla::Runnable instead of nsRunnable. Also fixed commit message.
I made sure it compiles and TB starts ;-)

https://hg.mozilla.org/comm-central/rev/f1755f28374d
https://hg.mozilla.org/comm-central/rev/3133a94f3a1b

Comment 59

[:aceman]

Thanks Jorg!
If it doesn't build on trunk, it may be a different bustage (notice all builds failed even with this patch backed out).

Comment 60

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #59)
> If it doesn't build on trunk, it may be a different bustage (notice all
> builds failed even with this patch backed out).
Indeed.

I compiled locally with M-C being a few day old. Since on Treeherder the build after the backout failed, it can't be us.

Comment 61

[aleth [:aleth]]

(In reply to Jorg K (PTO during summer, NI me) from comment #60)
> I compiled locally with M-C being a few day old. Since on Treeherder the
> build after the backout failed, it can't be us.

Yes, that's an unrelated configure issue.

Comment 62

[baffclan]

Cannot import address book from Becky!2
I file new bug. see bug 1273477.

Comment 63

[Philip Chee]

Attachment: Strings patch for SeaMonkey. checked-in [Comment 66]

See https://hg.mozilla.org/comm-central/rev/3133a94f3a1b#l1.1

Comment 64

[Ian Neal]

Comment on attachment 8753879 [details] [diff] [review]
Strings patch for SeaMonkey. checked-in [Comment 66]

a=me if required too

Comment 65

[baffclan]

I file new bug. 
bug 1274228 Crash when import a settings from Becky!2
bug 1274229 Nothing happened when import a Filters from Becky!2

Comment 66

[Philip Chee]

Comment on attachment 8753879 [details] [diff] [review]
Strings patch for SeaMonkey. checked-in [Comment 66]

http://hg.mozilla.org/comm-central/rev/56079a1b0bc0