1508054 - reduce exposed API of nsMsgHdr

Status: RESOLVED FIXED

(MailNews Core :: Database, enhancement)

Description

[:aceman]

The class nsMsgHdr implementing nsIMsgDBHdr is exposing some methods as public even if there is no user (internal methods, not in the public API of nsIMsgDBHdr).
It also allows access to some internal m_ members from external callers, which should use the available proper accessors.

Comment 1

[:aceman]

Attachment: 1508054.patch

Comment 2

[:aceman]

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

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9025884 [details] [diff] [review]
1508054.patch

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

I stopped looking when I found the ref counting problem.

::: mailnews/db/msgdb/public/nsMsgHdr.h
@@ +19,5 @@
>  class nsMsgHdr : public nsIMsgDBHdr {
>  public:
>      NS_DECL_NSIMSGDBHDR
>      friend class nsMsgDatabase;
> +    friend class nsImapMailDatabase;

Wow, IMAP??

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +2010,5 @@
>  nsMsgDatabase::UndoDelete(nsIMsgDBHdr *aMsgHdr)
>  {
> +  if (aMsgHdr)
> +  {
> +    // force deleted flag, so SetHdrFlag won't bail out because  deleted flag isn't set

Need to fix this comment: Upper case, double space, full stop.

@@ +2013,5 @@
> +  {
> +    // force deleted flag, so SetHdrFlag won't bail out because  deleted flag isn't set
> +    uint32_t result;
> +    aMsgHdr->OrFlags(nsMsgMessageFlags::Expunged, &result);
> +    SetHdrFlag(aMsgHdr, false, nsMsgMessageFlags::Expunged); // clear deleted flag in db

Two spaces before the // (as linting requires it). No harm in uppercase and full stop here.

@@ -5010,2 @@
>        printf("hdr key = %u, author = %s subject = %s\n", key, author.get(), subject.get());
> -      NS_RELEASE(msgHdr);

This isn't right. This header handling is *extremely* ugly in the way it handles references. The addref happens here:
https://searchfox.org/comm-central/rev/c2cff130f8467671b9fdeb2811a1647afdef484f/mailnews/db/msgdb/src/nsMsgDatabase.cpp#704
or here
https://searchfox.org/comm-central/rev/c2cff130f8467671b9fdeb2811a1647afdef484f/mailnews/db/msgdb/src/nsMsgDatabase.cpp#516
which is called from GetMsgHdrForKey(). You must release here!

Would
nsCOMPtr<nsIMsgDBHdr> msg;
rv = GetMsgHdrForKey(key, getter_AddRefs(msg));
compile. Then the smart pointer will take care of it.

Comment 4

[:aceman]

Attachment: 1508054.patch v1.1

Thanks for catching that release problem.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1508054.patch v1.2

Thanks. I've reduced some indentation.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0a1e5f9bc313
reduce exposed API of nsMsgHdr. r=jorgk