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

Status: RESOLVED DUPLICATE of bug 647481

(MailNews Core :: Backend, defect)

Description

[Steffen Wilberg]

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

Comment 1

[Takeshi Kurosawa :takenspc]

Attachment: Patch

Drop usage of nsTime, use PRTime instead.

Comment 2

[Magnus Melin [:mkmelin]]

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

Comment 3

[Takeshi Kurosawa :takenspc]

Attachment: Patch rv.2

(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.

Comment 4

[David :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.

Comment 5

[[e-mail]]

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?

Comment 6

[Mark Banner (:standard8)]

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.

Comment 7

[Wayne Mery (:wsmwk)]

Takeshi will be looking at this next week

Comment 8

[Takeshi Kurosawa :takenspc]

In fact, this bug has been fixed by bug 647481.