Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Description • 10 years ago

Bug 77992 introduced an additional TimeStamp members to events. However, it is simply set to TimeStamp::Now() for all events except native Windows events where we attempt to convert the OS event time to an equivalent TimeStamp value.

We should do the same for all other platforms and then turn on the dom.event.highrestimestamp.enabled for all non-release builds.

Work began for this in bug 77992 but has been split off here.

Existing working:

* attachment 8439751 [details] [diff] [review]
Part 1: Factor out a common utility class for converting wrapping native times to TimeStamps
Review feedback: Bug 77992 comment 109

* attachment 8439753 [details] [diff] [review]
Part 2: Convert GTK native event times to timestamps
Review feedback: Bug 77992 comment 111
                 Bug 77992 comment 112

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 1 • 10 years ago

This just moves the code from widget/windows/nsWindow.cpp to a template class in
widget/xpwidgets/WrappingTimeConverter.h so that we can reuse this code for
other platforms (GTK at least).

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 2 • 10 years ago

This patch addresses the remaining review feedback from bug 77992 comment 109 not already addressed in attachment 8441885 [details] [diff] [review].

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 3 • 10 years ago

Address review feedback from bug 77992 comment 111 except for the matter of skewing.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 4 • 10 years ago

One follow-up comment on part 3 from the review on bug 77992.

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #111)
...
> >+namespace mozilla {
> >+class TimeStamp;
> >+}
> 
> >+    mozilla::TimeStamp GetEventTimeStamp(guint32 aEventTime);
> 
> I didn't know it was possible to forward declare a return value.

Yes, you can use an incomplete type for the return value of a function declaration (but obviously not a function definition).

Some discussion about where this is defined: http://stackoverflow.com/questions/9241255/where-is-the-standard-wording-allowing-incomplete-types-in-function-declarations

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 6 • 10 years ago

I started working on addressing skew between the two time sources in WIP part 4. I wonder:

a) Does the logic there resemble what you had in mind? I haven't checked it over at all but I wonder if you can tell me if I'm on the right track.

b) Any suggestions about how to do an async call to get the windowing time? I can always queue an nsRunnable but it'd need the GdkWindow which might disappear in the meantime; and that doesn't sound like what you were suggesting anyway.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 7 • 10 years ago

Minor formatting tweaks

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 8 • 10 years ago

Incorporate feedback from Karl

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 9 • 10 years ago

Comment on attachment 8446930 [details] [diff] [review]
part 4 - Adjust for skew

I've updated part 4 with the following feedback from Karl received on IRC:

<karlt> birtles: see https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkwindow-x11.c#n5573 for the request part, and https://developer.gnome.org/gtk3/3.12/GtkWidget.html#GtkWidget-property-notify-event can be processed when it arrives
<karlt> birtles: for forwards skew, i suggest returning roughlyNow for the timestamp, and adjusting the conversion so that aTime corresponds to roughlyNow
<karlt> birtles: that is simple, and avoids the chance of returning a timestamp earlier than that of the previous event
<karlt> birtles: if you make timeToNowByTimeStamp and deltaFromNow have Time units, then i think the wrapping code below can be considerably simplified
<karlt> birtles: if there is no skew, then timestamp to return is conceptually roughlyNow - TimeDuration::FromMilliseconds(deltaFromNow)
<karlt> i guess that may work with doubles too, but i suspect that timeSinceReference wrapping won't be an issue if working in Time units

At this stage I'd like to get an idea if this matches roughly what Karl had in mind.

Areas still needing work:
* Windows. I haven't implemented an async time update function for Windows yet.
* I need to verify if this does the right thing when we wake up the laptop from sleeping for 80 days or so. I'm pretty sure the old patch did the right thing there but I'm not sure if the simplified wrapping handling does that correctly.

	Karl Tomlinson (:karlt)
	Comment 10 • 10 years ago

Comment on attachment 8446930 [details] [diff] [review]
part 4 - Adjust for skew

