1612241 - Replace idl nsIArray usage with Array<T> in mailnews/addrbook/

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Ben Campbell]

covers:

mailnews/addrbook/public/nsIAbBooleanExpression.idl
mailnews/addrbook/public/nsIAbView.idl
mailnews/addrbook/public/nsIAbLDAPCard.idl
mailnews/addrbook/public/nsIAbDirectory.idl

searchfox search

Comment 1

[Geoff Lankow (:darktrojan)]

I'm making some changes in the address book that will affect this. Check with me before beginning.

Comment 2

[Ben Campbell]

(In reply to Geoff Lankow (:darktrojan) from comment #1)

I'm making some changes in the address book that will affect this. Check with me before beginning.

Sure thing. Got lots to go on with in the meantime - thanks for the heads-up!

Comment 3

[Geoff Lankow (:darktrojan)]

Doing this myself … it has annoyed me once too often!

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: 1612241-deletecards-array-1.diff

This only covers nsIAbDirectory.deleteCards, which is by far the most annoying bit. nsIAbView has gone, nsIAbLDAPCard is dead code and probably going, just leaving the nsIAbBooleanExpression bit.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9146696 [details] [diff] [review]
1612241-deletecards-array-1.diff

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

LGTM

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2420430174e2
Replace idl nsIArray usage with Array<T> in nsIAbDirectory.idl. r=mkmelin

Comment 7

[Ben Campbell]

Comment on attachment 9146696 [details] [diff] [review]
1612241-deletecards-array-1.diff

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

And a belated LGTM here :-)

::: mailnews/addrbook/src/nsAbOutlookDirectory.cpp
@@ +274,5 @@
>    nsAutoCString entryString;
>    nsMapiEntry cardEntry;
>  
>    for (i = 0; i < nbCards; ++i) {
> +    RefPtr<nsIAbCard> card = aCards[i];

There's also the fancy new C++ syntax for iterating over stuff:
```
for (auto card : aCards) {
    ...
}
```
Don't think we've got any official preference either way.

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: 1612241-booleanexpression-array-1.diff

This is for the array at nsIAbBooleanExpression.expressions, which appears to be an array of nsIAbBooleanConditionStrings and nsIAbBooleanExpressions, so I'm just calling it nsISupports and querying the interface where necessary.

But I'm stuck. In nsAbDirectoryQuerySimpleBooleanExpression::SetExpressions where I've commented out some lines, I don't know how to make that work. Also nsAbBoolExprToLDAPFilter::FilterExpression refuses to compile and I don't know what to do about that either.

Comment 9

[Ben Campbell]

Comment on attachment 9147471 [details] [diff] [review]
1612241-booleanexpression-array-1.diff

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

::: mailnews/addrbook/src/nsAbDirectoryQuery.cpp
@@ -74,5 @@
> -
> -  for (uint32_t i = 0; i < count; ++i) {
> -    queryExpression = do_QueryElementAt(aExpressions, i, &rv);
> -    if (NS_FAILED(rv)) return NS_ERROR_ILLEGAL_VALUE;
> -  }

This is just ensuring that every item in aExpressions is actually ansIAbBooleanConditionString, bailing out if any aren't.
So you could just do the same check - keep the queryExpression declaration from the old code and make sure it can be QIed.

But this check kind of implies that expressions attr can only ever hold ansIAbBooleanConditionString... which _might_ mean the idl definition could be the simpler:
attribute Array<nsIAbBooleanConditionString> expressions;

and avoid all the tedious faffing about with QIing from ISupports.
But I haven't confirmed it - from what you said, it seems other implementations store other types in there... sigh.
I can take a closer look at it tomorrow, if you like.

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1612241-booleanexpression-array-2.diff

I've figured out the problems I was having. I'm not entirely confident in what I'm doing in some places, so don't skim over it too quickly. :-)

Comment 11

[Ben Campbell]

Comment on attachment 9147839 [details] [diff] [review]
1612241-booleanexpression-array-2.diff

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

Sorry about the slow response, but I wanted to take a look through the expression code. It's frustrating there's no base expression interface so we could ditch the reliance on ISupports (but hey, out of scope for this).

Looks solid to me.
I'd recommend using the shorter array cloning syntax, but what you've got looks just fine anyway.

::: mailnews/addrbook/src/nsAbBoolExprToLDAPFilter.cpp
@@ -53,5 @@
>     */
>  
>    if (count == 1) {
> -    nsCOMPtr<nsIAbBooleanConditionString> childCondition(
> -        do_QueryElementAt(childExpressions, 1, &rv));

Hmm. Wonder why this was ever 1...

::: mailnews/addrbook/src/nsAbBooleanExpression.cpp
@@ +93,1 @@
[EDIT: oops - this was an array copy loop. I was trying to quote the whole block, but it just got the closing bracket!]
>    }

I think this is equivalent to:

mExpressions = aExpressions.Clone()

(it used to be that you could just do "mExpressions = aExpressions", but that's been disabled recently. Clone() will, in theory create a redundant temporary array, but nsTArray<> is clever enough to optimise that away).

@@ +98,5 @@
> +    const nsTArray<RefPtr<nsISupports>> &aExpressions) {
> +  mExpressions.Clear();
> +  for (auto expression : aExpressions) {
> +    mExpressions.AppendElement(expression);
> +  }

same here.
aExpressions = mExpressions.Clone();

::: mailnews/addrbook/src/nsAbDirectoryQuery.cpp
@@ +56,1 @@
>    }

aExpressions = mExpressions.Clone();

@@ +70,5 @@
>    // Values ok, so we can just save and return.
> +  mExpressions.Clear();
> +  for (auto expression : aExpressions) {
> +    mExpressions.AppendElement(expression);
> +  }

mExpressions = aExpressions.Clone();

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c2aeb710f211
Fix post-landing review comment. r=benc
https://hg.mozilla.org/comm-central/rev/b49e6cda4e01
Replace idl nsIArray usage with Array<T> in nsIAbBooleanExpression.idl. r=benc