[webkit-reviews] review granted: [Bug 49626] Rebaseline server: display test results : [Attachment 74062] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted Mihai Parparita
<mihaip at chromium.org>'s request for review:
Bug 49626: Rebaseline server: display test results
https://bugs.webkit.org/show_bug.cgi?id=49626

Attachment 74062: Patch
https://bugs.webkit.org/attachment.cgi?id=74062&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74062&action=review

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:48
> +	     return;

Nit: 4 space indent

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:53
> +	     case 'Left': event.preventDefault(); previousTest(); break;
> +	     case 'Right': event.preventDefault(); nextTest(); break;

Nit: 0 or 4 space indent

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:79
> +	     testsByFailureType[failureType] = [];
> +	     failureTypes.push(failureType);

Nit: 4 space indent

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:117
> +	   (test_name, test_extension) =
os.path.splitext(self.query['test'][0])

Nit: you don't need the ()s on the left side of the =.	Also, unused variables
are normally just called _. E.g.:
  test_name, _ = os.path.splitext(...

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:189
> +	   results_json_file = file(results_json_path)
> +	   results_json = simplejson.load(results_json_file)
> +	   results_json_file.close()

You could use 'with' here to close the file implicitly.