[webkit-reviews] review denied: [Bug 99588] Use forwarder2 in Chrome for Android layout tests. : [Attachment 177733] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Philippe Liard
<pliard at chromium.org>'s request for review:
Bug 99588: Use forwarder2 in Chrome for Android layout tests.
https://bugs.webkit.org/show_bug.cgi?id=99588

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

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


> Tools/Scripts/webkitpy/common/system/executive.py:388
> +		       return_exit_code_and_output=False,

Is this necessary?  Would it be sufficient to include an error_handler?  The
error_handler gets both the exit_code and the output and will always be called
before |run_command| returns the output.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:33
> +import sets

Nit: This appears to be unused (and should be unnecessary because of the
built-in "set" type).

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
> +	   (exit_code, output) =
Forwarder._run_shell_command_and_get_exit_code(

Nit: No () on the left side, although if you use an error handler, I don't
think you need to check the return value at all.  Same with the other calls to
_run_adb_shell_command_and_get_exit_code below.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:521
> +    @staticmethod
> +    def _run_adb_shell_command_and_get_exit_code(command_executor,
android_cmd, cmd_array):
> +	   # Note that the extra '*' character below is used to handle the case
where the process' output is not newline terminated.

Can we add some unit tests for this function?  It looks pretty fragile.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:528
> +	   if status_separator_pos == 0:

Nit: if not status_separator_pos:

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:563
> +	   self._android_cmd = AndroidCommands(

Nit: android_cmd -> android_commands. We don't normally use abbreviations for
variable names in WebKit.