[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





--- Comment #39 from Philippe Liard <pliard at chromium.org>  2012-12-17 07:45:07 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.

Indeed I didn't get that ScriptError was wrapping all the necessary information.

>> 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).

Done.

>> 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.

The error handler used with partial application does the trick indeed in a much nicer way.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:521
>> +        # 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.

We have been using it for quite a while in Chromium. I'm not very familiar with testing in Python/WebKit to be honest.
I think we will be notified early enough if it fails :)

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

Done.

>> 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.

Done.

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