[webkit-reviews] review denied: [Bug 52671] [chromium] [linux] if check-sys-deps fails, output the failure reason : [Attachment 79339] Patch

[bugzilla-daemon at webkit.org]

Mihai Parparita <mihaip at chromium.org> has denied Tony Chang
<tony at chromium.org>'s request for review:
Bug 52671: [chromium] [linux] if check-sys-deps fails, output the failure
reason
https://bugs.webkit.org/show_bug.cgi?id=52671

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

------- Additional Comments from Mihai Parparita <mihaip at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79339&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:127
> +	   def error_handler(script_error, error=local_error):

Is the error argument necessary (vs. referencing local_error inside directly)?
You never actually use it.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:197
> +	   class MockExecute:

Call this MockExecutive? Also, we already have
webkitpy.common.system.executive_mock.MockExecutive2 and
webkitpy.tool.mocktool.MockExecutive, albeit neither one does things with the
error_handler.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:215
> +	   needs_http = False

Not sure why this necessary (vs. using a named argument)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:220
> +	   class MockLog:

webkitpy.common.system.logtesting.LoggingTestCase should let you check that
things get logged without having to do your own MockLog class or
monkey-patching chromium._log.