Specify console.timeLog() + clean up timing #138

domfarolino · 2018-05-25T05:13:22Z

This PR:

Adds timeLog to the Console Standard (and specifies that it should use Printer instead of Logger so we ignore format specifiers appearing in a timer label.
Cleans up the way we interact with associated timer tables in the timing methods - this is a little prep work for later when we check (non)existence of timer table entries so we can formally report warnings.
Points to an issue Timing and Counting methods should report warnings when necessary #134 to show plans for having the timing methods formally report warnings.
Fixes timeEnd to actually remove a timer from the timer table (the reason we went with adding timeLog)
Adds an example show how someone might use the extra ...data passed into timeLog to distinguish calls to timeLog from each other throughout the duration of a timer.
Fixes countReset's permalink to be all lowercase (that seems to be the trend here)
Closes Intermediate values with console.time #84

Interested in getting some opinions from @xirzec, @nchevobbe, and @JosephPecoraro on this to make sure we have consensus and can get this in ASAP! (will file browser bugs soon)

Bugs

Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1461001
Safari: https://bugs.webkit.org/show_bug.cgi?id=186833
Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=854474
Edge: https://twitter.com/domfarolino/status/1009303140556931073

fmartin5 · 2018-05-26T17:30:14Z

index.bs

+
+<p class="note">See <a href="https://github.com/whatwg/console/issues/134">whatwg/console#134</a>
+for plans to make {{console/timeEnd()}} and {{console/timeLog()}} formally report warnings to the
+console when a give |label| does not exist in the associated <a>timer table</a>.


A small typo here, the word "given" is misspelled. This looks good otherwise :)

domfarolino · 2018-06-06T08:08:35Z

index.bs

-   associated <a>timer table</a>.
+1. Let |timerTable| be the associated <a>timer table</a>.
+1. Let |startTime| be |timerTable|[|label|].
+1. [=map/set|Remove=] |timerTable|[|label|].


This should probably map/remove but I have to take a closer look

You should be able to just do [=map/Remove=]

domenic

LGTM with nits, fun stuff

domenic · 2018-06-06T17:27:13Z

index.bs

-   associated <a>timer table</a>.
+1. Let |timerTable| be the associated <a>timer table</a>.
+1. Let |startTime| be |timerTable|[|label|].
+1. [=map/set|Remove=] |timerTable|[|label|].


You should be able to just do [=map/Remove=]

domenic · 2018-06-06T17:28:15Z

index.bs

@@ -389,7 +423,7 @@ their output similarly, in four broad categories. This table summarizes these co
    <td>log</td>
    <td>
      {{console/log()}}, {{console/trace()}}, {{console/dir()}}, {{console/dirxml()}},
-      {{console/group()}}, {{console/groupCollapsed()}}, {{console/debug()}}
+      {{console/group()}}, {{console/groupCollapsed()}}, {{console/debug()}} {{console/timeLog()}}


Missing comma

domenic · 2018-06-06T17:28:58Z

index.bs

-1. Perform <a abstract-op>Logger</a>("timeEnd", « |concat| »).
+1. Perform <a abstract-op>Printer</a>("timeEnd", « |concat| »).
+
+<h4 id="timelog" method for="console">timeLog(|label|, ...|data|)</h4>


I wonder if putting "log" between time ("start") and "end" might make be nicer?

domfarolino · 2018-06-07T04:44:05Z

Rebased this as well. Will get moving on bugs + tests.

nchevobbe

Some small comments, but this looks good to me

nchevobbe · 2018-06-08T01:14:04Z

index.bs

+1. Let |startTime| be |timerTable|[|label|].
+1. Let |duration| be a string representing the difference between the current time and
+   |startTime|, in an implementation-defined format.
+1. Let |concat| be the concatenation of |label|, U+003A (:), U+0020 SPACE, and |duration|.


