Remove xpidl [array] use in nsIMessenger
Categories
(MailNews Core :: Backend, task)
Tracking
(Not tracked)
People
(Reporter: benc, Assigned: benc)
References
Details
Attachments
(2 files, 2 obsolete files)
54.17 KB,
patch
|
benc
:
review+
|
Details | Diff | Splinter Review |
7.04 KB,
patch
|
benc
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
One of the bigger ones. Lots of char*
=> string class replacements rippling on down the layers.
Comment 2•4 years ago
|
||
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?
Assignee | ||
Comment 3•4 years ago
|
||
Assignee | ||
Comment 4•4 years ago
|
||
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.
Assignee | ||
Updated•4 years ago
|
Comment 5•4 years ago
|
||
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®exp=true&path=
Assignee | ||
Updated•4 years ago
|
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/11e9e7a7d60d
Remove xpidl [array] use in nsIMessenger. r=jorgk
Updated•4 years ago
|
Assignee | ||
Comment 7•4 years ago
|
||
Hmm. I seem to have missed getNavigateHistory()
.
Assignee | ||
Comment 8•4 years ago
|
||
Comment 9•4 years ago
|
||
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.
Assignee | ||
Comment 10•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Comment 11•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/324f339c85f4
Remove xpidl [array] use in nsIMessenger.getNavigateHistory(). r=mkmelin
Description
•