[webkit-reviews] review granted: [Bug 49991] Rebaseline server: list current baselines and platforms : [Attachment 74695] 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 49991: Rebaseline server: list current baselines and platforms
https://bugs.webkit.org/show_bug.cgi?id=49991

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

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

> WebKitTools/Scripts/webkitpy/common/system/filesystem_mock.py:65
> +	   return '/'.join(trimmed_comps)

I guess unittests hard code the expectation of '/' instead of using os.sep?

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:93
> +	   platforms.platforms.forEach(function(platform) {
> +	     var platformOption = document.createElement('option');
> +	     platformOption.value = platform;

Nit: 4 space indent

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:202
> +    for baseline_extension in ['.txt', '.checksum', '.png']:

Nit: () instead of []

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:65
> +		   'base': ['.txt'],
> +		   'mac': ['.checksum', '.png'],
> +		   'win': ['.checksum', '.png'],

() is normally preferred over [] (it's a tiny bit faster and enforces
const-ness), but for single item lists, it's a bit more error prone since you
need to remember the trailing comma.  Anyway, either way is fine here.