1612240 - Replace idl nsIArray usage with Array<T> in mailnews/import/

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Ben Campbell]

Covers:

mailnews/import/public/nsIImportAddressBooks.idl
mailnews/import/public/nsIImportService.idl
mailnews/import/public/nsIImportGeneric.idl
mailnews/import/public/nsIImportMail.idl

searchfox search

Comment 1

[Ben Campbell]

Attachment: [checked in] 1612240-remove-nsIArray-from-nsIImportAddressBooks-1.patch

Mostly straightforward, but nsIImportGeneric.GetData() and SetData() are tricky. They return/take an nsISupports, but the exact type passed depends on the dataId param:

  nsISupports GetData(in string dataId);
  void SetData(in string dataId, in nsISupports pData);

It returns an nsIArray for dataId of either "mailBoxes" or "addressBooks", but I can't see that they are ever used anywhere. I added a stopgap to build and return an nsIArray, so they will work. But if nothing is using them maybe I can just ditch those two cases entirely?

In any case, I think we should move away from interfaces which pass nsISupports objects as variant types. Unless there's any objection I'll start writing such cases up in their own bugs.

Try run here:

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a72a26944f22237d0adc9106287149e29f59e8cf

And again, for windows (with a one line fix, but otherwise identical):

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9b207f8e189536a5ccacf82469e185896ad9fb6a

Comment 2

[Ben Campbell]

Attachment: 1612240-remove-nsIArray-from-nsIImportMail-1.patch

And the equivalent for nsIImportMail. Same comments apply here as above (the nsImportGenericMail::SetData()/GetData() methods can take/return an nsIArray if "mailBoxes" is passes as the dataId)

try run here:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=cf51cf9c638ac5630bb8036d183fdefde9770318

And again, with compilation fixes for Windows and OSX:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=fa509873a20a2ab41302b434e2fde6d15bee4644

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9191538 [details] [diff] [review]
[checked in] 1612240-remove-nsIArray-from-nsIImportAddressBooks-1.patch

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

Looks good, r=mkmelin

::: mailnews/import/src/nsImportAddressBooks.cpp
@@ +142,5 @@
>    }
>    if (!PL_strcasecmp(dataId, "addressBooks")) {
> +    // Stopgap during nsIArray-removal (Bug 1612240).
> +    // TODO: SetData("addressBooks") doesn't seem to be used anywhere.
> +    // Maybe this can just be dropped?

Yeah, maybe do a follow-up patch for that.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9191543 [details] [diff] [review]
1612240-remove-nsIArray-from-nsIImportMail-1.patch

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

::: mailnews/import/public/nsIImportMail.idl
@@ +56,3 @@
>      mailbox.  The array is not sorted before display to the user.
>    */
> +  Array<nsIImportMailboxDescriptor> FindMailboxes(in nsIFile location);

Would fix the name to be findMailboxes while we're touching it.
Only the one js file that it changes anything for it seems.

Comment 5

[Ben Campbell]

Attachment: [checked in] 1612240-remove-nsIArray-from-nsIImportMail-2.patch

Same as previous nsIImportMail patch, but FindMailboxes() is now findMailboxes() in idl and javascript files.

Comment 6

[Ben Campbell]

Attachment: 1612240-rename-nsIImportAddressBooks.FindAddressBooks-1.patch

nsIImportAddressBooks.FindAddressBooks() isn't used at all by javascript, so I'm not sure what the policy is on lower-camel-casing interface methods we touch.
Hence a separate patch.
I'd guess none of the other methods have any javascript either, but I wouldn't change them without spending a little more time on testing/try runs etc... not sure it's worth it.

Comment 7

[Ben Campbell]

Attachment: 1612240-rename-TestMailImpoter-1.patch

And one typo-fixup, because it irked me :-)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/97258a0f665c
Remove nsIArray use from nsIImportAddressBooks. r=mkmelin
https://hg.mozilla.org/comm-central/rev/f2b3b28f14d1
Remove nsIArray use from nsIImportMail. r=mkmelin

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9191763 [details] [diff] [review]
1612240-rename-TestMailImpoter-1.patch

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

Well, it's unused as well. https://phabricator.services.mozilla.com/D86729

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e3a9cad7f5cc
Followup. Rename idl nsIImportAddressBooks.FindAddressBooks() to findAddressBooks(). r=mkmelin
https://hg.mozilla.org/comm-central/rev/2bcaef14a6f8
Followup. Rename TestMailImpoter to TestMailImporter. r=mkmelin