Update Event.timeStamp type to DOMHighResTimeStamp. #420

majido · 2017-03-08T19:27:45Z

Update Event.timeStamp type to DOMHighResTimeStamp.

This fixes #23 and the commit contains the following changes:

The text asks for the high resolution time of the event occurrence time to be used when
available and otherwise event object creation time.
Add note in legacy section describing the change in semantic.
Add note encouraging user agents to make high res time available for UI Events.
Add necessary spec reference for Bikeshed to pick up new definitions in hr-time spec.

Update text recommending use of high resolution event occurrence time where available. Add note in legacy section describing the change. Add necessary spec reference for Bikeshed to pick up new definitions.

domenic · 2017-03-08T19:36:47Z

dom.bs

+initialized to. If available, the attribute should be initialized with the high resolution time of
+the occurrence that the event is signaling, for example the moment when a particular user
+interaction occurred. Otherwise it should be initialized with the time representing the creation
+time of the <var>event</var>.


Nit: "s/the time representing the creation time/the creation time"

domenic · 2017-03-08T19:36:55Z

dom.bs

-<p class=XXX>This is highly likely to change and already does not reflect implementations well.
-Please see <a href=https://github.com/whatwg/dom/issues/23>dom #23</a> for more details.
+<p class=note>User agents are encouraged to make the high resolution time occurrence time
+available for as many events as possible in particular those related to user interaction.


Nit: comma after "as possible"

annevk

Do we have tests already?

annevk · 2017-03-09T08:59:32Z

dom.bs

+initialized to. If available, the attribute should be initialized with the high resolution time of
+the occurrence that the event is signaling, for example the moment when a particular user
+interaction occurred. Otherwise it should be initialized with the creation time of the
+<var>event</var>.


What does "creation time" mean?

This is the same terminology that was used before. The way I read it was the time where the event instance was constructed which is what Chrome implementation was also using.

Put it another way this is IMHO step 1 in section 2.4. Perhaps it should link to that.

The terminology before had specific instructions with respect to what the value should contain. "Creation time" by itself doesn't give me a number.

Done. The wording now says When an <a>event</a> is created the attribute must be initialized with a {{DOMHighResTimeStamp}} representing the high resolution time in milliseconds that has passed since the <a>time origin</a>.

This is similar to previous text but with new time type and origin. This initial value may change in event creation algorithm which now have the wording about occurrence time etc.

annevk · 2017-03-09T08:59:48Z

dom.bs

-00:00:00 UTC on 1 January 1970, ignoring leap seconds.
-<!-- leap seconds are ignored by JavaScript too -->
+The <dfn attribute for=Event><code>timeStamp</code></dfn> attribute must return the value it was
+initialized to. If available, the attribute should be initialized with the high resolution time of


Can we enumerate when it's not?

It seems that for synthetic events this would never be available.

What do you mean by synthetic events? If you are referring to those constructed in Javascript then that is probably correct and what for example Chrome implementation does. (BTW, I think issue #76 is proposing to make timestamp a constructor argument in which case this is not true anymore)

There may be other implementation related restrictions that makes the precise occurrence time unavailable. For example on some platforms (e.g., older Windows) there is no cross-process monotonic clock which in browsers with multi-process architecture can mean we cannot use the original timestamp. Or sometimes the driver uses a different clock and timestamp is not reliable. In such cases we end up using the closest estimation we can get. I think falling back to "creation time" is acceptable here too.

Yeah, a synthetic event is basically a "userland" event. It would be good if we were consistent in what value it would get there (and yes, maybe at some point we allow custom values there, but that would just end up being a branch there).

I'm not really sure how to best do it though. I only have a long term vision where user interaction events are defined in much more detail and they end up initializing this attribute properly (and by default it's just creation time, whatever that means).

Done. I now have exact details on what to use for synthetic events (i.e., use construction time).

annevk · 2017-03-09T09:01:55Z

dom.bs

- <dd>Returns the creation time of <var>event</var> as the number of milliseconds that
- passed since 00:00:00 UTC on 1 January 1970.
+ <dd>Returns the <var>event</var>'s timestamp as the number of milliseconds measured relative
+ from the <a>time origin</a>.


