Bug 155026

Summary: Add ability to retrieve raw data from release harness
Product: WebKit Reporter: Jon Lee <jonlee>
Component: WebCore Misc.Assignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, rniwa, sabouhallawa, simon.fraser
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch simon.fraser: review+

Description Jon Lee 2016-03-04 11:11:00 PST
SSIA
Comment 1 Jon Lee 2016-03-04 11:12:26 PST
Created attachment 273018 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-03-04 12:23:19 PST
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.
Comment 3 Jon Lee 2016-03-06 18:59:47 PST
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.
Comment 4 Jon Lee 2016-03-06 19:00:48 PST
Created attachment 273158 [details]
Patch
Comment 5 Simon Fraser (smfr) 2016-03-07 18:08:28 PST
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?
Comment 6 Jon Lee 2016-03-07 19:39:49 PST
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.
Comment 7 Jon Lee 2016-03-07 19:45:41 PST
Committed r197723: <http://trac.webkit.org/changeset/197723>