1562158 - remove [array] use in xpidl in some of Mailnews (more work in individual bugs)

Status: RESOLVED FIXED

(MailNews Core :: Backend, task)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1551704 +++
Per bug 1551704, replace usage of [array] from mailnews/

These:

• https://searchfox.org/comm-central/search?q=retval%2C+array%2C+size_is&case=false&regexp=false&path=mailnews%2F
• https://searchfox.org/comm-central/search?q=array%2C+size_is(aCount)%2C+retval&case=false&regexp=false&path=mailnews%2F
• https://searchfox.org/comm-central/search?q=%5Barray%2C+size_is(aCount)%5D&case=false&regexp=false&path=mailnews%2F

Might be best with one patch per interface.

Comment 1

[Ben Campbell]

Attachment: 1562158-part-one-nsIMsgDBView-getIndicesForSelection-1.patch [landed in comment #5]

First installment. Just one method in this one - seemed like enough change to justify it's own patch.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086565 [details] [diff] [review]
1562158-part-one-nsIMsgDBView-getIndicesForSelection-1.patch [landed in comment #5]

I'm glad we can start off the season of de-XPIDL-array with an easy one ;-) - I'll run the clang-format over it and that will most likely move the & in `nsTArray<nsMsgViewIndex>& indices` since it will maintain the prevalent style in the file.

Comment 3

[Ben Campbell]

Attachment: 1562158-part-two-nsIMsgDBView-getSelectedMsgHdrs-1.patch

Sticking with the one-patch-per-method approach here - let me know if you'd prefer to see the smaller ones bundled together.

I'm a little uncertain on the use of RefPtr<> on interface classes (eg RefPtr<nsIMsgDBHdr> - the xpcom docs say it's OK, but seem to hint that nsCOMPtr<> is more appropriate if it's not a concrete class). But I'm just going on the examples I found in M-C. And it seems sound so far.

Comment 4

[Ben Campbell]

Attachment: 1562158-part-three-nsIMsgDBView-getURIsForSelection-1.patch [landed in comment #12]

I went with returning URIs as UTF-8 strings on this one, which seems to be the convention elsewhere.
(that said, going by M-C examples I think it should probably be returning nsIURI instead, but that'd be a more intrusive change).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/258ad57c7aeb
Remove [array] use in xpidl for nsIMsgDBView::getIndicesForSelection(). r=jorgk

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086927 [details] [diff] [review]
1562158-part-three-nsIMsgDBView-getURIsForSelection-1.patch [landed in comment #12]

This seems like an easy win. An UTF-8 string appears like the appropriate replacement for string/char*.

I don't think we would want to swap string for a more complicated object of nsIURI. If you say "going by M-C examples" it would be great to quote those examples so the poor reviewer don't have to try to retrace your steps.

EDIT: I found a case where Boris replaced wstring with AString:
https://searchfox.org/mozilla-central/diff/d49981ee724394b41142bc22ce5efade2f81e464/dom/base/nsIDroppedLinkHandler.idl#90
So we want to handle more than ACString here, so AUTF8String is the way to go.

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086924 [details] [diff] [review]
1562158-part-two-nsIMsgDBView-getSelectedMsgHdrs-1.patch

(In reply to Ben Campbell from comment #3)
> Sticking with the one-patch-per-method approach here - let me know if you'd prefer to see the smaller ones bundled together.

Let's do one patch per interface like we started.

> I'm a little uncertain on the use of `RefPtr<>` on interface classes (eg `RefPtr<nsIMsgDBHdr>`  - the xpcom docs say it's OK, but seem to hint that `nsCOMPtr<>` is more appropriate if it's not a concrete class). But I'm just going on the examples I found in M-C. And it seems sound so far.

Sadly you haven't quoted those examples. Looking through the dependents of bug 1509981, I can see lots of string arrays being replaced, but so far, I haven't seen anything more complicated. Have you?

Yes, there are a few nsTArray<RefPtr<>> constructs involving interface classes:
https://searchfox.org/mozilla-central/search?q=nsTArray%3CRefPtr%3C.*I.*%3E&case=true&regexp=true&path=

but here are also nsTArray<nsCOMPtr<>> constructs:
https://searchfox.org/mozilla-central/search?q=nsTArray%3CnsCOMPtr%3C.*I.*%3E&case=true&regexp=true&path=

There are also COMArrays.

Anyway, we can ask Boris, if in doubt :-)

Hi Boris, we're doing the XPIDL array replacements here. Could you please look at this small patch and give us your feedback. Hmm, he's away until the 25th, but we can progress with other stuff.

Comment 8

[:aceman]

I also feel nsCOMPtr<> is the right thing for the nsI* interface classes.

Comment 9

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #6)

I don't think we would want to swap string for a more complicated object of
nsIURI.

No, I don't think so either. Just wanted to throw it out there to make sure I wasn't missing something.
(rest of this comment is just my line of thinking - not really relevant to this bug).

If you say "going by M-C examples" it would be great to quote those
examples so the poor reviewer don't have to try to retrace your steps.

Point taken ;-)
What I meant was that I took a look through the M-C interfaces to see what string types they used for URIs ($ find . -name "*.idl" -exec egrep -H "get.*URI" {} \;), and it seemed like they tend to use nsIURI rather than plain strings.
eg:

./toolkit/components/places/nsITaggingService.idl:  Array<AString> getTagsForURI(in nsIURI aURI);

I think the M-C nsIURI-based classes are all more-or-less just plain strings, with methods for easier manipulation of various parts.
Whereas most of the C-C URI classes have extra request state stuff (listeners etc). I think it probably makes sense to aspire to eventually move that stateful stuff out of the C-C URIs, (and use nsIURIs instead of strings), but that's a great big separate issue.

Comment 10

[Jorg K (CEST = GMT+2)]

Hmm, the example you've presented doesn't do an array of nsIURI to prove your point. Having suffered through all the URI upheavals in the last few years, I wouldn't subscribe to the "more-or-less just plain strings" either. There's a whole lot more to it under the covers, the spec string or its parts are just one part of it. But anyway, this is more-or-less unrelated discussion.

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1562158-part-two-nsIMsgDBView-getSelectedMsgHdrs-1.patch (rebased) [landed in comment #15]

I'll land part 3 first, so this is part 2 rebased. Boris, this one is the one we'd like some feedback on.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5a06fc688244
Remove [array] use in xpidl for nsIMsgDBView::getURIsForSelection(). r=jorgk DONTBUILD

Comment 13

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #7)

Sadly you haven't quoted those examples. Looking through the dependents of
bug 1509981, I can see lots of string arrays being replaced, but so far, I
haven't seen anything more complicated. Have you?

I haven't checked all the patches in the Bug 1509981 dependents, but the non-string ones I did find all seemed to be arrays of RefPtr<>.
eg:

In Bug 1558653 (xpcom/base/nsConsoleService.cpp:
nsConsoleService::GetMessageArray() ends up as

NS_IMETHODIMP
nsConsoleService::GetMessageArray(
nsTArray<RefPtr<nsIConsoleMessage>>& aMessages)

In Bug 1550930 ( https://phabricator.services.mozilla.com/D30770 )
in dom/ipc/BrowserChild.cpp, BrowserChild::RemoteDropLinks() becomes:

NS_IMETHODIMP
BrowserChild::RemoteDropLinks(
const nsTArray<RefPtr<nsIDroppedLinkItem>>& aLinks) {

There were a few others - I don't think I saw any being changed to arrays of nsCOMPtr<>.

Comment 14

[Boris Zbarsky [:bzbarsky]]

In general, you can use either RefPtr<nsIFoo> or nsCOMPtr<nsIFoo>, though the latter might generate slightly smaller code, maybe.

In the specific case of Array<nsIFoo> in xpidl, though, the declaration spit out by the header generator will have nsTArray<RefPtr<nsIFoo>>; see https://searchfox.org/mozilla-central/rev/325c1a707819602feff736f129cb36055ba6d94f/xpcom/idl-parser/xpidl/xpidl.py#451-452

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fe7a5773b9e2
Remove [array] use in xpidl for nsIMsgDBView::getSelectedMsgHdrs(). r=jorgk

Comment 16

[Ben Campbell]

Note to self: nsIAbLDAPCard [array]-removal is handled by Attachment 9090902 [details] [diff] (on Bug 1562157).

Comment 17

[Ben Campbell]

Attachment: 1562158-part-four-nsIAbLDAPAttributeMap-1.patch [landed in comment #20]

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9094428 [details] [diff] [review]
1562158-part-four-nsIAbLDAPAttributeMap-1.patch [landed in comment #20]

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

Looks OK by visual inspection. Does Leslie Alvarez still come up?

::: mailnews/addrbook/src/nsAbLDAPAttributeMap.js
@@ -38,5 @@
>      }
> -
> -    aAttrs = this.mPropertyMap[aProperty];
> -    aCount = aAttrs.length;
> -    return aAttrs;

My JS skills are somewhat limited, but that never worked, did it? It's all call by value/object, so to return something it should have been:
aCount.value = aAttrs.length;
like in getAllCardProperties() below.
And that aAttrs = this.mPropertyMap[aProperty]; was totally meaningless since the function returned it. Or am I missing something?

::: mailnews/addrbook/src/nsAbLDAPCard.cpp
@@ +100,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    nsAutoCString attr;
>    nsCString propvalue;
> +  for (const nsCString &prop : props) {

That looks pretty modern ;-)

Comment 19

[Jorg K (CEST = GMT+2)]

And that aAttrs = this.mPropertyMap[aProperty]; was totally meaningless since the function returned it. Or am I missing something?

Well, I meant to say: Assigning that to a function parameter was meaningless. It should have been a local variable.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/781d4961a514
remove xpidl [array] use from nsIAbLDAPAttributeMap. r=jorgk

Comment 21

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #18)

Comment on attachment 9094428 [details] [diff] [review]
1562158-part-four-nsIAbLDAPAttributeMap-1.patch [landed in comment #20]
Looks OK by visual inspection. Does Leslie Alvarez still come up?

Oh, good point. I just tried it and it works fine.
(And if Prof. Alvarez happens to stumble upon this: apologies, but we just ended up randomly settling on your name as a test!)

::: mailnews/addrbook/src/nsAbLDAPAttributeMap.js
@@ -38,5 @@

 }

• aAttrs = this.mPropertyMap[aProperty];
• aCount = aAttrs.length;
• return aAttrs;

My JS skills are somewhat limited, but that never worked, did it? It's all
call by value/object, so to return something it should have been:
aCount.value = aAttrs.length;
like in getAllCardProperties() below.
And that aAttrs = this.mPropertyMap[aProperty]; was totally meaningless
since the function returned it. Or am I missing something?

Yes, I think you're right. The original code looks pretty suspect without a aCount.value = ....

::: mailnews/addrbook/src/nsAbLDAPCard.cpp

• for (const nsCString &prop : props) {
  That looks pretty modern ;-)

Heh - I think that's the first time I've used that in real code. Give it another 20 years and there'll be a tight little core of C++ which is nice and terse and easy to read :- )

Comment 22

[Ben Campbell]

Attachment: 1562158-part-five-nsIMsgTagService-1.patch [landed in comment #62]

This one lightly touches a whole bunch of files, but the biggest change is to the tag sorting. Added some unit test coverage to go with it.
I'm pretty happy with it, but I saw some test fails locally (which I don't think were related), so I'll sync up and do a try run before setting r? on it.

Comment 23

[Ben Campbell]

Comment on attachment 9094502 [details] [diff] [review]
1562158-part-five-nsIMsgTagService-1.patch [landed in comment #62]

Try run is looking OK ( https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1afe33c21550cc419dcdc06487267be9b7f665c7 ).

Comment 24

[Ben Campbell]

Attachment: 1562158-part-six-nsIMsgSearchCustomTerm-1.patch [landed in comment #26]

A nice simple one.

Comment 25

[:aceman]

Comment on attachment 9095070 [details] [diff] [review]
1562158-part-six-nsIMsgSearchCustomTerm-1.patch  [landed in comment #26]

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

::: suite/mailnews/content/mailWidgets.xml
@@ +1099,3 @@
>                return [Ci.nsMsgSearchOp.Contains];
>              }
>              return this.validityTable.getAvailableOperators(this.searchAttribute,length);

Looks like this one can go next and then the 'length' object can be removed.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8c526a01fa75
Remove xpidl [array] use from nsIMsgSearchCustomTerm. r=mkmelin DONTBUILD

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 9095070 [details] [diff] [review]
1562158-part-six-nsIMsgSearchCustomTerm-1.patch  [landed in comment #26]

Nice that you leave the harder reviews to me ;-) - I'll get to it eventually.

Comment 28

[Ben Campbell]

Attachment: 1562158-part-seven-nsIMsgSearchValidityTable-1.patch

(In reply to :aceman from comment #25)

Comment on attachment 9095070 [details] [diff] [review]
1562158-part-six-nsIMsgSearchCustomTerm-1.patch [landed in comment #26]
::: suite/mailnews/content/mailWidgets.xml

         return this.validityTable.getAvailableOperators(this.searchAttribute,length);

Looks like this one can go next and then the 'length' object can be removed.

Indeed so! I don't want Jorg to feel left out, but I figured you might like to review this one :-)

Comment 29

[Ben Campbell]

Attachment: 1562158-part-eight-nsIMsgTraitService-1.patch

Comment 30

[Ben Campbell]

Attachment: 1562158-part-nine-nsIImapMailFolderSink-1.patch

A special one, just for you, Jorg :-)
The original code took me a while to decipher.
In nsImapProtocol::ProcessMailboxUpdate() it looked like msgIdList was going to cause either a memory leak or a use-after-free, but it turns out it is actually recycled for a slightly different purpose (the call to WaitForPotentialListOfBodysToFetch() sets it to a new value which doesn't need freeing).
So I tried to segregate the second use. But it could definitely do with a second set of eyes - I'm always a little wary of the IMAP code.

I did run a try build with these patches which I think looks OK:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=2f2a1f55d6e53533aa740525401e68a47805744c
(There is a Linux x64 opt test timeout which I think is unrelated - that test runs fine for me locally).

(and no great rush on these patches, by the way - they're mostly pretty independent of each other and I'll just keep plugging away at them. I think there's another 7 or so to go.)

Comment 31

[:aceman]

Comment on attachment 9095406 [details] [diff] [review]
1562158-part-seven-nsIMsgSearchValidityTable-1.patch

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

::: mailnews/base/search/src/nsMsgSearchAdapter.cpp
@@ +773,5 @@
>  
>    // count first
>    uint32_t totalAttributes = 0;
>    int32_t i, j;
>    for (i = 0; i < nsMsgSearchAttrib::kNumMsgSearchAttributes; i++) {

Is this loop still needed? I assume nsTArray grows automatically on .AppendElement().
I understand it was useful to avoid manual reallocating with moz_xmalloc(), but probably not with nsTArray.
Can you see how many elements do we talk here? At most nsMsgSearchAttrib::kNumMsgSearchAttributes ?
That would be 100, so in that case the counting seems useful.
What about just setting aResult.SetCapacity(nsMsgSearchAttrib::kNumMsgSearchAttributes)? Would that be too wasteful? :)

@@ +781,5 @@
>          break;
>        }
>      }
>    }
> +  aResult.SetCapacity(totalAttributes);

SetCapacity can fail so some check for the out of memory case could be in order. The original code had one.

@@ -797,5 @@
>        }
>      }
>    }
> -
> -  NS_ASSERTION(totalAttributes == numStored, "Search Attributes not lining up");

I wonder what this check actually achieved. When could those variables differ? m_table changing between the 2 loops over it?

@@ +810,5 @@
>    int32_t i;
>    for (i = 0; i < nsMsgSearchOp::kNumMsgSearchOperators; i++) {
>      if (m_table[attr][i].bitAvailable) totalOperators++;
>    }
> +  aResult.SetCapacity(totalOperators);

The same comments here.

Comment 32

[Jorg K (CEST = GMT+2)]

Comment on attachment 9095411 [details] [diff] [review]
1562158-part-eight-nsIMsgTraitService-1.patch

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

I think this can be improved.

::: mailnews/base/test/unit/test_nsMsgTraitService.js
@@ +57,5 @@
>    Assert.ok(ts.getEnabled(proId));
>    ts.setAntiId(proId, antiId);
>    Assert.equal(antiId, ts.getAntiId(proId));
> +  let proArray = [];
> +  let antiArray = [];

Why this change? The function assigns to .value, so it's more an object than an array.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +2096,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    rv = aJunkMailPlugin->ClassifyTraitsInMessage(
> +      aURI, proIndices.Length(), proIndices.Elements(), antiIndices.Elements(),
> +      this, aMsgWindow, this);

Can that be refactored more? Can you pass the array? I builds an array anyway, so why not use the one you have?
https://searchfox.org/comm-central/rev/a8c6db8009b437b362eb2087adaf42b1e5477ef5/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp#1795

@@ -2098,5 @@
>  
>    rv = aJunkMailPlugin->ClassifyTraitsInMessage(
> -      aURI, count, proIndices, antiIndices, this, aMsgWindow, this);
> -  free(proIndices);
> -  free(antiIndices);

Strange to see this free. Where was that originally allocated?

@@ +2118,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  rv = aJunkMailPlugin->ClassifyTraitsInMessages(
> +      aURICount, aURIArray, proIndices.Length(), proIndices.Elements(),
> +      antiIndices.Elements(), this, aMsgWindow, this);

And here.

Comment 33

[Ben Campbell]

(In reply to :aceman from comment #31)

Comment on attachment 9095406 [details] [diff] [review]
1562158-part-seven-nsIMsgSearchValidityTable-1.patch
::: mailnews/base/search/src/nsMsgSearchAdapter.cpp
@@ +773,5 @@

// count first
uint32_t totalAttributes = 0;
int32_t i, j;
for (i = 0; i < nsMsgSearchAttrib::kNumMsgSearchAttributes; i++) {

Is this loop still needed? I assume nsTArray grows automatically on
.AppendElement().
I understand it was useful to avoid manual reallocating with moz_xmalloc(),
but probably not with nsTArray.
Can you see how many elements do we talk here? At most
nsMsgSearchAttrib::kNumMsgSearchAttributes ?
That would be 100, so in that case the counting seems useful.
What about just setting
aResult.SetCapacity(nsMsgSearchAttrib::kNumMsgSearchAttributes)? Would that
be too wasteful? :)

The Commodore 64 programmer in me shudders at the breathtaking extravagance of allocating 400 bytes we might not need :-)

I guess I ended up just trying to emulate the original code as closely as possible.
I have no real feel for what a typical ballpark figure is likely to be for the count (I just don't know enough about how the code is used, and that's a rabbit-hole I'd like to avoid diving into just now).

Stepping back, I'd be surprised if this code was ever on a performance-critical path, so I think I lean toward removing the first loop, ditching SetCapacity() altogether and keeping the code as simple as possible. And just sucking up whatever extra allocations that entails (nsTArray grows by doubling).

What do you think?

@@ +781,5 @@

     break;
   }
 }

}

• aResult.SetCapacity(totalAttributes);

SetCapacity can fail so some check for the out of memory case could be in
order. The original code had one.

No, nsTArray is an infallible collection by default (it'll just abort when memory allocations fail). There is a success bool returned, but it's only if you pass in a second param to specify a fallible allocation (ie ones where you've got a proper path to recovery).

• NS_ASSERTION(totalAttributes == numStored, "Search Attributes not lining up");

I wonder what this check actually achieved. When could those variables
differ? m_table changing between the 2 loops over it?

My reading of it was just being defensive/paranoid that logic of the two loops stayed consistent.
I don't think there are any nefarious threading implications in play here.

Comment 34

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #32)

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +2096,5 @@

NS_ENSURE_SUCCESS(rv, rv);

rv = aJunkMailPlugin->ClassifyTraitsInMessage(

•  aURI, proIndices.Length(), proIndices.Elements(), antiIndices.Elements(),
  

•  this, aMsgWindow, this);
  

Can that be refactored more? Can you pass the array? I builds an array
anyway, so why not use the one you have?

Definitely. But I was going to leave that for the comm/mailnews/base/search/public/nsIMsgFilterPlugin.idl patch (which is where classifyTraitsInMessage() is defined).

https://searchfox.org/comm-central/rev/
a8c6db8009b437b362eb2087adaf42b1e5477ef5/mailnews/extensions/bayesian-spam-
filter/src/nsBayesianFilter.cpp#1795

@@ -2098,5 @@

rv = aJunkMailPlugin->ClassifyTraitsInMessage(

•  aURI, count, proIndices, antiIndices, this, aMsgWindow, this);
  

• free(proIndices);
• free(antiIndices);

Strange to see this free. Where was that originally allocated?

By the call to GetEnabledIndices(), a few lines above. Caller is responsible for freeing the returned data. Ugh.
I have to say, it's a real pleasure going through and replacing this kind of stuff with nsTArray<> instead!

Comment 35

[Jorg K (CEST = GMT+2)]

OK, we can take it then. Just clarify the {} vs [] bit.

Comment 36

[:aceman]

Thanks, if nsTArray grows by doubling, then there are at most 6 reallocs, that should be fine and we can drop the counting.

Comment 37

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #32)

Comment on attachment 9095411 [details] [diff] [review]
1562158-part-eight-nsIMsgTraitService-1.patch
::: mailnews/base/test/unit/test_nsMsgTraitService.js
@@ +57,5 @@

Assert.ok(ts.getEnabled(proId));
ts.setAntiId(proId, antiId);
Assert.equal(antiId, ts.getAntiId(proId));

• let proArray = [];
• let antiArray = [];

Why this change? The function assigns to .value, so it's more an object than
an array.

That was me screwing it up and completely missing the .value, both in this unit test and in the getEnabledIndices() implementation.
And because they were both wrong, the test still worked. Sigh.

Magnus: Style call. What is the proper way to handle multiple return values in IDL interfaces?
getEnabledIndices() needs to return two arrays.
https://searchfox.org/comm-central/search?q=getEnabledIndices&path=

The old xpidl system used object parameters to return the arrays (by setting the .value field).

1. I could ensure the two params passed in are arrays, carefully clear them (without losing the reference), and fill them up with the returned data. This is the current approach (with objects), but seems a little fragile (accidentally passing in an object rather than an array would likely screw it up).
2. I could return an array containing the data (but could be a bit cumbersome to use on the C++ side...), ie

const { pro,anti } = getEnabledAttrs()

3. split the interface up into two separate calls (getEnabledProAttrs()/getEnabledAntiAttrs()).

2 seems like the most 'javascripty' way, but I it's only ever called from the C++ side, so 3 might be a better plan...

Comment 38

[Magnus Melin [:mkmelin]]

Yeah agreed #3 makes most sense (just split it up.)

Comment 39

[Ben Campbell]

Attachment: 1562158-part-seven-nsIMsgSearchValidityTable-2.patch

New version with the separate counting loop removed.
aceman: No other changes to the patch but I figured there was enough scope there for me to have missed something, hence the new r?.

Comment 40

[:aceman]

Comment on attachment 9097152 [details] [diff] [review]
1562158-part-seven-nsIMsgSearchValidityTable-2.patch

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

Thanks, what about the GetAvailableOperators() ?

Comment 41

[Ben Campbell]

Attachment: 1562158-part-seven-nsIMsgSearchValidityTable-3.patch [landed in comment #45]

Doh!
New version, now with simplified GetAvailableOperators().

Comment 42

[Ben Campbell]

Attachment: 1562158-part-eight-nsIMsgTraitService-2.patch [landed in comment #45]

New nsIMsgTraitService patch, with getEnabledIndices() split up into two separate functions.

Comment 43

[:aceman]

Comment on attachment 9097162 [details] [diff] [review]
1562158-part-seven-nsIMsgSearchValidityTable-3.patch [landed in comment #45]

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

Thanks.

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 9097163 [details] [diff] [review]
1562158-part-eight-nsIMsgTraitService-2.patch [landed in comment #45]

Thanks, I'll get to the other stuff one day when less fires are burning hot. You owe me some reviews and NI answers, too, BTW ;-)

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3b9226ead3a9
Remove xpidl [array] use from nsIMsgSearchValidityTable. r=aceman
https://hg.mozilla.org/comm-central/rev/440d884740e7
Remove xpidl [array] use from nsIMsgTraitService. r=jorgk

Comment 46

[Jorg K (CEST = GMT+2)]

Comment on attachment 9095415 [details] [diff] [review]
1562158-part-nine-nsIImapMailFolderSink-1.patch

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

Looks OK. One question needs answering, see below. Also:
void NotifyBodysToDownload(out unsigned long keys, in unsigned long count);
That's an evil trick to get a pointer passed in on the first argument. Surely this should also be fixed, no?

::: mailnews/imap/src/nsImapProtocol.cpp
@@ -2441,4 @@
>          bool more;
>          m_imapMailFolderSink->GetMsgHdrsToDownload(
> -            &more, &m_progressExpectedNumber, &msgCount, &msgIdList);
> -        if (msgIdList) {

Why is it OK to remove this condition and not replace it with
if (msgIdList.Length() > ) {
?

Comment 47

[Ben Campbell]

Attachment: 1562158-part-nine-nsIImapMailFolderSink-2.patch [landed in comment #55]

(In reply to Jorg K (GMT+2) from comment #46)

Comment on attachment 9095415 [details] [diff] [review]
1562158-part-nine-nsIImapMailFolderSink-1.patch

void NotifyBodysToDownload(out unsigned long keys, in unsigned long count);
That's an evil trick to get a pointer passed in on the first argument.
Surely this should also be fixed, no?

Oh yes - I hadn't spotted that particular atrocity!
Fixed in this new patch.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ -2441,4 @@

     bool more;
     m_imapMailFolderSink->GetMsgHdrsToDownload(

•        &more, &m_progressExpectedNumber, &msgCount, &msgIdList);
  

•    if (msgIdList) {
  

Why is it OK to remove this condition and not replace it with
if (msgIdList.Length() > ) {
?

Good point. Now fixed.
(oddly, I remember considering that one - no idea why I came down on the side of "break it"!)

Thanks for that - those two could've been nasty.

Comment 48

[Ben Campbell]

Attachment: 1562158-part-ten-nsIMsgHeaderParser-1.patch

Comment 49

[Ben Campbell]

Attachment: 1562158-part-eleven-msgIStructuredHeaders-1.patch [landed in comment #56]

A couple more mixed JS/C++ stand-alone patches. Just stashing these here for the weekend. They're ready to review if anyone wants 'em, but they can wait till next week.

Comment 50

[Ben Campbell]

Comment on attachment 9098731 [details] [diff] [review]
1562158-part-eleven-msgIStructuredHeaders-1.patch [landed in comment #56]

(Sorry Magnus, didn't intend the r? - figured you'd have enough else to wade through before being away next week!)

Comment 51

[Magnus Melin [:mkmelin]]

Comment on attachment 9098731 [details] [diff] [review]
1562158-part-eleven-msgIStructuredHeaders-1.patch [landed in comment #56]

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

LGTM
No need to wait to set flags. I always have things in queue...

Comment 52

[Jorg K (CEST = GMT+2)]

Part nine and ten assume that fixArray() has two parameters when it still has three. Did you forget to upload part 8.5 or am I missing something?

Comment 53

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #52)

Part nine and ten assume that fixArray() has two parameters when it still has three. Did you forget to upload part 8.5 or am I missing something?

No, the third param (count) is optional. I'm aiming to get rid of it, but there are still a couple of places where it's used, so it'll get handled in a later patch.

Comment 54

[Jorg K (CEST = GMT+2)]

Comment on attachment 9098727 [details] [diff] [review]
1562158-part-nine-nsIImapMailFolderSink-2.patch [landed in comment #55]

Thanks for the additional changes, and sorry about the delays.

Comment 55

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5fcde2aba579
Remove xpidl [array] use from nsIImapMailFolderSink. r=jorgk

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f47342bf3567
Remove xpidl [array] use from msgIStructuredHeaders. r=mkmelin DONTBUILD

Comment 57

[Jorg K (CEST = GMT+2)]

Comment on attachment 9098729 [details] [diff] [review]
1562158-part-ten-nsIMsgHeaderParser-1.patch

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

Looks OK, one nit and some remarks. BTW, Aceman and I last messed with this in bug 1390337, so I'm qualified ;-)

Doing this review instead of the one I'm meant to do is of course a procrastinator's strategy, but I'll get to it.

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +370,5 @@
>              if (NS_SUCCEEDED(rv) && !undisclosedRecipients.IsEmpty()) {
>                nsCOMPtr<nsIMsgHeaderParser> headerParser(
>                    mozilla::services::GetHeaderParser());
>                nsCOMPtr<msgIAddressObject> group;
> +              nsTArray<RefPtr<msgIAddressObject>> empty;

In some other patch you had: nsTArray<nsMsgKey> noBodies;
So maybe better: noRecipients?

::: mailnews/mime/src/MimeHeaderParser.cpp
@@ +76,4 @@
>    MOZ_ASSERT(NS_SUCCEEDED(rv), "Javascript jsmime returned an error!");
> +  if (NS_SUCCEEDED(rv) && addresses.Length() > 0) {
> +    retval.SetCapacity(addresses.Length());
> +    for (auto &addr : addresses) {

Wow, so modern ;-) - But why not append them all in one hit:
retval.AppendElements(addresses);
I remember seeing that elsewhere.

@@ +97,5 @@
>    MOZ_ASSERT(NS_SUCCEEDED(rv), "This should never fail!");
> +  if (NS_SUCCEEDED(rv) && addresses.Length() > 0) {
> +    retval.SetCapacity(addresses.Length());
> +    for (auto &addr : addresses) {
> +      retval.AppendElement(addr);

See above.

@@ +116,5 @@
>    MOZ_ASSERT(NS_SUCCEEDED(rv), "This should never fail!");
> +  if (NS_SUCCEEDED(rv) && addresses.Length() > 0) {
> +    retval.SetCapacity(addresses.Length());
> +    for (auto &addr : addresses) {
> +      retval.AppendElement(addr);

And here.

Comment 58

[Jorg K (CEST = GMT+2)]

Comment on attachment 9094502 [details] [diff] [review]
1562158-part-five-nsIMsgTagService-1.patch [landed in comment #62]

Interesting :-) - Now that I had a peaceful half hour on a Sunday evening, it looked like an easy review.

Some comments:
Old code:
```
-  // if we have only one element, it wins
-  if (!element1 && !element2) return TAG_CMP_EQUAL;
-  if (!element2) return TAG_CMP_LESSER;
-  if (!element1) return TAG_CMP_GREATER;
```
Why did they think that the compare neede to handle null elements being passed? So strange.

New code: Can you give me a reference to the array Sort() function that we're using now instead of qsort(). I guess you've looked it up already (but didn't paste the reference).

Comment 59

[Ben Campbell]

Attachment: 1562158-part-ten-nsIMsgHeaderParser-2.patch [landed in comment #62]

(In reply to Jorg K (GMT+2) from comment #57)

1562158-part-ten-nsIMsgHeaderParser-1.patch
Review of attachment 9098729 [details] [diff] [review]:

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +370,5 @@

         if (NS_SUCCEEDED(rv) && !undisclosedRecipients.IsEmpty()) {
           nsCOMPtr<nsIMsgHeaderParser> headerParser(
               mozilla::services::GetHeaderParser());
           nsCOMPtr<msgIAddressObject> group;

•          nsTArray<RefPtr<msgIAddressObject>> empty;
  

In some other patch you had: nsTArray<nsMsgKey> noBodies;
So maybe better: noRecipients?

Good point. New patch, tweaked accordingly.

::: mailnews/mime/src/MimeHeaderParser.cpp
@@ +76,4 @@

MOZ_ASSERT(NS_SUCCEEDED(rv), "Javascript jsmime returned an error!");

• if (NS_SUCCEEDED(rv) && addresses.Length() > 0) {
• retval.SetCapacity(addresses.Length());
• for (auto &addr : addresses) {

Wow, so modern ;-) - But why not append them all in one hit:
retval.AppendElements(addresses);
I remember seeing that elsewhere.

retval is nsCOMArray<msgIAddressObject>, while addresses is nsTArray<RefPtr<msgIAddreessObject>>.
Your comment got me thinking that there must be some way...
I thought there might be an AppendElements(iterator begin, iterator end), which would handle it nicely... But no. There's only AppendElements(const T* elements, uint32_t count), and you can't cast an array ofRefPtr`s to an array of raw pointers.

Comment 60

[Ben Campbell]

(In reply to Jorg K (GMT+2) from comment #58)

Comment on attachment 9094502 [details] [diff] [review]
Old code:

-  // if we have only one element, it wins
-  if (!element1 && !element2) return TAG_CMP_EQUAL;
-  if (!element2) return TAG_CMP_LESSER;
-  if (!element1) return TAG_CMP_GREATER;

Why did they think that the compare neede to handle null elements being
passed? So strange.

Yeah, I wondered that too. My best guess was that it was just an artifact left over from some previous iteration. Comparator functions always give me a headache, so maybe someone else felt the same way and just left it as is, even after null elements were no longer a possibility...

New code: Can you give me a reference to the array Sort() function that
we're using now instead of qsort(). I guess you've looked it up already (but
didn't paste the reference).

It's using nsTArray<>::Sort(), which is just a wrapper around NS_QuickSort(). Seems like a slightly missed opportunity to be honest - like qsort() it treats elements as raw chunks of memory rather than being properly C++ aware (like std::sort, for example). But hey.

Comment 61

[Jorg K (CEST = GMT+2)]

Comment on attachment 9099114 [details] [diff] [review]
1562158-part-ten-nsIMsgHeaderParser-2.patch [landed in comment #62]

Thanks, for the update. Somehow you dropped the hunk in mailnews/addrbook/content/abMailListDialog.js now:
https://bugzilla.mozilla.org/attachment.cgi?oldid=9098729&action=interdiff&newid=9099114&headers=1

I'll put it back.

EDIT: Sorry, that was already taken out elsewhere.

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a9df26051fec
Remove xpidl [array] use from nsIMsgTagService. r=jorgk
https://hg.mozilla.org/comm-central/rev/471b8c1ca0f4
Remove xpidl [array] use from nsIMsgHeaderParser. r=jorgk

Comment 63

[Jorg K (CEST = GMT+2)]

Still a few left: nsIMsgTraitService.idl and some here:
https://searchfox.org/comm-central/search?q=%5Barray%2C+size_is(aCount)%5D&case=false&regexp=false&path=mailnews%2F

Comment 64

[Ben Campbell]

Attachment: 1562158-part-twelve-nsIMsgFolder-1.patch

A mostly simple one.
I ended up removing the offending getSortKey() from the interface entirely. It was only ever used internally and I thought there was a good rationale for ditching it: there's already a sortOrder attribute to allow folders tweak their relative ordering.
Internally I converted it into the helper fn BuildFolderSortKey(), and made the caller pass in the folder explicitly - the intention is to make it clear that the algorithm won't be overriden by other folder implementations (but those folders are free to override their sortOrder and name attrs).

Comment 65

[Ben Campbell]

I make it another 12 or so to go:

https://searchfox.org/comm-central/search?q=size_is%28&path=mailnews%2F

Comment 66

[:aceman]

Comment on attachment 9100366 [details] [diff] [review]
1562158-part-twelve-nsIMsgFolder-1.patch

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

Why? How do you know getSortKey() wasn't used/implemented by addons?
But I see the return argument was strange, an array of octets (individual characters of the key)? I wonder how JS used it then.
If you pass folder into BuildFolderSortKey() then it wouldn't need to be part of nsMsgDBFolder  class. Can you leave it operating on the current folder (this object), but still hide it from the idl?

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +4936,5 @@
>  
> +// Helper function for CompareSortKeys().
> +// Builds a collation key for a given folder based on "{sortOrder}{name}"
> +nsresult nsMsgDBFolder::BuildFolderSortKey(nsIMsgFolder *aFolder,
> +                                           uint32_t *aLength, uint8_t **aKey) {

It is ugly that these need to be pointers, when there is a single key, but I assume the gCollationKeyGenerator methods require them these way.

Comment 67

[Ben Campbell]

(In reply to :aceman from comment #66)

Comment on attachment 9100366 [details] [diff] [review]
1562158-part-twelve-nsIMsgFolder-1.patch

Why? How do you know getSortKey() wasn't used/implemented by addons?

Ahh. Fair point. But there was a comment in the IDL file indicating that getSortKey() probably shouldn't be used.
If the interface is changing, addons will need to be updated anyway:

• anything calling getSortKey() for sorting purposes should be using compareSortKeys() instead.
• anything implementing getSortKey() should just use sortOrder instead.

I don't like getSortKey() being exposed, but maybe I should leave it in (maybe with a deprecation comment recommending the alternatives?).

But I see the return argument was strange, an array of octets (individual
characters of the key)? I wonder how JS used it then.

It all boils down to the nsICollation interface (in M-C). It works on raw bytes and still uses the old [array] interfaces - I'm not sure of their plans there yet (Bug 1509981 comment 1). There are some other C-C interfaces which make more heavy use of nsICollation.

If you pass folder into BuildFolderSortKey() then it wouldn't need to be
part of nsMsgDBFolder class. Can you leave it operating on the current
folder (this object), but still hide it from the idl?

I originally had it as a standalone static function, but it needed gCollationKeyGenerator from the class. However, I just noticed gCollationKeyGenerator is a static member, so it can be a static function after all (just has to belong to the class).
New patch coming (as soon as I can get my build to stop crashing!)

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +4936,5 @@

+// Helper function for CompareSortKeys().
+// Builds a collation key for a given folder based on "{sortOrder}{name}"
+nsresult nsMsgDBFolder::BuildFolderSortKey(nsIMsgFolder *aFolder,

•                                       uint32_t *aLength, uint8_t **aKey) {
  

It is ugly that these need to be pointers, when there is a single key, but I
assume the gCollationKeyGenerator methods require them these way.

Yes. I'm hoping M-C is planning to upgrade nsICollation to use Array<> types. Not such a big deal for this tiny patch, but for things like nsIMsgHdr there could be performance implications if we upgrade ours first (continuously converting between raw malloc-ed buffers and nsTArray<>).

Comment 68

[Ben Campbell]

Hmm. I think it probably makes sense for the rest of these mailnews [array]-removal patches to be broken out into their own bugs. I shall do that unless anyone objects.

Comment 69

[Magnus Melin [:mkmelin]]

(In reply to Ben Campbell from comment #67)

I don't like getSortKey() being exposed, but maybe I should leave it in (maybe with a deprecation comment recommending the alternatives?).

Just take it out. We won't support old-style add-ons anymore so this is not an issue.

Comment 70

[Jorg K (CEST = GMT+2)]

Comment on attachment 9100366 [details] [diff] [review]
1562158-part-twelve-nsIMsgFolder-1.patch

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

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +4965,2 @@
>    NS_ENSURE_SUCCESS(rv, rv);
> +  rv = BuildFolderSortKey(aFolder, &sortKey2Length, &sortKey2);

Why pass this and aFolder? Just aFolder->BuildFolderSortKey() on the second one. What am I missing?

Comment 71

[Jorg K (CEST = GMT+2)]

Comment on attachment 9100366 [details] [diff] [review]
1562158-part-twelve-nsIMsgFolder-1.patch

Getting this out of my queue until comment #70 is answered.

Comment 72

[Jorg K (CEST = GMT+2)]

Comment on attachment 9100366 [details] [diff] [review]
1562158-part-twelve-nsIMsgFolder-1.patch

OK, I tried the changes I had suggested and it didn't compile:
0:07.22 c:/mozilla-source/comm-central/comm/mailnews/base/util/nsMsgDBFolder.cpp(4965,17): error: no member named 'BuildFolderSortKey' in 'nsIMsgFolder'
0:07.22   rv = aFolder->BuildFolderSortKey(&sortKey2Length, &sortKey2);

So let's go with the original patch.

Comment 73

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c5b6c5441aba
Remove xpidl [array] use from nsIMsgFolder. r=jorgk DONTBUILD

Comment 74

[Ben Campbell]

Yep, it's because BuildFolderSortKey() is on the concrete class but aFolder is a generic nsIMsgFolder (the idea being to freeze the sorting algorithm and move it out of the interface, while leaving the sortOrder attribute available for derived classes).
I think I can make BuildFolderSortKey static which will clarify it slightly, so I'll do that soon (my tree is in a slightly... deconstructed state right now while I track down some threading issues).

Comment 75

[Jorg K (CEST = GMT+2)]

I think we don't expect more patches here.