Closed
Bug 1275306
Opened 8 years ago
Closed 8 years ago
Use more TimeDuration and TimeStamp in the shell
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
Details
Attachments
(2 files)
2.52 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
6.04 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
Here are two more pieces of the puzzle. They do not block the ConditionVariable patch directly, but they're a nice cleanup that I'd like to see landed. The first patch here replaces MAX_TIMEOUT_INTERVAL with a TimeDuration. The second replaces watchdog[Has]Timeout with a Maybe<TimeStamp>.
Attachment #8755910 -
Flags: review?(jdemooij)
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8755911 -
Flags: review?(jdemooij)
Comment 2•8 years ago
|
||
Comment on attachment 8755910 [details] [diff] [review] use_timeduration_for_MAX_TIMEOUT_INTERVAL-v0.diff Review of attachment 8755910 [details] [diff] [review]: ----------------------------------------------------------------- Much more readable. ::: js/src/shell/js.cpp @@ +3099,3 @@ > > /* NB: The next condition also filter out NaNs. */ > + if (duration > MAX_TIMEOUT_INTERVAL) { This comment about filtering NaN no longer applies right? What will we do for NaN now, does TimeDuration check for it? @@ +3286,5 @@ > static bool > SetTimeoutValue(JSContext* cx, double t) > { > /* NB: The next condition also filter out NaNs. */ > + if (TimeDuration::FromSeconds(t) > MAX_TIMEOUT_INTERVAL) { Same here.
Attachment #8755910 -
Flags: review?(jdemooij) → review+
Comment 3•8 years ago
|
||
Comment on attachment 8755911 [details] [diff] [review] use_TimeStamp_for_watchdogTimeout-v0.diff Review of attachment 8755911 [details] [diff] [review]: ----------------------------------------------------------------- Splendid.
Attachment #8755911 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 4•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12fe0ea05d6669181144ad17652cc0d92a81de8 Bug 1275306 - Part 1: Use TimeDuration for MAX_TIMEOUT_INTERVAL in the JS shell; r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/4942b307cf4a1784168dd7a6d167a0c7a62c6790 Bug 1275306 - Part 2: Use a TimeStamp to implement watchdogTimeout; r=jandem
Comment 5•8 years ago
|
||
Backed out for likely causing intermittent crashes in 13.3.0_7.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/0667549f62c0b41938ec448951e1b476ea7e88f5 https://hg.mozilla.org/integration/mozilla-inbound/rev/8fa26a17137868e2d72a98b294398940855ae8ad The backout of bug 956899 was insufficient.
Flags: needinfo?(terrence)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d58e8b384d3e https://hg.mozilla.org/integration/mozilla-inbound/rev/10a2a0ed4d32
Comment 7•8 years ago
|
||
Relanded the patches because the issue with jsreftests persisted after the backouts.
Flags: needinfo?(terrence)
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d58e8b384d3e https://hg.mozilla.org/mozilla-central/rev/10a2a0ed4d32
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•