Closed Bug 1594877 Opened 4 years ago Closed 4 years ago

Remove xpidl [array] use in nsIMessenger

Categories

(MailNews Core :: Backend, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 72.0

People

(Reporter: benc, Assigned: benc)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Summary: remove xpidl [array] use in nsIMessenger → Remove xpidl [array] use in nsIMessenger
Assignee: nobody → benc
No longer blocks: 1562158
Blocks: 1551704

One of the bigger ones. Lots of char* => string class replacements rippling on down the layers.

Attachment #9109365 - Flags: review?(jorgk)
Comment on attachment 9109365 [details] [diff] [review]
1594877-remove-xpidl-array-in-nsimessenger-1.patch

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

Looks OK, a few tweak still required.

::: mailnews/base/src/nsMessenger.cpp
@@ +573,3 @@
>    NS_ENSURE_ARG_POINTER(aDestFolder);
> +  MOZ_ASSERT(aContentTypeArray.Length() == aUrlArray.Length() ==
> +             aDisplayNameArray.Length() == aMessageUriArray.Length());

I've never seen a==b==c==d.

@@ +810,5 @@
>    SetLastSaveDirectory(localFile);
>  
>    PathString dirName = localFile->NativePath();
>  
> +  nsTArray<nsCString> contentTypeArray;

Can you use
AutoTArray<nsCString, 1> contentTypeArray = {aContentType};
?

@@ +1290,5 @@
>  
>      // Ok, now save the message.
>      nsCOMPtr<nsIURI> dummyNull;
>      rv = messageService->SaveMessageToDisk(
> +        PromiseFlatCString(aMessageUriArray[i]).get(), saveToFile, false,

Why is that necessary? The element is an nsCString not an nsACString.

@@ +2107,5 @@
> +        mUrl(aUrl),
> +        mDisplayName(aDisplayName),
> +        mMessageUri(aMessageUri) {}
> +
> +  nsAutoCString mContentType;

According to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings, member variables should be nsCString and not use the auto-types.

@@ +2375,5 @@
>  nsDelAttachListener::OnStopRunningUrl(nsIURI *aUrl, nsresult aExitCode) {
>    nsresult rv = NS_OK;
> +  const nsCString &messageUri(mAttach->mAttachmentArray[0].mMessageUri);
> +  if (mOriginalMessage &&
> +      Substring(messageUri, 0, 13).EqualsLiteral("imap-message:")) {

Sigh. What is this crufty thing here? We don't need to check for mailbox-message, etc.? And below.

@@ +2569,2 @@
>                               true);
> +  nsTArray<nsCString> aContentTypeArray;

Same comment as above. Use auto array and initialise right away.

::: mailnews/base/test/unit/test_detachToFile.js
@@ +58,5 @@
>      Ci.nsIMessenger
>    );
>    let attachment = gCallbackObject.attachments[0];
>  
> +  dump("startDetach: " + JSON.stringify(attachment) + "\n");

We want the extra dump?

@@ +90,5 @@
>  
>    let messageContent = getContentFromMessage(msgHdr);
> +  dump("testDetach, hdr\n");
> +  dump("testDetach, content\n");
> +  dump(messageContent);

and these?
Attachment #9109365 - Flags: review?(jorgk) → review+
Attachment #9109365 - Attachment is obsolete: true
Attachment #9110090 - Flags: review+

Thanks Jorg - All good stuff and now addressed in the new patch.

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

Comment on attachment 9109365 [details] [diff] [review]
::: mailnews/base/src/nsMessenger.cpp
@@ +573,3 @@

NS_ENSURE_ARG_POINTER(aDestFolder);

  • MOZ_ASSERT(aContentTypeArray.Length() == aUrlArray.Length() ==
  •         aDisplayNameArray.Length() == aMessageUriArray.Length());
    

I've never seen a==b==c==d.

Yeah, it's a little icky, but couldn't think of a better alternative.

@@ +810,5 @@

SetLastSaveDirectory(localFile);
PathString dirName = localFile->NativePath();

  • nsTArray<nsCString> contentTypeArray;

Can you use
AutoTArray<nsCString, 1> contentTypeArray = {aContentType};
?

Oooh! That is cool - I'd never heard of std::initializer before this, but it's exactly the kind of thing I'd always wanted C++ to have :-)
aContentType is an nsACString, so ended up with: AutoTArray<nsCString, 1> contentTypeArray = {nsCString(aContentType)};

@@ +2375,5 @@

nsDelAttachListener::OnStopRunningUrl(nsIURI *aUrl, nsresult aExitCode) {
nsresult rv = NS_OK;

  • const nsCString &messageUri(mAttach->mAttachmentArray[0].mMessageUri);
  • if (mOriginalMessage &&
  •  Substring(messageUri, 0, 13).EqualsLiteral("imap-message:")) {
    

Sigh. What is this crufty thing here? We don't need to check for
mailbox-message, etc.? And below.

It looks like a nasty little hack to defer the detach until after the IMAP roundtrip.
I don't understand the wider context here, but it'd be nice to fix some time (I want to map out and understand the IMAP stuff, so I'll aim to revisit this then).

(Also, I hate string comparison functions. I originally screwed up this one, and spend ages digging about in Mime code trying to figure out why the unit tests were failing on detaching...)

::: mailnews/base/test/unit/test_detachToFile.js
@@ +58,5 @@

 Ci.nsIMessenger

);
let attachment = gCallbackObject.attachments[0];

  • dump("startDetach: " + JSON.stringify(attachment) + "\n");

We want the extra dump?

Doh. Leftover cruft from nailing down the imap problems I caused with my borked string comparison above.
Now removed.

AutoTArray<nsCString, 1> contentTypeArray = {nsCString(aContentType)};

I think you want PromiseFlatCString. I didn't find an example in M-C, but they have:
https://searchfox.org/mozilla-central/search?q=AutoTArray%3CnsCString.*depen&case=false&regexp=true&path=

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/11e9e7a7d60d
Remove xpidl [array] use in nsIMessenger. r=jorgk

Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0

Hmm. I seem to have missed getNavigateHistory().

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #9116950 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9116950 [details] [diff] [review]
1594877-nsIMessenger.getNavigateHistory-1.patch

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

::: mailnews/base/public/nsIMessenger.idl
@@ +130,5 @@
>      // history pos, and *not* adding the loaded message to the history queue.
>      ACString getMsgUriAtNavigatePos(in long aPos);
>      ACString getFolderUriAtNavigatePos(in long aPos);
>      attribute long navigatePos;
> +    // Fetch the history, as an array of (msgURI, folderURI) pairs.

Please change to proper documentation comments while you're here. That is, /** */

Rather odd method. Looks like the "pair" is that each second item is a msgURI, and then the next one after it the folderURI (both as strings). Please add that to the documentation if correct.
Attachment #9116950 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9116950 - Attachment is obsolete: true
Attachment #9117400 - Flags: review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/324f339c85f4
Remove xpidl [array] use in nsIMessenger.getNavigateHistory(). r=mkmelin

Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Regressions: 1634731
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: