[webkit-reviews] review denied: [Bug 60964] Allow sorting in RebaselineServer based on 'metric' field in unexpected_results.json : [Attachment 93776] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 27 11:54:49 PDT 2011
Mihai Parparita <mihaip at chromium.org> has denied Tom Hudson
<tomhudson at google.com>'s request for review:
Bug 60964: Allow sorting in RebaselineServer based on 'metric' field in
unexpected_results.json
https://bugs.webkit.org/show_bug.cgi?id=60964
Attachment 93776: Patch
https://bugs.webkit.org/attachment.cgi?id=93776&action=review
------- Additional Comments from Mihai Parparita <mihaip at chromium.org>
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.
More information about the webkit-reviews
mailing list