should we indicate what the duration represent (milliseconds, seconds, ...) ?
It seems that there is a bit more information about it in the timeEnd section.

I think what I'll do here is move the example in timeEnd up to here since the first instance of time represented in an implementation-defined format should probably do the explaining. Probably don't need to add it again in timeEnd since it's a repeat step and already will have an example here.

nchevobbe · 2018-06-08T01:15:16Z

index.bs

+
+  <pre><code class="lang-javascript">
+    console.time("MyTimer");
+    console.timeLog("MyTimer", "Starting application up...");


nit: Replace ... with the proper ellipsis char …

nchevobbe · 2018-06-08T01:16:29Z

index.bs

+    console.timeLog("MyTimer", "UI is setup, making API calls now");
+    // Perhaps some fetch()'s here filling the app with data
+    // ...
+    console.timeLog("MyTimer", "All done!");


Not sure if this case would make sense since timeEndalready prints the final timer duration.

xirzec · 2018-06-26T07:02:39Z

Also seems reasonable to me. /cc @TheLarkInn

(and apologies for the delay!)

Refs: whatwg/console#138

Refs: whatwg/console#138 PR-URL: #21312 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

domfarolino added the needs tests Moving the issue forward requires someone to write tests label May 25, 2018

fmartin5 reviewed May 26, 2018

View reviewed changes

domfarolino added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label May 27, 2018

domfarolino mentioned this pull request Jun 6, 2018

Intermediate values with console.time #84

Closed

domfarolino commented Jun 6, 2018

View reviewed changes

domfarolino requested a review from domenic June 6, 2018 16:56

domenic approved these changes Jun 6, 2018

View reviewed changes

domfarolino added 3 commits June 6, 2018 21:41

Specify console.timeLog() + clean up timing

d7aa947

Fix typo

1c7b668

Address latest review

c7745ce

domfarolino force-pushed the add-timelog-clean-up-timing branch from a60c63e to c7745ce Compare June 7, 2018 04:43

domfarolino mentioned this pull request Jun 7, 2018

Consolidate label conversion tests + add timeLog web-platform-tests/wpt#11401

Merged

nchevobbe approved these changes Jun 8, 2018

View reviewed changes

targos mentioned this pull request Jun 13, 2018

console: implement timeLog method nodejs/node#21312

Closed

4 tasks

Address review comments

7af927c

domfarolino mentioned this pull request Jun 20, 2018

Add console timing manual tests web-platform-tests/wpt#11584

Merged

domfarolino removed the needs tests Moving the issue forward requires someone to write tests label Jun 20, 2018

targos added a commit to targos/node that referenced this pull request Jun 30, 2018

console: implement timeLog method

32bae6b

Refs: whatwg/console#138

domfarolino merged commit 9690f7a into master Jul 4, 2018

domfarolino deleted the add-timelog-clean-up-timing branch July 4, 2018 05:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specify console.timeLog() + clean up timing #138

Specify console.timeLog() + clean up timing #138

domfarolino commented May 25, 2018 •

edited

Loading

fmartin5 May 26, 2018

domfarolino Jun 6, 2018

domenic Jun 6, 2018

domenic left a comment

domenic Jun 6, 2018

domenic Jun 6, 2018

domenic Jun 6, 2018

domfarolino commented Jun 7, 2018

nchevobbe left a comment

nchevobbe Jun 8, 2018

domfarolino Jun 20, 2018

nchevobbe Jun 8, 2018

nchevobbe Jun 8, 2018

xirzec commented Jun 26, 2018 •

edited

Loading

Specify console.timeLog() + clean up timing #138

Specify console.timeLog() + clean up timing #138

Conversation

domfarolino commented May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic 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

domfarolino commented Jun 7, 2018

nchevobbe 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

xirzec commented Jun 26, 2018 • edited Loading

domfarolino commented May 25, 2018 •

edited

Loading

xirzec commented Jun 26, 2018 •

edited

Loading