bz for bug 116088 added:
> mEvent->time = PR_Now();
I have a few bones to pick with this;
first my compiler doesn't like it:
mozilla\content\events\src\nsDOMEvent.cpp(1638) : warning C4244: '=' :
conversion from 'PRTime' to 'PRUint32', possible loss of data
PRTime is a 64bit creature (sometimes a struct).
Second, dreftool doesn't like it
mozilla.sh ./dreftool -d ~/mozilla/content/events/src/ -e
~/mozilla/tools/dreftool/nullcall.lst -f ~/mozilla/tools/dreftool/nullfunc.lst
Summary for /mnt/ibm/mozhack/mozilla/content/events/src/:
third, the comment in the member variable definition doesn't match the critter
which is being stuffed into it:
/// elapsed time, in milliseconds, from the time the system was started to
the time the message was created
The right thing to be storing in there should be the time in milliseconds since
the epoch, not the time since system start.
So I think simply dividing PR_Now() by PR_USEC_PER_MSEC should be what we want
Anyone wanna whip up a patch for this?
Created attachment 146266 [details] [diff] [review]
per comment 1, also strips trailing whitespace from nsGUIEvent.h
probably needs changing.
So do a whole bunch of places in widget/src/gtk/nsWidget.cpp and
widget/src/gtk/nsWindow.cpp that assign event.time.
In fact, I bet almost every single line listed in
http://lxr.mozilla.org/seamonkey/search?string=event-%3Etime needs changing....
Comment on attachment 146266 [details] [diff] [review]
per comment 1, also strips trailing whitespace from nsGUIEvent.h
- In nsDOMEvent::AllocateEvent():
+ LL_DIV(now, now, microtomilli);
+ mEvent->time = now;
+ LL_DIV(mEvent->time, now, microtomilli);
r+sr=jst for the patch so far, but as bz pointed out, there's more to be done
in certain cases LL_DIV will access the first param a lot, which isn't cheap.
i'd rather do local operations and then only access the pointer once.
It should probably use PR_IntervalNow() rather than PR_Now(), since it is mostly used to compute differences, and since it might be compared against non-script-generated events, which use PR_IntervalNow(). And it definitely needs to be milliseconds, not microseconds, since using microseconds breaks the mochitest in bug 424780.
should be milliseconds, any epoch is ok
set in microseconds (wrong)
set in Intervals (which happen to be milliseconds on Mac and Windows!?)
Created attachment 315353 [details] [diff] [review]
Smaug, jst thought you might know more about whether event timestamps should use PR_IntervalNow (for accurate intervals) or PR_Now (for accurate dates).
As per spec I'd say PR_Now.
In practice, what is the difference between PR_IntervalNow and PR_Now in this
With PR_Now(), if the computer's clock is reset while you're typing into a <select>, the wrong item might be selected (nsListControlFrame.cpp).
http://www.w3.org/TR/DOM-Level-2-Events/events.html#Events-Event-timeStamp says we can choose the epoch, so PR_IntervalNow() is also correct per spec, isn't it?
I think I'd prefer to stay away from PR_IntervalNow() as the interval units you get back from it are platform dependent and have an unknown and in deterministic epoch. nsDOMEvent::GetTimeStamp() already returns a PRUint64, which is what you get back from PR_Now() and also matches our definition of nsIDOMEvent.timeStamp, so I think I'd prefer to use that across the board, even if that means changing other event internals (i.e. widget code etc).
I won't be able to make those larger changes, and this bug blocks a leak fix (bug 424780) :(
Any chance attachment 146266 [details] [diff] [review] is going to be landed? It would be nice to implement timestamps consistently so we can use them for SMIL (which relies on them to avoid synchronisation slew between animations).
SMIL relies on DOMEvent timestamps?
Note, per DOM Events spec it is ok to have 0 as the timestamp.
(But sure, we should handle time stamps consistently.)
And if SMIL relies on timestamps, I wonder how? Does it add some event listeners?
If so, where? Or do SMIL elements implement PostHandleEvent?
Sorry, I posted the link in bug 77992 but not here.
Here it is:
"In order to make the model operate consistently and remove the effects of synchronization slew in a chain of event times, the timestamp value associated with events such as the beginEvent, endEvent, and repeat events is not (necessarily) the actual time that the event is raised, nor is it the time when a time dependent is actually notified of the event."
When you read the following descriptions of click events and so on it appears that what is expected is that the animation should be considered to start at the time the click was generated rather than when the click event was received. In order to do that we need to preserve the time the event was generated in its timestamp. This time is then preserved in SMIL's own TimeEvents' timestamps (although an offset may or may not be added, hence the somewhat confusing sentence about the timestamp of the propagated TimeEvent not necessarily being the same as the time the original event was generated).
Or at least that's my understanding.
It's all about removing synchronisation slew that would otherwise be introduced by the time taken to propagate events.
SMIL will add event listeners when we come to implement event-timing (bug 485157) which we're hoping to get in for Firefox 4.
However if we choose not to fix this then the result is simply that SMIL animations get a little out of sync which may be acceptable for the next release anyway. However, it could possibly complicate testing a little if we can't rely on the animations being synchronised.
Come to think of it, my previous message was half wrong.
We need to store timestamps correctly for the events generated by SMIL (TimeEvents). To get everything perfectly synced we should also do it for other events too (e.g. click etc.) but for the examples given in the linked document that's not necessary.
What is necessary though is to have some sensible (and preferably consistent) way of storing timestamps and using 64-bit int would sure make that a lot more straightforward. I'm not overly fussed about whether it's an interval time or epoch time although an epoch time would simplify the SMIL implementation.
If we want to store timestamps (as opposed to wall-clock times) we should be using mozilla::TimeStamp. That's what it's designed for. It guarantees things like timestamps being nondecreasing (which PR_Now() does NOT guarantee).
(In reply to comment #17)
> When you read the following descriptions of click events and so on it appears
> that what is expected is that the animation should be considered to start at
> the time the click was generated rather than when the click event was received.
Is that the time that the user clicked or the time that the DOM click event was generated (when the browser received the OS click event)?
(In reply to comment #17)
> It's all about removing synchronisation slew that would otherwise be introduced
> by the time taken to propagate events.
Is this (only) the slew that would be introduced propagating a single event?
Or are relative times of related events also important?
(e.g. should buttonup and click [or keydown and keypress] have the same timestamp if they are produced by the same user action?)
(In reply to comment #20)
> Is that the time that the user clicked or the time that the DOM click event was
> generated (when the browser received the OS click event)?
I don't know enough about our event handling to answer that. SMIL needs to be able to register as a listener of arbitrary events. If it's possible and advantageous to do that in a way that doesn't rely on DOM events being created (i.e. just nsEvents) then I guess we need the timestamp in the nsEvent to be set appropriately.
For events generated outside SMIL, the time itself isn't so important. What's important is simply synchronisation so that two animations keyed off the same click event are perfectly in sync.
For the events generated by SMIL itself, the time itself is important. We need to set a timestamp in the nsEvent that we can later pick up and convert to a time in SMIL's timeline that matches the time the event was generated.
> (In reply to comment #17)
> Is this (only) the slew that would be introduced propagating a single event?
> Or are relative times of related events also important?
> (e.g. should buttonup and click [or keydown and keypress] have the same
> timestamp if they are produced by the same user action?)
From my understanding that is not necessary. The main use case is that if two animations are keyed off the same event they should be in sync.
*** Bug 702015 has been marked as a duplicate of this bug. ***
DOM4 does no longer allow 0 nor milliseconds from system start.
> The timeStamp attribute must return the value it was initialized to. When an event is created the attribute must be initialized to the number of milliseconds that has passed since 00:00:00 UTC on 1 January 1970.
Having the 'timeStamp' field set to 0 for the 'mouseleave' and 'mouseout' DOM events on Firefox ESR 17.0.5 on 64-bit Gentoo.
The 'timeStamp' field does contain a valid value for the 'mouseup' DOM event.
Created attachment 740562 [details]
This file 'FF-no-timeStamp-demo.html' demonstrates this problem.
Attaching 'FF-no-timeStamp-demo.html' that demonstrates this problem.
(In reply to maratbn from comment #24)
> Having the 'timeStamp' field set to 0 for the 'mouseleave' and 'mouseout'
> DOM events on Firefox ESR 17.0.5 on 64-bit Gentoo.
> The 'timeStamp' field does contain a valid value for the 'mouseup' DOM event.
Actually noticed that the 'timeStamp' for the 'mouseup' event is also invalid, but non-0.
Created attachment 740592 [details]
Attaching 'FF-no-timeStamp-demo.html' that demonstrates this problem, as well as compares the invalid timestamps with the actual current time.
*** Bug 960988 has been marked as a duplicate of this bug. ***
*** Bug 960459 has been marked as a duplicate of this bug. ***
Is there any progress on this ticket?
Some events still have event.timeStamp = 0 or some small number instead of microseconds since 1970-01-01 00:00:00 UTC.
What is interesting -- events, generated on media element (e.g. onplay, onpause) -- are giving correct numbers while common events (onclick, onmouseover) -- are giving these crazy results.
Reproduced on OSX 64-bit (10.9.5 to be precise) -- so it is not x86 or Windows-specific issue
Is it correct that it is simply int32 issue? I.e. we try to push very long number into 32-bit variable and we loose data or break it then?
Example: timeStamp = 1438777470568000 # (new Date()).getTime() * 1000
But maximum value for uint32 is: 4294967295.
P.S. It is very strange to see this bug open for 11.5 years while it is obvious bug and it is in the core of the platform.
Stumbled upon this bug report after a failing test which relied on event.timeStamp from a blur event (FF 46.0.1, Windows 7 enterprise 64 bit).