77992 - Make Event.timeStamp return a DOMHighResTimeStamp on Windows (was Event.timeStamp should be relative to 1st January 1970 rather than the system start)

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[david]

The timeStamp field of Event objects should be a Date according to the spec.
See section 1.1.6 of the Core spec:

Note: Even though the DOM uses the type DOMTimeStamp [p.18] , bindings may use
different types. For example for Java, DOMTimeStamp is bound to the long type.
In ECMAScript, TimeStamp is bound to the Date type because the range of the
integer type is too small.

The attached testcase demonstrates that Mozilla 0.8.1 returns a number instead
of a Date object.

Comment 1

[david]

Attachment: testcase

Comment 2

[david]

Furthermore, the numbers in the timeStamp field seem to be pretty much random.
They are not millisecond values that can be passed to new Date(), and they don't
bear much relationship to each other.  At least for mouse events, as the second
attached test case demonstrates.

Comment 3

[david]

Attachment: another testcase

Comment 4

[Vladimir Ermakov]

I didnt realise this, but I actually noticed the same problem. And it is also 
different for different event types. I'll attach a better testcase which shows 
that timestamp only changes for mouseup, mousedown, and mouse move events. For 
the rest the value stays constant, or changes rarely and randomly.

Comment 5

[Vladimir Ermakov]

Attachment: A better testcase

Comment 6

[Johnny Stenback (:jst)]

Joki, I don't know much about this stuff, but event.time = ::GetMessageTime()
(in widget/src/windows/nsWindow.cpp) seems wrong to me, shouldn't that be
event.time = PR_Now()?

Once that's fixed I can make event.timeStamp be a Data unless you want to.

Comment 7

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 8

[Radha on family leave (not reading bugmail)]

Taking from joki..

Comment 9

[Radha on family leave (not reading bugmail)]

Attachment: Initial Patch

This patch takes care of undefined/wrong time issue in the windows platform.
The patch initialises the time globally at the definition. So all events should
have their time set to the time of event creation. However, other platforms
(like gtk) have init code (apart from the global definition of the structure in
nsGUIEvent.h) for event.time member. So, essentially, the testcases will
probably fail for certain events in other platforms. However the spec says that
 event.time could be set to 0. 

>timeStamp of type DOMTimeStamp, readonly

>Used to specify the time (in milliseconds relative to the epoch) at which the
>event was created. Due to the fact that some systems may not provide this
>information the value of timeStamp may be not available for all events. When
not >available, a value of 0 will be returned. Examples of epoch time are the
time of >the system start or 0:0:0 UTC 1st January 1970.

Comment 10

[Radha on family leave (not reading bugmail)]

jst, Can you please provide your comments to the patch. 

Comment 11

[saari (gone)]

Should we should make sure all events are synched to the same time source? In
other words, how bad would it be to use PR_NOW for some events and
GetMessageTime for others? I can't see why that would cause major issues right
now...

Comment 12

[saari (gone)]

Comment on attachment 101321 [details] [diff] [review]
Initial Patch

r=saari if jst double checks it

Comment 13

[Radha on family leave (not reading bugmail)]

I ran the Tp pageloader tests with the patch. I did not see any performance
degradation. I think that Tp tests are probably not the best way to measure the
performance  effects of this patch. I think it is events like mousemove that
would be affected more performance wise with this patch than other events that
are bound to occur less frequently.  I will try to attach a patch for specific
mouse events only. 

Comment 14

[timeless]

Comment on attachment 101321 [details] [diff] [review]
Initial Patch

>+      time((PRUint64)(PR_Now()/PR_USEC_PER_MSEC)),      

this patch was never right since PRUint64 can be a struct. it's also no longer
right because toastie reorg'd dom events last week. There's now an nsTime
class, so the fix should be fairly easy.

Comment 15

[Ilya Konstantinov]

Timeless, you take it? 

Comment 16

[Bjoern Hoehrmann]

Event.timeStamp is a Number in DOM Level 3 Events, that part of the bug report has become invalid. The .timeStamp value still does not conform to the spec, though.

Comment 17

[Jaroslav Zaruba]

It seems that the timeStamp property contains number of millisesonds from system start.

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #17)
> It seems that the timeStamp property contains number of millisesonds from
> system start.

... which the DOM spec allows, although it's less useful (e.g. you can't compare it to Date.now()).

Comment 19

[Dão Gottwald [:dao]]

document.createEvent("Events").timeStamp gives me a different kind of numbers than real events do.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

I compiled this only on OSX, but posted to tryserver.

I think the only possibly problematic part is in nsWindow::BeginResizeDrag.
Karl, could you look at that? Is using GDK_CURRENT_TIME ok there?
Based on Gtk/Gdk documentation it should be.
(I'll test that once I'm back on my linux machine.)

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: alternative

Remove .time from nsEvent and add mTime to nsDOMEvent.
This way we don't slow down painting etc. which uses nsEvents, but never nsDOMEvents.

I think I prefer this one, but jst, jonas, you might have some comments?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 404463 [details] [diff] [review]
patch

Er, no this is not possible because of plugins :/

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Attachment: + change to nsObjectFrame

Based on testing GdkEvent.time is the interval time.

Comment 24

[Karl Tomlinson (:karlt)]

Comment on attachment 404598 [details] [diff] [review]
+ change to nsObjectFrame

(In reply to comment #23)
> Based on testing GdkEvent.time is the interval time.

PR_IntervalNow() uses gettimeofday().

Some X servers use gettimeofday() and so their time would align with
PR_IntervalNow() when the server and client app are on the same machine.

However since 2006 xorg-server uses clock_gettime with CLOCK_MONOTONIC when
available so this will be different even when the server is on the same
machine.

(This is a source of inconsistencies in the current events implementation.)

(In reply to comment #20)
> I think the only possibly problematic part is in nsWindow::BeginResizeDrag.
> Karl, could you look at that? Is using GDK_CURRENT_TIME ok there?
> Based on Gtk/Gdk documentation it should be.

The GDK documentation doesn't spell out the consequences of using
GDK_CURRENT_TIME but it is best avoided.  Any calculation of the current time
is no better.  It should be the timestamp from the event that began the drag.

When a mouse button is pressed on X11, the server begins a "passive" pointer
grab so that pointer events are sent to the same window that received the
button press.  The passive grab is automatically released on button release.
The timestamp is used to end this passive grab (if it is still in effect).  If
GDK_CURRENT_TIME is used it will end any subsequent grabs, which can mean that
a window that has started a subsequent mouse drag may not receive the button
release event, so the drag will continue until the pointer is again over the
original window (if that is even visible) and the button is pressed and
released again.

GDK_CURRENT_TIME might be a reasonable fallback for synthesized events, though.

>           case NS_MOUSE_BUTTON_DOWN:
>           case NS_MOUSE_BUTTON_UP:
>             {
>               XButtonEvent& event = pluginEvent.xbutton;
>               event.type = anEvent.message == NS_MOUSE_BUTTON_DOWN ?
>                 ButtonPress : ButtonRelease;
>               event.root = root;
>-              event.time = anEvent.time;
>+              event.time = PR_IntervalNow();

It is important that the plugin gets the time of the user event, not the time
that the event is sent to the plugin.  This time may be used for
distinguishing between clicks and drags, and between two single clicks and a
double click, etc.

By setting nsGUIEvent::nativeMsg for more events, the original native event and its timestamp would usually be available.  (If not available, then it looks like anEvent.time was probably wrong anyway, though CurrentTime (== 0) doesn't seem a bad indicator that we don't know the time.)

I'm a bit surprised that nsEvent::time isn't useful elsewhere.

Calculating an Epoch-based time for DOM events from an X server timestamp for
the user action would be a bit tricky, but could probably be done and would
seem more useful than the time the nsDOMEvent is instantiated.

>@@ -3944,17 +3942,16 @@ nsEventStateManager::CheckForAndDispatch
>     nsMouseEvent event(NS_IS_TRUSTED_EVENT(aEvent), NS_MOUSE_CLICK, aEvent->widget,
>                        nsMouseEvent::eReal);
>     event.refPoint = aEvent->refPoint;
>     event.clickCount = aEvent->clickCount;
>     event.isShift = aEvent->isShift;
>     event.isControl = aEvent->isControl;
>     event.isAlt = aEvent->isAlt;
>     event.isMeta = aEvent->isMeta;
>-    event.time = aEvent->time;

This ensured that the CLICK event has the same time as the BUTTON_UP
event.  Is that maintained for DOM events somehow?

I guess recording the time the nsDOMEvent is instantiated is useful when there
are multiple handlers for the same event that might want to check whether it
is the same event.  But I wonder how often that happens, so the information
provided seems only slightly more useful than what could be obtained in
event handlers from date/time functions.

However, what seems to be lost here is the consistency of timestamps of
multiple events that derive from the same user action.  e.g.
keydown and keypress.

Comment 25

[Karl Tomlinson (:karlt)]

To be more clear:

nativeMsg can be used to solve the GDK/X11-plugin issues.

Perhaps you can persuade me that Epoch-based time-stamps are more important than the inter-event timeStamp equivalence.  But, unless you can argue otherwise, I think some effort should be made to keep some of the timeStamp equivalence.

X server time / client time syncing may be more effort than is worthwhile right now.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #25)
> To be more clear:
> 
> nativeMsg can be used to solve the GDK/X11-plugin issues.
OK, thanks.

> Perhaps you can persuade me that Epoch-based time-stamps are more important
> than the inter-event timeStamp equivalence.
DOM 3 Events requires Epoch based timeStamps.
And whatever we do, we should do it consistently everywhere.

> I think some effort should be made to keep some of the timeStamp
> equivalence.
I'm not sure what this means.

Comment 27

[Karl Tomlinson (:karlt)]

What concerns me here is the change in the timestamp for user input events
from being the time of user action to being the time of instantiating the
nsDOMEvent.  I'm concerned that there is a significant loss in usefulness of
the timestamp.

If the timestamp is just the time of nsDOMEvent instantiation, then this is
often going to be almost the same as the time of processing the event.  If the
timestamp were always the same as the time of processing the event then there
would be no point in having a timestamp.  I wonder what the timestamp is used
for.

I can think of two ways the timestamp might have been used that would have
worked well when it was the time of user action but won't work well with the
time of nsDOMEvent instantiation.

1) Previously, events associated with the same user action would have the same
   timestamp.  This could have been used to determine with a set of events was
   associated with the same action.  This was what I was referring to with
   "timeStamp equivalence".

2) Timestamps are currently used to measure intervals between user actions to
   determine what the user intended by the action.

   Some examples are here (really it is the same example 4 times):
http://hg.mozilla.org/mozilla-central/annotate/5bbfae9a5c7e/layout/xul/base/src/nsMenuPopupFrame.cpp#l1345
http://hg.mozilla.org/mozilla-central/annotate/5bbfae9a5c7e/layout/forms/nsListControlFrame.cpp#l2690
http://hg.mozilla.org/mozilla-central/annotate/5bbfae9a5c7e/toolkit/content/widgets/listbox.xml#l650
http://hg.mozilla.org/mozilla-central/annotate/5bbfae9a5c7e/toolkit/content/widgets/tree.xml#l299

   It seems reasonable that web apps would have similar uses.

Really, the right thing to do seems to be to work out how to map native event
timestamps with Epoch-time (probably in widget code).  I'm sure there must be
a science behind interpreting times from foreign clocks but I don't have much
experience.

I'm also concerned that doing the right thing may be complicated.  But, if not
doing the "right thing", there needs to be serious thought re what the new
benefits are and whether they outweigh the regressions.

If it's possible to make a reasonable mapping from native timestamps to Epoch
time then I think that is the approach that should be used.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The use cases for .timeStamp so far has been really weak. In fact, during last round of asking no use cases were presented. So I'd rather not spend too much complexity on it. (In fact, i wouldn't mind removing it entirely).

