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

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from felipe <felipeg at chromium.org>  2012-10-25 03:39:39 PST ---
(From update of attachment 170423)
View in context: https://bugs.webkit.org/attachment.cgi?id=170423&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:42
>> +import webkitpy.thirdparty.autoinstalled.pexepct as pexpect
> 
> Why not 'from webkitpy.thirdparty.autoinstalled import pexpect' like the other import statements?

done.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:78
>> +FORWARD_PORTS = [8000, 8080, 8443, 8880, 9323]
> 
> Use a tuple instead of a list since this is a constant.

Done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:348
>> +class AndroidCommands(Object):
> 
> Object? I think you mean object.

Done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:353
>> +        self._original_governors = {}
> 
> Maybe _original_governors_file_contents?  It wasn't obvious to me what this dictionary is for.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:368
>> +        # Disable CPU scaling and drop ram cache to reduce noise in tests
> 
> Nit: Please end the sentence with a period.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:416
>> +        return result
> 
> Nit: Maybe return directly and remove the temp variable?

dpne

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:423
>> +      return self._port._path_to_device_forwarder()
> 
> indentation is not a multiple of four  [pep8/E111] [5]

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:426
>> +      return self._port._path_to_host_forwarder()
> 
> indentation is not a multiple of four  [pep8/E111] [5]

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:450
>> +                        device_port_for_host_port method.
> 
> Rather than passing in a list of tuples, can you make a small class for device port and host port and pass in a list of the class? E.g.,
> 
> class Ports(object):
>     def __init__(device_port, host_port):
>         self.device_port = device_port
>         self.host_port = host_port

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:458
>> +        self._host_to_device_port_map = dict()
> 
> Nit: Why dict() rather than {}?  You use {} above.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:473
>> +        timeout_sec = 5
> 
> This looks like a constant. Maybe name it TIMEOUT_SECONDS or KILL_TIMEOUT_SECONDS?

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:505
>> +            # Failure
> 
> I would remove this comment.  It seems obvious from the exception being raised.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:522
>> +                                           forward_string)
> 
> The wrapping makes this hard for me to read. Maybe put it on a single line (WebKit doesn't have an 80 col limit) or just do a 4 space indent rather than trying to align with the open paren.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:536
>> +                # Success
> 
> I would remove this comment. It seems obvious from the lack of exception being raised.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:542
>> +                # Failure
> 
> I would remove this comment.  It seems obvious from the exception being raised.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:563
>> +        wait_period = 0.1
> 
> Nit: elapsed_seconds and wait_period_seconds.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:577
>> +        wait_period = 0.1
> 
> Nit: elapsed_seconds and wait_period_seconds.

done

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:599
>> +        self._close_process()
> 
> Do we need both close and _close_process?  I would just have a single close_process.

done

>>> Tools/Scripts/webkitpy/thirdparty/__init__.py:96
>>> +                             "pexpect-2.3")
>> 
>> It looks like pexpect is MIT licensed: http://www.noah.org/wiki/Pexpect#License
>> That means we can just check in the files rather than using autoinstall.  It'll be much less flaky to check the files in.
> 
> I thought that we could only check code licensed with BSD or LGPL 2.1 in. Is this documented somewhere? It would indeed be easier if we can check the pexpect library in.

Ok,
How should I do that ?
Should I make a separate CL ?

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