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
Jon Lee
2016-03-04 11:11:00 PST
Created attachment 273018 [details]
Patch
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 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. Created attachment 273158 [details]
Patch
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 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. Committed r197723: <http://trac.webkit.org/changeset/197723> |