Closed Bug 508558 Opened 11 years ago Closed 8 years ago

move xpcom/ds/nsTime.h to Mailnews, or stop using it

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 647481

People

(Reporter: steffen.wilberg, Unassigned)

References

()

Details

(Whiteboard: [needs new patch][patchlove])

Attachments

(1 file, 1 obsolete file)

Bug 486081 wants to get rid of nsTime, and Mailnews is the only consumer.

From bug 486081 comment 7:
> Mailnews should import the nsTime code if it wishes to continue using it.

http://mxr.mozilla.org/comm-central/search?string=nsTime&case=on&find=%2Fmailnews%2F
Attached patch Patch (obsolete) — Splinter Review
Drop usage of nsTime, use PRTime instead.
Thx for the patch! 
The LL_*** macros are obsolete and shouldn't be used. (You should be able to use normal math expressions nowadays.)

When you've fixed that, make sure to flag the patch for review (from an appropriate reviewer).

http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree
Assignee: nobody → taken.spc
Status: NEW → ASSIGNED
Attached patch Patch rv.2Splinter Review
(In reply to comment #2)
> The LL_*** macros are obsolete and shouldn't be used. (You should be able to
> use normal math expressions nowadays.)

Fixed.
Attachment #397575 - Attachment is obsolete: true
Attachment #398934 - Flags: review?(bienvenu)
Comment on attachment 398934 [details] [diff] [review]
Patch rv.2

Looks good, thanks. I don't think you need to get rid of the nsInt64 stuff, but PRInt64 should work OK. If not, Tinderbox will tell us.
Attachment #398934 - Flags: superreview?(neil)
Attachment #398934 - Flags: review?(bienvenu)
Attachment #398934 - Flags: review+
Comment on attachment 398934 [details] [diff] [review]
Patch rv.2

[Actually I thought nsInt64 was deprecated as well.]

>-      nsTime adjustedDate = nsTime(m_value.u.date);
>+      PRTime adjustedDate = m_value.u.date;
>       adjustedDate += 60*60*24; // we want to be greater than the next day....
[What units is this supposed to be in? Normally PRTime is microseconds, no?]

> typedef struct {
> 	nsCOMPtr<nsIMsgIncomingServer> server;
>-	nsTime nextBiffTime;
>+	PRTime nextBiffTime;
[This actually saves us code, since nsTime used to default to PR_Now.]

>+              PRInt64 minDelayBetweenPurges = mMinDelayBetweenPurges;
>+              PRInt64 microSecondsPerMinute = 60000000;
>+              PRTime nextPurgeTime = curFolderLastPurgeTime + (minDelayBetweenPurges * microSecondsPerMinute);
Is there any reason not to write this as
PRTime nextPurgeTime = curFolderLastPurgeTime + mMinDelayBetweenPurges * 60000000LL;
as is done elsewhere in the code?

>-        nsTime nextPurgeTime = curJunkFolderLastPurgeTime + nsInt64(mMinDelayBetweenPurges * 60000000 /* convert mMinDelayBetweenPurges to into microseconds */);
>-        nsTime currentTime;
>-        if (nextPurgeTime < currentTime)
>+        PRTime nextPurgeTime = curJunkFolderLastPurgeTime + PRInt64(mMinDelayBetweenPurges * 60000000 /* convert mMinDelayBetweenPurges to into microseconds */);
This calculation doesn't look right; mMinDelayBetweenPurges and 60000000 are both 32-bit integers so this will overflow for any value over 35 minutes.
(The old calculation wasn't right either). Another case of
PRTime nextPurgeTime = curJunkFolderLastPurgeTime + mMinDelayBetweenPurges * 60000000LL;
perhaps?

>               // if we've never purged this folder, do it...
>-              if (curJunkFolderLastPurgeTime == nsInt64(0))
>+              if (curJunkFolderLastPurgeTime == 0)
Would it make more sense to use the ! operator instead of comparing to zero?
Whiteboard: [needs new patch]
Whiteboard: [needs new patch] → [needs new patch][patchlove]
Comment on attachment 398934 [details] [diff] [review]
Patch rv.2

Takeshi, this looks like a good patch, would you be able to update the patch and/or respond to Neil's comments?

Cancelling review request for now, as the questions need answering. Please re-request when they are answered and/or a new patch is posted. Thanks.
Attachment #398934 - Flags: superreview?(neil)
Takeshi will be looking at this next week
In fact, this bug has been fixed by bug 647481.
Assignee: taken.spc → nobody
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 647481
You need to log in before you can comment on or make changes to this bug.