282512 - A few MailNews cleanup, found while working on bug 90906

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Serge Gautherie (:sgautherie)]

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1)

Matthias: Could you try to compile, and possibly test, this patch ? Thanks.

Comment 2

[Matthias Bockelkamp]

Comment on attachment 174527 [details] [diff] [review]
(Av1)

>--- mozilla/mailnews/local/src/nsPop3Sink.cpp	16 Feb 2005 11:50:13 -0000	1.118
>+++ mozilla/mailnews/local/src/nsPop3Sink.cpp	16 Feb 2005 22:51:15 -0000
>@@ -158,7 +158,7 @@ struct partialRecord
> 
>   nsCOMPtr<nsIMsgDBHdr> m_msgDBHdr;
>   nsCString m_uidl;
>-};
>+}

Withthat change taken out it compiles without any additional warnings, and it
seems to work (I used the self-compiled Build for two days now). I'll attach
the fixed patch for review.

Comment 3

[Matthias Bockelkamp]

Attachment: (Av1) fixed

This is the fixed patch.

FYI, the problem with the original patch was the following:

e:/Mozilla/mozilla/mailnews/local/src/nsPop3Sink.cpp:163: error: ISO C++
   forbids defining types within return type
e:/Mozilla/mozilla/mailnews/local/src/nsPop3Sink.cpp:163: error: return type
   specification for constructor invalid

Comment 4

[[e-mail]]

Sorry, but what are the point of these changes?

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> Sorry, but what are the point of these changes?

No reported "buggy" behaviour:
simple code nits that I found while working on these files for the other bug...

Comment 6

[Wayne Mery (:wsmwk)]

might not be too badly bitrotted - not many bug changes to nsMsgGroupThread.cpp since 2005

Comment 7

[Wayne Mery (:wsmwk)]

even slightly more bitrotted since david's threading changes, but shouldn't be hard to clean up.

Comment 8

[Wayne Mery (:wsmwk)]

assignee=nobody, so >status=new

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> assignee=nobody, so >status=new

Not sure why Philip unassigned me.
Let me get back to this.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 175111 [details] [diff] [review]
(Av1) fixed

Patch has bitrotted.

$ patch --dry-run < ~/Desktop/tbTestPatches/282512.diff 
(Stripping trailing CRs from patch.)
patching file nsMsgGroupThread.cpp
Hunk #1 succeeded at 47 (offset 1 line).
Hunk #2 FAILED at 64.
Hunk #3 succeeded at 589 with fuzz 2 (offset 464 lines).
Hunk #4 FAILED at 635.
Hunk #5 FAILED at 656.
Hunk #6 FAILED at 693.
Hunk #7 FAILED at 737.
Hunk #8 FAILED at 798.
Hunk #9 FAILED at 884.
Hunk #10 FAILED at 893.
Hunk #11 FAILED at 909.
Hunk #12 FAILED at 934.
Hunk #13 FAILED at 945.
Hunk #14 succeeded at 576 (offset -53 lines).
Hunk #15 succeeded at 605 (offset -53 lines).
Hunk #16 succeeded at 670 (offset -53 lines).
Hunk #17 succeeded at 690 (offset -53 lines).
Hunk #18 succeeded at 705 (offset -53 lines).
Hunk #19 succeeded at 744 (offset -53 lines).
Hunk #20 succeeded at 758 (offset -53 lines).
Hunk #21 succeeded at 821 with fuzz 2 (offset -21 lines).
11 out of 21 hunks FAILED -- saving rejects to file nsMsgGroupThread.cpp.rej
(Stripping trailing CRs from patch.)
can't find file to patch at input line 274
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|Index: mozilla/mailnews/db/msgdb/src/nsMsgThread.cpp
|===================================================================
|RCS file: /cvsroot/mozilla/mailnews/db/msgdb/src/nsMsgThread.cpp,v
|retrieving revision 1.80
|diff -u -p -r1.80 nsMsgThread.cpp
|--- mozilla/mailnews/db/msgdb/src/nsMsgThread.cpp 16 Feb 2005 11:50:12 -0000  1.80
|+++ mozilla/mailnews/db/msgdb/src/nsMsgThread.cpp 16 Feb 2005 22:51:09 -0000
--------------------------
File to patch:

Comment 11

[Wayne Mery (:wsmwk)]

Serge, can you post an updated patch?

Comment 12

[Wayne Mery (:wsmwk)]

