Open Bug 1612239 Opened 5 months ago Updated 24 days ago

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

Categories

(MailNews Core :: General, task)

task
Not set
normal

Tracking

(Not tracked)

REOPENED
Thunderbird 76.0

People

(Reporter: benc, Assigned: benc)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open)

Attachments

(12 files, 3 obsolete files)

48.49 KB, patch
mkmelin
: review+
pmorris
: review+
Details | Diff | Splinter Review
47.45 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
36.10 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
8.49 KB, patch
benc
: review+
Details | Diff | Splinter Review
33.96 KB, patch
benc
: review+
Details | Diff | Splinter Review
54.54 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
10.14 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
20.56 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
20.92 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
13.46 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
13.92 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
51.54 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review

This covers:

mailnews/base/public/nsIMsgFolderNotificationService.idl
mailnews/base/public/nsIMsgCopyService.idl
mailnews/base/public/nsIMsgPluggableStore.idl
mailnews/base/public/nsIMsgFolderListener.idl
mailnews/base/public/nsIMsgFolder.idl
mailnews/base/public/nsIMsgFolderCompactor.idl
mailnews/base/public/nsIMsgAccount.idl
mailnews/base/public/nsIMsgAccountManager.idl

searchfox query

mailnews/base/public/nsIMsgAccountManager.idl deserved its own Bug - see Bug 1614846.

Depends on: 1614846
Comment on attachment 9137950 [details] [diff] [review]
[checked in] 1612239-nsIMsgAccount-1.patch (landed in comment 5)

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

::: calendar/base/modules/utils/calItipUtils.jsm
@@ +507,5 @@
>        return null;
>      }
>  
>      let identities;
>      let actMgr = MailServices.accounts;

While we're here I'd get rid of this and use MailServices.accounts directly where needed
Attachment #9137950 - Flags: review?(paul)
Attachment #9137950 - Flags: review?(mkmelin+mozilla)
Attachment #9137950 - Flags: review+
Status: NEW → ASSIGNED
Target Milestone: --- → Thunderbird 76.0
Comment on attachment 9137950 [details] [diff] [review]
[checked in] 1612239-nsIMsgAccount-1.patch (landed in comment 5)

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

r+ for calendar changes.  Good to see all these updates.
Attachment #9137950 - Flags: review?(paul) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/588fc64ebcb0
Remove nsIArray usage in nsIMsgAccount. r=mkmelin,pmorris DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Attachment #9137950 - Attachment description: 1612239-nsIMsgAccount-1.patch → 1612239-nsIMsgAccount-1.patch (landed in comment 5)

This one kept spilling out, so in the end I converted a bunch of related interfaces in different .idl files rather than a whole .idl file at once. The ones affected here are:

nsIMsgFolderListener.msgsDeleted()
nsIMsgFolderNotificationService.notifyMsgsDeleted()
nsIMsgPluggableStore.deleteMessages()
nsIMsgPluggableStore.changeFlags()
Attachment #9141141 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9141141 [details] [diff] [review]
[checked in] 1612239-nsIArray-removal-from-msgdelete-1.patch (landed in comment 9)

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

LGTM, r=mkmelin
Attachment #9141141 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/c1a719a87041
Remove nsIArray usage from various calls related to message deletion. r=mkmelin
Attachment #9141141 - Attachment description: 1612239-nsIArray-removal-from-msgdelete-1.patch → 1612239-nsIArray-removal-from-msgdelete-1.patch (landed in comment 9)

Try build here.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7766f7b3f76d48648ec41face1c8aa552a70ea51
There's a bunch of orange, but I don't think they're related to this patch. Still maybe better to hold off a little and get a clear try run?

Attachment #9143311 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9143311 [details] [diff] [review]
[checked in] 1612239-nsIArray-removal-from-msgClassified-1.patch (landed in comment 12)

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