Comment 29

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Implementing Event.timeStamp consistently is necessary for SMIL event-timing. Animation timing can be synchronised with events and, in order to avoid synchronisation slew from the time taken to propagate events, the animations use the event's timestamp.

I'll follow this up in bug 238041 as well since perhaps the first step is to use a 64-bit int for storing the timestamp.

Comment 30

[Brian Birtles (:birtles, Feb 26~ parental leave)]

SMIL use case: http://www.w3.org/TR/SMIL/smil-timing.html#q135

Comment 31

[Karl Tomlinson (:karlt)]

Does SMIL need an Epoch based time or a monotonically increasing time?

Comment 32

[Brian Birtles (:birtles, Feb 26~ parental leave)]

To avoid slew I guess a monotonically increasing time makes more sense but actually internally we use an epoch-based time as our absolute time reference.

So I think we could use either: the former is probably easier on us whilst the latter more correct.

SMIL is supposed to support "wallclock time" -- i.e. the ability to specify an animation to start at '1997-07-16T19:20+01:00' but we're not planning to implement that feature.

I'm pretty sure Batik uses epoch-based timestamps and it seems like DOM 3 events does as well.

Comment 34

[Brendan Eich [:brendan]]

Old bug, fresh dup -- reevaluate priority? jst, bz, anyone have any thoughts re: competition or new use-cases?

/be

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Propsal patch for gtk2

How about this for gtk2? This patch computes event generation time from native event generation time with old event handling time. Even if some native events come with same time stamp, this patch generates same time stamp for such events.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Karlt and Smaug:

If you agree my approach, I'll create a couple of patches for each platform.

Comment 37

[Karl Tomlinson (:karlt)]

Comment on attachment 478710 [details] [diff] [review]
Propsal patch for gtk2

If we want epoch-based time stamps then I'm probably OK with the goal of this patch.

>+        // If the current event is younger than the stored event,
>+        // we should use this time difference for computing the event time.
>+        if (now - aNativeTime < sEventTime - sEventNativeTime) {
>+            sEventTime = aEvent->time = now;
>+            sEventNativeTime = aNativeTime;
>+        } else {
>+            aEvent->time =
>+                sEventTime + (aNativeTime - sEventNativeTime);
>+        }

However, things are more complicated than indicated here.

The native time is often a monotonically increasing time rather than an epoch based time.
If the user suspends the computer, then, on resume, the native time may have stopped for some period, and so the calculated epoch-based times will continue to be in the distant past, native events getting out of sync with synthetic events.

>-
>-        event.time = xevent.xmotion.time;

I'm guessing this removal was an oversight.

Other random comments:

gdk_x11_get_server_time() may be useful but shouldn't be used more than absolutely necessary.

I wouldn't add nativeTime to nsGUIEvent for the sake of nsObjectFrame, as the
native event can be attached to the nsGUIEvent.

Comment 40

[Arkadiusz Michalski (Spirit)]

