Comment on attachment 272197[details]
Merged patch sans Tests changes
View in context: https://bugs.webkit.org/attachment.cgi?id=272197&action=review
Giving this a provisional r+ assuming Said will do an informal review. I'm not going to pretend I understood everything :)
> PerformanceTests/Animometer/resources/debug-runner/animometer.js:31
> + button.textContent = Strings.text.graph + "...";
Typographic nerds would prefer "…" :)
> PerformanceTests/Animometer/resources/debug-runner/animometer.js:500
> + if (dashboard.options["adjustment"] == "ramp") {
> + Headers.details[3].disabled = true;
> + } else {
Nit: Single line conditional
> PerformanceTests/Animometer/resources/debug-runner/graph.js:9
> + element.testResult = testResult;
> + element.testData = testData;
More of a general comment, but I wonder if we should prefix these custom attrs/properties with _
> PerformanceTests/Animometer/resources/debug-runner/graph.js:595
> + document.getElementById("complexity-regression-aggregate-raw").textContent = complexityRegression.complexity.toFixed(2) + ", ±" + complexityRegression.stdev.toFixed(2) + "ms";
I guess this is just a review tool bug - supposed to be the +/- sign?
> PerformanceTests/Animometer/resources/debug-runner/graph.js:603
> + "â",
Same here, but not sure what it is supposed to be.
> PerformanceTests/Animometer/resources/extensions.js:467
> + for (var i = 0; i < this._size; ++i) {
> + out += this._values[i];
> + if (i < this._size - 1)
> + out += ", ";
> + }
Can you use this._values.join(", ") here?
$ jsc
>>> [1, 2, 3, 4, 5].join(", ")
1, 2, 3, 4, 5
Comment on attachment 272197[details]
Merged patch sans Tests changes
View in context: https://bugs.webkit.org/attachment.cgi?id=272197&action=review>> PerformanceTests/Animometer/resources/debug-runner/animometer.js:31
>> + button.textContent = Strings.text.graph + "...";
>
> Typographic nerds would prefer "…" :)
Done.
>> PerformanceTests/Animometer/resources/debug-runner/animometer.js:500
>> + } else {
>
> Nit: Single line conditional
Done.
>> PerformanceTests/Animometer/resources/debug-runner/graph.js:9
>> + element.testData = testData;
>
> More of a general comment, but I wonder if we should prefix these custom attrs/properties with _
We could. Done.
>> PerformanceTests/Animometer/resources/debug-runner/graph.js:595
>> + document.getElementById("complexity-regression-aggregate-raw").textContent = complexityRegression.complexity.toFixed(2) + ", ±" + complexityRegression.stdev.toFixed(2) + "ms";
>
> I guess this is just a review tool bug - supposed to be the +/- sign?
Yes, it is.
>> PerformanceTests/Animometer/resources/debug-runner/graph.js:603
>> + "â",
>
> Same here, but not sure what it is supposed to be.
en dash.
>> PerformanceTests/Animometer/resources/extensions.js:467
>> + }
>
> Can you use this._values.join(", ") here?
>
> $ jsc
I didn't touch this code; I was just moving it, but yes we could do that.
2016-02-25 02:11 PST, Jon Lee
2016-02-25 02:11 PST, Jon Lee
2016-02-25 02:11 PST, Jon Lee
2016-02-25 02:12 PST, Jon Lee
2016-02-25 02:12 PST, Jon Lee
2016-02-25 02:12 PST, Jon Lee
2016-02-25 02:13 PST, Jon Lee
2016-02-25 02:13 PST, Jon Lee
2016-02-25 02:13 PST, Jon Lee
2016-02-25 02:14 PST, Jon Lee
2016-02-25 02:17 PST, Jon Lee
2016-02-25 02:18 PST, Jon Lee
2016-02-25 02:18 PST, Jon Lee
2016-02-25 02:18 PST, Jon Lee
2016-02-25 02:22 PST, Jon Lee
2016-02-25 02:25 PST, Jon Lee