[Webkit-unassigned] [Bug 99588] Use forwarder2 in Chrome for Android layout tests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=99588


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #177733|review?                     |review-
               Flag|                            |




--- Comment #35 from Tony Chang <tony at chromium.org>  2012-12-05 13:30:02 PST ---
(From update of attachment 177733)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.