It seems to me that this term is also important for the normative definition, yet it doesn't appear there.

majido · 2017-03-09T22:31:05Z

Do we have tests already?

No I have not upstreamed the test yet. I was thinking to have this PR first and gain some confidence that it is accurate and then refer to it when upstreaming the test.

Also, hr-time spec has recommendation on limiting the minimum resolution of the clock exposed via performance.now() to prevent certain types of timing attacks. Given that synthetic events will use the same clock, they can be used to the same effect (e.g., repeatedly creating events and reading their timestamp). We have made sure to prevent this in Chrome. I think we should add some language to the spec (perhaps a note?) recommending the same thing. wdyt?

the recommended minimum resolution of the Performance interface should be set to 5 microseconds.

annevk · 2017-03-10T07:25:38Z

Yeah, a class=warning block that points there below the normative definition of the attribute seems good.

Normative definition of timeStamp attribute: - Declare that it is initialized to equivalent of performance.now() on creation - Add warning about 5 microsends minimum resolution from hrtime spec Event creation: - Clarify that in this case, if available, occurence time should be used Event constructio and Document.createEvent - Clarify that in this case, the time that the method is invoked (i.e., construction) time should be used.

majido · 2017-06-22T20:49:20Z

@annevk thanks for bearing with me on this.

So I just added a new commit which I think should address your feedback. Here are the changes I made:

Normative definition of timeStamp attribute now defines that it should be initialized to equivalent of performance.now() on creation. I also added warning about 5 microsends minimum resolution from hrtime spec and referenced that spec.

I also added details on how this attribute should be initialized in each algorithm where we create or construct an Event.

On event creation, if available the occurrence time should be used to initialize the property.
On event construction or when Document.createEvent is used, the time that the method is invoked should be used. **

** I actually feel that this may not be necessary since the attribute definition specifies the same but I followed the example of how isTrusted is spec'd. 😕

majido · 2017-06-22T20:56:39Z

As I mentioned in #76, there may be potential for simplifying this if we add timestamp to EventInit with a default value. But perhaps that is best done in a separate patch.

majido · 2017-06-26T13:35:33Z

@annevk I also have a Blink patch to upstream parts of our tests for this change. I will land those once this patch is lgtm'd. Please take a look there an let me know if you have any comments on the tests as well.

annevk · 2017-06-28T23:00:28Z

I think the issue I mention in #76 also applies to this patch.

annevk

In general this looks reasonable, but I'm still a little unhappy with how sketchy some of it is.

Apologies for the delay in review. The main reason is that I've been away for 5+ weeks.

annevk · 2017-07-04T13:19:43Z

dom.bs

- <dd>Returns the creation time of <var>event</var> as the number of milliseconds that
- passed since 00:00:00 UTC on 1 January 1970.
+ <dd>Returns the <var>event</var>'s timestamp as the number of milliseconds measured relative
+ from the <a>time origin</a>.


relative to, no?

annevk · 2017-07-04T13:20:24Z

dom.bs

+The <dfn attribute for=Event><code>timeStamp</code></dfn> attribute must return the value it was
+initialized to. When an <a>event</a> is created the attribute must be initialized with a
+{{DOMHighResTimeStamp}} representing the high resolution time in milliseconds that has passed
+since the <a>time origin</a>.


It seems we don't need to define what it is set to when the event is created as we already do that later separately, no?

Removed. To be honest, I found it odd too but was cargo culting the definition of isTrusted just above it.

annevk · 2017-07-04T13:21:05Z

dom.bs

-<p class=XXX>This is highly likely to change and already does not reflect implementations well.
-Please see <a href=https://github.com/whatwg/dom/issues/23>dom #23</a> for more details.
+<p class=warning> User agents are strongly encouraged to set minimum resolution of the
+{{Event/timeStamp}} attribute to 5 microseconds following existing <a>clock resolution</a>


the existing*

Doesn't read well otherwise.

annevk · 2017-07-04T13:21:37Z

dom.bs

@@ -875,6 +879,10 @@ must be run, given the arguments <var>type</var> and <var>eventInitDict</var>:

 <li><p>Initialize <var>event</var>'s {{Event/type}} attribute to <var>type</var>.

+ <li><p>Initialize <var>event</var>'s {{Event/timeStamp}} attribute to a {{DOMHighResTimeStamp}}
+ representing the <var>event</var>'s <dfn>construction time</dfn> which is the high resolution
+ time from the <a>time origin</a> to the occurrence of the call to the <a>constructor</a>.


What is the benefit of the new <dfn> here? Can we reword this without introducing a new <dfn>?

Done. Removed.

annevk · 2017-07-04T13:21:43Z

dom.bs

+ <li><p>If available, set <var>event</var>'s {{Event/timeStamp}} attribute to a
+ {{DOMHighResTimeStamp}} representing the <var>event</var>'s <dfn>occurence time</dfn> which is
+ the high resolution time from the <a>time origin</a> to the occurrence that the event is
+ signaling, for example the moment when a particular user interaction occurred.


Also, if what is available? Should we not just make this a requirement and not allow user agents not to have this?

I think we should also have a note here that gives a far more detailed example. E.g., the name of a particular system call on some OS that gives you the time we'd expect to see exposed here.

I removed "If available". I was thinking of cases where there is not any high resolution time available on the underlying event, e.g., some older Windows devices don't have a high resolution clock or the clock is not usable across processes. In such cases, Chromium uses the timestamp when event was received by renderer process as the closest thing we can have to the "occurrence time".

I guess there is no need to have the spec special case this as the term "occurrence time" gives enough flexibility to user agents to handle such limitations. Also per your suggestion, I added an example (for macOs) which should help guide how occurrence time should be interpreted by implementations.

annevk · 2017-07-04T13:23:00Z

dom.bs

+ which is the high resolution time from the <a>time origin</a> to the occurrence of the call to
+ {{Document/createEvent()}}.
+
+


One newline too many as far as I can tell.

It also seems ugly that we initialize it twice.

Done.

As for duplicate initialization, I am not sure if I understand. The way I was reading the spec was that for non-synthetic events we do the 2.4 - concept-event-create that does not require invoking 2.4. - concept-event-constructor which is only needed for synthetic events.

- Grammatical corrections. - Remove unnecessary definition. - Add example for NSEvent on macOS and remove "If available" for the non-synthetic event case.

majido · 2017-07-10T15:45:35Z

Apologies for the delay in review. The main reason is that I've been away for 5+ weeks.

Not at all. Vacations are great 😄 and there is not particular rush for this. 👍

annevk

Couple minor nits left. With respect to isTrusted I think we simply forgot to remove "When an event is created the attribute must be initialized to false." when we added the other bits. I'll create a pull request for that.

annevk · 2017-07-11T12:05:18Z

dom.bs


- <p class=note>User agents are encouraged to make the high resolution occurrence time available
- for as many events as possible, in particular those related to user interaction.
+ <p class=note>For example, in macOs the occurrence time for input actions are available via the


annevk · 2017-07-11T12:08:34Z

dom.bs

+ representing the high resolution time from the <a>time origin</a> to the occurrence that the
+ event is signaling.
+
+ <p class=note>For example, in macOs the occurrence time for input actions are available via the


annevk · 2017-07-11T12:12:15Z

dom.bs

@@ -5154,6 +5170,10 @@ invoked, must run these steps:

 <li><p>Initialize <var>event</var>'s {{Event/type}} attribute to the empty string.

+ <li><p>Initialize <var>event</var>'s {{Event/timeStamp}} attribute to a {{DOMHighResTimeStamp}}
+ which is the high resolution time from the <a>time origin</a> to the occurrence of the call to
+ {{Document/createEvent()}}.


This was what I was talking about with respect to duplication, but I guess that's also following the isTrusted precedent. Probably okay. Can this become "from the time origin to now"? It's a little weird for the algorithm to refer to itself this way.

Replaced self-reference with "now".

annevk · 2017-07-11T12:13:56Z

Oh also, could you add an explicit pointer to the tests somewhere? I haven't been able to find them.

annevk · 2017-07-11T12:16:28Z

Actually, it seems that for isTrusted we don't define anything in the constructor. So that's why it's slightly different with respect to that.

majido · 2017-07-11T14:52:19Z

Tests are here: https://codereview.chromium.org/2744553007/
I have not moved them to WPT yet as I was waiting for spec patch to land. Happy to address any comments you may have on them.

annevk · 2017-07-12T08:39:47Z

Thanks, those look good to me.

Do you want to add your name to the Acknowledgments section as well? With that we can land this I think. I'll go ahead and file a bug against Edge so that's covered as well.

majido · 2017-07-12T12:36:18Z

Do you want to add your name to the Acknowledgments section as well?

Absolutely. 🎖️

majido · 2017-07-12T12:36:43Z

I will go ahead an upstream the tests now.

annevk · 2017-07-12T14:12:19Z

dom.bs

 <li><p>Initialize <var>event</var>'s {{Event/isTrusted}} attribute to false.

 <li><p>Unset <var>event</var>'s <a>initialized flag</a>.

- <li><p>Return <var>event</var>.
+ <li><p>Return <var>event</var>.`


Seems you added this by accident in the last commit.

Ops! fixed.

This is in anticipation of DOM spec patch landing [1]. Upstreaming two tests: - verify constructed events get a high-resolution time from perfomance.now() - verify the minimum resolution is larger that 5µs. [1] whatwg/dom#23 [2] whatwg/dom#420 BUG=538600 Review-Url: https://codereview.chromium.org/2744553007 Cr-Commit-Position: refs/heads/master@{#485966}

annevk · 2017-07-13T07:39:04Z

Thanks!

Use DOMHighResTimeStamp for Event.timeStamp.

388b856

Update text recommending use of high resolution event occurrence time where available. Add note in legacy section describing the change. Add necessary spec reference for Bikeshed to pick up new definitions.

domenic reviewed Mar 8, 2017

View reviewed changes

Address @domenic's feedback

c53e8c7

annevk reviewed Mar 9, 2017

View reviewed changes

annevk added the needs tests Moving the issue forward requires someone to write tests label Mar 16, 2017

annevk mentioned this pull request Apr 17, 2017

Should EventInit contain 'timestamp'? #76

Open

annevk requested changes Jul 4, 2017

View reviewed changes

Address annevk@ feedback

198fe36

- Grammatical corrections. - Remove unnecessary definition. - Add example for NSEvent on macOS and remove "If available" for the non-synthetic event case.

annevk reviewed Jul 11, 2017

View reviewed changes

Minor corrections

0ae7247

Add Majid Valipour to credits

f7f84d7

annevk reviewed Jul 12, 2017

View reviewed changes

Fix inadvertant mistake

3f9f333

chromium-wpt-export-bot mentioned this pull request Jul 12, 2017

Upstream high resolution event timestamp test to WPT. web-platform-tests/wpt#6532

Merged

annevk merged commit 912d587 into whatwg:master Jul 13, 2017

majido deleted the high-res-timestamp branch July 13, 2017 12:32

domenic mentioned this pull request Jan 22, 2020

pageshow event timeStamp in BFCache whatwg/html#5226

Open

		which is the high resolution time from the <a>time origin</a> to the occurrence of the call to
		{{Document/createEvent()}}.

Update Event.timeStamp type to DOMHighResTimeStamp. #420

Update Event.timeStamp type to DOMHighResTimeStamp. #420

Conversation

majido commented Mar 8, 2017 • edited by pr-preview bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majido commented Mar 9, 2017

annevk commented Mar 10, 2017

majido commented Jun 22, 2017

majido commented Jun 22, 2017

majido commented Jun 26, 2017

annevk commented Jun 28, 2017

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majido commented Jul 10, 2017

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Jul 11, 2017

annevk commented Jul 11, 2017

majido commented Jul 11, 2017

annevk commented Jul 12, 2017

majido commented Jul 12, 2017

majido commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Jul 13, 2017

majido commented Mar 8, 2017 •

edited by pr-preview bot

Loading