[Webkit-unassigned] [Bug 60964] Allow sorting in RebaselineServer based on 'metric' field in unexpected_results.json

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=60964


Mihai Parparita <mihaip at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #93776|review?                     |review-
               Flag|                            |




--- Comment #6 from Mihai Parparita <mihaip at chromium.org>  2011-05-27 11:54:50 PST ---
(From update of attachment 93776)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.