1612237 - Replace idl nsIArray usage with Array<T> in mailnews/base/search/

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Ben Campbell]

The idl files involved are:

mailnews/base/search/public/nsIMsgFilter.idl
mailnews/base/search/public/nsIMsgFilterService.idl
mailnews/base/search/public/nsIMsgSearchValidityTable.idl
mailnews/base/search/public/nsIMsgFilterCustomAction.idl

Comment 1

[Ben Campbell]

Attachment: [checked in] 1612237-remove-nsIArray-from-nsIMsgFilter.patch

A reasonably straightforward one to kick things off...

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9123590 [details] [diff] [review]
[checked in] 1612237-remove-nsIArray-from-nsIMsgFilter.patch

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

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +652,5 @@
>      // actions are async
>      bool loggingEnabled = false;
>      if (m_filters) (void)m_filters->GetLoggingEnabled(&loggingEnabled);
>  
> +    nsTArray<RefPtr<nsIMsgRuleAction>> actionList;

since it holds interfaces, should it be nsCOMPtr instaed of RefPtr? same for other places

Comment 3

[Ben Campbell]

(In reply to Magnus Melin [:mkmelin] from comment #2)

• nsTArray<RefPtr<nsIMsgRuleAction>> actionList;

since it holds interfaces, should it be nsCOMPtr instaed of RefPtr? same for
other places

Technically, probably. But the C++ mappings for xpidl is:

Array<nsIThingy>   =>  nsTArray<RefPtr<nsIThingy>>

(hmm... bugzilla markdown spuriously escaping less-than signs in code blocks?)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/596c14b19879
Remove nsIArray use from nsIMsgFilter interface. r=mkmelin

Comment 5

[Ben Campbell]

Attachment: [checked in] 1612237-nsIMsgFilterService-1.patch

try run here:

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

(just linux, but I'll be running some more comprehensive runs which include this patch)

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9154442 [details] [diff] [review]
[checked in] 1612237-nsIMsgFilterService-1.patch

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

Looks good,  r=mkmelin

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/41df9fb58b55
Remove nsIArray use from nsIMsgFilterService. r=mkmelin

Comment 8

[Ben Campbell]

Attachment: [checked in] 1612237-remove-nsIMsgSearchValidityTable.validateTerms-1.patch

Turns out that validateTerms() method in nsIMsgSearchValidityTable isn't actually being used. It's not immediately obvious because there are other validateTerms() methods in the search code.
Try run looks OK I think:

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

(I thought toolkit/components/search/tests/xpcshell/test_webextensions_startup_remove.js looked like a new orange light, but it times out for me with or without the patch).

Comment 9

[Ben Campbell]

Attachment: [checked in] 1612237-remove-nsIArray-in-nsIMsgFilterCustomAction-1.patch

And this one excises the nsIArray param in nsIMsgFilterCustomAction.apply().
(note: apply() is a really bad name to give a function that you might want to be looking for later! :-)

Another try build, same possible-spurious fail, but I think otherwise fine:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=1087d8d77b3e999e69c923e5fdcb8c3d8d13347b

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/26d3e7756730
Remove unused validateTerms() method in nsIMsgSearchValidityTable. r=mkmelin

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 9186178 [details] [diff] [review]
[checked in] 1612237-remove-nsIArray-in-nsIMsgFilterCustomAction-1.patch

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

LGTM, r=mkmelin with the above adjustment

::: mailnews/search/public/nsIMsgFilterCustomAction.idl
@@ +69,5 @@
>     * @param filterType   type of filter being applied
>     * @param msgWindow    message window
>     */
>  
> +  void apply(in Array<nsIMsgDBHdr> msgHdrs,

Agreed that's a bad name. I think we should take the opportunity to rename it now that we're changing the signature anyway. applyAction?

Comment 12

[Ben Campbell]

Attachment: [checked in] 1612237-rename-nsIMsgFilterCustomAction.apply-1.patch

This one renames apply() to applyAction().
Separate patch to simplify review - it touches a bunch of other places the previous patch doesn't.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9186533 [details] [diff] [review]
[checked in] 1612237-rename-nsIMsgFilterCustomAction.apply-1.patch

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

LGTM, r=mkmelin

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c0718957546c
Remove nsIArray use in nsIMsgFilterCustomAction.apply(). r=mkmelin
https://hg.mozilla.org/comm-central/rev/7ca679f99d8b
Rename nsIMsgFilterCustomAction.apply() to applyAction(). r=mkmelin