Looks good. The try run also looks fine (under the circumstances).

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +6885,5 @@
> +      nsMsgLabelValue label;
> +      msgDBHdr->GetLabel(&label);
> +      if (label != 0) {
> +        nsAutoCString labelStr;
> +        labelStr.AppendInt(label);

Oh the joy of pointless typedeffing (nsMsgLabelValue)...
Attachment #9143311 - Flags: review?(mkmelin+mozilla) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/e7ab0d2b7290
Remove nsIArray use in nsIMsgFolderListener.msgsClassified(). r=mkmelin

Attachment #9143311 - Attachment description: 1612239-nsIArray-removal-from-msgClassified-1.patch → 1612239-nsIArray-removal-from-msgClassified-1.patch (landed in comment 12)
Attachment #9149669 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9149670 [details] [diff] [review]
1612239-remove-nsIArray-from-msgsMoveCopyCompleted-1.patch

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

::: mailnews/db/gloda/modules/IndexMsg.jsm
@@ +2441,5 @@
>              GlodaDatastore._mapFolder(srcMsgFolder).dirtyStatus ==
>                GlodaFolder.prototype.kFolderFilthy
>            ) {
>              // Local case, just modify the destination headers directly.
> +            if (aDestMsgHdrs.length > 0) {

no need to check the length. if length is 0 it just won't loop, which is no big deal

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +4940,5 @@
> +        if (imapAction == nsIImapUrl::nsImapOnlineMove) {
> +          nsTArray<RefPtr<nsIMsgDBHdr>> hdrs;
> +          MsgHdrsToTArray(m_copyState->m_messages, hdrs);
> +          notifier->NotifyMsgsMoveCopyCompleted(true, hdrs, this, {});
> +

nit: extra empty line here

::: mailnews/test/resources/folderEventLogHelper.js
@@ +86,1 @@
>        args.push(msgHdr);

instead of looping, could just do

args.concat(aSrcMsgs);

or args.push(... aSrcMsgs);

@@ +93,1 @@
>          args.push(msgHdr);

same here
Attachment #9149670 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9149669 [details] [diff] [review]
1612239-remove-nsIArray-from-nsIMsgPluggableStore-changeKeywords-1.patch

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

::: mailnews/local/src/nsMsgBrkMBoxStore.cpp
@@ +844,5 @@
>    nsCOMPtr<nsIOutputStream> outputStream;
>    nsCOMPtr<nsISeekableStream> seekableStream;
>    int64_t restoreStreamPos;
>  
> +  if (aHdrArray.IsEmpty()) return NS_ERROR_INVALID_ARG;

would be nicer to have this first in the method

::: mailnews/local/src/nsMsgMaildirStore.cpp
@@ +1326,4 @@
>    nsCOMPtr<nsIOutputStream> outputStream;
>    nsCOMPtr<nsISeekableStream> seekableStream;
>  
> +  if (aHdrArray.IsEmpty()) return NS_ERROR_INVALID_ARG;

ditto
Attachment #9149669 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/bc7e17995e19
Remove nsIArray use from nsIMsgPluggableStore.changeKeywords(). r=mkmelin
https://hg.mozilla.org/comm-central/rev/1f1357f245c1
Remove nsIArray usage for msgsMoveCopyCompleted notification. r=mkmelin DONTBUILD

Attachment #9150018 - Attachment description: 1612239-remove-nsIArray-from-msgsMoveCopyCompleted-2.patch → [checked in] 1612239-remove-nsIArray-from-msgsMoveCopyCompleted-2.patch
Attachment #9150017 - Attachment description: 1612239-remove-nsIArray-from-nsIMsgPluggableStore-changeKeywords-2.patch → [checked in] 1612239-remove-nsIArray-from-nsIMsgPluggableStore-changeKeywords-2.patch

Replaces nsIMsgFolder.ListDescendants() with nsIMsgFolder.descendants (which is simpler and clearer - ListDescendants() appended to the passed-in array).

These two descendants-related patches are covered by this try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=39de08a7f5f31704ce13b59d4615391944a3b6aa
(There's a third patch in there for nsIFolderCompactor, but I'll hold back until these two are clear)

Attachment #9152976 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9152976 [details] [diff] [review]
[checked in] 1612239-remove-nsIMsgFolder-ListDescendants-1.patch

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

LGTM, r=mkmelin
Attachment #9152976 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9152975 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/61ac6bd14371
Remove nsIArray usage from nsIMsgFolder.descendants. r=mkmelin
https://hg.mozilla.org/comm-central/rev/e16540d657cc
Remove redundant nsIMsgFolder.ListDescendants(). r=mkmelin DONTBUILD

Attachment #9152975 - Attachment description: 1612239-remove-nsIArray-from-nsIMsgFolder-descendants-1.patch → [checked in] 1612239-remove-nsIArray-from-nsIMsgFolder-descendants-1.patch
Attachment #9152976 - Attachment description: 1612239-remove-nsIMsgFolder-ListDescendants-1.patch → [checked in] 1612239-remove-nsIMsgFolder-ListDescendants-1.patch

The logic in nsFolderCompactState::CompactFolders() was a little tricky to follow, but I've tried to clarify it a bit in the patch. Basically there are three cases:

  • folders only
  • folders and offline folders (the offline folders are saved in m_offlineFolderArray and a new nsFolderCompactState instance is spawned to process them when the other folders have finished).
    -offline folders only (m_compactingOfflineFolders is set to true )
Attachment #9153327 - Flags: review?(mkmelin+mozilla)

Oh, forgot to add: the nsIMsgFolderCompactor patch was included in the previous try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=39de08a7f5f31704ce13b59d4615391944a3b6aa

Comment on attachment 9153327 [details] [diff] [review]
[checked in] 1612239-remove-nsIArray-from-nsIMsgFolderCompactor-1.patch

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

Looks ok to me. r=mkmelin
Attachment #9153327 - Flags: review?(mkmelin+mozilla) → review+
Whiteboard: [to land: 1612239-remove-nsIArray-from-nsIMsgFolderCompactor-1.patch]

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/2db662df8f37
Remove nsIArray usage from nsIMsgFolderCompactor. r=mkmelin

Attachment #9153327 - Attachment description: 1612239-remove-nsIArray-from-nsIMsgFolderCompactor-1.patch → [checked in] 1612239-remove-nsIArray-from-nsIMsgFolderCompactor-1.patch

Try run here, which looks good:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8fbecff076633fa09255c5b66355be20b0be5a7f

This one took a whole heap of debugging. It broke lots of tests, and I'm not entirely sure what change got it working. I was looking at the wrong bug a lot of the time - turns out I'd triggered a pre-existing issue where copying an empty folder never finished (nsMsgLocalMailFolder::CopyFolderAcrossServer() was notifying on the wrong folder). I think somewhere while I was being misled on that one, I accidentally fixed whatever it was I'd broken. Sigh.

Long story short: Keep an eye out, the nsIMessageCopyService code seems to have some fragility to it...

Attachment #9153723 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9153723 [details] [diff] [review]
1612239-remove-nsIArray-from-nsIMsgCopyService-copyFolders-1.patch

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

::: mailnews/base/src/nsMsgCopyService.cpp
@@ +486,4 @@
>    nsCOMPtr<nsIMsgFolder> curFolder;
>    nsCOMPtr<nsISupports> support;
>  
> +  MOZ_ASSERT(folders.Length() == 1);  // Can only copy one folder.

Why are we passing in an array then?
At the very least this should throw a runtime error instead - it's accessible from js.
Attachment #9153723 - Flags: review?(mkmelin+mozilla)

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

::: mailnews/base/src/nsMsgCopyService.cpp
@@ +486,4 @@

nsCOMPtr<nsIMsgFolder> curFolder;
nsCOMPtr<nsISupports> support;

  • MOZ_ASSERT(folders.Length() == 1); // Can only copy one folder.

Why are we passing in an array then?

I had enough grief with this patch that I figured that could be handled in another bug (I've just written it up - Bug 1643208).

At the very least this should throw a runtime error instead - it's
accessible from js.

Good point. New patch returns NS_ERROR_INVALID_ARG if the array isn't just a single folder.

Attachment #9153723 - Attachment is obsolete: true
Attachment #9154055 - Flags: review?(mkmelin+mozilla)

This one is pretty self contained and shouldn't clash with other patches in this series.
Try run looks OK:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d2a5f0935ad717696232a205b90fff053fbd0fc0

Attachment #9154060 - Flags: review?(mkmelin+mozilla)
Attachment #9154060 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9154055 - Flags: review?(mkmelin+mozilla) → review+
Whiteboard: [to land: 1612239-remove-nsIArray-from-nsIMsgFolderCompactor-1.patch]
Attachment #9143311 - Attachment description: 1612239-nsIArray-removal-from-msgClassified-1.patch (landed in comment 12) → [checked in] 1612239-nsIArray-removal-from-msgClassified-1.patch (landed in comment 12)
Attachment #9141141 - Attachment description: 1612239-nsIArray-removal-from-msgdelete-1.patch (landed in comment 9) → [checked in] 1612239-nsIArray-removal-from-msgdelete-1.patch (landed in comment 9)
Attachment #9137950 - Attachment description: 1612239-nsIMsgAccount-1.patch (landed in comment 5) → [checked in] 1612239-nsIMsgAccount-1.patch (landed in comment 5)

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/4f8d3dff196e
Remove nsIArray use from nsIMsgCopyService.copyFolders. r=mkmelin
https://hg.mozilla.org/comm-central/rev/c74497b57d8c
Remove nsIArray usage from nsIMsgPluggableMailStore.copyMessages. r=mkmelin

Comment on attachment 9154527 [details] [diff] [review]
[checked in] 1612239-remove-nsIArray-from-nsIMsgFolder-getFoldersWithFlags-1.patch

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

Looks good. r=mkmelin
Attachment #9154527 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9154055 - Attachment description: 1612239-remove-nsIArray-from-nsIMsgCopyService-copyFolders-2.patch → [checked in] 1612239-remove-nsIArray-from-nsIMsgCopyService-copyFolders-2.patch
Attachment #9154060 - Attachment description: 1612239-remove-nsIArray-from-nsIMsgPluggableStore-copyMessages-1.patch → [checked in] 1612239-remove-nsIArray-from-nsIMsgPluggableStore-copyMessages-1.patch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/d146b4e2061c
Remove nsIArray usage from nsIMsgFolder.getFoldersWithFlags(). r=mkmelin

Attachment #9154527 - Attachment description: 1612239-remove-nsIArray-from-nsIMsgFolder-getFoldersWithFlags-1.patch → [checked in] 1612239-remove-nsIArray-from-nsIMsgFolder-getFoldersWithFlags-1.patch

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b36d20f09a11e8a3af3076d85ba00c5da913613b

(I originally thought it was breaking a mochitest, but there's another try run without this patch which does the same)

Attachment #9156605 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9156605 [details] [diff] [review]
[checked in] 1612239-remove-nsIArray-from-nsIMsgFolder-markMessagesRead-et-al-1.patch

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

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ -8392,5 @@
>  
>  NS_IMETHODIMP
> -nsImapMailFolder::SetJunkScoreForMessages(nsIArray *aMessages,
> -                                          const nsACString &aJunkScore) {
> -  NS_ENSURE_ARG(aMessages);

isn't this still needed, if one pass in null from js?
Attachment #9156605 - Flags: review?(mkmelin+mozilla) → review+

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

isn't this still needed, if one pass in null from js?

No, on the C++ side it's a nsTArray<> reference now, so you can't pass in null (well, obviously you still pass null in from JS, but I'd guess the bindings either throw an error or pass it on as an empty array to the C++ side).

Anyway, there is only one JS use, and it always passes in an array containing exactly one message.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/93610e507d37
Remove nsIArray use in nsIMsgFolder markMessagesRead, markMessagesFlagged, setLabelForMessages, setJunkScoreForMessages. r=mkmelin

Attachment #9156605 - Attachment description: 1612239-remove-nsIArray-from-nsIMsgFolder-markMessagesRead-et-al-1.patch → [checked in] 1612239-remove-nsIArray-from-nsIMsgFolder-markMessagesRead-et-al-1.patch

Ah yes, passing in null will just give
NS_ERROR_XPC_CANT_CONVERT_PRIMITIVE_TO_ARRAY: Cannot convert primitive JavaScript value into an array

You need to log in before you can comment on or make changes to this bug.