876935 - Alarm API should support millisecond resolution

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

AlarmService currently does not support alarms which are added less than a second into the future. It rounds down a Date in ms to a (seconds, nanoseconds) pair as required by HAL, but passes nanoseconds as zero. In our documentation we make no claims about supporting only seconds. But attempting to set an alarm a few milliseconds in the future leads to truncation. But this truncation occurs when AlarmService dispatches the call to HAL, which means from HAL's point of view the alarm is in the past. Yet AlarmService has not errored, because it continues to deal with milliseconds. This is very confusing.

Further the Javascript Date object, and dates and times on most platforms in general have long supported millisecond resolution and it is quite intuitive to use milliseconds.

In addition, I can't think of a single operating system which runs Gecko that does not support millisecond resolution, although precision may not be down to the tens of milliseconds, so there is no technical difficulty in support milliseconds.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Calculate seconds and nanoseconds from date.

Comment 2

[Gene Lian (I already quit Mozilla)]

Comment on attachment 755069 [details] [diff] [review]
Calculate seconds and nanoseconds from date.

Review of attachment 755069 [details] [diff] [review]:
-----------------------------------------------------------------

Review- because the calculation is wrong. It's no big deal, though. :)

::: dom/alarm/AlarmService.jsm
@@ +87,5 @@
>    },
>    set _currentAlarm(aAlarm) {
>      this._alarm = aAlarm;
>      if (!aAlarm)
>        return;

Since you're here, let's do:

if (!aAlarm) {
  return;
}

@@ +90,5 @@
>      if (!aAlarm)
>        return;
>  
> +    var alarmTimeInMs = this._getAlarmTime(aAlarm);
> +    var ns = (alarmTimeInMs % 1000) * 1000;

s/var/let

Wait! 1 millisecond = 1000000 nanoseconds. Right? It should be:

let ns = (alarmTimeInMs % 1000) * 1000000;

Btw, time is too long for me to recall why I didn't do this since it's very trivial to do so. Just wondering if I intentionally did it for some reasons (or not). Please make sure the alarm still fires on time while considering nanoseconds. Thanks!

@@ +96,1 @@
>        throw Components.results.NS_ERROR_FAILURE;

Please also add |{...}| for this one-line if block. Thanks!

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Calculate seconds and nanoseconds from date.

Ugh! Stupid mistake.

Comment 4

[Gene Lian (I already quit Mozilla)]

Comment on attachment 755463 [details] [diff] [review]
Calculate seconds and nanoseconds from date.

Review of attachment 755463 [details] [diff] [review]:
-----------------------------------------------------------------

Nice! Thanks Nikhil. Please go ahead to land this once you're sure alarms can still fire on time while taking the nanosecond resolution into consideration.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

http://hg.mozilla.org/projects/birch/rev/b787552d00b2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b787552d00b2