I just want to say that this bug was reported in 2001 and still exists today. Chrome, Opera (12.x), IE9 (and next) working better (don't know if 100% correct in all cases). 

I just want to add, that in current Firefox run sth like this: button.click(); return 0 for Event.timeStamp. In some case we get very small time (for trust event) or the time in microseconds (for untrusted event), but it has already been reported earlier.

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Karl Tomlinson (:karlt, back May 5) from comment #37)
> The native time is often a monotonically increasing time rather than an
> epoch based time.
> If the user suspends the computer, then, on resume, the native time may have
> stopped for some period, and so the calculated epoch-based times will
> continue to be in the distant past, native events getting out of sync with
> synthetic events.

Agreed; I've always thought wanting event timestamps to be in epoch-based time was a mistake in the spec, since it introduces a bunch of clock-change-related bugs for anyone who's using them.  The W3C does now at least have a spec for such a concept at http://www.w3.org/TR/hr-time/ .  We should at least propose a new field for events that does that.

Comment 42

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #41)
> Agreed; I've always thought wanting event timestamps to be in epoch-based
> time was a mistake in the spec, since it introduces a bunch of
> clock-change-related bugs for anyone who's using them.  The W3C does now at
> least have a spec for such a concept at http://www.w3.org/TR/hr-time/ .  We
> should at least propose a new field for events that does that.

I posted this as a proposal to whatwg: http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-May/254243.html

Comment 43

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Brian Birtles (:birtles) from comment #42)
> I posted this as a proposal to whatwg:
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2014-May/254243.html

Current outcome is to switch timeStamp to DOMHighResTimeStamp using the same zero time as performance.now(), i.e. https://w3c.github.io/web-performance/specs/HighResolutionTime2/Overview.html#sec-time-origin. If not too much breaks, Anne will update the spec to match.

I can work on this but probably won't get to it for a few weeks. If anyone else has cycles before then please feel free to pick it up.

Comment 44

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP part 1.0 - Store event timestamps as mozilla::TimeStamp

Comment 45

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: WIP part 1.1 - Convert Windows event times to timestamps

Comment 46

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Here are some WIP patches for making Event.timeStamp use our TimeStamp class. I haven't done the platform-specific code for converting native event times to TimeStamps except for on Windows.

A few things I'm not sure about:

* How important is converting native event times to timestamps? It can require a bit of complexity since event times tend to wrap around a fixed range.

* The ctor for WidgetEvent in most cases calls TimeStamp::Now(). I'm not sure what the performance implications are of this. It seems we mostly use that time after setting it, but occasionally we overwrite it in which case setting it in the ctor is busy-work. I suppose using NowLoRes would not offer enough accuracy.

* Reverting this patch series entirely would be difficult. If we face compatibility issues down the line due to the change in data type at the interface level that's easy to revert. If we face compatibility issues due to changes to the time origin we could possibly perform a conversion from a TimeStamp at the interface level although it would probably mean either that times could change when the clock changes or that they become less and less accurate.

* I probably need to split part 1.0 up further to make it easier to review.

I haven't done the interface changes from an integer to a double (DOMHighResTimeStamp) since that's straight forward and should be a separate patch so we can easily revert it.

For now all tests seem to pass on Windows anyway.

Comment 47

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.0 - Add timeStamp to WidgetEvent

This patch adds a timeStamp member to WidgetEvent alongside the existing 'time'
member. In the future we would like to remove 'time' and just keep timeStamp but
that depends on it being web-compatible. For now we introduce both members
side-by-side. Later we will add a pref to determine which one to return. If no
compatibility issues arise we will remove 'time' altogether.

Comment 48

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.1 - Convert Windows native event times to timestamps

Comment 49

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Make event.timeStamp use TimeStamp value based on a pref

Note that this pref isn't fully live since the timeStamp member of the event
interface is [Pure] and so values will be cached.

Comment 50

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Make event.timeStamp return a DOMHighResTimeStamp

In order to allow returning either a DOMHighResTimeStamp or DOMTimeStamp
depending on whether dom.event.highrestimestamp.enabled is set the WebIDL is
adjusted to use the any type (since double and unsigned long long are not
distinguishable we can't just use a union).

Once we drop the old code (assuming there are no compatibility issues) we can
switch the IDL to just DOMHighResTimeStamp.

Comment 51

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Add mochitests for event.timeStamp

Comment 52

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Make AsyncPanZoomController use the event timestamp for recording the last event

Comment 53

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 6 - Make nsListControlFrame use the event timestamp

Comment 54

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 8 - Make nsIDOMEvent::GetTimeStamp use the timeStamp value

Comment 55

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 8 - Make Event.timeStamp always a DOMHighResTimeStamp (remove pref)

Comment 56

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 10 - Remove Event.time member

This patch removes all usage of Event.time now that we always use timeStamp.

Comment 57

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Make event.timeStamp use TimeStamp value based on a pref

Comment 58

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 9 - Make Event.timeStamp always a DOMHighResTimeStamp (remove pref)

Comment 59

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Roll-up patch for reference (parts 1~10)

Comment 60

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8426741 [details] [diff] [review]
part 1.0 - Add timeStamp to WidgetEvent

I'm only putting the first patch up for review for now since there are a few open questions that may mean I need to rewrite everything.

Questions for reviewers:

1. Is this approach ok with regards to compatibility?

A summary of the changes exposed to Web content:

* event.timeStamp is now always set (previously it was often 0 for many events)
* event.timeStamp is now always milliseconds (previously we sometimes used microseconds, sometimes millis)
* event.timeStamp is now relative to the time origin (typically navigation start; previously it was roughly system start)
* event.timeStamp is now a double (previously it was unsigned long long)

These patches take a very conservative approach, maintaining the old code alongside the new toggled via a pref. The idea is:

i. Land up to part 4 and let it bake for a while (wait until it hits release even?)
ii. (a) If everything is ok, land the remaining parts which remove the old code and the pref
    (b) If there are compatibility issues, flip the pref until we work out what to do

During (i) there's a cost since we end up storing *both* the time and the timeStamp (e.g. we'll often call both PR_IntervalNow and TimeStamp::Now for the one event). Then there's the memory cost too.
    
Two alternative approaches are:

a. Just drop all the old code at once. If we hit compatibility issues paper over them.
   e.g., If there is mass breakage due to the conversion to a double, do a cast at the interface level
   e.g. 2, If website A breaks because the timeStamp is now milliseconds, get website A to stop sniffing Gecko (since other UAs should be using milliseconds) and possibly write a patch that somehow forces that particular event only to be reported in microseconds until until website A gets updated.
b. Use a compile-time flag to switch between the two code paths.


2. Is setting the timestamp to TimeStamp::Now() in the event ctor acceptable?

This simplifies the code a lot and means we can drop a lot of these 'InitEvent' utility methods. It also means we get timestamps on all events, not just the ones we remember to set.

The cost is that for some events we'll call TimeStamp::Now() and then overwrite it later. For example, we do that for native events. We could possibly create an additional ctor that left the timestamp as null and use that when we know we'll set the time ourselves. I'm just not sure how costly calling TimeStamp::Now() is.

Alternatively we could use TimeStamp::NowLoRes() but that seems fairly rough.


3. How much more accurate are native event times?

The code for converting Windows native event times to mozilla::TimeStamps is a little complex and makes me wonder if the additional accuracy we get warrants the cost.

Comment 61

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8426741 [details] [diff] [review]
part 1.0 - Add timeStamp to WidgetEvent

Moving review request to Mounir since I notice Olli is away this week. Mounir, please let me know if someone else is more suitable. Thanks!

Comment 62

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8426741 [details] [diff] [review]
part 1.0 - Add timeStamp to WidgetEvent

Re-assigning to Olli since he's only sort-of away and Mounir definitely isn't available.

Comment 63

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8426741 [details] [diff] [review]
part 1.0 - Add timeStamp to WidgetEvent

How slow is TimeStamp::Now()?

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Brian Birtles (:birtles) from comment #60)
> Comment on attachment 8426741 [details] [diff] [review]
> part 1.0 - Add timeStamp to WidgetEvent
> 
> I'm only putting the first patch up for review for now since there are a few
> open questions that may mean I need to rewrite everything.
> 
> Questions for reviewers:
> 
> 1. Is this approach ok with regards to compatibility?

Well, changing the api is regression risky, for certain.


> * event.timeStamp is now always milliseconds (previously we sometimes used
> microseconds, sometimes millis)
Yeah, it really should be milliseconds.


> * event.timeStamp is now relative to the time origin (typically navigation
> start; previously it was roughly system start)
I assume you mean WidgetEvent.timeStamp, since part 1.0 doesn't chance DOM event.timeStamp.
(and I don't think we can make that to return values from the beginning of navigation)


> * event.timeStamp is now a double (previously it was unsigned long long)
That is probably ok. Tiny bit risky, but probably ok

> 2. Is setting the timestamp to TimeStamp::Now() in the event ctor acceptable?
I think it is, assuming it doesn't show up in the performance profiles.

Comment 65

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Olli Pettay [:smaug] (vacation-ish May 26-30) from comment #63)
> Comment on attachment 8426741 [details] [diff] [review]
> part 1.0 - Add timeStamp to WidgetEvent
> 
> How slow is TimeStamp::Now()?

I really don't know. I'm just going off the comment in TimeStamp.h:

> NowLoRes() has been introduced to workaround performance problems of
> QueryPerformanceCounter on the Windows platform.  NowLoRes() is giving
> lower precision, usually 15.6 ms, but with very good performance benefit.
> Use it for measurements of longer times, like >200ms timeouts.

I'll run some performance checks before landing.

(In reply to Olli Pettay [:smaug] (vacation-ish May 26-30) from comment #64)
> (In reply to Brian Birtles (:birtles) from comment #60)
[...]
> > * event.timeStamp is now relative to the time origin (typically navigation
> > start; previously it was roughly system start)
> I assume you mean WidgetEvent.timeStamp, since part 1.0 doesn't chance DOM
> event.timeStamp.
> (and I don't think we can make that to return values from the beginning of
> navigation)

Yes, sorry, the questions were meant to refer to the patch series as a whole. Not just part 1.0.

My main concern was whether it is necessary to go to all the trouble of:

* Adding a pref
* Maintaining both code paths just in case we need to switch back to the old behavior
* Later removing the pref and the old code path

Or whether we could just replace the old code path right away and patch up any compatibility issues as they arise.

I guess we can try with the pref for now if you're ok with that. I'll assign the rest of the patches to masayuki since you're only partly available this week. Feel free to add any drive-by comments though.

Comment 66

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8426745 [details] [diff] [review]
part 4 - Make event.timeStamp return a DOMHighResTimeStamp

This is a temporary step where I'm trying to support either returning a DOMTimeStamp or DOMHighResTimeStamp based on the value of a pref. A later patch in the series removes this and replaces it with DOMHighResTimeStamp.

Comment 67

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8426746 [details] [diff] [review]
part 5 - Add mochitests for event.timeStamp

The header in this patch says "part 4" but it should be "part 5". I've fixed that locally.

Comment 68

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Only putting up parts 1.1~5 for review for now since that's all we need to start gauging compatibility (and to get MSE on YouTube to work).

I still need to write the extra platform-specific code for converting native platform event times to timestamps before we can land any of this though. Any help with GTK/Android/Gonk/Mac appreciated.

Comment 69

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8426755 [details] [diff] [review]
part 3 - Make event.timeStamp use TimeStamp value based on a pref

I can review just code level about this bug, though.

>@@ -953,16 +955,64 @@ Event::DefaultPrevented(JSContext* aCx) 
>   }
> 
>   // If preventDefault() has been called by content, return true.  Otherwise,
>   // i.e., preventDefault() has been called by chrome, return true only when
>   // this is called by chrome.
>   return mEvent->mFlags.mDefaultPreventedByContent || IsChrome(aCx);
> }
> 
>+uint64_t
>+Event::TimeStamp() const
>+{
>+  if (NS_IsMainThread()) {
>+    if (Preferences::GetBool("dom.event.highrestimestamp.enabled")) {

If you check if the pref is disabled, it becomes simpler.

    if (!Preferences::GetBool("dom.event.highrestimestamp.enabled")) {
      return mEvent->time;
    }

...and reduce following indent.

>+      if (!mOwner)
>+        return 0L;

Use {}. I guess that this is unusual case. If so, please use |if (NS_WARN_IF(!mOwner)) {|. But if you see a lot of warning with it, please keep it without NS_WARN_IF().

>+      nsPerformance* perf = mOwner->GetPerformance();
>+      if (!perf)
>+        return 0L;

Same. Use {} and NS_WARN_IF().

>+      DOMHighResTimeStamp eventTime =
>+        !mEvent->timeStamp.IsNull() ?
>+        perf->GetDOMTiming()->TimeStampToDOMHighRes(mEvent->timeStamp) :
>+        0;

nit: Indent the last two lines.

>+      return static_cast<uint64_t>(eventTime);
>+    } else {
>+      return mEvent->time;
>+    }
>+  } else {

I don't think this else since if block is always return from this method. So, you can remove this else and reduce following indent.

>+    // We can't query preferences from a worker context so we just always use
>+    // the event timestamp. There's not likely to be much content depending on
>+    // event timestamps in workers and for any content that does exist, it is
>+    // most likely only interested in the difference between timestamps, not
>+    // their absolute values, and so they should should be unaffected by the

"should should"?

>+    // change of time origin.
>+    //
>+    // For dedicated workers, we should make times relative to the navigation
>+    // start of the document that created the worker. We currently don't have
>+    // that information handy so for now we treat shared workers and dedicated
>+    // workers alike and make times relative to the worker creation time. We can
>+    // fix this when we implement WorkerPerformance.

I cannot review this behavior except it's defined in standard spec. If you decide this behavior without discussion without Smaug, please ask him.

>+    workers::WorkerPrivate* workerPrivate =
>+      workers::GetCurrentThreadWorkerPrivate();
>+    MOZ_ASSERT(workerPrivate);
>+
>+    if (!mEvent->timeStamp.IsNull()) {
>+      TimeDuration duration =
>+        mEvent->timeStamp - workerPrivate->CreationTimeStamp();
>+      return static_cast<uint64_t>(duration.ToMilliseconds());
>+    } else {
>+      return 0L;
>+    }

nit:

    if (mEvent->timeStamp.IsNull()) {
      return 0L;
    }
    TimeDuration duration =
      mEvent->timeStamp - workerPrivate->CreationTimeStamp();
    return static_cast<uint64_t>(duration.ToMilliseconds());

>+  }
>+}

>diff --git a/dom/smil/test/test_smilTimeEvents.xhtml b/dom/smil/test/test_smilTimeEvents.xhtml
>--- a/dom/smil/test/test_smilTimeEvents.xhtml
>+++ b/dom/smil/test/test_smilTimeEvents.xhtml
>-  // Currently we set event timestamps to 0 which DOM 2 allows. This isn't
>-  // correct since SMIL uses this field to avoid synchronisation slew but first
>-  // we need to fix bug 323039 and bug 77992 which involve storing timestamps as
>-  // 64-bit integers and deciding whether those timestamps should be related to
>-  // the epoch or system start.
>-  is(evt.timeStamp, 0, "Event timeStamp should be 0");
>+  if (SpecialPowers.getBoolPref("dom.event.highrestimestamp.enabled")) {
>+    ok(evt.timeStamp > 0 && evt.timeStamp < window.performance.now(),
>+       "Event timeStamp should be > 0 but before the current time");

I think that you should log evt.timeStamp value and window.performance.now() value.

I.e.,
var timeStamp = evt.timeStamp;
var now = window.performance.now();
ok(timeStamp > 0 && timeStamp < now,
   "Event timeStamp should be > 0 but before the current time (timeStamp=" + timeStamp + ", now=" + now + ")");

This must be useful if somebody meets regression.

>diff --git a/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html b/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html
>--- a/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html
>+++ b/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html
>@@ -62,20 +62,27 @@ function runTests()
>+      if (SpecialPowers.getBoolPref("dom.event.highrestimestamp.enabled")) {
>+        var duration = Math.abs(window.performance.now() - aEvent.timeStamp);
>+        ok(duration < 30 * 1000,
>+           "perhaps, timestamp wasn't set correctly :" + aEvent.timeStamp);

I think duration should be logged too.

>+      } else {
>+        var eventTime = new Date(aEvent.timeStamp);
>+        var duration = Math.abs(Date.now() - aEvent.timeStamp);
>+        ok(duration < 30 * 1000,
>+           "perhaps, timestamp wasn't set correctly :" +
>+           eventTime.toLocaleString());
>+      }

Same. Although, not your fault.

>diff --git a/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html b/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html
>--- a/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html
>+++ b/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html
>-      var eventTime = new Date(aEvent.timeStamp);
>-      var duration = Math.abs(Date.now() - aEvent.timeStamp);
>-      ok(duration < 30 * 1000,
>-         "perhaps, timestamp wasn't set correctly :" + eventTime.toLocaleString());
>+      if (SpecialPowers.getBoolPref("dom.event.highrestimestamp.enabled")) {
>+        var duration = Math.abs(window.performance.now() - aEvent.timeStamp);
>+        ok(duration < 30 * 1000,
>+           "perhaps, timestamp wasn't set correctly :" + aEvent.timeStamp);

Same.

>+      } else {
>+        var eventTime = new Date(aEvent.timeStamp);
>+        var duration = Math.abs(Date.now() - aEvent.timeStamp);
>+        ok(duration < 30 * 1000,
>+           "perhaps, timestamp wasn't set correctly :" +
>+           eventTime.toLocaleString());
>+      }

Same.

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -789,16 +789,17 @@ pref("privacy.popups.disable_from_plugin
> // "do not track" HTTP header, disabled by default
> pref("privacy.donottrackheader.enabled",    false);
> //   0 = tracking is acceptable
> //   1 = tracking is unacceptable
> pref("privacy.donottrackheader.value",      1);
> 
> pref("dom.event.contextmenu.enabled",       true);
> pref("dom.event.clipboardevents.enabled",   true);
>+pref("dom.event.highrestimestamp.enabled",  true);

Will it enabled in default settings from now? If so, why do we need the old path? And did you discuss about this with Smaug or somebody?

Comment 70

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8426745 [details] [diff] [review]
part 4 - Make event.timeStamp return a DOMHighResTimeStamp

Just return a DOMHighResTimestamp in the IDL instead of messing about with "any".  It'll mean you do the conversion to double in C++ code instead of in the binding code (which is what happens anyway when you try to stuff a uint64_t into the JS Number type), so won't change behavior from what we have now, but save you a bunch of trouble.

Comment 71

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8426746 [details] [diff] [review]
part 5 - Add mochitests for event.timeStamp

>+if (SpecialPowers.getBoolPref("dom.event.highrestimestamp.enabled")) {
>+  SimpleTest.waitForExplicitFinish();
>+  testRegularEvents();
>+} else {
>+  todo(false, "Event.timeStamp tests require " +
>+              "dom.event.highrestimestamp.enabled to be set");
>+}

Why don't you use setBoolPref() and always test it? If you would do so, use clearUserPref() at finishing the test.

r=masayuki with above change (but if there is a reason why you cannot do it, you don't need to change it).

Comment 72

[Matthew Gregan [:kinetik]]

pushPrefEnv is even better; no need to remember to unset the pref.

Comment 73

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Matthew Gregan [:kinetik] from comment #72)
> pushPrefEnv is even better; no need to remember to unset the pref.

Oh, interesting...
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#848

Comment 74

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.0 - Add timeStamp to WidgetEvent;

Fix bitrot

Comment 75

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Make event.timeStamp use TimeStamp value based on a pref

Address Masayuki's review feedback

Comment 76

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Make event.timeStamp return a DOMHighResTimeStamp

Also fixing the mess I made of patch numbering at the same time

Comment 77

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8429792 [details] [diff] [review]
part 3 - Make event.timeStamp return a DOMHighResTimeStamp

r=me

Comment 78

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Make event.timeStamp use TimeStamp value based on a pref

Fix changes to all.js to only turn on new timestamps for Windows

Comment 79

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Make event.timeStamp return a DOMHighResTimeStamp;

Fix bug in return type of TimeStamp()

Comment 80

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Add mochitests for event.timeStamp;

Use setBoolPref as suggested in comment 71

Comment 81

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 5 - Make AsyncPanZoomController use the event timestamp for recording the last event

Fix bitrot

Comment 82

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8426742 [details] [diff] [review]
part 1.1 - Convert Windows native event times to timestamps

>diff --git a/widget/windows/nsWindow.h b/widget/windows/nsWindow.h
>+  TimeStamp               GetMessageTimeStamp(LONG aEventTime);
>+  void                    UpdateFirstEventTime(DWORD aEventTime);

>+  // For converting native event times to timestamps we record the time of the
>+  // first received event in each time scale
>+  DWORD     mFirstEventTime;
>+  TimeStamp mFirstEventTimeStamp;

I feel them odd, why aren't they static? I don't understand the reason why they should be managed by each window separately.

>diff --git a/widget/windows/nsWindow.cpp b/widget/windows/nsWindow.cpp
>+TimeStamp
>+nsWindow::GetMessageTimeStamp(LONG aEventTime) {

nit: put the last "{" to the next line.

>+  // Conversion from native event time to timestamp is complicated by the fact
>+  // that native event time wraps. Also, for consistency's sake we avoid calling
>+  // ::GetTickCount() each time and for performance reasons we only use the
>+  // TimeStamp::NowLoRes except when recording the first event's time.
>+
>+  // We currently require the event time to be passed in. This is an interim
>+  // measure to avoid calling GetMessageTime twice for each event--once when
>+  // storing the time directly and once when converting to a timestamp.
>+  //
>+  // Once we no longer store both a time and a timestamp on each event we can
>+  // perform the call to GetMessageTime here instead.
>+  DWORD eventTime = (DWORD)aEventTime;

Use static_cast<DWORD>() instead of C style cast.

>+  double timeSinceFirstEvent =
>+    mFirstEventTime <= eventTime ?
>+    eventTime - mFirstEventTime :
>+    static_cast<double>(kEventTimeRange) + eventTime - mFirstEventTime;

Indent the last two lines.

>+  // Event time may have wrapped several times since we recorded the first event
>+  // time (it wraps every ~50 days) so extend eventTimeStamp as needed. There is
>+  // some imprecision here since we are using TimeStamp::NowLoRes and
>+  // mFirstEventTime and mFirstEventTimeStamp may not be *exactly* the same
>+  // moment so we do an estimate at how many times event time has wrapped and
>+  // then adjust the result based on which side of the first event time the
>+  // current event time lands
>+
>+  double timesWrapped = (roughlyNow - mFirstEventTimeStamp).ToMilliseconds() /
>+                        kEventTimeRange;

nit: perhaps, moving after "=" to the next line, you don't need to wrap the line in the middle of the computation.

i.e.,
foo =
  bar / buz;

>+  double intervalFraction = fmod(timesWrapped, 1);
>+  // Check if timesWrapped is just after the wrap point but the event time is
>+  // just before it
>+  if (intervalFraction < 0.5 && timeSinceFirstEvent > kEventTimeHalfRange) {
>+    timesWrapped = floor(timesWrapped - 1);
>+  // Check if timesWrapped is just before the wrap point but the event time is
>+  // just after it
>+  } else if (intervalFraction > 0.5 &&
>+             timeSinceFirstEvent < kEventTimeHalfRange) {
>+    timesWrapped = ceil(timesWrapped);
>+  } else {
>+    timesWrapped = floor(timesWrapped);
>+  }

I don't understand this.

|(roughlyNow - mFirstEventTimeStamp).ToMilliseconds() / kEventTimeRange| can be over 1.0?

And then what means |intervalFraction < 0.5 && timeSinceFirstEvent > kEventTimeHalfRange| and |intervalFraction > 0.5 && timeSinceFirstEvent < kEventTimeHalfRange|?

And are you ignore internalFraction == 0.5 case intentionally?

I need more explanation about this.

>+  if (timesWrapped > 0) {
>+    eventTimeStamp +=
>+      TimeDuration::FromMilliseconds(kEventTimeRange * timesWrapped);
>+  }
>+
>+  return eventTimeStamp;
>+}
>+
>+void
>+nsWindow::UpdateFirstEventTime(DWORD aEventTime) {

nit: put the "{" to the next line.

>+  mFirstEventTime = aEventTime;
>+  DWORD currentTime = ::GetTickCount();
>+  TimeStamp currentTimeStamp = TimeStamp::Now();
>+  double timeSinceFirstEvent =
>+    aEventTime <= currentTime ?
>+    currentTime - aEventTime :
>+    static_cast<double>(kEventTimeRange) + currentTime - aEventTime;

Indent the last two lines.

>+  mFirstEventTimeStamp =
>+    currentTimeStamp - TimeDuration::FromMilliseconds(timeSinceFirstEvent);
>+}

Comment 83

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 7 - Make nsIDOMEvent::GetTimeStamp use the timeStamp value

Fix bitrot

Comment 84

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 8 - Make Event.timeStamp always a DOMHighResTimeStamp (remove pref)

Fix bitrot

Comment 85

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 9 - Remove Event.time member

Fix bitrot

Comment 86

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Thanks Nakano-san for the review!

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #82)
> I feel them odd, why aren't they static? I don't understand the reason why
> they should be managed by each window separately.

At the time I couldn't decide which was better so I made it non-static. Fixed.

> >+  double timeSinceFirstEvent =
> >+    mFirstEventTime <= eventTime ?
> >+    eventTime - mFirstEventTime :
> >+    static_cast<double>(kEventTimeRange) + eventTime - mFirstEventTime;
> 
> Indent the last two lines.

I checked the style guide for this and changed it to:

  double a = b
             ? c
             : d;

(Or actually a little bit different since 'd' is long)

> >+  double intervalFraction = fmod(timesWrapped, 1);
> >+  // Check if timesWrapped is just after the wrap point but the event time is
> >+  // just before it
> >+  if (intervalFraction < 0.5 && timeSinceFirstEvent > kEventTimeHalfRange) {
> >+    timesWrapped = floor(timesWrapped - 1);
> >+  // Check if timesWrapped is just before the wrap point but the event time is
> >+  // just after it
> >+  } else if (intervalFraction > 0.5 &&
> >+             timeSinceFirstEvent < kEventTimeHalfRange) {
> >+    timesWrapped = ceil(timesWrapped);
> >+  } else {
> >+    timesWrapped = floor(timesWrapped);
> >+  }
> 
> I don't understand this.
> 
> |(roughlyNow - mFirstEventTimeStamp).ToMilliseconds() / kEventTimeRange| can
> be over 1.0?

Yes. If the browser has been running for more than 50 days it will be > 1.0.

> And then what means |intervalFraction < 0.5 && timeSinceFirstEvent >
> kEventTimeHalfRange| and |intervalFraction > 0.5 && timeSinceFirstEvent <
> kEventTimeHalfRange|?

Windows will give us a timestamp as a 32-bit unsigned integer that wraps every ~50 days. See:

  http://blogs.msdn.com/b/oldnewthing/archive/2014/01/22/10491576.aspx

That wrapping could occur very soon after starting the browser so it's not only for cases where the browser is running for a long time.

We record a first event time, such as 100ms. Then when we get an event we get its message time which might be, say, 200ms. That *could* mean the event happened 100 milliseconds since the first event, or, it could mean it occured 50days + 100ms, or 100days + 100ms, etc. later. So we use the timestamps 'roughlyNow' and the first event *timestamp* to work out how many times the system time has wrapped since we recorded the first event time. Then we add that to the result.

However, there is some inaccuracy involved since 'roughlyNow' is not *exactly* the current time. Also, the first event time and first event timestamp are not guaranteed to be exactly the same moment.

So we might, for example, have the following:

  firstEventTime = 5000ms
  eventTime = 4995ms

We will calculate 'timeSinceFirstEvent' as 50days + 4995 - 5000 = 50days + 5ms.

However, when we go to calculate the number of times we have wrapped, due to the inaccuracies above we might decide that event time has *just* completed a full cycle since firstEventTimeStamp. E.g. roughlyNow might equal firstEventTimeStamp + 50days + ~1ms.

In that case timesWrapped = 1.00000001 (roughly, not actually) but it *should* equal 0.9999999..

If we don't detect the error, we will end up adding an *extra* 50 days to the timestamp we calculate which would mean we get a timestamp of ~100days since the first event instead of ~50days.

So we have to detect that situation.

We calculate intervalFraction which will give us 0.00000001 in this case.

Then we have the following condition

  if (intervalFraction < 0.5 && timeSinceFirstEvent > kEventTimeHalfRange)

Which here is:

  if (0.000001 < 0.5 && (50days + 5ms) > 25days)

The first part of the condition checks if, according to the *timestamps*, we think we have just wrapped.
The second part of the condition checks if the *message time* has *not* just wrapped.

Then we do:

  timesWrapped = floor(timesWrapped - 1);

i.e. in this case we'll update timesWrapped from 1.00000001 to 0 and avoid adding an extra ~50days.

The second condition checks the opposite situation: when we *don't* think the message time has just wrapped according to the *timestamps*, but the message time *has* just wrapped.

> And are you ignore internalFraction == 0.5 case intentionally?

No, not intentionally. This will never happen unless the inaccuracies mentioned above (i.e. from using TimeStamp::NowLoRes and because the moment when we first recorded the system time and recorded the corresponding timestamp might be *slightly* different) are greater than 25 days. I suppose that could happen if you put the computer to sleep precisely between executing the following two lines in UpdateFirstEventTime:

  DWORD currentTime = ::GetTickCount();
  TimeStamp currentTimeStamp = TimeStamp::Now()

Perhaps we should fetch the timestamp twice here to check that hasn't happened?

Comment 87

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.1 - Convert Windows native event times to timestamps

Address review feedback

Comment 88

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.1 - Convert Windows native event times to timestamps

Further formatting tweaks after running clang-format. Sorry for the bug-spam.

Comment 89

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8430540 [details] [diff] [review]
part 1.1 - Convert Windows native event times to timestamps

> I checked the style guide for this and changed it to:
> 
>   double a = b
>              ? c
>              : d;
> 
> (Or actually a little bit different since 'd' is long)

Indeed... It's defined too late since there are a lot of different styles... But I don't have strong objection for that.

>+TimeStamp
>+nsWindow::GetMessageTimeStamp(LONG aEventTime)
>+{

>+  // Event time may have wrapped several times since we recorded the first event
>+  // time (it wraps every ~50 days) so extend eventTimeStamp as needed. There is
>+  // some imprecision here since we are using TimeStamp::NowLoRes and
>+  // sFirstEventTime and sFirstEventTimeStamp may not be *exactly* the same
>+  // moment so we do an estimate at how many times event time has wrapped and
>+  // then adjust the result based on which side of the first event time the
>+  // current event time lands
>+
>+  double timesWrapped =
>+    (roughlyNow - sFirstEventTimeStamp).ToMilliseconds() / kEventTimeRange;

Thank you for a lot of explanation!

I find some points which make me confused. I don't like the name of timesWrapped, but I have no idea. But following wrappedCount variable and comments which explains what should do in each condition should make the meaning clearer.

>+  double intervalFraction = fmod(timesWrapped, 1);

First, I think that you should define new int variable here like,

int32_t wrappedCount = floor(timesWrapped);

Then, as you explained, this value may not be invalid due to including inaccuracy. Therefore, let's modify this value when we detect the inaccuracy.

>+  // Check if timesWrapped is just after the wrap point but the event time is
>+  // just before it
>+  if (intervalFraction < 0.5 && timeSinceFirstEvent > kEventTimeHalfRange) {

The comment should be like:

// If the rough time stamp indicating now is immediately after the wrap point
// but the event time isn't wrapped yet, the wrappedCount includes extra cycle.
// Therefore, we need to ignore the extra wrap.

>+    timesWrapped = floor(timesWrapped - 1);

Then, this should be: wrappedCount--;

>+  // Check if timesWrapped is just before the wrap point but the event time is
>+  // just after it
>+  } else if (intervalFraction > 0.5 &&
>+             timeSinceFirstEvent < kEventTimeHalfRange) {

And this comment should be like:

// If the rough time stamp indicating now is immediately before the wrap point
// but the event time is already wrapped, the wrappedCount should be 

>+    timesWrapped = ceil(timesWrapped);

And then, this should be wrappedCount++;

>+  } else {
>+    timesWrapped = floor(timesWrapped);
>+  }

And this else block isn't necessary.

If you have objection, please request review to me again. Otherwise, fix the comments with your better English before landing.

Comment 90

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.1 - Convert Windows native event times to timestamps;

Address review feedback

Comment 91

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Make event.timeStamp use TimeStamp value based on a pref

Only enable in Nightly/Aurora as discussed with Olli

Comment 92

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8431230 [details] [diff] [review]
part 2 - Make event.timeStamp use TimeStamp value based on a pref

>+Event::TimeStamp() const
>+{
>+  if (NS_IsMainThread()) {
Just use mIsMainThreadEvent



>+    if (!Preferences::GetBool("dom.event.highrestimestamp.enabled")) {
Please cache the pref value. event.timeStamp could be used in quite 
perf critical code paths.
So, AddIntVarCache


>+
>+    DOMHighResTimeStamp eventTime =
>+      perf->GetDOMTiming()->TimeStampToDOMHighRes(mEvent->timeStamp) :
>+    return static_cast<uint64_t>(eventTime);
So this returns time from navigation start. Why would we want that?


> function sanityCheckEvent(evt)
> {
>   is(evt.target, gAnim, "Unexpected event target");
>   is(evt.currentTarget, gAnim, "Unexpected event current target");
>   is(evt.eventPhase, evt.AT_TARGET);
>   is(evt.bubbles, false, "Event should not bubble");
>   is(evt.cancelable, false, "Event should not be cancelable");
>-  // Currently we set event timestamps to 0 which DOM 2 allows. This isn't
>-  // correct since SMIL uses this field to avoid synchronisation slew but first
>-  // we need to fix bug 323039 and bug 77992 which involve storing timestamps as
>-  // 64-bit integers and deciding whether those timestamps should be related to
>-  // the epoch or system start.
>-  is(evt.timeStamp, 0, "Event timeStamp should be 0");
>+  if (SpecialPowers.getBoolPref("dom.event.highrestimestamp.enabled")) {
>+    var now = window.performance.now();
>+    ok(evt.timeStamp > 0 && evt.timeStamp < now,
Should it be <=, not < ?

> pref("dom.event.contextmenu.enabled",       true);
> pref("dom.event.clipboardevents.enabled",   true);
>+#ifdef XP_WIN && defined(RELEASE_BUILD)
>+pref("dom.event.highrestimestamp.enabled",  true);
>+#else
>+pref("dom.event.highrestimestamp.enabled",  false);
>+#endif
We don't want the new stuff to be enabled in release builds.
event.timeStamp should behave the same way in all the platforms.

Comment 93

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Make event.timeStamp use TimeStamp value based on a pref

(In reply to Olli Pettay [:smaug] from comment #92)
> Comment on attachment 8431230 [details] [diff] [review]
> part 2 - Make event.timeStamp use TimeStamp value based on a pref
> 
> >+Event::TimeStamp() const
> >+{
> >+  if (NS_IsMainThread()) {
> Just use mIsMainThreadEvent

Fixed.

> >+    if (!Preferences::GetBool("dom.event.highrestimestamp.enabled")) {
> Please cache the pref value. event.timeStamp could be used in quite 
> perf critical code paths.
> So, AddIntVarCache

Fixed.

> >+    DOMHighResTimeStamp eventTime =
> >+      perf->GetDOMTiming()->TimeStampToDOMHighRes(mEvent->timeStamp) :
> >+    return static_cast<uint64_t>(eventTime);
> So this returns time from navigation start. Why would we want that?

Because that's what we agreed to do on the whatwg mailing list. See comment 43.

> > function sanityCheckEvent(evt)
> > {
> >   is(evt.target, gAnim, "Unexpected event target");
> >   is(evt.currentTarget, gAnim, "Unexpected event current target");
> >   is(evt.eventPhase, evt.AT_TARGET);
> >   is(evt.bubbles, false, "Event should not bubble");
> >   is(evt.cancelable, false, "Event should not be cancelable");
> >-  // Currently we set event timestamps to 0 which DOM 2 allows. This isn't
> >-  // correct since SMIL uses this field to avoid synchronisation slew but first
> >-  // we need to fix bug 323039 and bug 77992 which involve storing timestamps as
> >-  // 64-bit integers and deciding whether those timestamps should be related to
> >-  // the epoch or system start.
> >-  is(evt.timeStamp, 0, "Event timeStamp should be 0");
> >+  if (SpecialPowers.getBoolPref("dom.event.highrestimestamp.enabled")) {
> >+    var now = window.performance.now();
> >+    ok(evt.timeStamp > 0 && evt.timeStamp < now,
> Should it be <=, not < ?

That would mean that zero time elapsed from the time the event was created, through the time it was queued, handled, and then filtered through the other checks in the test code. Normally I could see that happening if the timer resolution was low for a given platform, but given that we are using the high-resolution timing interface here, I don't think that should never happen. If it does I think it suggests something is wrong.

> > pref("dom.event.contextmenu.enabled",       true);
> > pref("dom.event.clipboardevents.enabled",   true);
> >+#ifdef XP_WIN && defined(RELEASE_BUILD)
> >+pref("dom.event.highrestimestamp.enabled",  true);
> >+#else
> >+pref("dom.event.highrestimestamp.enabled",  false);
> >+#endif
> We don't want the new stuff to be enabled in release builds.
> event.timeStamp should behave the same way in all the platforms.

Fixed! (I noticed this myself this morning too!)

Comment 94

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8434598 [details] [diff] [review]
part 2 - Make event.timeStamp use TimeStamp value based on a pref

>+Event::TimeStamp() const
>+{
>+  if (!sReturnHighResTimeStamp) {
>+    return mEvent->time;
>+  }
>+
>+  if (mEvent->timeStamp.IsNull()) {
>+    return 0L;
>+  }
>+
>+  if (mIsMainThreadEvent) {
>+    if (NS_WARN_IF(!mOwner)) {
>+      return 0L;
>+    }
So this will return 0 for XHR's event used in JS components. I guess 
that is an edge case.


Ok, let's try this on Nightly/Aurora.
My guess is that this won't stick in, but we'll need to use
times from epoch.

Comment 95

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8434598 [details] [diff] [review]
part 2 - Make event.timeStamp use TimeStamp value based on a pref

> pref("dom.event.contextmenu.enabled",       true);
> pref("dom.event.clipboardevents.enabled",   true);
>+#if defined(XP_WIN) && !defined(RELEASE_BUILD)
>+pref("dom.event.highrestimestamp.enabled",  true);
>+#else
>+pref("dom.event.highrestimestamp.enabled",  false);
>+#endif


Actually, remove defined(XP_WIN) &&
We need to test this everywhere.

Comment 96

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #95)
> Actually, remove defined(XP_WIN) &&
> We need to test this everywhere.

FYI: But widget part is implemented only for Windows.

Comment 97

[Olli Pettay [:smaug][[e-mail]]]

Ah, right. This is even riskier since we don't get testing on Android or b2g etc.
But ok, Nightly/Aurora on Windows it is then.

Comment 98

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.0 - Add timeStamp to WidgetEvent;

Fix missing change to AndroidJavaWrappers.cpp

Comment 99

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8435367 [details] [diff] [review]
part 1.0 - Add timeStamp to WidgetEvent;

Hi Olli, can you check the change to AndroidJavaWrappers.cpp? Thanks.

Comment 100

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8435367 [details] [diff] [review]
part 1.0 - Add timeStamp to WidgetEvent;

r+ for the changes from the previous version of the patch

Comment 101

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Try: https://tbpl.mozilla.org/?tree=Try&rev=609ecf9b98a4

Follow-up try run with changes to Android: https://tbpl.mozilla.org/?tree=Try&rev=7df65cececab
 
I'll land once the Android try run completes if everything looks ok.

Comment 102

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55d21a229ee2
https://hg.mozilla.org/integration/mozilla-inbound/rev/16f49acfc4e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/2820efae7316
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa7d44e5eb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b92083bb0e8

Comment 103

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/55d21a229ee2
https://hg.mozilla.org/mozilla-central/rev/16f49acfc4e9
https://hg.mozilla.org/mozilla-central/rev/2820efae7316
https://hg.mozilla.org/mozilla-central/rev/4fa7d44e5eb3
https://hg.mozilla.org/mozilla-central/rev/5b92083bb0e8

Comment 104

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.2 - Fix a bug in Windows timestamp calculations

Comment 105

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.3 - Factor out a common utility class for converting wrapping native times to TimeStamps

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).

Comment 106

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1.4 - Convert GTK native event times to timestamps

This patch also enables using the DOMHighResTimeStamp behavior for
event.timeStamp on Linux.

Comment 107

[Rick Byers]

This is great!  Please see https://code.google.com/p/chromium/issues/detail?id=160524 for discussions on this issue we had in Chromium.  In particular, here are some of the potential web compatibility concerns that were raised with changing the semantics (and we weren't even entertaining the idea of changing the type):

1) existing code may assume that events are always delivered in timestamp order.  eg. that the delta between the timestamp of one event of one type and a following event of another type will always be positive.  We could break this, for example, by delivering a keyboard event ahead of a touch event that technically occurred further in the past.

2) semantics of synthesized events could be surprising.  eg. mousemove is often generated by hardware but sometimes generated synthetically in blink.  If someone is comparing the timestamps of a stream of mousemove events they could get surprising behavior (such as negative deltas like above).

3) semantics of coalesced events are unspecified.  eg. when two touchmove events are collapsed into one, what timestamp is reported?

I'd love to be convinced that these aren't a major concern in practice.  If not, then we should just add a new property to Event.

Comment 108

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Rick Byers from comment #107)
> This is great!  Please see
> https://code.google.com/p/chromium/issues/detail?id=160524 for discussions
> on this issue we had in Chromium.

Thanks Rick, I had a look at your notes there.

We've been reporting native event times in the timeStamp field for a while now. I'm not aware of any compatibility issues but we set Event.timeStamp so inconsistently I'd be impressed if anyone was able to make much sense of it.

I'm not sure how important it is to address the points you listed but if you have any specific proposals then perhaps we can follow up on the WHATWG mailing list.

Comment 109

[Karl Tomlinson (:karlt)]

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

>+class WindowsGetCurrentTime {
>+public:
>+  DWORD operator() () {

I would prefer the name CurrentWindowsTimeGetter so that constructing a
temporary doesn't look like a function call, but if there are existing
conventions for naming functors then it is OK to follow them.
"WindowsTime" also makes it clearer that this is not just a windows method to
get the current time.

>+static WrappingTimeConverter<DWORD> sTimeConverter =
>+  WrappingTimeConverter<DWORD>();

I don't know that the compiler can be trusted to use constant initialization
here.  Usually we avoid declaring non-POD non-local static variables because
their constructors inhibit startup speed.  Running static constructors
requires executing small pieces of code from throughout the executable,
missing the benefits of readahead.

The simplest solution may be move the static storage variable to local scope
of a function.  The variable will then be constructed when the function is first
executed.

>+template <typename Time>
>+const Time WrappingTimeConverter<Time>::kTimeRange =
>+  std::numeric_limits<Time>::max();
>+
>+template <typename Time>
>+const Time WrappingTimeConverter<Time>::kTimeHalfRange =
>+  WrappingTimeConverter<Time>::kTimeRange / 2;

Similarly here, I don't know what the compiler will do.
I suggest making these static methods.

>+EXPORTS.mozilla += [
>+    'WrappingTimeConverter.h',
>+]

I don't think this needs to be exported, as I expect it is only used in widget
code, which will have '../xpwidgets' in LOCAL_INCLUDES.

Below are some other things, at least most of which should not be addressed in
this patch, which should be about moving code.

>+    double timeSinceReference =
>+      aTime <= currentTime
>+        ? currentTime - aTime
>+        : static_cast<double>(kTimeRange) + currentTime - aTime;

Time is unsigned, so this is equivalent to 

 double timeSinceReference = currentTime - aTime;

and so could be

 Time timeSinceReference = currentTime - aTime;

Similarly somewhere else.

>+    // We can tell we have (b) if the time is a little bit before the reference
>+    // 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.
>+    if (aTime < mReferenceTime &&

This won't detect (b) if mReferenceTime is small.

>+    int32_t cyclesToAdd = static_cast<int32_t>(timesWrapped); // floor

This differs from floor in that -ve values (from LoRes) are rounded to zero,
which is important because of its consistency with fmod() below.  A comment to
explain that might prevent someone making changes, assuming this behaves like
floor().

Comment 110

[Karl Tomlinson (:karlt)]

(In reply to Rick Byers from comment #107)
> 1) existing code may assume that events are always delivered in timestamp
> order.  eg. that the delta between the timestamp of one event of one type
> and a following event of another type will always be positive.  We could
> break this, for example, by delivering a keyboard event ahead of a touch
> event that technically occurred further in the past.
>
> 2) semantics of synthesized events could be surprising.  eg. mousemove is
> often generated by hardware but sometimes generated synthetically in blink. 
> If someone is comparing the timestamps of a stream of mousemove events they
> could get surprising behavior (such as negative deltas like above).

I'm not sure I fully understand the example in 1, but different types of
events will also get out of order if they are processed in different queues.

Events from hardware/OS may be waiting in a queue while renderer generated
events are dispatched synchronously (or from a different queue).  Synthesized
events are one example of that, perhaps the most problematic because they are
the same "type" of event.

Perhaps a special timestamp (zero perhaps) could be used for synthesized
events to indicate that they should be handled differently.

> 3) semantics of coalesced events are unspecified.  eg. when two touchmove
> events are collapsed into one, what timestamp is reported?

When pointer motion events are coalesced, the useful time is the time when the
pointer reached the reported point in space, so the latter timestamp would be
reported.  I suspect the same principal could apply to gestures, but I'm not
so sure about that.

Comment 111

[Karl Tomlinson (:karlt)]

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

I'm not sure what to do here.  WrappingTimeConverter handles wrapping very
thoroughly but does not handle clock skew between TimeStamp and the windowing system time.

The easier clock skew situation to detect is when a windowing system event
reports a time in the future of what would be expected from TimeStamp::Now()
(or low res version).  When that happens, the difference could be recorded and
added to future event timestamps that are not based on windowing system times,
but then performance.now() might also need adjusting (if that should be consistent).

The harder situation to detect is when windowing system events are in the
past.  They are always in the past and will be in the more distant past when
they queue up because the browser is busy.  If that happens, then the browser
doesn't want to spend more time trying to determine the current windowing
system time.  This could be addressed, on detection of a lag, by recording
TimeStamp::Now() and requesting the windowing system time asynchronously.
When the reply is processed it will provide the necessary lower bound for the
windowing time to TimeStamp conversion.  Some events may get the wrong
timestamp while waiting for the reply, but this will later be corrected.

This patch is arguably an improvement because it makes things close for some
initial period of time at least, but I wonder whether it would be better if
the times were never in sync (as with existing code) so that no one started
assuming that they are always in sync.

If you want to get something into nightly to test compatibility, then I think
this approach might be OK for that.  Skew can be addressed before shipping on
other branches.

If you are looking for something to ship, then I wouldn't use
WrappingTimeConverter as it is currently, but merely provide that intervals
between events of the same type are accurate.

>+class GdkGetCurrentTime {

CurrentX11TimeGetter

>+    MOZ_ASSERT(mWindow, "No window for getting current time");

There may be no window if an nsWindow has been Destroy()ed (before it is
deleted), so this needs to be handled somewhere.  IIUC events won't be
received on GdkWindows that have been destroyed, and so most callers will be
OK, but DispatchDragEvent() is different.  If it is GetEventTimeStamp()'s
caller's responsibility to handle this then that should be documented.

>+static WrappingTimeConverter<guint32> sTimeConverter =
>+  WrappingTimeConverter<guint32>();

static constructor

Also watch out that 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.

>+namespace mozilla {
>+class TimeStamp;
>+}

>+    mozilla::TimeStamp GetEventTimeStamp(guint32 aEventTime);

I didn't know it was possible to forward declare a return value.

Perhaps an incomplete type error is only necessary if the method is used.
Are forward declarations sufficient for (non-pointer) parameters also, relying
on the caller to include the necessary files to apply the calling convention?

Forward declarations are good, but are you sure the forward declaration is what is making this compile?

Comment 112

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (needinfo?:karlt) from comment #111)
> The easier clock skew situation to detect is when a windowing system event
> reports a time in the future of what would be expected from TimeStamp::Now()
> (or low res version).  When that happens, the difference could be recorded and
> added to future event timestamps that are not based on windowing system times,
> but then performance.now() might also need adjusting (if that should be
> consistent).

Actually it need not be that complicated.  The upper bound for the
windowing time to TimeStamp conversion can be decreased without going backward in TimeStamp time, and event timestamps based on TimeStamp need not be affected.

Comment 113

[Olli Pettay [:smaug][[e-mail]]]

Since several patches landed already for FF31, could we perhaps move the new patches to a new bug?
That way possible backouts and such on branches are easier to track.
(Not that I'm expecting any backouts, but one never knows...)

Comment 114

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Moving to bug 1026803

Comment 115

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Moving to bug 1026803

Comment 116

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8429835 [details] [diff] [review]
part 5 - Make AsyncPanZoomController use the event timestamp for recording the last event

Moving parts 5-7 to bug 1026806.

Comment 117

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8429856 [details] [diff] [review]
part 8 - Make Event.timeStamp always a DOMHighResTimeStamp (remove pref)

Moving parts 8 and 9 to bug 1026809.

Comment 118

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21977cdadf43

Comment 119

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Olli Pettay [:smaug] from comment #113)
> Since several patches landed already for FF31, could we perhaps move the new
> patches to a new bug?

I've split off the following bugs for the remaining work:
Bug 1026803 - Convert native event times to TimeStamps for remaining platforms
Bug 1026804 - Make Event.timeStamp return a DOMHighResTimeStamp by default (switch on pref)
Bug 1026806 - Use the timeStamp member of events when performing time calculations
Bug 1026809 - Remove Event.time value

Comment 120

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21977cdadf43

Comment 121

[Avi Halachmi (:avih)]

Not sure if it's relevant to this bug, but with Firefox 31 I see these e.timeStamp values on 3 events about 1 second apart:

- 18446744071575587000
- 18446744071575587000
- 18446744071575589000

The system has an uptime of just more than 25 days, and something started acting weird simetime during the past 24h window, I think. It seems the resolution dropped to 1s and the first two events happened within the same "bucket".

Maybe some overflow due to the system uptime? Maybe in combination with some of the patches which landed on 31?

FWIW, Date.time() returns "expected" values still, like 1407057517056.

Comment 122

[Avi Halachmi (:avih)]

Also, comment 121 is with Windows 8.1, and I didn't notice this behavior with Firefox 30, and I _think_ I also had 25+ days of uptime while I was using Firefox 30, but I can't be sure of that - if there's a log of uptimes someplace then please point me to it and I'll report back.

Comment 123

[Avi Halachmi (:avih)]

Hmm.. 2^64 is 18,446,744,073,709,551,616

Rr (without the commas):

18446744073709551616

(In reply to Avi Halachmi (:avih) from comment #121)
> - 18446744071575587000
> - 18446744071575587000
> - 18446744071575589000

This is too close to be a coincidence. I think it overflows 64b and the resolution drops to 1s.

Comment 124

[Emanuel Hoogeveen [:ehoogeveen]]

Given the 25 day uptime and the millisecond frequency it sounds like this is being stored as an int32_t at some point, then being converted to an uint64_t: an int32_t would take (2^31 - 1) / (1000*60*60*24) = 24.86 days to overflow.

Comment 125

[Emanuel Hoogeveen [:ehoogeveen]]

For comparison, overflowing an uint64_t would take about 584.5 *million years* ;)

Comment 126

[Karl Tomlinson (:karlt)]

The landings in comment 103 were in Aurora 32 but not Aurora 31.
I don't know of any patches that landed here for Firefox 31.

Avi, are you able to file a separate bug, please, and include what kind of events are involved?  Different kinds of events get timestamps from different places, so that might help identify where the bad values are coming from.

Comment 127

[Avi Halachmi (:avih)]

(In reply to Karl Tomlinson (:karlt) from comment #126)
> The landings in comment 103 were in Aurora 32 but not Aurora 31.
> I don't know of any patches that landed here for Firefox 31.

I didn't follow all the patches of this bug, but based my Firefox 31 comment on this:

(In reply to Olli Pettay [:smaug] from comment #113)
> Since several patches landed already for FF31, could we perhaps move the new
> patches to a new bug?
> That way possible backouts and such on branches are easier to track.
> (Not that I'm expecting any backouts, but one never knows...)


(In reply to Karl Tomlinson (:karlt) from comment #126)
> Avi, are you able to file a separate bug, please, and include what kind of
> events are involved?  Different kinds of events get timestamps from
> different places, so that might help identify where the bad values are
> coming from.

Filed bug 1048096 for DOM wheel event.timeStamp changing to seconds resolution after 25 days uptime.

FWIW, I didn't notice it with Nightly 34.

Comment 128

[Jean-Yves Perrier [:teoli]]

Brian, in order to be able to document this, can you confirm/correct the following assertions:
- this bug changes the return type of Event.timeStamp from DOMTimeStamp to DOMHighResTimeStamp
- there should be no compatibility problem as DOMTimeStamp is an integer and DOMHighResTimeStamp is a double with its rounded value being the same as the previous DOMTimeStamp; but there is a risk there.
- in Beta and Release builds the returned value is the same as before (just the return type changed).
- in Nightly and Aurora, and for Windows only, non integer values are now returned.
- generation of non integer values for other platforms will be implemented in another bug.
- generation of non-integer values on Beta and Release builds will be activated in another bug.

Finally is this in Fx 31 or 33?

Thanks for your help!

Comment 129

[Olli Pettay [:smaug][[e-mail]]]

This change isn't enabled on release channels.
(But apparently it anyhow caused issues on release channels, so we should probably back this out for now)

To enable this stuff, see Bug 1026804.

Comment 130

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Jean-Yves Perrier [:teoli] from comment #128)
> Brian, in order to be able to document this, can you confirm/correct the
> following assertions:
> - this bug changes the return type of Event.timeStamp from DOMTimeStamp to
> DOMHighResTimeStamp

Like Olli said, it will when it is turned on. (Since Javascript just has Number, the return type doesn't actually change per se, just whether what we put in the Number comes from an integer or a double.)

> - there should be no compatibility problem as DOMTimeStamp is an integer and
> DOMHighResTimeStamp is a double with its rounded value being the same as the
> previous DOMTimeStamp; but there is a risk there.

There is a risk. We hope it is ok but we don't know until it hits release really. Other vendors are watching us to see how it goes.

> - in Beta and Release builds the returned value is the same as before (just
> the return type changed).

The returned value isn't changed in Beta/Release. There should be no observable changes in Beta/Release except where implementation bugs have been introduced or fixed.

> - in Nightly and Aurora, and for Windows only, non integer values are now
> returned.

Correct.

> - generation of non integer values for other platforms will be implemented
> in another bug.

We currently store both a timestamp and a (integer) millisecond count. The timestamp is just initialized to the current time when the event is created. On Windows, for platform events, we overwrite the timestamp after converting the time from the platform event to a timestamp. On other platforms we haven't done that yet so at the interface level we continue returning the integer millisecond count on those platforms.

The work to convert time values from native events to timestamps is going on in bug 1026803.

> - generation of non-integer values on Beta and Release builds will be
> activated in another bug.

Bug 1026804 will make us always return the timestamp value as a floating-point value.

> Finally is this in Fx 31 or 33?

There *should* be no observable changes in either (with the exception of the bug mentioned below).

(In reply to Olli Pettay [:smaug] from comment #129)
> This change isn't enabled on release channels.
> (But apparently it anyhow caused issues on release channels, so we should
> probably back this out for now)

Was that just an implementation bug though, not a compatibility one? Bug 1048096?

If it's just an implementation bug then hopefully the work in bug 1026803 fixes that. Unfortunately I won't be able to look at it until September so maybe backing out is best.

Comment 131

[Karl Tomlinson (:karlt)]

(In reply to Brian Birtles (:birtles, travelling 20~30 Aug, may be slower to respond) from comment #130)
> (In reply to Olli Pettay [:smaug] from comment #129)
> > This change isn't enabled on release channels.
> > (But apparently it anyhow caused issues on release channels, so we should
> > probably back this out for now)
> 
> Was that just an implementation bug though, not a compatibility one? Bug
> 1048096?

I don't know what issues Olli noticed this caused.

Changes landed in this bug, when enabled, fix bug 1048096, which existed in Fx 31, which has none of the changes from this bug.  That wouldn't be a reason to back this out.