Bug 60964 – Allow sorting in RebaselineServer based on 'metric' field in unexpected_results.json

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Tom Hudson]

Allow sorting in RebaselineServer based on 'metric' field in unexpected_results.json

[Tom Hudson]

Created attachment 93776 [details]
Patch

[Tom Hudson]

Comment on attachment 93776 [details]
Patch

Takes input from bug 60569 via bug 60957.

[Dirk Pranke]

I'm probably not the right person to review this, since my javascript-fu is pretty weak. However, it looks like this code will assume we're only ever writing one metric into the JSON file from a given run (and hence can only sort on one column).

Does it make more sense to modify this so that we can handle N different sortable metrics? I don't really know what you have in mind so if it's easier to just start with one metric, that's fine too.

[Dirk Pranke]

Oh, also cc'ing other people that might be interested and/or reviewers.

For what it's worth, you get much faster turnaround times on reviews in my experience if you explicitly CC people than you do by just marking a change for review and hoping someone will notice. I forget to do this all the time, myself, but I try to usually cc people.

[Tom Hudson]

On multiple metrics: Yes, that's the next large feature we want, and I've already got that code half-drafted. I uploaded this set of patches because we seemed to be stuck on the middle step (bug 60957), and it seems to me that the next set of patches will be incremental on top of this, but I didn't want to sink more time into them until we'd come to some agreement on 60957.

Given that Tony, Ojan, and Mihai are CC'd on some of the email threads, I should have thought to ask them to review, I was just (1) waiting for 60957 and (2) still not feeling like I understand the norms.

[Mihai Parparita]

Comment on attachment 93776 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/index.html:54
> +    <span id="toggle-sort" class="link">Sort Tests</span>

Call this "Sort tests by metric"?

> Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:49
> +var shouldSortImages = false;

There's image-specific about this code, this should be called "shouldSortTestsByMetric".

> Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:50
> +var wasSortingImages = false;

Do you actually need this variable? If you only check shouldSortTestsByMetric in the selectedTypeIsSortable branch, you don't need to clear it to false and restore it.

> Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:157
> +    $('toggle-sort').style.color = '';

Rather than manually setting the color style, can you add/remove a 'disabled' class and then control the disabled appearance from the CSS?

> Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:254
> +    var selectedTypeIsSortable = results.tests[sampleSelectedTest]['metric'];

"'metric' in results.tests[sampleSelectedTest]" is a better test (your current test will return false if the metric value is 0).

> Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:263
> +            testsByState[state].sort(function(a, b) {

Given that this modifies testsByState, does turning off sorting actually work? It seems like you'd need to re-sort alphabetically in that case.

[Tom Hudson]

(In reply to comment #6)
> (From update of attachment 93776 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93776&action=review
> 
> > Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/index.html:54
> > +    <span id="toggle-sort" class="link">Sort Tests</span>
> 
> Call this "Sort tests by metric"?

Sure, for now. When we have support for multiple metrics, I expect to replace this with a drop-down box to choose which metric; then it makes sense to me to have the label be "Sort tests by:" and a default value of "alphabetical order".

> > Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:50
> > +var wasSortingImages = false;
> 
> Do you actually need this variable? If you only check shouldSortTestsByMetric in the selectedTypeIsSortable branch, you don't need to clear it to false and restore it.

The intent was to have a bit of UI memory; if we change from looking at IMAGE to TEXT failure cases, and then back, we restore the sortedness/unsortedness from before.

I'll at least rename to wasSortingTestsByMetric to be consistent with shouldSortImages -> shouldSortTestsByMetric.

> > Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:157
> > +    $('toggle-sort').style.color = '';
> 
> Rather than manually setting the color style, can you add/remove a 'disabled' class and then control the disabled appearance from the CSS?

My Javascript-fu is presumably far weaker than Dirk's; sounds like I need to learn a little CSS as well. This was just extrapolation from selectTest()/displayImageResults()/...

> > Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:263
> > +            testsByState[state].sort(function(a, b) {
> 
> Given that this modifies testsByState, does turning off sorting actually work? It seems like you'd need to re-sort alphabetically in that case.

testsByState is being regenerated (in its default alphabetical order) every time selectDirectory() is called, and the $('toggle-sort').onclick calls selectDirectory() in part to make sure we end up alpha afterwards. Since that's a non-obvious reason I've added a comment to explain.

[Tom Hudson]

Created attachment 95465 [details]
Patch

[Mihai Parparita]

(In reply to comment #7)
> The intent was to have a bit of UI memory; if we change from looking at IMAGE to TEXT failure cases, and then back, we restore the sortedness/unsortedness from before.

Right, but if you only do the sorting (currently at lines 264 to 268) inside the selectedTypeIsSortableBranch (line 258), then I don't think you need to touch shouldSortTestsByMetric in enable/DisableSorting, hence you don't need wasSortingTestsByMetric.

[Ojan Vafai]

Comment on attachment 95465 [details]
Patch

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

Mihai, you're comment makes me think this should be R-'ed, no?

> Tools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:261
> +    if (selectedTypeIsSortable) {
> +        enableSorting();
> +    } else {
> +        disableSorting();
> +    }

Nit: WebKit style is to not use curly braces with single-line if/else statements.

[Tom Hudson]

(In reply to comment #9)
> (In reply to comment #7)
> > The intent was to have a bit of UI memory; if we change from looking at IMAGE to TEXT failure cases, and then back, we restore the sortedness/unsortedness from before.
> 
> Right, but if you only do the sorting (currently at lines 264 to 268) inside the selectedTypeIsSortableBranch (line 258), then I don't think you need to touch shouldSortTestsByMetric in enable/DisableSorting, hence you don't need wasSortingTestsByMetric.

Actually, the sorting currently checks selectedTypeIsSortable && shouldSortByMetric, so it doesn't even need to be moved, but I went ahead and moved it to condense two consecutive similar conditionals.

[Tom Hudson]

(In reply to comment #10)
> Nit: WebKit style is to not use curly braces with single-line if/else statements.

Of course, my next Skia code reviewer wrote "Nit: Skia style is to always use curly braces, even for single-line if/else statements."

[Tom Hudson]

Created attachment 96446 [details]
Patch

[WebKit Review Bot]

Comment on attachment 96446 [details]
Patch

Clearing flags on attachment: 96446

Committed r88381: <http://trac.webkit.org/changeset/88381>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.