647481 - Remove nsInt64, nsUint64 and nsTime from comm-central

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Ian Neal]

Bug 646071 removed nsInt64 and nsUint64 so we need to do the same on comm-central (as we're broken at the moment).
http://mxr.mozilla.org/comm-central/search?string=nsint64&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
http://mxr.mozilla.org/comm-central/search?string=nsUint64&find=mailnews&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 1

[Ian Neal]

Attachment: Move to PRint64 and PRUint64 patch v1.0

As the reimplementation of nsTime stopped += operator from working on nsTime objects, two lines also had to be changed to workaround this.
The other option would be to add that back into nsTime implementation.

Comment 2

[[e-mail]]

Comment on attachment 523825 [details] [diff] [review]
Move to PRint64 and PRUint64 patch v1.0

>       nsTime adjustedDate = nsTime(m_value.u.date);
>-      adjustedDate += 60*60*24; // we want to be greater than the next day....
>+      // We want to be greater than the next day....
>+      adjustedDate = adjustedDate + 60*60*24;
Would it be easier to use PRTime instead?

>   // XXX: This truncates 64-bit to 32-bit
>-  return OnProgressChange(nsnull, request, nsUint64(aProgress), nsUint64(aProgressMax),
>-                          nsUint64(aProgress) /* current total progress */, nsUint64(aProgressMax) /* max total progress */);
>+  return OnProgressChange(nsnull, request, PRUint64(aProgress), PRUint64(aProgressMax),
>+                          PRUint64(aProgress) /* current total progress */, PRUint64(aProgressMax) /* max total progress */);
PRInt32 would be clearer, as that's what we're really doing...

>+        nsTime nextPurgeTime = curJunkFolderLastPurgeTime + PRInt64(mMinDelayBetweenPurges * 60000000 /* convert mMinDelayBetweenPurges to into microseconds */);
[Is the cast still necessary?]

>+            if ((oldestPurgeTime == PRInt64(0)) || (curJunkFolderLastPurgeTime < oldestPurgeTime))
[Is the cast still necessary?]

[And several further variants of the above.]

>   PRInt64 sizeOnDisk;
...
>+    PRInt64 folderSize(sizeOnDisk);
Useless variable.

Comment 3

[Ian Neal]

Attachment: Move to PRint64 and PRUint64 patch v1.1

Changes since v1.0:
* Used PRTime for adjustedDate
* Used PRInt32 for OnProgressChange
* Removed unnecessary casts
* Removed useless folderSize variable

Comment 4

[[e-mail]]

Comment on attachment 523835 [details] [diff] [review]
Move to PRint64 and PRUint64 patch v1.1

Weird, this patch fails to build for me in places not actually patched...

Comment 5

[[e-mail]]

Comment on attachment 523835 [details] [diff] [review]
Move to PRint64 and PRUint64 patch v1.1

>--- a/mailnews/base/src/nsMsgBiffManager.cpp
>+++ b/mailnews/base/src/nsMsgBiffManager.cpp
>-      biffEntry.nextBiffTime += jitter;
>+      biffEntry.nextBiffTime = biffEntry.nextBiffTime + jitter;
Ah, I found this change that actually causes my build to fail.

>--- a/mailnews/base/src/nsMsgPurgeService.cpp
>+++ b/mailnews/base/src/nsMsgPurgeService.cpp
Something here doesn't build for me either but I'm not sure why.

Comment 6

[[e-mail]]

(In reply to comment #5)
>(From update of attachment 523835 [details] [diff] [review])
>>--- a/mailnews/base/src/nsMsgBiffManager.cpp
>>+++ b/mailnews/base/src/nsMsgBiffManager.cpp
>>-      biffEntry.nextBiffTime += jitter;
>>+      biffEntry.nextBiffTime = biffEntry.nextBiffTime + jitter;
>Ah, I found this change that actually causes my build to fail.
>>--- a/mailnews/base/src/nsMsgPurgeService.cpp
>>+++ b/mailnews/base/src/nsMsgPurgeService.cpp
>Something here doesn't build for me either but I'm not sure why.
Both of these build on trunk, but not on the 2.0 branch...

Comment 7

[[e-mail]]

Comment on attachment 523835 [details] [diff] [review]
Move to PRint64 and PRUint64 patch v1.1

OK, so what's missing here is that nsTime isn't working out for us. It's mostly a thin wrapper around a PRTime so I guess we should use that instead. (Note that a "blank" nsTime defaults to PR_Now, but PRTime has no default.)

Comment 8

[[e-mail]]

(In reply to comment #7)
> OK, so what's missing here is that nsTime isn't working out for us. It's mostly
> a thin wrapper around a PRTime so I guess we should use that instead. (Note
> that a "blank" nsTime defaults to PR_Now, but PRTime has no default.)
Oh, and the reason for that is that nsTime uses an nsInt64 on branch..

Comment 9

[[e-mail]]

Comment on attachment 523835 [details] [diff] [review]
Move to PRint64 and PRUint64 patch v1.1

r- due to lack of time.

Comment 10

[Ian Neal]

Attachment: Move to PRint64, PRUint64 and PRTime patch v1.2

Changes since v1.1:
* Switched from nsTime to PRTime setting PR_Now() where required.

Comment 11

[[e-mail]]

Comment on attachment 524017 [details] [diff] [review]
Move to PRint64, PRUint64 and PRTime patch v1.2

>-      nsTime currentTime;
>+      PRTime currentTime = PR_Now();
>       rv = SetNextBiffTime(biffEntry, currentTime);

>-    nsTime currentTime;
>+    PRTime currentTime = PR_Now();
>     m_startTime = currentTime;
Nit: In both these cases the nsTime instance is only used for its default value and the temporary variable can be removed.

Comment 12

[[e-mail]]

Comment on attachment 524017 [details] [diff] [review]
Move to PRint64, PRUint64 and PRTime patch v1.2

>+nsresult nsMsgBiffManager::SetNextBiffTime(nsBiffEntry &biffEntry, const PRTime currentTime)
...
>+	nsresult SetNextBiffTime(nsBiffEntry &biffEntry, const PRTime currentTime);
[We could probably drop the const here]

Comment 13

[Ian Neal]

Attachment: Move to PRint64, PRUint64 and PRTime patch v1.3 [Checked in: Comment 14]

Changes since v1.2:
* Removed unneeded currentTime code as suggested.
* Removed const as suggested.

Carrying forward r+ and requesting sr

Comment 14

[Ian Neal]

Comment on attachment 524109 [details] [diff] [review]
Move to PRint64, PRUint64 and PRTime patch v1.3 [Checked in: Comment 14]

http://hg.mozilla.org/comm-central/rev/c831c107de25

Comment 15

[[e-mail]]

(In reply to comment #13)
> * Removed unneeded currentTime code as suggested.
Actually you removed even more currentTime code than I had spotted :-)

I overlooked one of the changed existing lines which had a trailing space :-(