Specify console.timeLog() + clean up timing by domfarolino · Pull Request #138 · whatwg/console

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

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

-   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

-   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

    <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

-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

+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

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

+    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 <cjihrig@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com>

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

Conversation

domfarolino commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domenic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

domfarolino commented Jun 7, 2018

Uh oh!

nchevobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xirzec commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

domfarolino commented May 25, 2018 •

edited

Loading

xirzec commented Jun 26, 2018 •

edited

Loading