Bug 155026 – Add ability to retrieve raw data from release harness

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Jon Lee]

SSIA

[Jon Lee]

Created attachment 273018 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 273018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273018&action=review

> PerformanceTests/Animometer/developer.html:98
> +                'j': Show JSON results<br/>

Why is the JSON overlay invoked through a shortcut and not by clicking a button?

> PerformanceTests/Animometer/resources/runner/animometer.js:401
> +            benchmarkController.getDebugInfo();

We do not check whether the JSON overlay is visible or not before creating a new one. So the JSON results overlay will be stacked on top of each other every time 'j' is clicked. And because the "Done" button is stacked also, if I click the 'j' key 30 times, I will have to click the "Done" button 30 times to go back to the results page.

> PerformanceTests/Animometer/resources/runner/animometer.js:434
> +        };

Can't we have the overlay elements inside a hidden section in the index.html instead of creating the elements dynamically?

> PerformanceTests/Animometer/resources/runner/animometer.js:436
> +        var button = Utilities.createElement("button", {}, container);

It is weird that the overlay is added by the keyboard but removed by the button "Done".  I would expect if I invoke the overlay by clicking a key, pressing the Esc key or clicking outside the overlay should make it disappear.

> PerformanceTests/Animometer/resources/runner/animometer.js:445
> +        target.selectRange = ((target.selectRange || 0) + 1) % 3;

Why do we start target.selectRange at 1? Shouldn't we do it like that:

target.selectRange = ((target.selectRange || -1) + 1) % 3;

> PerformanceTests/Animometer/resources/runner/animometer.js:452
> +                range.selectNode(document.getElementById("results-score"));

This change will not be needed if we fix the initial value of target.selectRange.

> PerformanceTests/Animometer/resources/runner/animometer.js:457
> +                range.setEndAfter(document.querySelector("#results-score > tr:last-of-type"), 0);

Ditto.

[Jon Lee]

Comment on attachment 273018 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273018&action=review

>> PerformanceTests/Animometer/developer.html:98
>> +                'j': Show JSON results<br/>
> 
> Why is the JSON overlay invoked through a shortcut and not by clicking a button?

Goes through the same path as the release harness.

>> PerformanceTests/Animometer/resources/runner/animometer.js:401
>> +            benchmarkController.getDebugInfo();
> 
> We do not check whether the JSON overlay is visible or not before creating a new one. So the JSON results overlay will be stacked on top of each other every time 'j' is clicked. And because the "Done" button is stacked also, if I click the 'j' key 30 times, I will have to click the "Done" button 30 times to go back to the results page.

Fixed.

>> PerformanceTests/Animometer/resources/runner/animometer.js:434
>> +        };
> 
> Can't we have the overlay elements inside a hidden section in the index.html instead of creating the elements dynamically?

We could. I didn't want to expose it outright in the markup.

>> PerformanceTests/Animometer/resources/runner/animometer.js:436
>> +        var button = Utilities.createElement("button", {}, container);
> 
> It is weird that the overlay is added by the keyboard but removed by the button "Done".  I would expect if I invoke the overlay by clicking a key, pressing the Esc key or clicking outside the overlay should make it disappear.

Added 'esc' key.

>> PerformanceTests/Animometer/resources/runner/animometer.js:445
>> +        target.selectRange = ((target.selectRange || 0) + 1) % 3;
> 
> Why do we start target.selectRange at 1? Shouldn't we do it like that:
> 
> target.selectRange = ((target.selectRange || -1) + 1) % 3;

No. It will always be 0.

>> PerformanceTests/Animometer/resources/runner/animometer.js:452
>> +                range.selectNode(document.getElementById("results-score"));
> 
> This change will not be needed if we fix the initial value of target.selectRange.

See above.

>> PerformanceTests/Animometer/resources/runner/animometer.js:457
>> +                range.setEndAfter(document.querySelector("#results-score > tr:last-of-type"), 0);
> 
> Ditto.

See above.

[Jon Lee]

Created attachment 273158 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 273158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273158&action=review

> PerformanceTests/Animometer/resources/runner/animometer.js:422
> +        if (!!document.getElementById("overlay"))

Is the !! required?

[Jon Lee]

Comment on attachment 273158 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273158&action=review

>> PerformanceTests/Animometer/resources/runner/animometer.js:422
>> +        if (!!document.getElementById("overlay"))
> 
> Is the !! required?

no. removed.

[Jon Lee]

Committed r197723: <http://trac.webkit.org/changeset/197723>