1610605 - remove use of nsAutoPtr from C-C

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Jorg K (CEST = GMT+2)]

Attachment: nsAutoPtr-p1.patch

See bug 1610067.

Comment 1

[Jorg K (CEST = GMT+2)]

Missed
.\addrbook\src\nsAbOSXCard.mm:#include "nsAutoPtr.h"
.\addrbook\src\nsAbOSXDirectory.mm:#include "nsAutoPtr.h"

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1610605-nsAutoPtr-part1.patch

OK, with missed Mac files now.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1610605-nsAutoPtr-part1.patch

Uff, that stuff is needlessly everywhere :-( - Now also removed from mail, ldap and calendar.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/61106214bab9
remove use of nsAutoPtr from C-C: Part 1: Remove unneeded includes. r=me

Comment 5

[Jorg K (CEST = GMT+2)]

OK, Ben, over to you. After Searchfox has caught up, it should be easy to see what's left to do. Instructions in bug 1610067 comment #0.

Feel free to review the patch I landed. I've added the include three times to it became clear from where it came instead of relying on some fragile inclusion through a chain of other include files. I trust you agree :-)

Comment 6

[Jorg K (CEST = GMT+2)]

Still used in 25 files including nsIMsgSend.idl which I missed to remove in part 1 :-(
https://searchfox.org/comm-central/search?q=nsAutoPtr&case=false&regexp=false&path=mailnews

Comment 7

[Ben Campbell]

Attachment: 1610605-nsAutoPtr-part2.patch

This replaces the vast bulk of nsAutoPtr uses.
(It leaves one use, in nsAbWinHelper, which I'll do separately)
try build: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7ace4400a8f70ca17fc2d59564895104fbfd9029

Comment 8

[Ben Campbell]

Attachment: 1610605-nsAutoPtr-part3.patch

And here's a prospective patch for nsAbWinHelper.
Seemed wrong using UniquePtr for a static variable. My first crack at this just turned it into a bare pointer (the lifecycle was being managed in by the nsAbWinHelper ctor/dtor anyway).
Then I saw a bunch of places in m-c which use StaticAutoPtr<>, which seems like a cleaner way to go.
Possible downsides:

1. is StaticAutoPtr being deprecated too? (a quick search doesn't seem to indicate so, but you never know)
2. it loses the string identifier (StaticAutoPtr uses default ctor), which could have been useful if anything goes wrong.
   (windows try build here:
   https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=54ae183e73ffcfdb23ab48981b74da8e1bcaacb0)

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9123242 [details] [diff] [review]
1610605-nsAutoPtr-part3.patch

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

No deeper insights to this, but looks ok.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9123240 [details] [diff] [review]
1610605-nsAutoPtr-part2.patch

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

In nsMsgSend.cpp, you use .reset and .SetEncoder() which due to the naming seems a little inconsistent.

Comment 11

[Ben Campbell]

(In reply to Jorg K (GMT+1) (no longer working on Thunderbird) from comment #10)

In nsMsgSend.cpp, you use .reset and .SetEncoder() which due to the naming
seems a little inconsistent.

Did you mean this bit of the patch, for nsMsgSend.cpp?

   if (ma->m_encoder) {
-    part->SetEncoder(ma->m_encoder.forget());
+    part->SetEncoder(ma->m_encoder.release());
   }

Or did you mean the SetEncoder() implementation in nsMsgSendPart.h?

+  void SetEncoder(MimeEncoder *encoder) { m_encoder.reset(encoder); }

I think they're both a tad odd, but I don't have any significantly better name ideas for SetEncoder()...

Comment 12

[Jorg K (CEST = GMT+2)]

Well, .reset() seems to set a new value, or null if no argument is given (I didn't look at the definition). Since SetEncoder() is a one liner, I'd inline it, or create a SetEncoder() to replace the .reset() calls. That's what I meant by "consistent".

Comment 13

[Ben Campbell]

(reset() deletes any resource held by the UniquePtr<>, and optionally assigns another in it's place, taking ownership.
release() detaches the resource from the UniquePtr without deleting it.)

Can't inline uses of nsMsgSendPart::SetEncoder() without exposing the underlying m_encoder member (which is a UniquePtr<>, but no other classes need to know that).
The nsMsgComposeAndSend class is the only caller of SetEncoder(), and most of the time it just passes in a newly-allocated raw pointer (4 of the 5 SetEncoder() calls). There's one case, where it transfers over an encoder from a public member of nsMsgAttachmentHandler (which is also a UniquePtr<>, but since it's public, it's left up to the caller (nsMsgComposeAndSend) to handler transferring ownership via release()).

I think the 'proper' thing to do would be to have SetEncoder() take a UniquePtr<> param, to be explicit that ownership was being transferred. But
seems like overkill here - it'd complicate the 4 raw pointer cases (would have to wrap them with MakeUnique<> and/or some kind of move semantics I think), at the expense of the single UniquePtr<> case.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/36e93a246513
remove use of nsAutoPtr from C-C: Part 2: Use UniquePtr instead. r=mkmelin
https://hg.mozilla.org/comm-central/rev/eca9d62cfb2c
remove use of nsAutoPtr from C-C: Part 3: nsAbWinHelper. r=mkmelin DONTBUILD

Comment 15

[Magnus Melin [:mkmelin]]

Looks like we're done.