Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25555003:
More improvements to HTTP baseline viewer (for GM results) (Closed)
|
Created:
7 years, 2 months ago by epoger Modified:
7 years, 2 months ago CC:
skia-review_googlegroups.com, Jvsquare, robertphillips, senorblanco, jcgregorio, rmistry Base URL:
http://skia.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionMore improvements to HTTP baseline viewer (for GM results)
(SkipBuildbotRuns)
R=borenet@google.com, bsalomon@google.com
Committed: https://code.google.com/p/skia/source/detail?r=11581
Patch Set 1 #Patch Set 2 : delete_commented_out_HTML #
Total comments: 5
Patch Set 3 : various_changes #
Total comments: 11
Patch Set 4 : show_only_failed_by_default #
Total comments: 8
Patch Set 5 : todos_n_tabs #
Messages
Total messages: 14 (0 generated)
Ready for review at patchset 2. You can try it out at http://wpgntat-ubiq141.hot.corp.google.com:8888/ . Give it a minute to load... https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/resul... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/resul... gm/rebaseline_server/results.py:208: test_data.append(results_for_this_test) Now that we limit the number of results shown, it's actually reasonable for the client to download them all. Even the successes.
Comments welcome from anyone, of course (EXCEPT about my 1997-style webpage design). But I wanted to put one or two people primarily on the hook for reviewing it.
On 2013/10/01 21:17:57, epoger wrote: > Comments welcome from anyone, of course (EXCEPT about my 1997-style webpage > design). But I wanted to put one or two people primarily on the hook for > reviewing it. Looks pretty nice (for 1997 ;-)). Some comments: 1) It'd be nice if it was initially sorted by either builder or test. My inclination is to go with builder, but I could go either way. 2) Are there plans to add diff functionality, or at least an easy way to flip between images?
https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:38: ng:click="toggleHiddenResultType(resultType)"> Is there a reason this isn't ng-click, i.e. keep everything consistently "ng-"? https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:79: <p> <em ng-hide="categories">(Spinner GIF goes here) Loading Data... </em>
Plans to add next/prev page for case when the # results exceeds the max?
Thanks, all! http://wpgntat-ubiq141.hot.corp.google.com:8888/ has been updated to reflect patchset 3. If you have a few minutes to discuss any of this live sometime today, please grab me! https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:38: ng:click="toggleHiddenResultType(resultType)"> On 2013/10/02 13:24:05, jcgregorio wrote: > Is there a reason this isn't ng-click, i.e. keep everything consistently "ng-"? Done. https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... gm/rebaseline_server/static/view.html:79: <p> On 2013/10/02 13:24:05, jcgregorio wrote: > <em ng-hide="categories">(Spinner GIF goes here) Loading Data... </em> Thanks, did that above! https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/resu... File gm/rebaseline_server/results.py (right): https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/resu... gm/rebaseline_server/results.py:127: Results._EnsureIncludedInCategoryDict(category_dict, 'resultType', [ Added this so that the UI shows failures(0) if there are no failures. Before, it just didn't list failures as a possibility in that case. https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:48: config<br> In a coming CL, I will add the ability to filter by test name. Any advice on how we should do this? I could imagine: 1. A list of checkboxes like we have for 'config' already. It will be a long list, though, so I guess it'll have to be scrollable. 2. A dropdown list. But that will only allow results for a single test at a time. 3. A text field for partial matching. ("clip" would show all tests containing the substring "clip") https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:77: Update Results Any thoughts on the "Update Results" button needing to be hit to refresh the results (except when you click on a column header, which forces it immediately)? I'm torn... sometimes it's nice for mouse-clicks to be instantly reflected in the output; other times, it's nice to be able to click a few things before the updates are shown. https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:85: TODO(epoger): Add more columns, such as pixel diffs, notes/bugs, Made more TODOs visible in the UI, rather than hidden within the HTML. https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:93: Found {{filteredTestData.length}} matches, and displaying the first On 2013/10/02 15:23:14, bsalomon wrote: > Plans to add next/prev page for case when the # results exceeds the max? No. :-) My hope was that filtering operations would make going to the next page unnecessary in most cases, and in the cases where one page just isn't enough the escape valve is to show more results. That would allow me to skip the complexity of paging operations. Let's discuss in person... https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:110: <input type="radio" Better labeling for the "expected image" and "actual image" columns https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:126: <tr ng-repeat="result in limitedTestData"> On 2013/10/02 12:54:05, JimVV wrote: > 1) It'd be nice if it was initially sorted by either builder or test. My > inclination is to go with builder, but I could go either way. When the page is first loaded, it is initially sorted by test (at least for me)... look for the checked radiobutton in that column. Is that how it's behaving for you too? Do you think I should do something to make the default sort order more obvious? Maybe change the order of the columns, so that test is further to the left? Proposed order: resultType, test, builder, config, images Also, I could change it so that the sorting column is always leftmost... but I fear it would be MORE confusing for the columns to be rearranged when the data is re-sorted. > 2) Are there plans to add diff functionality, or at least an easy way to flip > between images? Yes. Added a TODO within the UI.
On 2013/10/02 16:05:59, epoger wrote: > Thanks, all! > > http://wpgntat-ubiq141.hot.corp.google.com:8888/ has been updated to reflect > patchset 3. > > If you have a few minutes to discuss any of this live sometime today, please > grab me! > > https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... > File gm/rebaseline_server/static/view.html (right): > > https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... > gm/rebaseline_server/static/view.html:38: > ng:click="toggleHiddenResultType(resultType)"> > On 2013/10/02 13:24:05, jcgregorio wrote: > > Is there a reason this isn't ng-click, i.e. keep everything consistently > "ng-"? > > Done. > > https://codereview.chromium.org/25555003/diff/3001/gm/rebaseline_server/stati... > gm/rebaseline_server/static/view.html:79: <p> > On 2013/10/02 13:24:05, jcgregorio wrote: > > <em ng-hide="categories">(Spinner GIF goes here) Loading Data... </em> > > Thanks, did that above! > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/resu... > File gm/rebaseline_server/results.py (right): > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/resu... > gm/rebaseline_server/results.py:127: > Results._EnsureIncludedInCategoryDict(category_dict, 'resultType', [ > Added this so that the UI shows failures(0) if there are no failures. Before, > it just didn't list failures as a possibility in that case. > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... > File gm/rebaseline_server/static/view.html (right): > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... > gm/rebaseline_server/static/view.html:48: config<br> > In a coming CL, I will add the ability to filter by test name. Any advice on > how we should do this? I could imagine: > > 1. A list of checkboxes like we have for 'config' already. It will be a long > list, though, so I guess it'll have to be scrollable. > > 2. A dropdown list. But that will only allow results for a single test at a > time. > > 3. A text field for partial matching. ("clip" would show all tests containing > the substring "clip") > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... > gm/rebaseline_server/static/view.html:77: Update Results > Any thoughts on the "Update Results" button needing to be hit to refresh the > results (except when you click on a column header, which forces it immediately)? > > I'm torn... sometimes it's nice for mouse-clicks to be instantly reflected in > the output; other times, it's nice to be able to click a few things before the > updates are shown. > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... > gm/rebaseline_server/static/view.html:85: TODO(epoger): Add more columns, such > as pixel diffs, notes/bugs, > Made more TODOs visible in the UI, rather than hidden within the HTML. > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... > gm/rebaseline_server/static/view.html:93: Found {{filteredTestData.length}} > matches, and displaying the first > On 2013/10/02 15:23:14, bsalomon wrote: > > Plans to add next/prev page for case when the # results exceeds the max? > > No. :-) > > My hope was that filtering operations would make going to the next page > unnecessary in most cases, and in the cases where one page just isn't enough the > escape valve is to show more results. That would allow me to skip the > complexity of paging operations. > > Let's discuss in person... > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... > gm/rebaseline_server/static/view.html:110: <input type="radio" > Better labeling for the "expected image" and "actual image" columns > > https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... > gm/rebaseline_server/static/view.html:126: <tr ng-repeat="result in > limitedTestData"> > On 2013/10/02 12:54:05, JimVV wrote: > > 1) It'd be nice if it was initially sorted by either builder or test. My > > inclination is to go with builder, but I could go either way. > > When the page is first loaded, it is initially sorted by test (at least for > me)... look for the checked radiobutton in that column. Is that how it's > behaving for you too? > > Do you think I should do something to make the default sort order more obvious? > Maybe change the order of the columns, so that test is further to the left? > Proposed order: resultType, test, builder, config, images > > Also, I could change it so that the sorting column is always leftmost... but I > fear it would be MORE confusing for the columns to be rearranged when the data > is re-sorted. > > > 2) Are there plans to add diff functionality, or at least an easy way to flip > > between images? > > Yes. Added a TODO within the UI. ui lgtm
Eric- let me know if you have any concerns (with either UI or coding). Otherwise, I'll commit later today. https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:48: config<br> On 2013/10/02 16:05:59, epoger wrote: > In a coming CL, I will add the ability to filter by test name. Any advice on > how we should do this? I could imagine: > > 1. A list of checkboxes like we have for 'config' already. It will be a long > list, though, so I guess it'll have to be scrollable. > > 2. A dropdown list. But that will only allow results for a single test at a > time. > > 3. A text field for partial matching. ("clip" would show all tests containing > the substring "clip") Live discussion with Brian: we will go with #3, for both test name and builder name. I have added a TODO below. https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:77: Update Results On 2013/10/02 16:05:59, epoger wrote: > Any thoughts on the "Update Results" button needing to be hit to refresh the > results (except when you click on a column header, which forces it immediately)? Live discussion: Brian is happy with the current functionality in this regard. https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:93: Found {{filteredTestData.length}} matches, and displaying the first On 2013/10/02 16:05:59, epoger wrote: > On 2013/10/02 15:23:14, bsalomon wrote: > > Plans to add next/prev page for case when the # results exceeds the max? > > No. :-) > > My hope was that filtering operations would make going to the next page > unnecessary in most cases, and in the cases where one page just isn't enough the > escape valve is to show more results. That would allow me to skip the > complexity of paging operations. > > Let's discuss in person... Live discussion with Brian: he is happy with the current functionality. https://codereview.chromium.org/25555003/diff/13001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:126: <tr ng-repeat="result in limitedTestData"> On 2013/10/02 16:05:59, epoger wrote: > On 2013/10/02 12:54:05, JimVV wrote: > > 1) It'd be nice if it was initially sorted by either builder or test. My > > inclination is to go with builder, but I could go either way. > > When the page is first loaded, it is initially sorted by test (at least for > me)... look for the checked radiobutton in that column. Is that how it's > behaving for you too? > > Do you think I should do something to make the default sort order more obvious? > Maybe change the order of the columns, so that test is further to the left? > Proposed order: resultType, test, builder, config, images Live discussion with Jim: he is happy with the current functionality. https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/loader.js (right): https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:42: 'failure-ignored', 'no-comparison', 'succeeded']; Brian requested that we only show "failed" results by default. Done.
Nice! I'm impressed by the responsiveness, even when I crank up the image size and max records. Code LGTM with one nit, a question/concern, and a UI request for the future. https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/loader.js (right): https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:45: $scope.updateResults(); Nit: tabs https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:91: $scope.filteredTestData, $scope.displayLimit); This creates a second array, right? Is it possible to do the filtering as we're displaying the items so that we don't have to store copies? I guess I'm thinking python generator-like behavior. If the data set is very large, this code could be slow. https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:145: </table> I noticed some funny behavior with the column widths: when there are records hidden behind "max records to display" the column widths will change depending on their contents. Maybe you can fix the column widths?
Addressed Eric's comments in patchset 5. https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/loader.js (right): https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:45: $scope.updateResults(); On 2013/10/02 18:30:30, borenet wrote: > Nit: tabs Fixed. I don't know how those started cropping up; I don't remember changing any settings in my editor. Maybe I have multiple personality disorder. Or maybe *I* do. https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:91: $scope.filteredTestData, $scope.displayLimit); On 2013/10/02 18:30:30, borenet wrote: > This creates a second array, right? Is it possible to do the filtering as we're > displaying the items so that we don't have to store copies? I guess I'm > thinking python generator-like behavior. If the data set is very large, this > code could be slow. Agreed, but I think AngularJS forces us to do it this way. Added a TODO to investigate. https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/view.html (right): https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... gm/rebaseline_server/static/view.html:145: </table> On 2013/10/02 18:30:30, borenet wrote: > I noticed some funny behavior with the column widths: when there are records > hidden behind "max records to display" the column widths will change depending > on their contents. Maybe you can fix the column widths? Right, that would make for a nicer experience when re-sorting. Added a TODO.
Message was sent while issue was closed.
Committed patchset #5 manually as r11581 (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... File gm/rebaseline_server/static/loader.js (right): https://codereview.chromium.org/25555003/diff/20001/gm/rebaseline_server/stat... gm/rebaseline_server/static/loader.js:45: $scope.updateResults(); On 2013/10/02 18:57:14, epoger wrote: > On 2013/10/02 18:30:30, borenet wrote: > > Nit: tabs > > Fixed. I don't know how those started cropping up; I don't remember changing > any settings in my editor. Maybe I have multiple personality disorder. Or > maybe *I* do. Please remove the personality who thinks tabs are a good idea.
Message was sent while issue was closed.
On 2013/10/02 19:07:35, borenet wrote: > Please remove the personality who thinks tabs are a good idea. Done [CARRIER LOST]
Message was sent while issue was closed.
On 2013/10/02 19:18:49, epoger wrote: > On 2013/10/02 19:07:35, borenet wrote: > > Please remove the personality who thinks tabs are a good idea. > > Done [CARRIER LOST] P.S. That's a modem joke. I'm old. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