(In reply to Brian Birtles (:birtles) from comment #9)
> * I need to verify if this does the right thing when we wake up the laptop
> from sleeping for 80 days or so. I'm pretty sure the old patch did the right
> thing there but I'm not sure if the simplified wrapping handling does that
> correctly.

I don't think there is a "right thing" for this situation.  The time sources
are monotonically increasing, but they are not expected to align with real
time over periods where the system has been asleep.  We can just do our best
to make TimeStamp differences align closely with real time while the system is
awake.

>+    void UpdateReferenceTimeAsync()
>+    {
>+        // Check if there is a request in progress
>+        if (sPropertyNotifyHandlerId)
>+            return;
>+
>+        sPropertyNotifyHandlerId = g_signal_connect(&mWidget,
>+            "property-notify-event",
>+            G_CALLBACK(&CurrentX11TimeGetter::PropertyNotifyHandler),
>+            nullptr);
>+        // Getting X11 server time will trigger property-notify-event.
>+        gdk_x11_get_server_time(&mWindow);

We want the XChangeProperty() part of
https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkwindow-x11.c?id=65022c40786bfad21d68d16cc229534451e938d5#n5573
without the XIfEvent() waiting for the reply.

This request/response process will take some time to get the result, so think
in terms of upper and lower bounds for the TimeStamp offset from a given Time.
If we know a TimeStamp is created after a Time is created, as happens when
events are processed, then that provides an upper bound.  What we need here is
a lower bound, so the TimeStamp must be created before this request is sent.

Perhaps an XFlush() after the request would give a little more precision
here as it ensures the request queue is sent immediately.

The other thing to watch out for here is that the window may be destroyed
before the property-notify event is received, in which case it will never be
received.  It may be simpler to track things on each window separately.  If
things stall because one window is destroyed, then they will continue to
happen on other windows.

>+    static gboolean PropertyNotifyHandler(GtkWidget *aWidget,
>+                                          GdkEvent  *aEvent,
>+                                          gpointer   aUserData)

This should check that aEvent->atom matches our special property and we are
not getting the time of change of some other property.

Need to watch out for events bubbled from child windows too.
If they return TRUE when the atom matches, then I think we're OK.

>     // time (i.e. less than half the range) and the timestamp is only a little
>     // bit past the reference timestamp (again less than half the range).
>     // In that case we should adjust the reference time so it is the
>     // earliest time.
>     Time timeSinceReference = aTime - mReferenceTime;
>     if (timeSinceReference > kTimeHalfRange &&
>         roughlyNow - mReferenceTimeStamp <
>           TimeDuration::FromMilliseconds(kTimeHalfRange)) {
>-      UpdateReferenceTime(aTime, aGetCurrentTimeFunc);
>+      UpdateReferenceTime(aTime, aCurrentTimeGetter);
>       timeSinceReference = aTime - mReferenceTime;
>     }

I wonder whether this code is still required.

>+    // i. The duration from the reference time to the passed-in time.
>+    //    (Already calculated above as timeSinceReference)
>+    // ii. The duration from the reference timestamp to the current time
>+    //     based on TimeStamp::NowLoRes.
>+    //     (Calculated below as timeToNowByTimeStamp)
>+    //
>+    // If (ii) - (i) is negative (and greater in magnitude than some tolerance
>+    // to account for the inaccuracy in NowLoRes), then the source of Time
>+    // values is getting "ahead" of TimeStamp. We call this "forwards" skew
>+    // below.

>+    // For the reverse case, if (ii) - (i) is getting greater and greater then
>+    // we can assume we have "backwards" skew.

We *may* have backwards skew.  We can't "assume" because event Times will (or
should) always be in the past.

>+    Time timeToNowByTimeStamp =
>+      static_cast<Time>((roughlyNow - mReferenceTimeStamp).ToMilliseconds());
>+    Time deltaFromNow = timeToNowByTimeStamp - timeSinceReference;

"A prvalue of a floating point type can be converted to a prvalue of an
integer type. The conversion truncates; that is, the fractional part is
discarded. The behavior is undefined if the truncated value cannot be
represented in the destination type."

The Integral Conversions section describes unsigned conversions with the expected modulo behavior so I guess the modulo behavior is not defined for double to guint32 conversions.

if ToMilliseconds() might return > UINT32_MAX, then convert to uint64_t before
guint32.

>+    // Check for forwards skew (since deltaFromNow is an unsigned integer,we
>+    // detect the minus case by seeing if it has underflowed).
>+    if (deltaFromNow > kTimeHalfRange) {
>+      // Only adjust the reference time if skew is greater than the tolerance
>+      if (deltaFromNow < kTimeRange - kTolerance) {

If aTime is ahead of roughlyNow, then I think it is reasonable to assume that
the offset needs updating.  On WINNT, aTime and NowLoRes() are both based on
GetTickCount() for the source.  NowLoRes() just includes an offset based on a
QueryPerformanceCounter() call made some time in the past.  On other systems, NowLoRes() has full resolution.

Making this assumption means that the TimeStamp returned from this method will
not go backwards when kTolerance is reached.

>+      // Accumulate lag if the delta is growing, otherwise reset it
>+      if (deltaFromNow > mPrevDeltaFromNow) {
>+        mAccumulatedLag += deltaFromNow - mPrevDeltaFromNow;
>+      } else {
>+        mAccumulatedLag = 0;
>+      }

deltaFromNow is likely to oscillate a bit.  When two events have the same
aTime it will increase; when the next event has a newer aTime, it will likely
decrease.  That's going to mean that this is reset frequently, and gradual
skew will not be detected.

It's difficult to choose what to do here.  Heuristics from the previous async
update will give a hint as to the amount of lag to expect, but then a long lag
during a busy time could lead to inaccuracies during not so busy times.
Perhaps it's easier to use some combination of a small tolerance and a period.
e.g. start an update when aTime has advanced a certain period since starting
the last update, either forward or reverse.

>+  UpdateReferenceTimeToNow(Time aNow) {

This will provide a lower bound, so should compare the offset with the current
and only change it if the bound is higher.

	Karl Tomlinson (:karlt)
	Comment 11 • 10 years ago

If we assume that Time and TimeStamp will only go out of sync slowly, then any
aTime that is within tolerance (or ahead) of TimeStamp indicates that it won't
be necessary to check for reverse skew for a reasonably large interval.  If
the interval is large enough and times are in sync, then most likely the event
queue will empty before it expires and the next event after the queue empties
will be processed quickly.

That I think suggests a system with frequent resetting, kind of similar to
what's in the latest patch, but with one fewer levels of differencing.

The exception, I assume is when waking from sleep.  The length of the interval
could be moderated against that.  I think we can tolerate some stale
timestamps of events shortly after resume - we won't know for sure
whether they occurred before or after suspend anyway.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 12 • 10 years ago

Fix bitrot

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 13 • 10 years ago

Address feedback

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 14 • 10 years ago

(In reply to Karl Tomlinson (back July 17 :karlt) from comment #10)
> >+    void UpdateReferenceTimeAsync()
> >+    {
> >+        // Check if there is a request in progress
> >+        if (sPropertyNotifyHandlerId)
> >+            return;
> >+
> >+        sPropertyNotifyHandlerId = g_signal_connect(&mWidget,
> >+            "property-notify-event",
> >+            G_CALLBACK(&CurrentX11TimeGetter::PropertyNotifyHandler),
> >+            nullptr);
> >+        // Getting X11 server time will trigger property-notify-event.
> >+        gdk_x11_get_server_time(&mWindow);
> 
> We want the XChangeProperty() part of
> https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkwindow-x11.
> c?id=65022c40786bfad21d68d16cc229534451e938d5#n5573
> without the XIfEvent() waiting for the reply.

Fixed.

> This request/response process will take some time to get the result, so think
> in terms of upper and lower bounds for the TimeStamp offset from a given
> Time.
> If we know a TimeStamp is created after a Time is created, as happens when
> events are processed, then that provides an upper bound.  What we need here
> is
> a lower bound, so the TimeStamp must be created before this request is sent.

I've rewritten the code in these terms but see my comment later on: I don't think the code is using these bounds in the way you intended.
 
> Perhaps an XFlush() after the request would give a little more precision
> here as it ensures the request queue is sent immediately.

Fixed.

> The other thing to watch out for here is that the window may be destroyed
> before the property-notify event is received, in which case it will never be
> received.  It may be simpler to track things on each window separately.  If
> things stall because one window is destroyed, then they will continue to
> happen on other windows.

Fixed. This is quite a significant change. I'm not completely sure the current code is robust with regards to object lifetimes but I've added a comment to that effect with an alternative approach outlined.

> >+    static gboolean PropertyNotifyHandler(GtkWidget *aWidget,
> >+                                          GdkEvent  *aEvent,
> >+                                          gpointer   aUserData)
> 
> This should check that aEvent->atom matches our special property and we are
> not getting the time of change of some other property.

Fixed.

> Need to watch out for events bubbled from child windows too.
> If they return TRUE when the atom matches, then I think we're OK.

Fixed (I added a check on the widget identity which I guess excludes child windows?)

> >     // time (i.e. less than half the range) and the timestamp is only a little
> >     // bit past the reference timestamp (again less than half the range).
> >     // In that case we should adjust the reference time so it is the
> >     // earliest time.
> >     Time timeSinceReference = aTime - mReferenceTime;
> >     if (timeSinceReference > kTimeHalfRange &&
> >         roughlyNow - mReferenceTimeStamp <
> >           TimeDuration::FromMilliseconds(kTimeHalfRange)) {
> >-      UpdateReferenceTime(aTime, aGetCurrentTimeFunc);
> >+      UpdateReferenceTime(aTime, aCurrentTimeGetter);
> >       timeSinceReference = aTime - mReferenceTime;
> >     }
> 
> I wonder whether this code is still required.

Removed.

> >+    // i. The duration from the reference time to the passed-in time.
> >+    //    (Already calculated above as timeSinceReference)
> >+    // ii. The duration from the reference timestamp to the current time
> >+    //     based on TimeStamp::NowLoRes.
> >+    //     (Calculated below as timeToNowByTimeStamp)
> >+    //
> >+    // If (ii) - (i) is negative (and greater in magnitude than some tolerance
> >+    // to account for the inaccuracy in NowLoRes), then the source of Time
> >+    // values is getting "ahead" of TimeStamp. We call this "forwards" skew
> >+    // below.
> 
> >+    // For the reverse case, if (ii) - (i) is getting greater and greater then
> >+    // we can assume we have "backwards" skew.
> 
> We *may* have backwards skew.  We can't "assume" because event Times will (or
> should) always be in the past.

Fixed.

> >+    Time timeToNowByTimeStamp =
> >+      static_cast<Time>((roughlyNow - mReferenceTimeStamp).ToMilliseconds());
> >+    Time deltaFromNow = timeToNowByTimeStamp - timeSinceReference;
> 
> "A prvalue of a floating point type can be converted to a prvalue of an
> integer type. The conversion truncates; that is, the fractional part is
> discarded. The behavior is undefined if the truncated value cannot be
> represented in the destination type."
> 
> The Integral Conversions section describes unsigned conversions with the
> expected modulo behavior so I guess the modulo behavior is not defined for
> double to guint32 conversions.
> 
> if ToMilliseconds() might return > UINT32_MAX, then convert to uint64_t
> before
> guint32.

Fixed.

> >+    // Check for forwards skew (since deltaFromNow is an unsigned integer,we
> >+    // detect the minus case by seeing if it has underflowed).
> >+    if (deltaFromNow > kTimeHalfRange) {
> >+      // Only adjust the reference time if skew is greater than the tolerance
> >+      if (deltaFromNow < kTimeRange - kTolerance) {
> 
> If aTime is ahead of roughlyNow, then I think it is reasonable to assume that
> the offset needs updating.  On WINNT, aTime and NowLoRes() are both based on
> GetTickCount() for the source.  NowLoRes() just includes an offset based on a
> QueryPerformanceCounter() call made some time in the past.  On other
> systems, NowLoRes() has full resolution.

Fixed.

> >+  UpdateReferenceTimeToNow(Time aNow) {
> 
> This will provide a lower bound, so should compare the offset with the
> current
> and only change it if the bound is higher.

I'm not sure I understand this comment. For now I just took the midpoint of the two bounds as the time to use.

(In reply to Karl Tomlinson (back July 17 :karlt) from comment #11)
> If we assume that Time and TimeStamp will only go out of sync slowly, then
> any
> aTime that is within tolerance (or ahead) of TimeStamp indicates that it
> won't
> be necessary to check for reverse skew for a reasonably large interval.  If
> the interval is large enough and times are in sync, then most likely the
> event
> queue will empty before it expires and the next event after the queue empties
> will be processed quickly.
> 
> That I think suggests a system with frequent resetting, kind of similar to
> what's in the latest patch, but with one fewer levels of differencing.
> 
> The exception, I assume is when waking from sleep.  The length of the
> interval
> could be moderated against that.  I think we can tolerate some stale
> timestamps of events shortly after resume - we won't know for sure
> whether they occurred before or after suspend anyway.

So, can I check the logic is something like:

ctor:
  Time mLastBackwardsSkewCheck = 0;
  
static const Time kBackwardsSkewCheckInterval = 10000;
  
if (<<forwards skew>>) {
  // Update time
  mLastBackwardsSkewCheck = aTime;
} else if (aTime - mLastBackwardsSkewCheck > kBackwardsSkewCheckInterval) {
  if (deltaFromNow > mPrevDeltaFromNow &&
      deltaFromNow - mPrevDeltaFromNow > kTolerance) {
    // Trigger async time update
  }
  mLastBackwardsSkewCheck = aTime;
}

i.e. drop the accumulated lag

In that case, should we drop the async time update as well?

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 15 • 10 years ago

IRC discussion with Karl:

14:46 <karl> birtles: i don't think i understand the "drop the async time update" question
14:47 <karl> birtles: i assume we still need the async time update
14:47 <karl> birtles: do you mean the initial sync update, perhaps?
14:47 <•birtles> karl: I thought the idea was that we'd wait a long time in-between detecting backwards skew so that by the time we came to check for it the event queue would be empty
14:48 <•birtles> karl: and the reason we added the async time update was because we assumed it would take place at a time when the system was already bogged down dispatching events
14:48 <•birtles> karl: but I'm happy to leave it in
14:50 <karl> what i meant by empty, was empty at some point in time, but it may not be empty when actually checked, and still there may be non-event things happening that we don't want to jank
14:50 <•birtles> sure
14:51 <karl> birtles: can we drop mPrevDeltaFromNow?
14:51 <•birtles> karl: I guess so
14:52 <•birtles> hmmm... let me think
14:53 <•birtles> karl: yeah, I think that works
14:55 <karl> birtles: sorry i don't have time to look at the whole patch, but i didn't understand the need for mPrevDeltaFromNow from the previous version
14:55 <•birtles> karl: no, I think you're right, I don't think we want it
14:57 <karl> birtles: my other comment on abbreviated algorithm in the bug comment is that there needs to be some path where mLastBackwardsSkewCheck is not set,
14:57 <karl> birtles: but otherwise, yes, that kind of thing
14:58 <karl> birtles: i'm going to be away for a week; are there any other specific questions?
14:58 <•birtles> karl: no, that's probably it
14:58 <•birtles> karl: oh wait
14:59 <•birtles> karl: regarding the lifetime issue, did you see the "REVIEW" comment in the patch about adding a method to nsWindow?
14:59 <•birtles> karl: I think that approach is probably more robust
14:59 <•birtles> so if you agree I'll work that in along with the simplified backwards skew logic and put up a patch for review while you're away
14:59 <•birtles> I hope that sort of timeframe is not an issue for the MSE work
15:02 <karl> birtles: i'm not sure we need this
15:02 <karl> birtles: PropertyNotifyHandler has a widget
15:03 <karl> ok, you're checking its the same widget
15:03 <•birtles> right, but as per the comment, we could navigate from widget -> nsWindow -> CurrentTimeGetter
15:03 <karl> birtles: i think if the widget is destroyed, we won't get the event at all
15:03 <•birtles> rather than using userdata
15:04 <•birtles> I'm ok with either approach
15:04 <•birtles> but navigating from the widget seemed more robust than relying on the userdata to still be pointing to valid memory
15:04 <karl> birtles: is it just that you want to pass the GtkWidget* to PropertyNotifyHandler?
15:05 <•birtles> no I want to pass the CurrentTimeGetter to it
15:05 <karl> birtles: that will be called with the same GtkWidget* that the signal handler was registered with
15:05 <•birtles> the check for widget identity is not really important
15:05 <•birtles> it's more of a sanity check
15:05 <karl> birtles: but why do you want the CurrentTimeGetter?
15:06 <•birtles> because it has some internal state I want to access/update
15:06 <•birtles> like the notification ID for unregistering
15:07 <•birtles> err disconnecting
15:07 <•birtles> karl: and more importantly, the time recorded before we sent the async request to get the time
15:09 <karl> birtles: can we store those on the nsWindow?
15:09 <•birtles> karl: if we're going to do that, we may as well just add a method to the nsWindow to return the CurrentTimeGetter
15:11 <•birtles> karl: we could store it on the SystemTimeConverter
15:11 <karl> the SystemTimeConverter may have more than 1 in flight
15:12 <•birtles> karl: really?
15:12 <•birtles> karl: it currently drops additional requests if it already has one in flight
15:13 <karl> birtles: there was the issue with the window getting destroyed and not getting the reply
15:13 <•birtles> karl: but now the SystemTimeConverter is per-window
15:14 <karl> i thought the CurrentTimeGetter was per window
15:14 <•birtles> karl: both are per-window
15:15 <karl> birtles: i think we need a single SystemTimeConverter to get monotonic times
15:15 <•birtles> karl: ok
15:16 <karl> well, to know what events from different windows still have suitable relative times
15:17 <•birtles> all the events get converted to a time relative to navigationStart
15:17 <•birtles> so does it really make sense to compare events from different windows?
15:17 <•birtles> since navigationStart is per-document / worker
15:17 <karl> birtles: popup windows have the same document as their parent
15:18 <•birtles> right but I wonder how important monotonicity is even in that case since we don't guarantee it for all sorts of other circumstances
15:18 <•birtles> e.g. OS events vs browser events
15:19 <karl> we can't guarantee it for events from different sources
15:19 <karl> but mouse events should have monotonically increasing time stamps
15:20 <•birtles> ok, I can try to make us use a single SystemTimeConverter but I'm not sure where I'll find time yet
15:20 <karl> g_signal_handlers_disconnect_by_func means that it is not necessary to install the handler id
15:20 <karl> so the main piece of info required is the time from Now()
15:22 <karl> unfortunately the user_data is only 4 bytes on some systems, so that gets a bit fiddly
15:23 <•birtles> like you said, we can stick it on the nsWindow
15:23 <karl> i don't think it matters whether it or the whole getter is on the nsWindow
15:23 <karl> CurrentX11TimeGetter, i mean
15:24 <karl> birtles: thanks for your email, btw, and all this work
15:25 <•birtles> karl: not at all, thanks for your help
15:25 <•birtles> unfortunately July and August are quite busy for me
15:25 <karl> birtles: perhaps it's best to wait until i have a proper look at your patch anyway

In summary, remaining work is:

* Drop mPrevDeltaFromNow
* Make SystemTimeConverter a singleton
* Rework lifetime management
 - Use g_signal_handlers_disconnect_by_func to disconnect
 - Store start of async time update operation on the window (or some property of the window like the CurrentTimeGetter)
 - Then the callback for the async time update operation could be entirely covered by a static method

	Karl Tomlinson (:karlt)
	Comment 16 • 10 years ago

Comment on attachment 8452093 [details] [diff] [review]
part 4 - Adjust for skew

(In reply to Brian Birtles (:birtles) from comment #14)
> (In reply to Karl Tomlinson (:karlt) from comment #10)
> This request/response process will take some time to get the result, so
> think in terms of upper and lower bounds for the TimeStamp offset from a
> given Time.  If we know a TimeStamp is created after a Time is created, as
> happens when events are processed, then that provides an upper bound.  What
> we need here is a lower bound, so the TimeStamp must be created before this
> request is sent.

> > >+  UpdateReferenceTimeToNow(Time aNow) {
> > 
> > This will provide a lower bound, so should compare the offset with the
> > current and only change it if the bound is higher.
> 
> I'm not sure I understand this comment. For now I just took the midpoint of
> the two bounds as the time to use.

The UpdateReferenceTimeWithLowerBound() method can't make Time time go
backwards relative to TimeStamp time; if it did subsequent events may have
TimeStamps earlier than previous events (apparent monotonicity is lost).  It
never needs to do this either because Time time will be moved backwards if
necessary when event Times are converted.

If the values passed to UpdateReferenceTimeWithLowerBound() would make Time
time go backwards relative to TimeStamps, then Time time has not fallen behind Timestamp time, no update is required, and the reference times should not be changed.

(One benefit of using the async update is that native events either side of
update will not be generated from the same user input action, and so will not
need to maintain equal timestamps during the update to the conversion offset.
Equal timestamps may be used by content to detect related events.) 

(In reply to Brian Birtles (:birtles, away 1-11 Aug) from comment #15)
> * Rework lifetime management
>  - Use g_signal_handlers_disconnect_by_func to disconnect
>  - Store start of async time update operation on the window (or some
> property of the window like the CurrentTimeGetter)
>  - Then the callback for the async time update operation could be entirely
> covered by a static method

One option is to connect the signal handler in nsWindow::Create() to
eventWidget (when non-null), and then there is no need to remove it again.
The nsWindow can be obtained from the GdkWindow* in the event.

It may be simpler to pass the nsWindow* as the CurrentTimeGetter for
SystemTimeConverter::GetTimeStampFromSystemTime() and implement the necessary
methods on nsWindow, than to have a separate CurrentX11TimeGetter class.

>+        // widget and get its CurrentTimeGetter. That might be safer if
>+        // there's ever a chance that PropertyNotifyHandler handler could be
>+        // called (with appropriate arguments) after the nsWindow has been
>+        // destructed

FWIW, the property-notify-event signal won't be sent if the window is
destroyed.

>+    static gboolean PropertyNotifyHandler(GtkWidget *aWidget,
>+                                          GdkEvent  *aEvent,
>+                                          gpointer   aUserData)
>+    {
>+        if (!aWidget || !aUserData) {
>+            return FALSE;
>+        }
>+
>+        GdkEventProperty* propertyEvent =
>+            reinterpret_cast<GdkEventProperty*>(aEvent);

GdkEvent is a union so convention is to use aEvent->property instead of a
cast.

However the signature of PropertyNotifyHandler() is never checked.  Other
event signal handlers in this file just declare aEvent as a pointer to the
documented event type, which would be GdkEventProperty *aEvent here.

>+    static Atom TimeStampPropAtom(GtkWidget& aWidget) {

Convention is to pass GtkWidget by pointer; otherwise it looks like a copy.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 26 • 10 years ago

This round of patches should address all the feedback discussed in comment 14, comment 15, and comment 16.

After this, however, there is still work to be done for other platforms (OSX, Android, and, for Window, just the clock skew adjustment part).

	Karl Tomlinson (:karlt)
	Comment 27 • 10 years ago

Comment on attachment 8488354 [details] [diff] [review]
part 1 - Factor out a common utility class for converting wrapping native times to TimeStamps

>+#ifndef __mozilla_widget_SystemTimeConverter_h__

The style guide now requests just SystemTimeConverter_h, for an unexported header included as "SystemTimeConverter.h".

	Karl Tomlinson (:karlt)
	Comment 28 • 10 years ago

Comment on attachment 8488355 [details] [diff] [review]
part 2 - Add an assertion that SystemTimeConverter's template parameter is unsigned

>+    MOZ_ASSERT(!IsSigned<Time>::value, "Expected Time to be unsigned");

I think you should be able to use static_assert here.
See for example
http://hg.mozilla.org/mozilla-central/annotate/5bd6e09f074e/js/src/vm/NumericConversions.h#l114

	Karl Tomlinson (:karlt)
	Comment 29 • 10 years ago

Comment on attachment 8488358 [details] [diff] [review]
part 4 - Convert GTK native event times to timestamps

>+    CurrentX11TimeGetter(GdkWindow* aWindow) : mWindow(*aWindow) { }
>+    guint32 operator() () {
>+        return gdk_x11_get_server_time(&mWindow);
>+    }
>+
>+private:
>+    GdkWindow& mWindow;

Is there a reason to convert from pointer to reference and back?
I think it would be simpler and easier to follow with a pointer.
If you want to indicate that it will always be the same GdkWindow object,
then a const could be added (but I'm not saying that is necessary).

>+TimeStamp
>+nsWindow::GetEventTimeStamp(guint32 aEventTime)
>+{
>+    if (MOZ_UNLIKELY(!mGdkWindow)) {
>+        // nsWindow has been Destroy()ed. We don't expect to get events in this
>+        // case but if we do, just return the current timestamp.
>+        return TimeStamp::Now();
>+    }

We do expect this to be possible due to DispatchDragEvent(), so just leave the
comment as "nsWindow has been Destroy()ed."

>+    if (aEventTime == 0) {
>+        // Some X11 and GDK events may be received with a time of 0 to indicate
>+        // that they are synthetic events. Some input method editors do this.
>+        // In this case too, just return the current timestamp.
>+        return TimeStamp::Now();
>+    }

I liked having a special null timestamp for synthetic events where there was
no known timestamp.  However, I guess it's not exactly clear whether it is
better to have a timestamp that is close but potentially out of order.

	Karl Tomlinson (:karlt)
	Comment 30 • 10 years ago

Comment on attachment 8488360 [details] [diff] [review]
part 6 - Make SystemTimeConverter detect clock skew

>+    // Check for forwards skew (since deltaFromNow is an unsigned integer, we
>+    // detect the minus case by seeing if it has underflowed).
>+    if (deltaFromNow > kTimeHalfRange) {
>+      if (kTimeRange - deltaFromNow > kTolerance) {
>+        // Make aTime correspond to roughlyNow
>+        UpdateReferenceTime(aTime, roughlyNow);
>+        // Then set deltaFromNow to zero to ensure the timestamp
>+        // we end up returning is equal to roughlyNow.
>+        deltaFromNow = 0;
>+      }
>+      // We didn't have backwards skew so don't bother checking for
>+      // backwards skew again for a little while.
>+      mLastBackwardsSkewCheck = aTime;

Attachment 8452093 [details] [diff] removed the kTolerance check for forwards skew, but now it
is back again.
Is this the correct patch?

Also deltaFromNow was used for the return value in attachment 8452093 [details] [diff] [review], but now
it is no longer used and mReferenceTimeStamp +
TimeDuration::FromMilliseconds(timeSinceReference) will not be correct with
mReferenceTimeStamp reset to roughlyNow.

It's probably simplest and clearest to return early here with roughlyNow.

>+    // Check for backwards skew if we haven't checked in a while
>+    } else if (aTime - mLastBackwardsSkewCheck > kBackwardsSkewCheckInterval) {
>+      if (deltaFromNow > kTolerance) {
>+        aCurrentTimeGetter.GetTimeAsyncForPossibleBackwardsSkew(roughlyNow);
>+      }
>+      mLastBackwardsSkewCheck = aTime;
>     }

This will check deltaFromNow for a single event against kTolerance, every
kBackwardsSkewCheckInterval.  In busy times, it is likely that some events are
going to have Times in the past, so it is possible that this will check about
every 2 seconds.

I think the rechecking can be avoided more often.
What I had in mind was that if any event was within tolerance, then we know
that we don't have clock skew and mLastBackwardsSkewCheck can be reset to
avoid checking for at least another kBackwardsSkewCheckInterval.

  } else if (deltaFromNow <= kTolerance) {
    mLastBackwardsSkewCheck = aTime;
  } else if (aTime - mLastBackwardsSkewCheck > kBackwardsSkewCheckInterval) {
    aCurrentTimeGetter.GetTimeAsyncForPossibleBackwardsSkew(roughlyNow);
    mLastBackwardsSkewCheck = aTime;
  }

(This is still assuming that it takes less than kTolerance for the events to
get from the system to Gecko, but long latencies are probably harder to
accommodate.)

>+    return
>       mReferenceTimeStamp + TimeDuration::FromMilliseconds(timeSinceReference);

I suspect this would not be correct when timeSinceReference overflows.

I liked the roughlyNow - TimeDuration::FromMilliseconds(deltaFromNow) version
better.

>+    // Suppose when we get (c), we detect the backwards skew and trigger
>+    // an async request for the current time.

"we detect possible backwards skew" would be more accurate.

>+    // If the duration (aLowerBound - mReferenceTimeStamp) is greater than
>+    // (aReferenceTime - mReferenceTime) then we know we have backwards skew.

Nice explanation.

>+    if (aReferenceTime - mReferenceTime
>+        > (aLowerBound - mReferenceTimeStamp).ToMilliseconds()) {
>+      return;
>     }

but I think unsigned modulo 2^N arithmetic like the deltaFromNow code is
needed to handle overflow in aReferenceTime - mReferenceTime.

Perhaps a GetInterScaleDelta() method, for want of a better name, can be
shared between GetTimeStampFromSystemTime() and CompensateForBackwardsSkew().

>+    // We have backwards skew, use the midpoint of the timestamp interval
>+    // as the new reference timestamp.
>+    TimeStamp upperBound = TimeStamp::Now();
>+    TimeStamp rangeMidPoint =
>+      aLowerBound + ((upperBound - aLowerBound) / 2);
>+    UpdateReferenceTime(aReferenceTime, rangeMidPoint);

I'm not comfortable including this upper bound here.  aReferenceTime may have
been waiting in the event queue for some time, and so this may move
mReferenceTime significantly further forward than desired.

UpdateReferenceTime(aReferenceTime, aLowerBound) is guaranteed to improve the
estimate in reference difference, so I think just take that free improvement
here.  There will be another chance to further improve later.

	Karl Tomlinson (:karlt)
	Comment 31 • 10 years ago

Comment on attachment 8488361 [details] [diff] [review]
part 7 - Store the CurrentX11TimeGetter as a member object on nsWindow

The only piece of state that CurrentX11TimeGetter really provides is the
timestamp.  It might be nice to avoid the extra allocation and pointer
indirection to allocate this separately from the nsWindow.  Making the
CurrentX11TimeGetter object instead of its pointer a member of nsWindow would save the allocation, but still requires a container pointer or container_of() conversion.  Private inheritance with static_casts is an option, if you want to keep the CurrentTimeGetter code separate, but I think it would be simpler to pass the nsWindow as the CurrentTimeGetter for
SystemTimeConverter::GetTimeStampFromSystemTime() and implement the necessary
methods on nsWindow, than to have a separate CurrentX11TimeGetter class.
However, the existing design keeps CurrentX11TimeGetter out of the header file
and nicely contained, so leave the existing design, if you like.

>asynchronous request of the current time, this patch revises the lifetime of
>such objects so that they are stored one nsWindow.

*on* nsWindow.

>+nsWindow::GetCurrentTimeGetter() {
>+    if (MOZ_UNLIKELY(!mGdkWindow)) {
>+        return nullptr;
>+    }

No need to null-check mGdkWindow in GetCurrentTimeGetter() because the caller
already checks.  An assert would be fine.

	Karl Tomlinson (:karlt)
	Comment 32 • 10 years ago

Comment on attachment 8488362 [details] [diff] [review]
part 8 - Add ability for CurrentX11TimeGetter to perform an asynchronous request of the current time

>+        guchar c = 'a';

We can't really avoid having both GDK and Xlib types and methods, but this
variable is used only with XChangeProperty, so please make this "unsigned
char".

>+    static gboolean PropertyNotifyHandler(GtkWidget* aWidget,
>+                                          GdkEventProperty* aEvent,
>+                                          CurrentX11TimeGetter&
>+                                              aCurrentTimeGetter)

If this method needs a CurrentX11TimeGetter or nsWindow, and I expect it does,
then remove static and make it an instance method.

>+    static Atom TimeStampPropAtom(GtkWidget* aWidget) {
>+        return gdk_x11_get_xatom_by_name_for_display(
>+            gtk_widget_get_display(aWidget),
>+            "GDK_TIMESTAMP_PROP");
>+    }
>+
>     // This is safe because this class is stored as a member of mWindow and
>     // won't outlive it.
>     GdkWindow& mWindow;
>+    GtkWidget& mWidget;

Use gdk_window_get_display() or gdk_display_get_default() instead of
gtk_widget_get_display(), so that mWidget is not required.

> static gboolean
>+property_notify_event_cb(GtkWidget* aWidget, GdkEventProperty* aEvent)
>+{
>+    if (!aWidget)
>+        return FALSE;

No need to null check, GObject signals are always dispatched to objects
(GtkWidget).

>+    nsWindow *window = get_window_for_gtk_widget(aWidget);
>+    if (!window)
>+        return FALSE;

This nsWindow may be a parent window, because the container widget can have
multiple child nsWindows.

The correct nsWindow can be obtained from the GdkWindow* in the event.

>+    CurrentX11TimeGetter* currentTimeGetter = window->GetCurrentTimeGetter();
>+    if (!currentTimeGetter)
>+        return FALSE;

currentTimeGetter won't be null here.

>+        // widget and get its CurrentTimeGetter. That might be safer if
>+        // there's ever a chance that PropertyNotifyHandler handler could be
>+        // called (with appropriate arguments) after the nsWindow has been
>+        // destructed

FWIW, the property-notify-event signal won't be sent if the window is
destroyed.

	Karl Tomlinson (:karlt)
	Comment 33 • 10 years ago

Comment on attachment 8488363 [details] [diff] [review]
part 9 - Return DOMHighResTimeStamp objects from Event.timeStamp on Linux for Nightly/Aurora builds

>+#if defined(XP_WIN) && !defined(RELEASE_BUILD) || defined(MOZ_WIDGET_GTK) && !defined(RELEASE_BUILD)

This is more than 80 columns, but there is no need to have
!defined(RELEASE_BUILD) twice anyway.  I assume we are not going to release on
one platform before another.

  #if (defined(XP_WIN) || defined(MOZ_WIDGET_GTK)) && !defined(RELEASE_BUILD)

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 34 • 9 years ago

This just moves the code from widget/windows/nsWindow.cpp to a template class in
widget/xpwidgets/WrappingTimeConverter.h so that we can reuse this code for
other platforms (GTK at least).

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 35 • 9 years ago

Various operations in the time conversion assume that the native event time is
an unsigned integer so this patch adds and assertion to check this precondition
is met.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 36 • 9 years ago

This patch exploits the fact that the underlying Time type is an unsigned
integer to simplify some of the overflow checks in SystemTimeConvert by relying
on unsigned integer overflow behavior, which, unlike signed integer overflow, is
well-defined.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 37 • 9 years ago

This patch adds a helper class to widgets/gtk/nsWindow.cpp similar to the one in
widgets/windows/nsWindow.cpp and uses it to convert native event times to
mozilla::TimeStamp objects on events returned by this class.

This is similar to the operations performed for Windows native event times that
were added in bug 77992.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 38 • 9 years ago

Previously the CurrentX11TimeGetter and CurrentWindowsTimeGetter classes acted
as functors with a single operator() method. In preparation for handling clock
skew, this patch refactors these two helper classes into regular classes with
two named methods:

  GetCurrentTime which matches the existing operator() method, and

  GetTimeAsyncForPossibleBackwardsSkew which will be used when possible
  backwards skew is detected to fetch the current time asynchronously.

Some renaming is also included to match the expanded role of these classes.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 39 • 9 years ago

This patch revises the logic in SystemTimeConverter to detect backwards and
forwards skew between the two time sources: the native time source (represented
by the Time type) and the time source used to generate TimeStamp objects.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 40 • 9 years ago

In preparation for making CurrentX11TimeGetter capable of performing an
asynchronous request of the current time, this patch revises the lifetime of
such objects so that they are stored on nsWindow.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 44 • 9 years ago

I need to do further testing of these but putting them up for review for now.

	Karl Tomlinson (:karlt)
	Comment 45 • 9 years ago

Comment on attachment 8566867 [details] [diff] [review]
part 6 - Make SystemTimeConverter detect clock skew

>+    // We call (ii) - (i), "deltaFromNow".
>+    //
>+    // Graphically:
>+    //
>+    //                    mReferenceTime              aTime
>+    // Time scale:      ........+.......................*........
>+    //                          |---timeSinceReference--|
>+    //
>+    //                  mReferenceTimeStamp             roughlyNow
>+    // TimeStamp scale: ........+...........................*....
>+    //                          |----timeToNowByTimeStamp---|
>+    //
>+    //                                                  |---|
>+    //                                               deltaFromNow

The diagram is very helpful here.

Having forgotten all this and trying to remember, my first instinct was that
something called "deltaFromNow" would be negative if the time was before now.
I wondered whether "deltaToNow" or "age" might indicate the sign better, but
this will become irrelevant with part 8b anyway.

>+    // If it looks like we might have backwards skew and we haven't checked
>+    // for a while, do it now
>+    if (deltaFromNow > kTolerance &&
>+        aTime - mLastBackwardsSkewCheck > kBackwardsSkewCheckInterval) {
>+      aCurrentTimeGetter.GetTimeAsyncForPossibleBackwardsSkew(roughlyNow);
>+    }
>+    mLastBackwardsSkewCheck = aTime;

This now resets mLastBackwardsSkewCheck whenever not checking, and so I
suspect it is unlikely that a check will ever happen.  See comment 30.

	Karl Tomlinson (:karlt)
	Comment 46 • 9 years ago

Comment on attachment 8566870 [details] [diff] [review]
part 8b - Factor out an InterScaleDelta method

>+  enum InterScaleDeltaResult {
>+    DeltaIsEqual,
>+    TimeDeltaIsGreater,
>+    TimeStampDeltaIsGreater
>+  };
>+
>+  void
>+  GetInterScaleDelta(Time aTimeA, Time aTimeB,
>+                     const TimeStamp& aTimeStampA, const TimeStamp& aTimeStampB,
>+                     Time& aDelta, InterScaleDeltaResult& aDeltaResult) {

How about using the member variables mReferenceTime and mReferenceTimeStamp
instead of passing them in each time for aTimeA and aTimeStampA?

The enum values can then be TimeIsEqual, TimeIsNewer, TimeIsOlder (or
TimeStampIsNewer).

Another option is to make the method

  bool IsTimeNewerThanTimestamp(Time aTime, TimeStamp aTimeStamp,
                                Time* aDeltaResult)

I prefer pointers for out parameters to make it clearer what will be modified.

>+    Time timeDelta = aTimeB - aTimeA;
>+    MOZ_ASSERT(timeDelta <= kTimeHalfRange, "Got a negative time delta");

I don't think we can know and assert this.  We might wake up after half the range has passed.  But that is OK.  See below.

>+    } else if (timeStampDelta > timeDelta) {
>+      aDeltaResult = TimeStampDeltaIsGreater;
>+      aDelta = timeStampDelta - timeDelta;

To avoid problems with wrapping, instead use

    Time timeToTimeStamp = timeStampDelta - timeDelta;
    if (timeToTimeStamp == 0) {
      *aDeltaResult = TimeIsEqual;
      *aDelta = 0;
    } else if (timeToTimeStamp < kTimeHalfRange) {
      *aDeltaResult = TimeIsOlder;
      *aDelta = timeToTimeStamp;
    } else {
      *aDeltaResult = TimeIsNewer;
      *aDelta = -timeToTimeStamp;
    }

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 47 • 9 years ago

Fix bitrot

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 48 • 9 years ago

Fix bitrot

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 49 • 9 years ago

Address review feedback. I'll put this up for review again after a bit more testing.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 50 • 9 years ago

Fix bitrot

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 51 • 9 years ago

Fix bitrot

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 52 • 9 years ago

Fix bitrot and address review feedback

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 53 • 9 years ago

Fix bitrot

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 55 • 9 years ago

Comment on attachment 8646218 [details] [diff] [review]
part 6 - Make SystemTimeConverter detect clock skew

Addressed review feedback from comment 45. (The part about deltaFromNow is dealt with in part 8b)

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 56 • 9 years ago

Comment on attachment 8646748 [details] [diff] [review]
part 8b - Factor out an IsTimeNewerThanTimeStamp method

Addressed review feedback from comment 46 and also update a number of code comments that are no longer accurate.

I didn't change the parameter to a pointer type because I'd rather not pollute this with MOZ_ASSERTs for null checks.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 57 • 9 years ago

I added some debugging statements and ran through a few test cases locally and everything seemed sane.

Currently checking on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1df8a1c59da

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 58 • 9 years ago

Also, Karl, I added a line to mozgtk.c in order un-bitrot part 4 (attachment 8646212 [details] [diff] [review]) and part 8 (attachment 8646707 [details] [diff] [review]). Would you mind checking that that change is sane? Thanks!

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 59 • 9 years ago

Fix a warning on Windows

	Karl Tomlinson (:karlt)
	Comment 60 • 9 years ago

(In reply to Brian Birtles (:birtles) from comment #58)
> Also, Karl, I added a line to mozgtk.c in order un-bitrot part 4 (attachment
> 8646212 [details] [diff] [review]) and part 8 (attachment 8646707 [details] [diff] [review]
> [diff] [review]). Would you mind checking that that change is sane? Thanks!

Those are good, thanks.

	Karl Tomlinson (:karlt)
	Comment 61 • 9 years ago

Comment on attachment 8646789 [details] [diff] [review]
part 8b - Factor out an IsTimeNewerThanTimeStamp method

This is much better, thanks.

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
says "Use pointers instead of references for function out parameters, even for
primitive types".

>+    bool newer = IsTimeNewerThanTimestamp(aTime, roughlyNow, deltaFromNow);

I'm going to enforce the coding style quoted above.
It is not clear from the call site that the last parameter is calculated by
the function.

Discussion at
https://groups.google.com/forum/#!topic/mozilla.dev.platform/0uInUgEjvOE

(In reply to Brian Birtles (:birtles) from comment #56)
> I didn't change the parameter to a pointer type because I'd rather not
> pollute this with MOZ_ASSERTs for null checks.

No need for null assertions; we'll find out soon enough if someone passes a
null pointer.  No need for null checks either; this is a private function so
the callers will know what to do.

>+    } else {
>+      isNewer = true;
>+      aDelta = timeDelta - timeToTimeStamp;
>+    }

aDelta is incorrect here.

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 62 • 9 years ago

Address review comments

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 63 • 9 years ago

Work around warning on Windows:
"unary minus operator applied to unsigned type, result still unsigned"

	Pulsebot
	Comment 64 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c1a7265296
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b4b30fc2ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/67455b1c1e93
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c5dc6f367ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/a03ecb6d3d00
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf77ab3474c
https://hg.mozilla.org/integration/mozilla-inbound/rev/f11af0ae1811
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9a0b5dad97
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4be1361d6ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13c23f424f6

	Pulsebot
	Comment 65 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/02f4531e6418

	Ryan VanderMeulen [:RyanVM]
	Comment 66 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/d3c1a7265296
https://hg.mozilla.org/mozilla-central/rev/19b4b30fc2ba
https://hg.mozilla.org/mozilla-central/rev/67455b1c1e93
https://hg.mozilla.org/mozilla-central/rev/5c5dc6f367ac
https://hg.mozilla.org/mozilla-central/rev/a03ecb6d3d00
https://hg.mozilla.org/mozilla-central/rev/1bf77ab3474c
https://hg.mozilla.org/mozilla-central/rev/f11af0ae1811
https://hg.mozilla.org/mozilla-central/rev/3e9a0b5dad97
https://hg.mozilla.org/mozilla-central/rev/e4be1361d6ca
https://hg.mozilla.org/mozilla-central/rev/e13c23f424f6
https://hg.mozilla.org/mozilla-central/rev/02f4531e6418

	Kohei Yoshino [:kohei]
	Comment 67 • 9 years ago

Added site compatibility doc:

https://www.fxsitecompat.com/en-US/docs/2015/event-timestamp-now-returns-domhighrestimestamp-on-nightly-aurora-for-linux/

	Majid Valipour
	Comment 68 • 9 years ago

(In reply to Kohei Yoshino [:kohei] from comment #67)
> Added site compatibility doc:
> 
> https://www.fxsitecompat.com/en-US/docs/2015/event-timestamp-now-returns-
> domhighrestimestamp-on-nightly-aurora-for-linux/

I believe the information in the linked document is inaccurate. It reads:
"Starting with version 43, the event.timeStamp property returns a DOMHighResTimeStamp in microseconds, instead of a traditional time stamp in milliseconds"

DOMHighResTimeStamp is in *milleseconds* but with microsecond *resolution* as opposed to DOMTimeStamp which only provides millisecond resolution.

Also, it may be valuable to mention that the time origin for new timeStamp is navigation start and not the epoch time. (i.e., it is comparable to performance.now() as opposed to Date.now()).