Hi Jason,
	Those last jslint bugs are fixed. I don't know how those got through my check.

Thanks for your thoughts on the perf tests. I agree we should get this landed since this sprints focus is the performance results and not the actuall perf test utils. I suspect when we have more tests we will have a better idea of what design is best suited.

Just a little more thoughts on the ids... the reason I thought a numberic id would be more clutter is that the timer name already functions as a unique id. This makes mapping the test to the results pretty easy and it's simple to understand what test in running in the debugger since the name is right there. With a numeric id there's an additional level of indirection where the id maps to the test name with the results. This makes debugging harder and now the code has to manage id's and names. I agree either way we need to store a variable, but since a test needs a name anyway to show up in the results the name feels like it makes the easiest id to use. 

Regarding not using fullpaths in the tests. We definitely need to be able to associated tests with runtime information. I had thought we may want a way for perf utils to note the current file or function automatically, but that was out of scope for this story. If the test didn't mark the current file itself I guess we would have to wrap the test in a context that knows it. Since I think we are still pretty earlier in our performance testing strategy I think what we have now works for our  basic needs.

Ty 

-----Original Message-----
From: Jason San Jose [mailto:reply@reply.github.com] 
Sent: Friday, May 11, 2012 9:42 AM
To: Ty Voliter
Subject: Re: [brackets] File Open Performance Tests (#841)

>       */
>      function markStart(name) {
> -        if (activeTests[name]) {
> -            console.log("Recursive tests with the same name are not supported.");
> -            return;
> +        if (enabled) {
> +            if (activeTests[name]) {
> +                console.log("Recursive tests with the same name are not supported.");
> +                return;
> +            }
> +            
> +            activeTests[name] = {
> +                startTime: brackets.app.getElapsedMilliseconds()
> +            };
> +
> +            return name;

I did a search for all markStarts and didn't see a case where the addMeasurement wasn't reachable in the same closure.

I see the argument for making this lightweight and having less clutter, but in the end you're still keeping a variable in scope to hold the generated name returned by markStart, so we haven't avoided the clutter issue you mentioned.

Eventually yes, I agree we'll have perf tests that won't be in the same closure and/or module and you'll need some global identifier.

The more I think about it, maybe we shouldn't uses vars like fullPath in our performance markers in the first place. Just thinking out loud, but maybe other uniquely identifying arg can be passed separately.

Anyhow, I won't hold up merging this pull request over this particular issue. There's a few other jslint issues I found though.

---
Reply to this email directly or view it on GitHub:
https://github.com/adobe/brackets/pull/841/files#r809460