Closed Bug 282512 Opened 16 years ago Closed 4 years ago

A few MailNews cleanup, found while working on bug 90906

Categories

(MailNews Core :: Backend, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 54.0

People

(Reporter: sgautherie, Assigned: sgautherie)

Details

(Whiteboard: [good first bug][patchlove])

Attachments

(2 files, 7 obsolete files)

 
Attached patch (Av1) (obsolete) — Splinter Review
Matthias: Could you try to compile, and possibly test, this patch ? Thanks.
Assignee: general → gautheri
Status: NEW → ASSIGNED
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.
Attachment #174527 - Attachment is obsolete: true
Attached patch (Av1) fixed (obsolete) — Splinter Review
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
Attachment #175111 - Flags: review?(neil.parkwaycc.co.uk)
Sorry, but what are the point of these changes?
(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...
might not be too badly bitrotted - not many bug changes to nsMsgGroupThread.cpp since 2005
Keywords: helpwanted
Assignee: sgautherie.bz → nobody
Component: General → Backend
Product: SeaMonkey → MailNews Core
QA Contact: general → backend
even slightly more bitrotted since david's threading changes, but shouldn't be hard to clean up.
OS: Windows 98 → All
Hardware: x86 → All
Whiteboard: [good first bug] [patchlove]
assignee=nobody, so >status=new
Status: ASSIGNED → NEW
(In reply to comment #8)
> assignee=nobody, so >status=new

Not sure why Philip unassigned me.
Let me get back to this.
Assignee: nobody → sgautherie.bz
Status: NEW → ASSIGNED
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:
Attachment #175111 - Attachment is obsolete: true
Attachment #175111 - Flags: review?(neil)
Serge, can you post an updated patch?
Keywords: helpwanted
Whiteboard: [good first bug] [patchlove] → [good first bug][patchlove]
Severity: trivial → minor
(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.
Flags: needinfo?(sgautherie.bz)
Attached patch unbitrotted to current trunk (obsolete) — Splinter Review
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 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.
Attached patch updated patchSplinter Review
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
Attachment #696823 - Attachment is obsolete: true
Attachment #8838975 - Flags: review?(jorgk)
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?
(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
Flags: needinfo?(bugzillamozillaorg_serge_20140323)
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.
Attachment #8838975 - Flags: review?(jorgk) → review+
(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.
(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.
Attached patch 282512.patch (JK v1). (obsolete) — Splinter Review
I was trying to remove some more white-space in nsMsgGroupThread.cpp and came across some more clean-up opportunity. Sorry.
Attachment #8839067 - Flags: review?(acelists)
Attached patch 282512.patch (JK v2). (obsolete) — Splinter Review
Sorry, I went overboard and did another clean-up path through nsMsgGroupThread.cpp with lots of white-space adjustments.
Attachment #8839067 - Attachment is obsolete: true
Attachment #8839067 - Flags: review?(acelists)
Attachment #8839068 - Flags: review?(acelists)
Attached patch 282512.patch (JK v3). (obsolete) — Splinter Review
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
Attachment #8839068 - Attachment is obsolete: true
Attachment #8839068 - Flags: review?(acelists)
Attachment #8839078 - Flags: review?(acelists)
Attached patch 282512.patch (JK v3b). (obsolete) — Splinter Review
Let's lose the hunk ing nsPop3Sink.cpp. No need to change a file just to remove on empty line.
Attachment #8839078 - Attachment is obsolete: true
Attachment #8839078 - Flags: review?(acelists)
Attachment #8839121 - Flags: review?(acelists)
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?
Attachment #8839121 - Flags: review?(acelists) → review+
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.
Attachment #8839121 - Attachment is obsolete: true
Attachment #8839259 - Flags: review+
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
https://hg.mozilla.org/comm-central/rev/2c5390afd3dd9514e5337521739f44a27929c1e2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
You need to log in before you can comment on or make changes to this bug.