Closed Bug 647481 Opened 9 years ago Closed 9 years ago

Remove nsInt64, nsUint64 and nsTime from comm-central

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 5.0b1

People

(Reporter: iann_bugzilla, Assigned: iann_bugzilla)

References

Details

Attachments

(1 file, 3 obsolete files)

Assignee: nobody → iann_bugzilla
Status: NEW → ASSIGNED
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.
Attachment #523825 - Flags: review?(neil)
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.
Attachment #523825 - Flags: review?(neil)
Changes since v1.0:
* Used PRTime for adjustedDate
* Used PRInt32 for OnProgressChange
* Removed unnecessary casts
* Removed useless folderSize variable
Attachment #523825 - Attachment is obsolete: true
Attachment #523835 - Flags: review?(neil)
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 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.
(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 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.)
(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..
Summary: Remove nsInt64 and nsUint64 from comm-central → Remove nsInt64, nsUint64 and nsTime from comm-central
Comment on attachment 523835 [details] [diff] [review]
Move to PRint64 and PRUint64 patch v1.1

r- due to lack of time.
Attachment #523835 - Flags: review?(neil) → review-
Changes since v1.1:
* Switched from nsTime to PRTime setting PR_Now() where required.
Attachment #523835 - Attachment is obsolete: true
Attachment #524017 - Flags: review?(neil)
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 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]
Attachment #524017 - Flags: review?(neil) → review+
Changes since v1.2:
* Removed unneeded currentTime code as suggested.
* Removed const as suggested.

Carrying forward r+ and requesting sr
Attachment #524017 - Attachment is obsolete: true
Attachment #524109 - Flags: superreview?(bienvenu)
Attachment #524109 - Flags: review+
Attachment #524109 - Flags: superreview?(bienvenu) → superreview+
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
Attachment #524109 - Attachment description: Move to PRint64, PRUint64 and PRTime patch v1.3 → Move to PRint64, PRUint64 and PRTime patch v1.3 [Checked in: Comment 14]
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(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 :-(
Target Milestone: --- → Thunderbird 3.3a4
Duplicate of this bug: 508558
You need to log in before you can comment on or make changes to this bug.