[webkit-reviews] review denied: [Bug 42336] Add script to synchronize WebKit and Khronos WebGL tests : [Attachment 66732] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adrienne Walker <enne at google.com>'s
request for review:
Bug 42336: Add script to synchronize WebKit and Khronos WebGL tests
https://bugs.webkit.org/show_bug.cgi?id=42336

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=66732&action=prettypatch

Seems like your addition to layout_tests should be a class, not just a bunch of
free functions.  I'm also not sure why this belongs in layout_tests (then again
layout_tests is already a mess).

In general this looks fine.  Just needs some cleanup.

> WebKitTools/Scripts/update-webgl-conformance-tests:36
> +sys.path.append(os.path.join(webkitpy_directory, "layout_tests"))
No no no.  This is evil and wrong.  The new-run-webkit-tests hack like this is
also wrong.  NRWT was attempting to make it possible to run
run_chromium_webkit_tests.py directly from chromium w/o depending on python
2.5.  That hack should be removed these days, and not repeated here.

>
WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests.py:143

> +    if (len(args) == 0):
no parens.

>
WebKitTools/Scripts/webkitpy/layout_tests/update_webgl_conformance_tests_unitte
st.py:92
> +	       if (output):
> +		   output_text += construct_style(output)
> +	       else:
> +		   output_text += construct_style(input)
This seems like an overly verbose way of writing this ternary.