(In reply to Serge Gautherie (:sgautherie) from comment #9)
> (In reply to comment #8)
> > assignee=nobody, so >status=new
> 
> Not sure why Philip unassigned me.
> Let me get back to this.

Comment 13

[Nomis101]

Attachment: unbitrotted to current trunk

I've unbitrotted the latest patch to current trunk. Some places were already fixed and some places had changed a bit. I'm not sure if all changes are still correct, but TB builds with this patch.

Comment 14

[Serge Gautherie (:sgautherie)]

Comment on attachment 175111 [details] [diff] [review]
(Av1) fixed

Ftr, it seems this patch didn't apply correctly:
{{
patching file mailnews/imap/src/nsImapProtocol.cpp
Hunk #11 FAILED at 6365
1 out of 13 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapProtocol.cpp.rej
}}

The issue was:
{{
--- mozilla/mailnews/imap/src/nsImapProtocol.cpp  16 Feb 2005 11:50:12 -0000  1.596
+++ mozilla/mailnews/imap/src/nsImapProtocol.cpp  16 Feb 2005 22:51:14 -0000

@@ -6380,8 +6366,7 @@ void nsImapProtocol::DiscoverAllAndSubsc
             case kOtherUsersNamespace:
               boxSpec->box_flags |= kOtherUsersMailbox;
               break;
-            default: // (kUnknownNamespace)
}}
There is a tab before '//', not one space.

Comment 15

[:aceman]

Attachment: updated patch

Let's kick this out of the list of zombies ;)

Unbitrotted the patch. I dropped the hunks that remove 'break' from 'default:' cases of switch as those are controversial (some people want them in). I also dropped removal of 'default: break;' cases themselves. Yes, they do nothing but then the compiler complains about unhandled values (of the enumerated variable) in the switch and that breaks the build these days.

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

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8838975 [details] [diff] [review]
updated patch

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

What is this Little Shop of Horrors ;-( There are more trailing spaces you could kill.

::: mailnews/base/src/nsMsgGroupThread.cpp
@@ +281,4 @@
>    if (numChildren > 0)
>    {
> +    nsCOMPtr<nsIMsgDBHdr> curHdr;
> +    uint32_t childIndex = 0;

No need for = 0;

@@ +664,5 @@
>  }
>  
>  nsresult nsMsgGroupThread::GetChildHdrForKey(nsMsgKey desiredKey, nsIMsgDBHdr **result, int32_t *resultIndex)
>  {
> +  NS_ENSURE_ARG_POINTER(result);

Check resultIndex as well?

::: mailnews/db/msgdb/src/nsMsgThread.cpp
@@ +1121,5 @@
>        }
>      }
>    }
>  
> +  retHdr.forget(result);

Why this change? Isn't NS_IF_ADDREF(*result = retHdr); the "normal" thing to do?

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #16)
> Why this change? Isn't NS_IF_ADDREF(*result = retHdr); the "normal" thing to
> do?
I've done a little research on this.
NS_IF_ADDREF(*result = mXXXX);
I used when mXXXX is not being destroyed inside the function. So the value is assigned to *result and the refcount is increased.

If however the object is destroyed in the function, the xxx.forget(result) can be used to avoid increasing the refcount and then decreasing it when 'xxx' is destroyed, for example by going our of scope.

So I wonder why the .forget() method isn't more widely used, for example here:
https://dxr.mozilla.org/comm-central/rev/567601fc64d9e8cd4f2e417cf35d90d4cc48140f/mailnews/base/search/src/nsMsgFilterService.cpp#236
https://dxr.mozilla.org/comm-central/rev/567601fc64d9e8cd4f2e417cf35d90d4cc48140f/mailnews/addrbook/src/nsAbMDBDirectory.cpp#324

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8838975 [details] [diff] [review]
updated patch

More research. Bug 190746 with a huge patch wanted to introduce .forget() for nsCOMPtr. But they marked it a duplicate of bug 78016 and implemented .swap() instead. I still haven't worked out where/when .forget() was introduced.

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #18)
> I still haven't worked out where/when .forget() was introduced.
Looks like in bug 392493.

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #17)
> So I wonder why the .forget() method isn't more widely used, ...
See bug 1340972.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 282512.patch (JK v1).

I was trying to remove some more white-space in nsMsgGroupThread.cpp and came across some more clean-up opportunity. Sorry.

Comment 22

[Jorg K (CEST = GMT+2)]

Try the interdiff to see my additional changes:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8838975&action=interdiff&newid=8839067&headers=1

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 282512.patch (JK v2).

Sorry, I went overboard and did another clean-up path through nsMsgGroupThread.cpp with lots of white-space adjustments.

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: 282512.patch (JK v3).

Clean-up makes addictive. Now I hit nsMsgThread.cpp.

Note that |NS_ADDREF(*result = e);| I coded in nsMsgGroupThread.cpp was already used here. Also note the |new| is infallible, so no need to check:
https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 282512.patch (JK v3b).

Let's lose the hunk ing nsPop3Sink.cpp. No need to change a file just to remove on empty line.

Comment 26

[:aceman]

Comment on attachment 8839121 [details] [diff] [review]
282512.patch (JK v3b).

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

Thanks.

::: mailnews/base/src/nsMsgGroupThread.cpp
@@ +604,3 @@
>  NS_IMETHODIMP nsMsgGroupThread::GetRootHdr(int32_t *resultIndex, nsIMsgDBHdr **result)
>  {
> +  NS_ENSURE_ARG_POINTER(resultIndex);

Please do not check resultIndex. Wee sem to call the function with it being nullptr (https://dxr.mozilla.org/comm-central/source/mailnews/db/msgdb/src/nsMsgThread.cpp#347) and we properly check it later in the function. So this change is not valid, would change behaviour and probably abort legal uses.

@@ +662,5 @@
>  
>  nsresult nsMsgGroupThread::GetChildHdrForKey(nsMsgKey desiredKey, nsIMsgDBHdr **result, int32_t *resultIndex)
>  {
> +  NS_ENSURE_ARG_POINTER(result);
> +  NS_ENSURE_ARG_POINTER(resultIndex);

Better not check resultIndex here, it is done below. But it is crazy that the function arguments are in different order here.

::: mailnews/db/msgdb/src/nsMsgThread.cpp
@@ +1123,1 @@
>    return rv;

Can rv be different from NS_OK here?

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 282512.patch (JK v4).

OK, clean-up gone wild. I've moved |uint32_t childIndex| into the for-statement. Further white-space adjustments, removed
-  if ((int32_t) numChildren < 0)
-    numChildren = 0;
twice.

Check interdiff.

Comment 28

[Jorg K (CEST = GMT+2)]

Oh, of course I removed the |NS_ENSURE_ARG_POINTER(resultIndex);| as instructed.

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

Comment 29

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/2c5390afd3dd9514e5337521739f44a27929c1e2