Use high precision timestamp for Event.timestamp

Use high precision time for Event.timeStamp (behind REF)

Change Event.timestamp to expose high-precision monotonic time for Input
events behind a RuntimeEnabledFlag.

For input events we use the WebInputEvents.timeStampSeconds where available.
Where it is not available (e.g., non-input events or events created by the
script) use blink's monotonic time clock.

The timestamp is recorded in the Event object as is provided but we change the
timestamp's origin to the  target document's time origin base before exposing
it.

Also fixes a small bug where keyboard event timestamp was not being plumbed
correctly when it was converted from WebInputEvent to Platform event.

For testing I have added two new facilities:
1. A method to internals that allows conversion from platform time to document time.
2. A method to eventSender that allows obtaining timeStamp of last dispatched event.

BUG=160524
TEST=fast/events/hr-timestamp

Committed: https://crrev.com/bfabb071f4c01c0e0ff3c733946771f986bc4b9d
Cr-Commit-Position: refs/heads/master@{#352051}

[majidvp, 2015-09-18 15:18:10]

majidvp@chromium.org changed reviewers:
+ rbyers@chromium.org

[majidvp, 2015-09-18 15:18:10]

Here is a first try at having high precision time.

The part that I am not so sure about is the logic for changing the  the
timestamp time origin to document's time origin.

Current patch does it when timestamp is accessed and uses the current event
target  to determine the document relevant.
This is pretty simple but it means that the event should be
dispatched otherwise timestamp is 0.

Alternatively, I can try to translate the timestamp at event creation time
but that requires having the document instance available anywhere we create
events.

[Rick Byers, 2015-09-18 15:40:57]

On 2015/09/18 15:18:10, majidvp wrote:
> Here is a first try at having high precision time.
> 
> The part that I am not so sure about is the logic for changing the  the
> timestamp time origin to document's time origin.
> 
> Current patch does it when timestamp is accessed and uses the current event
> target  to determine the document relevant.
> This is pretty simple but it means that the event should be
> dispatched otherwise timestamp is 0.
> 
> Alternatively, I can try to translate the timestamp at event creation time
> but that requires having the document instance available anywhere we create
> events.

Couple initial comments on the CL description (before looking at the code):

 - Please mention this is currently behind the experimental flag (no behavior
change by default)
 - Reference the bug

[Rick Byers, 2015-09-18 15:55:49]

rbyers@chromium.org changed reviewers:
+ kinuko@chromium.org

[Rick Byers, 2015-09-18 15:56:33]

There's enough subtly (and privacy implications) in high-precision timing, that
I'd like a timing expert's input on this patch also.  kinuko@ it looks like
maybe that's you?

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
File Source/core/events/Event.cpp (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:38: static double monotonicTimeWithSafePrecision()
Can you share this logic with the usage in PerformanceBase?  Eg. maybe
core/timing should be the place where such helpers live and Event just uses
those?

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Event.h
File Source/core/events/Event.h (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.h:128: double timeStampForPlatformInSeconds() const;
nit: add comment for the new method (i.e. what "for platform" means).

[dtapuska, 2015-09-18 16:04:42]

dtapuska@chromium.org changed reviewers:
+ dtapuska@chromium.org

[dtapuska, 2015-09-18 16:04:42]

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
File Source/core/events/Event.cpp (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:281: timeStamp = 
m_target->toNode()->document().monotonicTimeToZeroBasedDocumentTime(m_uiCreateTime)
* millisPerSecond;
How does this work when the m_currentNode and the m_targetNode are in different
documents? Rick tells me that shadow documents exist; and they will propagate
the same event across the trees.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Event.h
File Source/core/events/Event.h (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.h:127: double timeStamp() const;
I think there should be a specific call for getting the timestamp from bindings
that you then apply the precision loss calcaulation to.

So that if native code calls this; the timestamp is reflected to the current
time. (Currently there is uiCreateTime which native code can get at the time).

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.h:231: double m_uiCreateTime; // For input events, the
time the event was recorded by the UI in seconds.
When indicating time; be sure to reference what clock. ie; monotonic clock.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
File Source/core/events/Event.idl (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.idl:47: readonly attribute DOMHighResTimeStamp
timeStamp;
Are the semantics of this unchanged since it is now a double; wrt NaN
initialization of a timestamp?

[Rick Byers, 2015-09-18 16:17:29]

I didn't try to trace through all the cases to verify that the returned
timestamp always matches the DOMHighResttimestamp semantics when your feature is
on.  Presumably you'll be able to verify this once you add a layout test, right?

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
File Source/core/events/Event.cpp (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:279: // hiResTime
By user-generated you mean real events, not script-generated ones?  There are
indirect ways that script can create real events (eg. calling focus()).  Can we
be sure none of those are getting full precision timestamps?

Perhaps it's safest to just reduce all timestamps to 5us precision here (like
Performance.now) - I think that's plenty accurate for our input scenarios.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:281: timeStamp = 
m_target->toNode()->document().monotonicTimeToZeroBasedDocumentTime(m_uiCreateTime)
* millisPerSecond;
Event::m_uiCreateTime is poorly defined (and perhaps also poorly named). 
Perhaps worth at least improving comments there as part of this CL (i.e. what's
the timebase, explicitly say it's the time from the host OS - often called
"platform time").

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:281: timeStamp = 
m_target->toNode()->document().monotonicTimeToZeroBasedDocumentTime(m_uiCreateTime)
* millisPerSecond;
Does the conversion really have to be done based on the target node's document? 
That seems quite strange, and there are edge cases where it may not be what
you'd expect.  Also this will be broken for events dispatched to the window (not
all EventTarget's are Nodes).  Perhaps you can get the Window or LocalFrame of
the Event instead?  This is, I think, easy for UIEvent (stored in m_view), but
not so much for Event.

Maybe the real problem is that you're doing this conversion too late.  What if
we changed m_uiCreateTime (or added a new field) which was to be the exact
DOMHighPerformanceTimestamp value to be returned?  Then we'd do the time base
conversion when creating the Event in the first place (where we have some more
context)?  That would also avoid conversion back and forth for JS created events
(where the script is giving you the exact value that should later be returned).

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
File Source/core/events/Event.idl (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.idl:47: readonly attribute DOMHighResTimeStamp
timeStamp;
On 2015/09/18 16:04:42, dtapuska wrote:
> Are the semantics of this unchanged since it is now a double; wrt NaN
> initialization of a timestamp?

Right, we need to make sure that this type change isn't visible to the web at
all (at least until we make the RuntimeEnabledFeature stable).  Eg. JS created
events with non-integer timestamps should have their timestamp coerced when the
feature is disabled in the exact same way it is before your code.  We'll
probably want some tests for this (that pass before and after your CL).

[majidvp, 2015-09-18 17:43:34]

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
File Source/core/events/Event.cpp (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:38: static double monotonicTimeWithSafePrecision()
On 2015/09/18 15:56:32, Rick Byers wrote:
> Can you share this logic with the usage in PerformanceBase?  Eg. maybe
> core/timing should be the place where such helpers live and Event just uses
> those?

Acknowledged.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:279: // hiResTime
On 2015/09/18 16:17:29, Rick Byers wrote:
> By user-generated you mean real events, not script-generated ones?  There are
> indirect ways that script can create real events (eg. calling focus()).  Can
we
> be sure none of those are getting full precision timestamps?
> 
> Perhaps it's safest to just reduce all timestamps to 5us precision here (like
> Performance.now) - I think that's plenty accurate for our input scenarios.

User generated = real events. It will definitely be safest to reduce precision
before returning the value. That ensures there is no subtle paths where we might
expose actual time. I was not sure if that is accurate enough for real events
but if it is then I prefer that way.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:281: timeStamp = 
m_target->toNode()->document().monotonicTimeToZeroBasedDocumentTime(m_uiCreateTime)
* millisPerSecond;
On 2015/09/18 16:17:29, Rick Byers wrote:
> Does the conversion really have to be done based on the target node's
document? 
> That seems quite strange, and there are edge cases where it may not be what
> you'd expect.  Also this will be broken for events dispatched to the window
(not
> all EventTarget's are Nodes).  Perhaps you can get the Window or LocalFrame of
> the Event instead?  This is, I think, easy for UIEvent (stored in m_view), but
> not so much for Event.
> 
> Maybe the real problem is that you're doing this conversion too late.  What if
> we changed m_uiCreateTime (or added a new field) which was to be the exact
> DOMHighPerformanceTimestamp value to be returned?  Then we'd do the time base
> conversion when creating the Event in the first place (where we have some more
> context)?  
I agree that the target node's document may not be the best time reference given
what Dave pointed out.

We can definitely use LocalFrame or window where the event is created as a basis
for translation. It just needs some extra plumbing to make these available at
any place we create event

Another option is to use the time origin of the Window where Event.timeStamp is
accessed. This is pretty straightforward as binding can expose ScriptState. This
has the advantage that even if an event is passed across document, the value
returned by Event.timeStamp will be correct in relation to the context it is
being used. 



That would also avoid conversion back and forth for JS created events
> (where the script is giving you the exact value that should later be
returned).
> 
I am not sure what you are referring here. We never take timestamp from script.
For script created events, their timestamp is be the monotonic  time at the time
of event creation. In any case, cost of changing the origin of a platform
monotonic time is negligible.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Event.h
File Source/core/events/Event.h (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.h:127: double timeStamp() const;
On 2015/09/18 16:04:42, dtapuska wrote:
> I think there should be a specific call for getting the timestamp from
bindings
> that you then apply the precision loss calcaulation to.
> 
> So that if native code calls this; the timestamp is reflected to the current
> time. (Currently there is uiCreateTime which native code can get at the time).

Agreed. I like to keep timeStamp() to be the method used by DOM that applies
precision filtering. This way it is easy to correspond it with IDL. I will use a
different naming for platform timestamp methods.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.h:128: double timeStampForPlatformInSeconds() const;
On 2015/09/18 15:56:32, Rick Byers wrote:
> nit: add comment for the new method (i.e. what "for platform" means).

Acknowledged.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.h:231: double m_uiCreateTime; // For input events, the
time the event was recorded by the UI in seconds.
On 2015/09/18 16:04:42, dtapuska wrote:
> When indicating time; be sure to reference what clock. ie; monotonic clock.

Acknowledged.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
File Source/core/events/Event.idl (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.idl:47: readonly attribute DOMHighResTimeStamp
timeStamp;
On 2015/09/18 16:17:29, Rick Byers wrote:
> On 2015/09/18 16:04:42, dtapuska wrote:
> > Are the semantics of this unchanged since it is now a double; wrt NaN
> > initialization of a timestamp?
> 
> Right, we need to make sure that this type change isn't visible to the web at
> all (at least until we make the RuntimeEnabledFeature stable).  Eg. JS created
> events with non-integer timestamps should have their timestamp coerced when
the
> feature is disabled in the exact same way it is before your code.  We'll
> probably want some tests for this (that pass before and after your CL).

Good point. I will make sure this is covered in my tests.

[Rick Byers, 2015-09-21 15:44:37]

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
File Source/core/events/Event.cpp (right):

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:279: // hiResTime
On 2015/09/18 17:43:33, majidvp wrote:
> On 2015/09/18 16:17:29, Rick Byers wrote:
> > By user-generated you mean real events, not script-generated ones?  There
are
> > indirect ways that script can create real events (eg. calling focus()).  Can
> we
> > be sure none of those are getting full precision timestamps?
> > 
> > Perhaps it's safest to just reduce all timestamps to 5us precision here
(like
> > Performance.now) - I think that's plenty accurate for our input scenarios.
> 
> User generated = real events. It will definitely be safest to reduce precision
> before returning the value. That ensures there is no subtle paths where we
might
> expose actual time. I was not sure if that is accurate enough for real events
> but if it is then I prefer that way.

5us is really short, in practice the variability in input latency is much more
than that.  We typically pay attention to changes on the order of 50us or maybe
a little smaller ourselves.  Let's start with the safer option - we can always
try to relax it later if we decide it would be valuable.

https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
Source/core/events/Event.cpp:281: timeStamp = 
m_target->toNode()->document().monotonicTimeToZeroBasedDocumentTime(m_uiCreateTime)
* millisPerSecond;
On 2015/09/18 17:43:33, majidvp wrote:
> On 2015/09/18 16:17:29, Rick Byers wrote:
> > Does the conversion really have to be done based on the target node's
> document? 
> > That seems quite strange, and there are edge cases where it may not be what
> > you'd expect.  Also this will be broken for events dispatched to the window
> (not
> > all EventTarget's are Nodes).  Perhaps you can get the Window or LocalFrame
of
> > the Event instead?  This is, I think, easy for UIEvent (stored in m_view),
but
> > not so much for Event.
> > 
> > Maybe the real problem is that you're doing this conversion too late.  What
if
> > we changed m_uiCreateTime (or added a new field) which was to be the exact
> > DOMHighPerformanceTimestamp value to be returned?  Then we'd do the time
base
> > conversion when creating the Event in the first place (where we have some
more
> > context)?  
> I agree that the target node's document may not be the best time reference
given
> what Dave pointed out.
> 
> We can definitely use LocalFrame or window where the event is created as a
basis
> for translation. It just needs some extra plumbing to make these available at
> any place we create event
> 
> Another option is to use the time origin of the Window where Event.timeStamp
is
> accessed. This is pretty straightforward as binding can expose ScriptState.
This
> has the advantage that even if an event is passed across document, the value
> returned by Event.timeStamp will be correct in relation to the context it is
> being used. 
> 
> 
> 
> That would also avoid conversion back and forth for JS created events
> > (where the script is giving you the exact value that should later be
> returned).
> > 
> I am not sure what you are referring here. We never take timestamp from
script.
> For script created events, their timestamp is be the monotonic  time at the
time
> of event creation. In any case, cost of changing the origin of a platform
> monotonic time is negligible.
> 
> 
> 

Ah, sorry!  I assumed EventInitDict had a timestamp field, but it doesn't:
https://dom.spec.whatwg.org/#interface-event.  I'll file a spec issue to ask
about that.  But in the meantime, yes I agree the problem I was thinking of
doesn't actually exist.

[majidvp, 2015-09-24 11:39:23]

On 2015/09/21 15:44:37, Rick Byers wrote:
>
https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
> File Source/core/events/Event.cpp (right):
> 
>
https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
> Source/core/events/Event.cpp:279: // hiResTime
> On 2015/09/18 17:43:33, majidvp wrote:
> > On 2015/09/18 16:17:29, Rick Byers wrote:
> > > By user-generated you mean real events, not script-generated ones?  There
> are
> > > indirect ways that script can create real events (eg. calling focus()). 
Can
> > we
> > > be sure none of those are getting full precision timestamps?
> > > 
> > > Perhaps it's safest to just reduce all timestamps to 5us precision here
> (like
> > > Performance.now) - I think that's plenty accurate for our input scenarios.
> > 
> > User generated = real events. It will definitely be safest to reduce
precision
> > before returning the value. That ensures there is no subtle paths where we
> might
> > expose actual time. I was not sure if that is accurate enough for real
events
> > but if it is then I prefer that way.
> 
> 5us is really short, in practice the variability in input latency is much more
> than that.  We typically pay attention to changes on the order of 50us or
maybe
> a little smaller ourselves.  Let's start with the safer option - we can always
> try to relax it later if we decide it would be valuable.
> 
>
https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
> Source/core/events/Event.cpp:281: timeStamp = 
>
m_target->toNode()->document().monotonicTimeToZeroBasedDocumentTime(m_uiCreateTime)
> * millisPerSecond;
> On 2015/09/18 17:43:33, majidvp wrote:
> > On 2015/09/18 16:17:29, Rick Byers wrote:
> > > Does the conversion really have to be done based on the target node's
> > document? 
> > > That seems quite strange, and there are edge cases where it may not be
what
> > > you'd expect.  Also this will be broken for events dispatched to the
window
> > (not
> > > all EventTarget's are Nodes).  Perhaps you can get the Window or
LocalFrame
> of
> > > the Event instead?  This is, I think, easy for UIEvent (stored in m_view),
> but
> > > not so much for Event.
> > > 
> > > Maybe the real problem is that you're doing this conversion too late. 
What
> if
> > > we changed m_uiCreateTime (or added a new field) which was to be the exact
> > > DOMHighPerformanceTimestamp value to be returned?  Then we'd do the time
> base
> > > conversion when creating the Event in the first place (where we have some
> more
> > > context)?  
> > I agree that the target node's document may not be the best time reference
> given
> > what Dave pointed out.
> > 
> > We can definitely use LocalFrame or window where the event is created as a
> basis
> > for translation. It just needs some extra plumbing to make these available
at
> > any place we create event
> > 
> > Another option is to use the time origin of the Window where Event.timeStamp
> is
> > accessed. This is pretty straightforward as binding can expose ScriptState.
> This
> > has the advantage that even if an event is passed across document, the value
> > returned by Event.timeStamp will be correct in relation to the context it is
> > being used. 
> > 
> > 
> > 
> > That would also avoid conversion back and forth for JS created events
> > > (where the script is giving you the exact value that should later be
> > returned).
> > > 
> > I am not sure what you are referring here. We never take timestamp from
> script.
> > For script created events, their timestamp is be the monotonic  time at the
> time
> > of event creation. In any case, cost of changing the origin of a platform
> > monotonic time is negligible.
> > 
> > 
> > 
> 
> Ah, sorry!  I assumed EventInitDict had a timestamp field, but it doesn't:
> https://dom.spec.whatwg.org/#interface-event.  I'll file a spec issue to ask
> about that.  But in the meantime, yes I agree the problem I was thinking of
> doesn't actually exist.

I have uploaded a new patch. Here is what is changes:
- Thanks to the blink merge the CL now include also changes
to event_sender needed to test this
- Use target document time origin
- Added layout tests to verify correctness (both input and other constructed
events) and safe resolution
- rename uiCreateTime to platformTimestamp
- Apply safe resolution always and refactored the code
- Better comments

PTAL.

[dtapuska, 2015-09-24 14:36:57]

On 2015/09/24 11:39:23, majidvp wrote:
> On 2015/09/21 15:44:37, Rick Byers wrote:
> >
>
https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
> > File Source/core/events/Event.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
> > Source/core/events/Event.cpp:279: // hiResTime
> > On 2015/09/18 17:43:33, majidvp wrote:
> > > On 2015/09/18 16:17:29, Rick Byers wrote:
> > > > By user-generated you mean real events, not script-generated ones? 
There
> > are
> > > > indirect ways that script can create real events (eg. calling focus()). 
> Can
> > > we
> > > > be sure none of those are getting full precision timestamps?
> > > > 
> > > > Perhaps it's safest to just reduce all timestamps to 5us precision here
> > (like
> > > > Performance.now) - I think that's plenty accurate for our input
scenarios.
> > > 
> > > User generated = real events. It will definitely be safest to reduce
> precision
> > > before returning the value. That ensures there is no subtle paths where we
> > might
> > > expose actual time. I was not sure if that is accurate enough for real
> events
> > > but if it is then I prefer that way.
> > 
> > 5us is really short, in practice the variability in input latency is much
more
> > than that.  We typically pay attention to changes on the order of 50us or
> maybe
> > a little smaller ourselves.  Let's start with the safer option - we can
always
> > try to relax it later if we decide it would be valuable.
> > 
> >
>
https://codereview.chromium.org/1352523002/diff/60001/Source/core/events/Even...
> > Source/core/events/Event.cpp:281: timeStamp = 
> >
>
m_target->toNode()->document().monotonicTimeToZeroBasedDocumentTime(m_uiCreateTime)
> > * millisPerSecond;
> > On 2015/09/18 17:43:33, majidvp wrote:
> > > On 2015/09/18 16:17:29, Rick Byers wrote:
> > > > Does the conversion really have to be done based on the target node's
> > > document? 
> > > > That seems quite strange, and there are edge cases where it may not be
> what
> > > > you'd expect.  Also this will be broken for events dispatched to the
> window
> > > (not
> > > > all EventTarget's are Nodes).  Perhaps you can get the Window or
> LocalFrame
> > of
> > > > the Event instead?  This is, I think, easy for UIEvent (stored in
m_view),
> > but
> > > > not so much for Event.
> > > > 
> > > > Maybe the real problem is that you're doing this conversion too late. 
> What
> > if
> > > > we changed m_uiCreateTime (or added a new field) which was to be the
exact
> > > > DOMHighPerformanceTimestamp value to be returned?  Then we'd do the time
> > base
> > > > conversion when creating the Event in the first place (where we have
some
> > more
> > > > context)?  
> > > I agree that the target node's document may not be the best time reference
> > given
> > > what Dave pointed out.
> > > 
> > > We can definitely use LocalFrame or window where the event is created as a
> > basis
> > > for translation. It just needs some extra plumbing to make these available
> at
> > > any place we create event
> > > 
> > > Another option is to use the time origin of the Window where
Event.timeStamp
> > is
> > > accessed. This is pretty straightforward as binding can expose
ScriptState.
> > This
> > > has the advantage that even if an event is passed across document, the
value
> > > returned by Event.timeStamp will be correct in relation to the context it
is
> > > being used. 
> > > 
> > > 
> > > 
> > > That would also avoid conversion back and forth for JS created events
> > > > (where the script is giving you the exact value that should later be
> > > returned).
> > > > 
> > > I am not sure what you are referring here. We never take timestamp from
> > script.
> > > For script created events, their timestamp is be the monotonic  time at
the
> > time
> > > of event creation. In any case, cost of changing the origin of a platform
> > > monotonic time is negligible.
> > > 
> > > 
> > > 
> > 
> > Ah, sorry!  I assumed EventInitDict had a timestamp field, but it doesn't:
> > https://dom.spec.whatwg.org/#interface-event.  I'll file a spec issue to ask
> > about that.  But in the meantime, yes I agree the problem I was thinking of
> > doesn't actually exist.
> 
> I have uploaded a new patch. Here is what is changes:
> - Thanks to the blink merge the CL now include also changes
> to event_sender needed to test this
> - Use target document time origin
> - Added layout tests to verify correctness (both input and other constructed
> events) and safe resolution
> - rename uiCreateTime to platformTimestamp
> - Apply safe resolution always and refactored the code
> - Better comments
> 
> PTAL.

non-owner lgtm

[Rick Byers, 2015-09-25 17:29:33]

This is looking really good.  Just a couple more minor suggestions.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/hr-timestamp/constructed-events.html
(right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/hr-timestamp/constructed-events.html:11:
assert_approx_equals(e.timeStamp, now, 0.05, "Timestamp should be within 0.05 ms
of the performance.now() when it was created");
This might be too sensitive to avoid flakiness (eg. imagine running on a very
loaded machine, possibly with Valgrind/ASAN instrumentation, etc.).  It also
could fail when running under a debugger.

Perhaps a better test that's immune to any amount of jank (or pausing in a
debugger) would be to just verify that the timestamp is between (or equal)
performance.now() from before the event was created and performance.now() from
after it was created.  WDYT?

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/events/hr-timestamp/input-events.html
(right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/hr-timestamp/input-events.html:2:
<body></body>
why this?

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5126: return 0;
any idea when this would happen in practice?  Just wondering what bugs might
results.  Is there precedent for using invalid timestamps in such situations? 
Ideally we'd be able to ASSERT that this doesn't happen.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/Event.cpp (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/Event.cpp:270: if (scriptState &&
scriptState->domWindow() && scriptState->domWindow()->document()) {
It feels a little weird to depend on a Document here, but I don't immediately
see a way to go direct to the LocalFrame.  I _think_ the special cases like
extensions will be OK but I won't be surprised if we're missing some case
(that'll show up with a timestamp of value 0 or with the wrong time base).  But
since this is still behind a flag, it's fine to land it as-is and then do some
experiments.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/Event.idl (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/Event.idl:47: [CallWith=ScriptState]
readonly attribute DOMHighResTimeStamp timeStamp;
Add a TODO here about the return type - mentioning it's sometimes actually a
DOMTimeStamp.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/TypeAhead.h (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/TypeAhead.h:67: double m_lastTypeTime;
Can this be a DOMHighRestTimestamp to make it clear what the semantics are?  Or
perhaps we should typedef something new?  Just a shame to loose some
documentation here about what exactly the double represents.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceBase.h:48: double
toSafePrecisionTime(double);
Maybe this should be a static method on PerformanceBase or something?  We tend
not to have a lot of global functions like this (but happy to be shown
precedent).

[majidvp, 2015-09-29 14:21:31]

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/hr-timestamp/constructed-events.html
(right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/hr-timestamp/constructed-events.html:11:
assert_approx_equals(e.timeStamp, now, 0.05, "Timestamp should be within 0.05 ms
of the performance.now() when it was created");
On 2015/09/25 17:29:33, Rick Byers wrote:
> This might be too sensitive to avoid flakiness (eg. imagine running on a very
> loaded machine, possibly with Valgrind/ASAN instrumentation, etc.).  It also
> could fail when running under a debugger.
> 
> Perhaps a better test that's immune to any amount of jank (or pausing in a
> debugger) would be to just verify that the timestamp is between (or equal)
> performance.now() from before the event was created and performance.now() from
> after it was created.  WDYT?

Makes sense. Done.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/events/hr-timestamp/input-events.html
(right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/hr-timestamp/input-events.html:2:
<body></body>
On 2015/09/25 17:29:33, Rick Byers wrote:
> why this?

Removed.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:5126: return 0;
On 2015/09/25 17:29:33, Rick Byers wrote:
> any idea when this would happen in practice?  Just wondering what bugs might
> results.  Is there precedent for using invalid timestamps in such situations? 
> Ideally we'd be able to ASSERT that this doesn't happen.

I imagine this can happen if for example one creates a document with
document.implementation.createHTMLDocument().

I changed the implementation so that it uses the same fallback logic as
performance::now() in such cases (i.e., time origin = 0). This will ensure we
are always consistent with performance::now() and timestamps remain relatively
valid.

I am not so sure about the correctness of the performance::now() fallback but
logged a separate ticket for that
https://code.google.com/p/chromium/issues/detail?id=536959

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/Event.cpp (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/Event.cpp:270: if (scriptState &&
scriptState->domWindow() && scriptState->domWindow()->document()) {
On 2015/09/25 17:29:33, Rick Byers wrote:
> It feels a little weird to depend on a Document here, but I don't immediately
> see a way to go direct to the LocalFrame.  I _think_ the special cases like
> extensions will be OK but I won't be surprised if we're missing some case
> (that'll show up with a timestamp of value 0 or with the wrong time base). 
But
> since this is still behind a flag, it's fine to land it as-is and then do some
> experiments.


There is a way to obtain the performance timing directly from window[1]  which
bypasses the document. I switched this to use it.


[1]https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/timing/DOMWindowPerformance.h&sq=package:chromium&l=26&q=DOMWindowPerformance::performance

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/TypeAhead.h (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/TypeAhead.h:67: double m_lastTypeTime;
On 2015/09/25 17:29:33, Rick Byers wrote:
> Can this be a DOMHighRestTimestamp to make it clear what the semantics are? 
Or
> perhaps we should typedef something new?  Just a shame to loose some
> documentation here about what exactly the double represents.

This is not really a DOMHighResTimeStamp but the actual platform timestamp in
seconds.
Converting to DOMHighRestTimeStamp and using Event.timeStamp is not appropriate
here because we only care about the delta and extra calculations to convert are
unnecessary and confusing.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceBase.h:48: double
toSafePrecisionTime(double);
On 2015/09/25 17:29:33, Rick Byers wrote:
> Maybe this should be a static method on PerformanceBase or something?  We tend
> not to have a lot of global functions like this (but happy to be shown
> precedent).

Some one have added an equivalent static method while I was working on this
patch. I switched using that instead.

[Rick Byers, 2015-09-29 15:57:59]

LGTM with nit

It's too bad that kinuko@ hasn't had a chance to look at this yet, but with the
new design I think the risk is low enough that I'm OK not blocking on her review
(of course you can always tweak in a follow-up CL if she or other
DOMHighResTimestamp experts have suggestions).

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/Event.cpp (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/Event.cpp:270: if (scriptState &&
scriptState->domWindow() && scriptState->domWindow()->document()) {
On 2015/09/29 14:21:31, majidvp wrote:
> On 2015/09/25 17:29:33, Rick Byers wrote:
> > It feels a little weird to depend on a Document here, but I don't
immediately
> > see a way to go direct to the LocalFrame.  I _think_ the special cases like
> > extensions will be OK but I won't be surprised if we're missing some case
> > (that'll show up with a timestamp of value 0 or with the wrong time base). 
> But
> > since this is still behind a flag, it's fine to land it as-is and then do
some
> > experiments.
> 
> 
> There is a way to obtain the performance timing directly from window[1]  which
> bypasses the document. I switched this to use it.
> 
> 
>
[1]https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/timing/DOMWindowPerformance.h&sq=package:chromium&l=26&q=DOMWindowPerformance::performance

Nice, this makes a lot more sense to me (and seems safer too) - thanks!

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/forms/TypeAhead.h (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/forms/TypeAhead.h:67: double m_lastTypeTime;
On 2015/09/29 14:21:31, majidvp wrote:
> On 2015/09/25 17:29:33, Rick Byers wrote:
> > Can this be a DOMHighRestTimestamp to make it clear what the semantics are? 
> Or
> > perhaps we should typedef something new?  Just a shame to loose some
> > documentation here about what exactly the double represents.
> 
> This is not really a DOMHighResTimeStamp but the actual platform timestamp in
> seconds.
> Converting to DOMHighRestTimeStamp and using Event.timeStamp is not
appropriate
> here because we only care about the delta and extra calculations to convert
are
> unnecessary and confusing. 

Thanks, makes sense.  Adding the comment is good enough for me.

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/1352523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceBase.h:48: double
toSafePrecisionTime(double);
On 2015/09/29 14:21:31, majidvp wrote:
> On 2015/09/25 17:29:33, Rick Byers wrote:
> > Maybe this should be a static method on PerformanceBase or something?  We
tend
> > not to have a lot of global functions like this (but happy to be shown
> > precedent).
> 
> Some one have added an equivalent static method while I was working on this
> patch. I switched using that instead.

Perfect!  Perhaps worth checking to see who that was and cc'ing them on this?

https://codereview.chromium.org/1352523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1352523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:433: return
monotonicTimeToDOMHighResTimeStamp(monotonicallyIncreasingTime()); // in
milliseconds
nit: this comment seems redundant (it's clear you're going to
DOMHighRestTimestamp - comments on units like this are necessary as a hack only
when using plain numeric types).

[majidvp, 2015-10-01 20:57:19]

rbyers: following our offline discussion I reverted to continue using epoch time
when converting core input event -> web input event. This ensures plugins such
as Pepper that expect epoch time continue to work as expected.

I have created a separate bug (crbug.com/538199) to track cleaning this up once
this lands.

https://codereview.chromium.org/1352523002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1352523002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/timing/PerformanceBase.cpp:433: return
monotonicTimeToDOMHighResTimeStamp(monotonicallyIncreasingTime()); // in
milliseconds
On 2015/09/29 15:57:59, Rick Byers wrote:
> nit: this comment seems redundant (it's clear you're going to
> DOMHighRestTimestamp - comments on units like this are necessary as a hack
only
> when using plain numeric types).

Done.

[Rick Byers, 2015-10-02 00:56:55]

Yeah, sounds good.  Hopefully changing the semantics of the plugin timestamp is
really easy / unlikely to break anyone, but that really is orthogonal (and has
it's own unrelated set of risks).

LGTM

[majidvp, 2015-10-02 13:24:11]

The CQ bit was checked by majidvp@chromium.org

[majidvp, 2015-10-02 13:24:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1352523002/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2015-10-02 13:24:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1352523002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1352523002/300001

[commit-bot: I haz the power, 2015-10-02 16:30:08]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-10-02 16:31:17]

Patchset 16 (id:??) landed as
https://crrev.com/bfabb071f4c01c0e0ff3c733946771f986bc4b9d
Cr-Commit-Position: refs/heads/master@{#352051}