Closed Bug 1612240 Opened 5 years ago Closed 4 years ago

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

Categories

(MailNews Core :: General, task)

Product:
MailNews Core
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(thunderbird_esr78 wontfix)

Status:
RESOLVED FIXED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: benc, Assigned: benc)

References

Details

Attachments

(4 files, 1 obsolete file)

[checked in] 1612240-remove-nsIArray-from-nsIImportAddressBooks-1.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review
[checked in] 1612240-remove-nsIArray-from-nsIImportMail-2.patch
(deleted), patch
benc
: review+
Details | Diff | Splinter Review
1612240-rename-nsIImportAddressBooks.FindAddressBooks-1.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review
1612240-rename-TestMailImpoter-1.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review

Covers:

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

searchfox search

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

Attachment #9191538 - Flags: review?(mkmelin+mozilla)

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

Attachment #9191543 - Flags: review?(mkmelin+mozilla)
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.
Attachment #9191538 - Flags: review?(mkmelin+mozilla) → review+
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.
Attachment #9191543 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
status-thunderbird_esr78: --- → wontfix
Target Milestone: --- → 85 Branch

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

Attachment #9191543 - Attachment is obsolete: true
Attachment #9191757 - Flags: review+
Keywords: checkin-needed-tb

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.

Attachment #9191761 - Flags: review?(mkmelin+mozilla)

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

Attachment #9191763 - Flags: review?(mkmelin+mozilla)
Keywords: leave-open

Pushed by mkmelin@iki.fi:
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

Keywords: checkin-needed-tb
Attachment #9191538 - Attachment description: 1612240-remove-nsIArray-from-nsIImportAddressBooks-1.patch → [checked in] 1612240-remove-nsIArray-from-nsIImportAddressBooks-1.patch
Attachment #9191757 - Attachment description: 1612240-remove-nsIArray-from-nsIImportMail-2.patch → [checked in] 1612240-remove-nsIArray-from-nsIImportMail-2.patch
Attachment #9191761 - Flags: review?(mkmelin+mozilla) → review+
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
Attachment #9191763 - Flags: review?(mkmelin+mozilla) → review+

Pushed by geoff@darktrojan.net:
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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: