Re: [whatwg/dom] Update Event.timeStamp type to DOMHighResTimeStamp. (#420)

annevk requested changes on this pull request.

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.

> @@ -628,8 +633,8 @@ algorithm below.
  false otherwise.
 
  <dt><code><var>event</var> . {{Event/timeStamp}}</code>
- <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?

> @@ -761,15 +766,14 @@ initialized to false.
 the user agent to dispatch an <a>event</a> whose {{Event/isTrusted}} attribute is initialized to
 false.
 
-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 to the number of milliseconds that have passed since
-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. 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?

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

> @@ -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>`?

> @@ -907,6 +915,14 @@ it, and optionally given a <a>Realm</a> <var>realm</var>, run these steps:</p>
  <a>dictionary member</a> and then set the attribute to the default value of that
  <a>dictionary member</a>.
 
+ <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.

Same here.

> @@ -907,6 +915,14 @@ it, and optionally given a <a>Realm</a> <var>realm</var>, run these steps:</p>
  <a>dictionary member</a> and then set the attribute to the default value of that
  <a>dictionary member</a>.
 
+ <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?

> @@ -5154,6 +5170,11 @@ 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()}}.
+
+

One newline too many as far as I can tell.

> @@ -5154,6 +5170,11 @@ 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()}}.
+
+

It also seems ugly that we initialize it twice.

> @@ -907,6 +915,14 @@ it, and optionally given a <a>Realm</a> <var>realm</var>, run these steps:</p>
  <a>dictionary member</a> and then set the attribute to the default value of that
  <a>dictionary member</a>.
 
+ <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.

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/dom/pull/420#pullrequestreview-47862885