Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20526007:
Create AJAX live-viewer of expected-vs-actual GM results (Closed)
|
Created:
7 years, 5 months ago by epoger Modified:
7 years, 4 months ago Reviewers:
Zach Reizner CC:
skia-review_googlegroups.com, bsalomon Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionCreate AJAX live-viewer of expected-vs-actual GM results
R=zachr@google.com
Committed: https://code.google.com/p/skia/source/detail?r=10520
Patch Set 1 #
Total comments: 18
Patch Set 2 : incorporate_comments #Patch Set 3 : change_tabs_to_spaces #Patch Set 4 : more_tabs_to_spaces #Patch Set 5 : simplify_javascript #Patch Set 6 : change_tabs_to_spaces #
Total comments: 8
Patch Set 7 : final_touches #
Messages
Total messages: 11 (0 generated)
This is the live, AJAX version of https://codereview.chromium.org/20151002/ ('add show_results tool: generate HTML with links to actual GM images from the bots')... whaddaya think? https://codereview.chromium.org/20526007/diff/1/gm/viewer/base-android-nexus-... File gm/viewer/base-android-nexus-10/Test-Android-Nexus10-MaliT604-Arm7-Release/base-android-nexus-10/actual-results.json (right): https://codereview.chromium.org/20526007/diff/1/gm/viewer/base-android-nexus-... gm/viewer/base-android-nexus-10/Test-Android-Nexus10-MaliT604-Arm7-Release/base-android-nexus-10/actual-results.json:2: "actual-results" : { This is just some canned data for testing... when view.html and module.js are installed within https://code.google.com/p/skia-autogen/source/browse/#svn%2Fgm-actual , they will refer to live data. https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js File gm/viewer/module.js (right): https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js#newcode1 gm/viewer/module.js:1: /* To check it out in action, go to https://x20web.corp.google.com/~epoger/viewer/view.html
On 2013/07/26 21:43:02, epoger wrote: > This is the live, AJAX version of https://codereview.chromium.org/20151002/ > ('add show_results tool: generate HTML with links to actual GM images from the > bots')... whaddaya think? > > https://codereview.chromium.org/20526007/diff/1/gm/viewer/base-android-nexus-... > File > gm/viewer/base-android-nexus-10/Test-Android-Nexus10-MaliT604-Arm7-Release/base-android-nexus-10/actual-results.json > (right): > > https://codereview.chromium.org/20526007/diff/1/gm/viewer/base-android-nexus-... > gm/viewer/base-android-nexus-10/Test-Android-Nexus10-MaliT604-Arm7-Release/base-android-nexus-10/actual-results.json:2: > "actual-results" : { > This is just some canned data for testing... when view.html and module.js are > installed within > https://code.google.com/p/skia-autogen/source/browse/#svn%252Fgm-actual , they > will refer to live data. > > https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js > File gm/viewer/module.js (right): > > https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js#newcode1 > gm/viewer/module.js:1: /* > To check it out in action, go to > https://x20web.corp.google.com/%7Eepoger/viewer/view.html I'm still reviewing the code, but from a UI standpoint, every time the user clicks an image, they have to go back and reuse the dropdown box. I suggest a div popup (not a new browser window) that causes no browser navigation and no additional pages to load.
https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js File gm/viewer/module.js (right): https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js#newcode24 gm/viewer/module.js:24: * 1. rawJsonData rawJsonData is not used in the front end. https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js#newcode45 gm/viewer/module.js:45: var thisResult = new Object(); Creating objects like this is clearer and more JavaScripty with object literals: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Values,_variabl... https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js#newcode47 gm/viewer/module.js:47: thisResult['testName'] = testName; testName is not used in the front end. Neither is hashType or hashValue. https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html File gm/viewer/view.html (right): https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode16 gm/viewer/view.html:16: <option selected="selected">base-fake/Fake-Platform-Made-Up/base-fake</option> Is the the plan to change this manually every time a platform is added/changed? https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode26 gm/viewer/view.html:26: <li ng-repeat="resultsOfOneType in actualResults"> You could use a loop over rawJsonData['actual-results'] here and a filter for extracting test names from image names and avoid most of the processing you do in JavaScript. Here's some code that may clarify how to do that <li ng-repeat="(resultType, resultsOfThisType) in rawResults['actual-results']"> <h3> {{ resultType }} ({{ resultsOfThisType.length }}) </h3> <table border="1"> <tr><th>Test/Config</th><th>Expected</th><th>Actual</th></tr> <tr ng-repeat="(imageName, result) in resultsOfThisType"> <td>{{ imageName }}</td> <td><a href="http://chromium-skia-gm.commondatastorage.googleapis.com/gm/{{ rawResults['expected-results'][imageName]['allowed-digests'][0][0] }}/{{ imageName|extractTestName }}/{{ rawResults['expected-results'][imageName]['allowed-digests'][0][1] }}.png"> {{ rawResults['expected-results'][imageName]['allowed-digests'][0][1] }} </a></td> <td><a href="http://chromium-skia-gm.commondatastorage.googleapis.com/gm/{{ result[0] }}/{{ imageName|extractTestName }}/{{ result[1] }}.png">{{ result[1] }}</a></td> </tr> </table> <p> </li> And then in your module.js: JsonDataModule.filter('extractTestName', function() { return function(input) { return input.replace(/_[^_]+\.png/, ""); }; });
Thanks for the advice, Zach... I followed half of it. :-) Please take a look. https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js File gm/viewer/module.js (right): https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js#newcode24 gm/viewer/module.js:24: * 1. rawJsonData On 2013/07/29 18:40:34, Zach Reizner wrote: > rawJsonData is not used in the front end. Correct. rawJsonData had been used by the frontend originally, but I ditched it in favor of the other forms. I figured it was cheap to provide, so I would leave it in place in case the frontend decides to use it later. Do you think that's reasonable, or do you think providing it comes at a nonnegligible cost? https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js#newcode45 gm/viewer/module.js:45: var thisResult = new Object(); On 2013/07/29 18:40:34, Zach Reizner wrote: > Creating objects like this is clearer and more JavaScripty with object literals: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Values%2C_varia... Cool, thanks for the tip! Done. https://codereview.chromium.org/20526007/diff/1/gm/viewer/module.js#newcode47 gm/viewer/module.js:47: thisResult['testName'] = testName; On 2013/07/29 18:40:34, Zach Reizner wrote: > testName is not used in the front end. Neither is hashType or hashValue. hashValue *is* used, but the others aren't... removed. https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html File gm/viewer/view.html (right): https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode16 gm/viewer/view.html:16: <option selected="selected">base-fake/Fake-Platform-Made-Up/base-fake</option> On 2013/07/29 18:40:34, Zach Reizner wrote: > Is the the plan to change this manually every time a platform is added/changed? My plan is to populate this manually when I first install the file in skia-autogen, and to soon make this automatic (somehow iterate over the directories in skia-autogen). https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode26 gm/viewer/view.html:26: <li ng-repeat="resultsOfOneType in actualResults"> On 2013/07/29 18:40:34, Zach Reizner wrote: > You could use a loop over rawJsonData['actual-results'] here and a filter for > extracting test names from image names and avoid most of the processing you do > in JavaScript. Here's some code that may clarify how to do that That is actually what I originally had in place, when the javascript just returned rawJsonResults. :-) One of the reasons that I moved away from that is that it resultsOfThisType.length doesn't work (IIRC). That's because resultsOfThisType is an Object, not an Array. My interim step that made that work was to write my own filter that transformed an Object (i.e. dictionary) into an array of the dictionary values, and then display the length of that array. That got me more into processing the JSON in the javascript... which led me to where the code is now. And I think it's clearer this way anyway (keeping the presentation logic simpler, and putting the processing in Javascript where it is easier to read anyway) Is there a compelling reason to put the processing logic within the HTML instead of the Javascript? (I'm an AJAX and AngularJS newbie, so I may be violating some Rule of the Universe here, I dunno) https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode35 gm/viewer/view.html:35: <td><a href="{{expectedResultsByImageName[result['imageName']]['url']}}"> Regarding your suggestion to use a div popup when the user clicks on this link: my thinking is that the most common use case will be to open expected and actual images as new tabs (by right-clicking on them), so that the user can toggle back and forth to better notice changes between the images. I see what you're saying, though, in that if the user left-clicks on the links, they will have to reuse the dropdown box if/when they return. Can you think of a good middle ground that handles both cases well?
https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html File gm/viewer/view.html (right): https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode16 gm/viewer/view.html:16: <option selected="selected">base-fake/Fake-Platform-Made-Up/base-fake</option> On 2013/07/29 22:38:39, epoger wrote: > On 2013/07/29 18:40:34, Zach Reizner wrote: > > Is the the plan to change this manually every time a platform is > added/changed? > > My plan is to populate this manually when I first install the file in > skia-autogen, and to soon make this automatic (somehow iterate over the > directories in skia-autogen). I understand https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode26 gm/viewer/view.html:26: <li ng-repeat="resultsOfOneType in actualResults"> On 2013/07/29 22:38:39, epoger wrote: > On 2013/07/29 18:40:34, Zach Reizner wrote: > > You could use a loop over rawJsonData['actual-results'] here and a filter for > > extracting test names from image names and avoid most of the processing you do > > in JavaScript. Here's some code that may clarify how to do that > > That is actually what I originally had in place, when the javascript just > returned rawJsonResults. :-) > > One of the reasons that I moved away from that is that it > resultsOfThisType.length doesn't work (IIRC). That's because resultsOfThisType > is an Object, not an Array. > > My interim step that made that work was to write my own filter that transformed > an Object (i.e. dictionary) into an array of the dictionary values, and then > display the length of that array. > > That got me more into processing the JSON in the javascript... which led me to > where the code is now. And I think it's clearer this way anyway (keeping the > presentation logic simpler, and putting the processing in Javascript where it is > easier to read anyway) > > Is there a compelling reason to put the processing logic within the HTML instead > of the Javascript? (I'm an AJAX and AngularJS newbie, so I may be violating > some Rule of the Universe here, I dunno) I see what you mean about too much logic inside the HTML. Halfway through writing that snippet, I broke a personal record on most subscripts in a single statement. In general, the AngularJS rule of the universe is to not fight the framework because you'll end up regretting it. In this situation, I think the most AngularJS friendly thing to do would be to process the rawJsonData in JavaScript to a point where the rendering logic is straightforward. That implies a JavaScript object like: { "failed": [ { "test":"bigmatrix", "config": "gpu", "expected_hash_value": "3711158016938030742", "actual_hash_value": "10894408024079689926", }, ... ], "no-comparison": [ { "test":"alphagradients", "config": "565", "actual_hash_value": "6493698615497174672", }, ... ], ... } This is enough to: - iterate the results types and get their names - get the number of results using the length of the array - print a row of test/config (Something like this "{{ result.test }}_{{ result.config }}.png"), expected hash link, and actual hash link - reconstruct the url for those links: http://chromium-skia-gm.commondatastorage.googleapis.com/gm/bitmap-64bitMD5/{{ result.test }}/{{ result.expected_hash_value }}.png https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode35 gm/viewer/view.html:35: <td><a href="{{expectedResultsByImageName[result['imageName']]['url']}}"> On 2013/07/29 22:38:39, epoger wrote: > Regarding your suggestion to use a div popup when the user clicks on this link: > my thinking is that the most common use case will be to open expected and actual > images as new tabs (by right-clicking on them), so that the user can toggle back > and forth to better notice changes between the images. > > I see what you're saying, though, in that if the user left-clicks on the links, > they will have to reuse the dropdown box if/when they return. > > Can you think of a good middle ground that handles both cases well? You can make the link always popup in a new tab. I always forget how, so here's a reference in case you forgot too: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-target
https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html File gm/viewer/view.html (right): https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode26 gm/viewer/view.html:26: <li ng-repeat="resultsOfOneType in actualResults"> On 2013/07/30 13:41:13, Zach Reizner wrote: > In general, the AngularJS rule of the universe is to not fight the framework > because you'll end up regretting it. In this situation, I think the most > AngularJS friendly thing to do would be to process the rawJsonData in JavaScript > to a point where the rendering logic is straightforward. I suppose "straightforward" is a sliding scale. :-) I'm not sure if it's cleaner to construct the URL in Javascript or the frontend, but I moved it into the frontend (following your example), it works fine, and seems fairly clean. Whaddaya think? (for easiest review, see diffs between patchset 4 and patchset 6) https://codereview.chromium.org/20526007/diff/1/gm/viewer/view.html#newcode35 gm/viewer/view.html:35: <td><a href="{{expectedResultsByImageName[result['imageName']]['url']}}"> On 2013/07/30 13:41:13, Zach Reizner wrote: > You can make the link always popup in a new tab. I always forget how, so here's > a reference in case you forgot too: > https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-target Seems reasonable. I added target="_blank", so it opens in a new tab (or window, depending on browser type and user settings!) by default, but the user can still right-click to determine where it opens.
LGTM except for small naming nits https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js File gm/viewer/module.js (right): https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js#newcode5 gm/viewer/module.js:5: var JsonDataModule = angular.module( The name of this module is very generic. Maybe something like GMViewer, or GMExpectationViewer. https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js#newco... gm/viewer/module.js:70: $scope.jsonResults = jsonResults; The name of this scope variable is very vague. How does expectionResults, or gmResults sound to you? https://codereview.chromium.org/20526007/diff/28001/gm/viewer/view.html File gm/viewer/view.html (right): https://codereview.chromium.org/20526007/diff/28001/gm/viewer/view.html#newco... gm/viewer/view.html:31: <table border="1"> It's good practice to put styles in a CSS document, but it may be overkill for internal sites.
Good suggestions, thanks. Please take one last look and tell me what you think. https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js File gm/viewer/module.js (right): https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js#newcode5 gm/viewer/module.js:5: var JsonDataModule = angular.module( On 2013/07/30 22:02:01, Zach Reizner wrote: > The name of this module is very generic. Maybe something like GMViewer, or > GMExpectationViewer. Changed to GMActualResultsLoader. You dig? https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js#newco... gm/viewer/module.js:70: $scope.jsonResults = jsonResults; On 2013/07/30 22:02:01, Zach Reizner wrote: > The name of this scope variable is very vague. How does expectionResults, or > gmResults sound to you? How does gmActualResults sound to YOU??? https://codereview.chromium.org/20526007/diff/28001/gm/viewer/view.html File gm/viewer/view.html (right): https://codereview.chromium.org/20526007/diff/28001/gm/viewer/view.html#newco... gm/viewer/view.html:31: <table border="1"> On 2013/07/30 22:02:01, Zach Reizner wrote: > It's good practice to put styles in a CSS document, but it may be overkill for > internal sites. Gotcha. As this gets more complicated, I will split out a CSS file.
SGTM https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js File gm/viewer/module.js (right): https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js#newcode5 gm/viewer/module.js:5: var JsonDataModule = angular.module( On 2013/08/02 14:59:27, epoger wrote: > On 2013/07/30 22:02:01, Zach Reizner wrote: > > The name of this module is very generic. Maybe something like GMViewer, or > > GMExpectationViewer. > > Changed to GMActualResultsLoader. You dig? Like Shia LeBeouf in Holes. https://codereview.chromium.org/20526007/diff/28001/gm/viewer/module.js#newco... gm/viewer/module.js:70: $scope.jsonResults = jsonResults; On 2013/08/02 14:59:27, epoger wrote: > On 2013/07/30 22:02:01, Zach Reizner wrote: > > The name of this scope variable is very vague. How does expectionResults, or > > gmResults sound to you? > > How does gmActualResults sound to YOU??? It sounds like sweet sweet music.
On 2013/08/02 15:17:57, Zach Reizner wrote: > > Changed to GMActualResultsLoader. You dig? > > Like Shia LeBeouf in Holes. I turn 40 in a few weeks, so get all your Shia LeBeouf references in NOW. (Of course, you're going back to school before then, so it's not really your concern)
Message was sent while issue was closed.
Committed patchset #7 manually as r10520 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
