Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console.time() multiple times with the same label #103

Closed
domenic opened this issue Mar 27, 2017 · 5 comments · Fixed by #112
Closed

console.time() multiple times with the same label #103

domenic opened this issue Mar 27, 2017 · 5 comments · Fixed by #112
Assignees

Comments

@domenic
Copy link
Member

domenic commented Mar 27, 2017

The current spec seems to imply this will reset the timer, but at least in some of my tests, it does not seem to. We should get to the bottom of this, and write some (manual) web platform tests.

@domfarolino
Copy link
Member

Simple test: https://jsbin.com/jaduboh/edit?js,output

Chrome + Canary: Resets an already existing timer on calls to time()
Firefox: Does not reset an already existing timer on calls to time()
Safari + Tech Preview: Does not reset an already existing timer on calls to time() (and furthermore gives a little warning message indicating a timer with that label exists)
Edge: ?

I think the behavior of multiple calls to time() with the same label resetting a timer is a little non-obvious, though it is a convenient way to reset a timer if you know about it. What about proposing a console.timeReset(label)? This an option?

@domenic
Copy link
Member Author

domenic commented Mar 29, 2017

Edge does not reset and gives a little warning message.

I'd go for:

  • A "may" step about adding a warning message
  • Fix the spec so that it doesn't overwrite
  • File a bug on Chrome
  • Write WPTs
  • A potential new issue to discuss timeReset.

However I think there's another issue here which is whether timeEnd() removes the timer from the timer table, thus allowing a new timer to start. Test: https://jsbin.com/biyucoxuqa/1/edit?js,output

  • Firefox: removes
  • Edge: removes
  • Chrome: removes
  • Safari: ?

@domenic
Copy link
Member Author

domenic commented Mar 29, 2017

Another test about where timeEnd() removes: https://jsbin.com/jewoxec/edit?js,output

  • Firefox: removes; second timeEnd() call does nothing
  • Edge: removes; second timeEnd() call gives warning
  • Chrome: does not seem to remove
  • Safari: ?

@domfarolino
Copy link
Member

domfarolino commented Mar 31, 2017

Fix the spec so that it doesn't overwrite

I can wait for #105 to get merged and then add text in the time() section indicating that it should not overwrite if that's the idea, unless you just want me to add it to the ongoing PR. Also I'll file the necessary Chromium bug about resetting when it shouldn't, and open an issue about a possible timeReset()


However I think there's another issue here which is whether timeEnd() removes the timer from the timer table, thus allowing a new timer to start.

I might be misunderstanding the intention of your two tests, but in your first test case you indicate that chrome removes a timer from the table on timeEnd(), but then in the next indicate that it doesn't right?

Also, I believe this is tracked here #84 fwiw. In that issue it seems like we're either going to go with allowing multiple calls to timeEnd(), or adding something like an intermediate timeLog(). Personally I think the shortest path to interop would be to make the timeLog() since no other implementations support multiple calls to timeEnd() (the less intuitive way to log an intermediate value IMO), but I can try and push that in that issue as opposed to here if that sounds good.

edit: regardless to answer your question marks, Safari does remove timers from table on calls to timeEnd() and prints a warning if the timer is not in the table

@domenic
Copy link
Member Author

domenic commented Apr 3, 2017

I can...

Sounds like a great plan. A separate PR on top of #105 makes sense to me.

I might be misunderstanding the intention of your two tests, but in your first test case you indicate that chrome removes a timer from the table on timeEnd(), but then in the next indicate that it doesn't right?

Yes. Basically Chrome's behavior makes no sense to me. I can come up with weird models that explain it I guess, but it's not great.

Also, I believe this is tracked here #84 fwiw ... I can try and push that in that issue as opposed to here if that sounds good.

Sounds good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants