Address outstanding L2 issues #35

philipwalton · 2017-12-22T18:43:05Z

This PR closes #21, #22, #26, and #31. It also fixes formatting inconsistencies, improves linking between specs, and adjusts the section hierarchy to be more consistent with other web-perf specs.

marcoscaceres

Couple of minor ReSpec thingies. You should probably run tidy over the document after this gets merged. The markup is a little messy, so will make review easier once cleaned up.

marcoscaceres · 2018-01-16T02:39:25Z

index.html


-          noLegacyStyle: true,
+        noLegacyStyle: true,


Remove this, as it does nothing :)

marcoscaceres · 2018-01-16T02:41:15Z

index.html

-               value: 'Test Suite repository',
-               href: 'https://github.com/w3c/web-platform-tests/tree/master/user-timing'
-             }]
+        otherLinks: [{


Replace github related things inotherLinks this with:

github: "https://github.com/w3c/user-timing/"

Once you do that, you can also drop edDraftURI, as it should be automatically generated for you.

Updated. @marcoscaceres can you confirm I did it right?

marcoscaceres · 2018-01-16T02:41:53Z

index.html

-    which expose a high precision, monotonically increasing timestamp so they can better measure the performance characteristics of their applications.</p>
-
+         },{
+           key: 'Mailing list',


Not sure how the group likes to work, but in other groups we've been discouraging mailing lists.

Ditto, we can point to GH.

marcoscaceres · 2018-01-16T02:42:45Z

index.html

+             value: 'Can I use User Timing?',
+             href: 'http://caniuse.com/#feat=user-timing'
+           }, {
+             value: 'Test Suite',


Please use testSuiteURI instead.

philipwalton · 2018-01-16T06:04:09Z

Polite ping: @igrigorik

igrigorik

Sorry about the delay! The formatting update made this a tricky one to review. A few of the suggestions I noted are not directly related to the earlier commits, but would be good to address as part of this update nonetheless.

igrigorik · 2018-01-17T07:35:41Z

index.html

-    which expose a high precision, monotonically increasing timestamp so they can better measure the performance characteristics of their applications.</p>
-
+         },{
+           key: 'Mailing list',


Ditto, we can point to GH.

igrigorik · 2018-01-17T07:38:39Z

index.html

+        <h2><dfn>mark()</dfn> method</h2>
+        <p>Stores a timestamp with the associated name (a "mark"). It MUST run these steps:</p>
+        <ol>
+          <li>Run the <a href="#ensure-name-is-valid">ensure name is valid</a> algorithm with <var>name</var> set to <var>markName</var>.</li>


To make thing simple, why not use same name in the algorithm definition to allow <a>concept</a>?

In this step it's not clear where markName is sourced from, as there is no mention of it in the definition.

To make thing simple, why not use same name in the algorithm definition to allow concept?

Done. Didn't know that was possible :)

In this step it's not clear where markName is sourced from, as there is no mention of it in the definition.

Is it not? markName is used exactly as it's used in the IDL, do I need to link to it or something? I'm not sure what you mean by "there is no mention of it in the definition".

igrigorik · 2018-01-17T07:40:41Z

index.html

+        <ol>
+          <li>Run the <a href="#ensure-name-is-valid">ensure name is valid</a> algorithm with <var>name</var> set to <var>markName</var>.</li>
+          <li>Create a new <a>PerformanceMark</a> object.</li>
+          <li>Set <code>name</code> to <var>markName</var>.</li>


This and following steps implicitly refer to the new object. Let's be explicit?

igrigorik · 2018-01-17T07:41:49Z

index.html

+        <h2><dfn>measure()</dfn> method</h2>
+        <p>Stores the <code>DOMHighResTimeStamp</code> duration between two marks along with the associated name (a "measure"). It MUST run these steps:</p>
+        <ol>
+          <li>Run the <a href="#ensure-name-is-valid">ensure name is valid</a> algorithm with <var>name</var> set to <var>measureName</var>.</li>


Same comment as mark() on measureName.

Sanity check.. the throw behavior for measure is not what we enforce in Chrome today. Did we agree that this is, indeed, the desired behavior?

Filed #36 to track this.

Same comment as mark() on measureName.

Done.

Added 242a0ea to address #36

igrigorik · 2018-01-17T07:42:45Z

index.html

+          <li>Let <var>end time</var> be <code>0</code>.</li>
+          <li>If <var>endMark</var> is omitted, let <var>end time</var> be the value that would be returned by the <code>Performance</code> object's <a data-cite="!HR-TIME-2#dom-performance-now"><code>now()</code></a> method. Otherwise:
+            <ol>
+              <li>If <var>endMark</var> has the same name as a <a data-cite="!WEBIDL#dfn-read-only">read only attribute</a> in the <code><a data-cite="!NAVIGATION-TIMING#performancetiming">PerformanceTiming</a></code> interface, let <var>end time</var> be the value return by running the <a href="#convert-a-name-to-a-timestamp">convert name to a timestamp</a> algorithm with <var>name</var> set to <var>endMark</var>.</li>


"value returned"? Multiple occurrences..

with name set to the value of endmark?

with name set to the value of endmark?

Done.

"value returned"? Multiple occurrences..

I don't see multiple values being returned. Is that what you're implying?

igrigorik · 2018-01-17T07:52:20Z

index.html

+            </ol>
+          </li>
+          <li>Create a new <a>PerformanceMeasure</a> object.</li>
+          <li>Set <code>name</code> to <var>measureName</var>.</li>


Same as mark for Set.. substeps.

igrigorik · 2018-01-17T08:00:47Z

index.html

+        <li>If <var>endTime</var> is <code>0</code>, <a data-cite="!WEBIDL#dfn-throw">throw</a> an <a data-cite="!WEBIDL#invalidaccesserror"><code>InvalidAccessError</code></a>.</li>
+        <li>Return result of subtracting <var>startTime</var> from <var>endTime</var>.</li>
+      </ol>
+    </section>


I think it would also be helpful to add a non-normative note, either as a preface, or after each algorithm here to note that: (a) PerformanceTiming was defined by NT1 and is now considered obsolete, (b) there are no plans to extend this functionality to PerformanceNavigationTiming (NT2).

nolanlawson · 2018-01-20T00:29:35Z

index.html

+  <section class="informative" id="introduction">
+    <h2>Introduction</h2>
+    <p>Web developers need the ability to assess and understand the performance characteristics of their applications. While JavaScript <a href="#ECMA262">[ECMA262]</a> provides a mechanism to measure application latency (retrieving the current timestamp from the <code>Date.now()</code> method), the precision of this timestamp varies between user agents.</p>
+    <p>This document defines the <a>PerformanceMark</a> and <a>PerformanceMeasure</a> interfaces, and extensions to the <a href="#extensions-performance-interface"><code>Performance</code></a> interface, which expose a high precision, monotonically increasing timestamp so they can better measure the performance characteristics of their applications.</p>


It may be worthwhile to mention here the benefits of mark()/measure() relative to performance.now(). Without mentioning performance.now(), Date.now() might seem like a straw man.

Perhaps mentioning Dev Tools support or support in tools like Windows Performance Analyzer and WebPageTest may be worth putting in a non-normative section. Not sure if it's out-of-scope for this document though.

Agreed. I opened #37 to track this so we can get this PR close quicker.

philipwalton · 2018-01-25T04:28:49Z

The formatting update made this a tricky one to review.

@igrigorik BTW, in case you're not aware, in Github you can add ?w=1 to the end of a diff URL and have it ignore whitespace. It's really helpful for diffs like this.

marcoscaceres · 2018-02-21T21:55:23Z

index.html

+      <p>The <a>PerformanceMark</a> interface extends the following attributes of the <a data-cite="!PERFORMANCE-TIMELINE-2#performanceentry">PerformanceEntry</a>
+      interface:</p>
+      <p>The <code>name</code> attribute will return the mark's name.</p>
+      <p>The <code>entryType</code> attribute will return the <a data-cite="!WEBIDL#idl-DOMString"><code>DOMString</code></a> <code>"mark"</code>.</p>


Are these redefinitions?

I'm not quite sure what you mean by that, but regardless, I didn't make any content changes here. Maybe @igrigorik can comment?

Sorry, let me be more clear. This section is about interface PerformanceMark, then it talks about entryType attribute. Where is entryType defined? Why is it being discussed here?

That is, PerformanceEntry defines the entryType attribute (which is in a different spec), thus this spec should not (re)define the entryType attribute. That's not great, imo.

@marcoscaceres PerformanceMark inherits entryType from PerformanceEntry, and we're defining what PerformanceMark should return for entryType; this use is consistent with how we intended this to be defined and the pattern we've followed across all of our specs.

I don't see any issues here, but open to suggestions if there's a better way to do this. That said, given that this is a standard pattern we've been using, we don't need to block this update on this. If there's a better way to do this, let's take that as a meta issue.

philipwalton · 2018-02-22T00:38:11Z

@igrigorik thanks for the feedback. I believe I addressed all the issues, so could you PTAL again?

igrigorik

@philipwalton thanks for iterating on this, I think we're close! A few questions and comments.

igrigorik · 2018-02-22T03:44:42Z

index.html

+      <p>The <a>PerformanceMark</a> interface extends the following attributes of the <a data-cite="!PERFORMANCE-TIMELINE-2#performanceentry">PerformanceEntry</a>
+      interface:</p>
+      <p>The <code>name</code> attribute will return the mark's name.</p>
+      <p>The <code>entryType</code> attribute will return the <a data-cite="!WEBIDL#idl-DOMString"><code>DOMString</code></a> <code>"mark"</code>.</p>


@marcoscaceres PerformanceMark inherits entryType from PerformanceEntry, and we're defining what PerformanceMark should return for entryType; this use is consistent with how we intended this to be defined and the pattern we've followed across all of our specs.

I don't see any issues here, but open to suggestions if there's a better way to do this. That said, given that this is a standard pattern we've been using, we don't need to block this update on this. If there's a better way to do this, let's take that as a meta issue.

igrigorik · 2018-02-22T03:49:15Z

index.html

@@ -1,144 +1,99 @@
 <!DOCTYPE html>
 <html>
-  <head>


Viewing current snapshot, I see a respec validation error:

"Found linkless element for 'performance' with text 'ensure name is valid' but no matching ."

Does this algorithm still exist / is needed?

Oops, forgot to remove this line when updating the spec to fix #36.

igrigorik · 2018-02-22T03:57:48Z

index.html

+          <li>Let <var>end time</var> be <code>0</code>.</li>
+          <li>If <var>endMark</var> is omitted, let <var>end time</var> be the value that would be returned by the <code>Performance</code> object's <a data-cite="!HR-TIME-2#dom-performance-now"><code>now()</code></a> method. Otherwise:
+            <ol>
+              <li>If <var>endMark</var> has the same name as a <a data-cite="!WEBIDL#dfn-read-only">read only attribute</a> in the <code><a data-cite="!NAVIGATION-TIMING#performancetiming">PerformanceTiming</a></code> interface, let <var>end time</var> be the value returned by running the <a>convert a name to a timestamp</a> algorithm with <var>name</var> set to the value of <var>endMark</var>.</li>


Current logic will throw a SyntaxError inside a Worker if you use NavigationTiming attribute name. Sanity check: is this intentional?

Yes, this is what we decided in #22.

igrigorik · 2018-02-22T03:57:56Z

index.html

+          </li>
+          <li>If <var>startMark</var> is omitted, let <var>start time</var> be <code>0</code>. Otherwise:
+            <ol>
+              <li>If <var>startMark</var> has the same name as a <a data-cite="!WEBIDL#dfn-read-only">read only attribute</a> in the <code><a data-cite="!NAVIGATION-TIMING#performancetiming">PerformanceTiming</a></code> interface, let <var>start time</var> be the value return by running the <a>convert a name to a timestamp</a> algorithm with <var>name</var> set to <var>startMark</var>.</li>


Same as above.

I believe so. This matches current implementations and tests as mentioned in #21.

igrigorik · 2018-02-22T03:58:54Z

index.html

+      </pre>
+      <p>The <a>PerformanceMark</a> interface extends the following attributes of the <a data-cite="!PERFORMANCE-TIMELINE-2#performanceentry">PerformanceEntry</a>
+      interface:</p>
+      <p>The <code>name</code> attribute will return the mark's name.</p>


s/will/must .. same for steps below.

igrigorik

I think this looks good.

As a side note, in #36 we're looking for confirmation from Mozilla on the measure+SyntaxError behavior. Technically, this PR does not change existing per-spec behavior, so in theory it's OK to merge. That said, it'd be nice to get confirmation on #36 and land the corresponding WPT updates to lock all of this in.

@marcoscaceres @digitarald can you please review?

toddreifsteck · 2018-02-22T19:16:46Z

Merging the update per the 2/22 call. Will follow up with Mozilla regarding #36 in that issue.

philipwalton added 5 commits December 11, 2017 10:49

Fix spelling and grammar issues

f18cfc0

Clarify that measure may throw a SyntaxError

48055f5

Clarify that measureName cannot be a NT name

38662fe

Add NT name support, fix links and hierarchy

e8b158f

Handle using NT names in a worker

2abb490

philipwalton requested a review from igrigorik December 22, 2017 18:43

philipwalton mentioned this pull request Dec 22, 2017

Clarify that measure() may throw a SyntaxError #30

Closed

marcoscaceres reviewed Jan 16, 2018

View reviewed changes

igrigorik reviewed Jan 17, 2018

View reviewed changes

philipwalton mentioned this pull request Jan 18, 2018

Update WP tests to not throw when NT1 names are used as the first arg in measure calls #36

Closed

nolanlawson approved these changes Jan 20, 2018

View reviewed changes

philipwalton and others added 2 commits February 21, 2018 13:28

Address initial review feedback

dc7f5d2

Fix linking issues

39b516a

marcoscaceres reviewed Feb 21, 2018

View reviewed changes

philipwalton added 2 commits February 21, 2018 14:13

Rename property to attribute for consistency

510c089

Simply linking and add a note about NT2 support

ea8c53b

philipwalton mentioned this pull request Feb 22, 2018

Update the introductory text to explain how this API differs from performance.now() #37

Closed

Remove the NT-name restriction on measure

242a0ea

igrigorik reviewed Feb 22, 2018

View reviewed changes

Address review feedback

c70f34e

igrigorik approved these changes Feb 22, 2018

View reviewed changes

toddreifsteck merged commit 3cdc6df into gh-pages Feb 22, 2018

This was referenced Feb 22, 2018

What does "navigationStart" mean as a second argument to performance.measure() in a web worker? #22

Closed

measure() may throw InvalidAccessError #26

Closed

Clarify what Navigation Timing entries are allowed and how to use their values #31

Closed

yoavweiss deleted the l2-missing-errors branch February 3, 2021 14:23

Address outstanding L2 issues #35

Address outstanding L2 issues #35

Conversation

philipwalton commented Dec 22, 2017

marcoscaceres 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

philipwalton commented Jan 16, 2018

igrigorik 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

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

philipwalton commented Jan 25, 2018

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

philipwalton commented Feb 22, 2018

igrigorik 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

igrigorik left a comment

Choose a reason for hiding this comment

toddreifsteck commented Feb 22, 2018