[Webkit-unassigned] [Bug 99588] Use the new forwarder2

[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 #171871|review?                     |review+
               Flag|                            |




--- Comment #22 from Tony Chang <tony at chromium.org>  2012-11-08 16:09:35 PST ---
(From update of attachment 171871)
View in context: https://bugs.webkit.org/attachment.cgi?id=171871&action=review

Sorry, just got back from vacation.  Feel free to ask someone else for a review if I don't respond in a day.

Overall this seems fine, but I suspect the code will break if you don't add unit tests.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:228
>          result = super(ChromiumAndroidPort, self).check_build(needs_http)
>          result = self._check_file_exists(self._path_to_md5sum(), 'md5sum utility') and result
> -        result = self._check_file_exists(self._path_to_forwarder(), 'forwarder utility') and result
> +        result = self._check_file_exists(self._path_to_device_forwarder(), 'device_forwarder utility') and result
> +        result = self._check_file_exists(self._path_to_host_forwarder(), 'host_forwarder utility') and result

Nit: I would probably merge these lines.
result = (super(...)
    and self._check_file_exists(...)
    and self._check_file_exists(...)
    ...)

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:487
> +            failure_m = failure_re.match(line)

Nit: failure_m -> failure_match

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:491
> +            success_m = success_re.match(line)

Nit: success_m -> success_match

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:501
> +    def close(self):
> +        self._process.stop(0.0)

This is OK, but it would be cleaner to use __enter__ and __exit__ to start/stop the process and use the 'with' statement in the calling code.  Then you wouldn't need to manually remember to call close() everywhere.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:629
> +        output = self._android_cmd.run_host_command(['lsof', '-nPi:%d' % host_port], ignore_error=True, return_exit_code=False)

I guess this is OK, but it requires that lsof is installed.  I assume you verified this was the case on our bots?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:639
> +        self._android_cmd.run_host_command(['pkill', '-f', host_pattern])

Same comment about pkill and pgrep (procps package) as for lsof.  Maybe these are installed with the other android specific dependencies?

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