Closed Bug 1275306 Opened 3 years ago Closed 3 years ago

Use more TimeDuration and TimeStamp in the shell

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: terrence, Assigned: terrence)

Details

Attachments

(2 files)

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)
Attachment #8755911 - Flags: review?(jdemooij)
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 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+
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
Relanded the patches because the issue with jsreftests persisted after the backouts.
Flags: needinfo?(terrence)
https://hg.mozilla.org/mozilla-central/rev/d58e8b384d3e
https://hg.mozilla.org/mozilla-central/rev/10a2a0ed4d32
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.