Closed Bug 1612241 Opened 5 years ago Closed 5 years ago

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

Categories

(MailNews Core :: General, task)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 78.0

People

(Reporter: benc, Assigned: darktrojan)

References

Details

(Keywords: leave-open)

Attachments

(2 files, 1 obsolete file)

1612241-deletecards-array-1.diff
(deleted), patch
benc
: review+
mkmelin
: review+
Details | Diff | Splinter Review
1612241-booleanexpression-array-2.diff
(deleted), patch
benc
: review+
Details | Diff | Splinter Review

covers:

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

searchfox search

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

(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!

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

Assignee: benc → geoff
Status: NEW → ASSIGNED

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.

Attachment #9146696 - Flags: review?(mkmelin+mozilla)
Attachment #9146696 - Flags: review?(benc)
Comment on attachment 9146696 [details] [diff] [review] 1612241-deletecards-array-1.diff Review of attachment 9146696 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #9146696 - Flags: review?(mkmelin+mozilla) → review+
Keywords: leave-open
Target Milestone: --- → Thunderbird 78.0
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/2420430174e2 Replace idl nsIArray usage with Array<T> in nsIAbDirectory.idl. r=mkmelin
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.
Attachment #9146696 - Flags: review?(benc) → review+
Attached patch 1612241-booleanexpression-array-1.diff (obsolete) (deleted) — — Splinter Review

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.

Attachment #9147471 - Flags: feedback?(benc)
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.
Attachment #9147471 - Flags: feedback?(benc) → feedback?

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. :-)

Attachment #9147471 - Attachment is obsolete: true
Attachment #9147471 - Flags: feedback?
Attachment #9147839 - Flags: review?(benc)
Depends on: 1637918
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();
Attachment #9147839 - Flags: review?(benc) → review+
Pushed by geoff@darktrojan.net: 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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: