Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10885005:
Upstream chrome_test_server_spawner.py for Android. (Closed)
|
Created:
8 years, 3 months ago by Philippe Modified:
8 years, 3 months ago CC:
chromium-reviews, peter+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org, Wei James(wistoch), Shouqun Liu Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpstream chrome_test_server_spawner.py for Android.
BUG=142571
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154429
Patch Set 1 #
Total comments: 61
Patch Set 2 : Address Marcus' comments #
Total comments: 4
Patch Set 3 : Address Marcus' comments #
Messages
Total messages: 16 (0 generated)
The rooted device issue is being addressed in parallel.
lgtm as it's the same as downstream :) but just adding FYI rsleevi: I think you already reviewed this right?
On 2012/08/28 17:43:00, bulach wrote: > lgtm as it's the same as downstream :) > > but just adding FYI rsleevi: I think you already reviewed this right? No, I never reviewed. However, please make sure someone with Python+Chromium readability is happy with this as well. This looks to be a lot of Python code to be dropping in.
looks fine, deferring review to others
ahn, sorry ryan, I thought you had reviewed this before :) I have python readability and will send some minor style comments in a bit, but it'd be nice to have someone looking into the functionality.. (I mean, this is already running fine downstream, but just in case..).
lgtm, but a few style nits below: http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... File build/android/pylib/chrome_test_server_spawner.py (right): http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:13: import json nit: before logging http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:26: nit: need another \n http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:35: 'http': '', nit: 35-39 needs indent by 4 http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:41: nit: need another \n http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:46: def CheckPortStatus(port, expected_status): nit: it seems that this function, and most of the methods in the classes below, are not "public".. perhaps we could add a "_" prefix to them? http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:66: command-line argument by following the option definitions in testserver.py. nit: need "Args:" and "Returns:".. also, it'd be nice to consolidate in a one line docstring, something like: """Returns the command-line argument for testserver.py. Args: server_type_string: the server type to be used. Returns: A string containing the command-line argument. """ http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:69: raise NotImplementedError('Unknown server type: %s' % server_type) nit: server_type is undefined, needs to be "server_type_string".. tbh, I think we can remove the "_string" suffix altogether.. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:77: """The class to run a test server in a separate process in a thread. nit: needs to be one line, probably something like: """A thread to run the test server in a separate process.""" http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:104: self.forwarder_device_port = 0; nit: remove ; http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:112: """Waits for the Python test server to start and get port it is using. The nit: needs a one line docstring. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:116: Return: nit: Returns: http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:120: (in_fds, out_fds, err_fds) = select.select([self.pipe_in,],[],[], nit: out_fds and err_fds seem unused, maybe replace with _ and __ ? also, spaces after the "," in the "select" arguments.. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:133: (data_length, ) = struct.unpack('=L', data_length) nit: no space after , http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:142: logging.info('Got port json data: %s' % port_json) nit: logging can do the formatting itself, so use "," rather than "%" to let it format. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:151: """Generate the command lines based on the input arguments to run nit: one line docstring http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:232: if self.process.poll() == None: nit: "is None" http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:247: """Blocks until the loop has finished. This must be called in another nit: one line http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:257: """A handler used to process http GET/POST request. nit: one line http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:263: to client. nit: one line http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:283: def StartTestServer(self): nit: needs docstring http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:313: logging.info('Test server is running on port: %d.' % nit: as above, s/%/,/ http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:321: def KillTestServer(self): nit: need docstring http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:332: logging.info('Test server on port %d is killed' % port) nit: s/%/,/ http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:341: logging.info('Action for POST method is: %s.' % action) nit: s/%/,/ http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:346: logging.info('Encounter unknown request: %s.' % action) nit: s/%/,/ http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:352: logging.info('Action for GET method is: %s.' % action) nit: s/%/,/ http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:354: logging.info('%s=%s' % (param, params[param][0])) nit: s/%/,/ http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:355: expect_exist = False nit: unused.. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:366: logging.info('Encounter unknown request: %s.' % action) nit: s/%/,/ http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:370: """The class used to start/stop a http server. nit: one line
On 2012/08/29 09:22:54, bulach wrote: > lgtm, but a few style nits below: > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > File build/android/pylib/chrome_test_server_spawner.py (right): > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:13: import json > nit: before logging > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:26: > nit: need another \n > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:35: 'http': '', > nit: 35-39 needs indent by 4 > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:41: > nit: need another \n > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:46: def CheckPortStatus(port, > expected_status): > nit: it seems that this function, and most of the methods in the classes below, > are not "public".. perhaps we could add a "_" prefix to them? > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:66: command-line argument by > following the option definitions in testserver.py. > nit: need "Args:" and "Returns:".. > also, it'd be nice to consolidate in a one line docstring, something like: > > """Returns the command-line argument for testserver.py. > > Args: > server_type_string: the server type to be used. > Returns: > A string containing the command-line argument. > """ > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:69: raise > NotImplementedError('Unknown server type: %s' % server_type) > nit: server_type is undefined, needs to be "server_type_string".. tbh, I think > we can remove the "_string" suffix altogether.. > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:77: """The class to run a test > server in a separate process in a thread. > nit: needs to be one line, probably something like: > """A thread to run the test server in a separate process.""" > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:104: > self.forwarder_device_port = 0; > nit: remove ; > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:112: """Waits for the Python > test server to start and get port it is using. The > nit: needs a one line docstring. > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:116: Return: > nit: Returns: > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:120: (in_fds, out_fds, > err_fds) = select.select([self.pipe_in,],[],[], > nit: out_fds and err_fds seem unused, maybe replace with _ and __ ? > also, spaces after the "," in the "select" arguments.. > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:133: (data_length, ) = > struct.unpack('=L', data_length) > nit: no space after , > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:142: logging.info('Got port > json data: %s' % port_json) > nit: logging can do the formatting itself, so use "," rather than "%" to let it > format. > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:151: """Generate the command > lines based on the input arguments to run > nit: one line docstring > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:232: if self.process.poll() == > None: > nit: "is None" > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:247: """Blocks until the loop > has finished. This must be called in another > nit: one line > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:257: """A handler used to > process http GET/POST request. > nit: one line > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:263: to client. > nit: one line > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:283: def > StartTestServer(self): > nit: needs docstring > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:313: logging.info('Test server > is running on port: %d.' % > nit: as above, s/%/,/ > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:321: def KillTestServer(self): > nit: need docstring > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:332: logging.info('Test server > on port %d is killed' % port) > nit: s/%/,/ > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:341: logging.info('Action for > POST method is: %s.' % action) > nit: s/%/,/ > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:346: logging.info('Encounter > unknown request: %s.' % action) > nit: s/%/,/ > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:352: logging.info('Action for > GET method is: %s.' % action) > nit: s/%/,/ > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:354: logging.info('%s=%s' % > (param, params[param][0])) > nit: s/%/,/ > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:355: expect_exist = False > nit: unused.. > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:366: logging.info('Encounter > unknown request: %s.' % action) > nit: s/%/,/ > > http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... > build/android/pylib/chrome_test_server_spawner.py:370: """The class used to > start/stop a http server. > nit: one line Thanks Marcus! I'm addressing your comments.
http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... File build/android/pylib/chrome_test_server_spawner.py (right): http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:13: import json On 2012/08/29 09:22:54, bulach wrote: > nit: before logging Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:26: On 2012/08/29 09:22:54, bulach wrote: > nit: need another \n Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:33: Do we need another \n here? I'm adding it to be consistent with your comment line 41. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:35: 'http': '', On 2012/08/29 09:22:54, bulach wrote: > nit: 35-39 needs indent by 4 Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:41: On 2012/08/29 09:22:54, bulach wrote: > nit: need another \n Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:46: def CheckPortStatus(port, expected_status): On 2012/08/29 09:22:54, bulach wrote: > nit: it seems that this function, and most of the methods in the classes below, > are not "public".. perhaps we could add a "_" prefix to them? Sounds good. Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:66: command-line argument by following the option definitions in testserver.py. On 2012/08/29 09:22:54, bulach wrote: > nit: need "Args:" and "Returns:".. > also, it'd be nice to consolidate in a one line docstring, something like: > > """Returns the command-line argument for testserver.py. > > Args: > server_type_string: the server type to be used. > Returns: > A string containing the command-line argument. > """ Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:69: raise NotImplementedError('Unknown server type: %s' % server_type) On 2012/08/29 09:22:54, bulach wrote: > nit: server_type is undefined, needs to be "server_type_string".. tbh, I think > we can remove the "_string" suffix altogether.. Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:77: """The class to run a test server in a separate process in a thread. On 2012/08/29 09:22:54, bulach wrote: > nit: needs to be one line, probably something like: > """A thread to run the test server in a separate process.""" Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:104: self.forwarder_device_port = 0; On 2012/08/29 09:22:54, bulach wrote: > nit: remove ; Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:112: """Waits for the Python test server to start and get port it is using. The On 2012/08/29 09:22:54, bulach wrote: > nit: needs a one line docstring. Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:116: Return: On 2012/08/29 09:22:54, bulach wrote: > nit: Returns: Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:120: (in_fds, out_fds, err_fds) = select.select([self.pipe_in,],[],[], On 2012/08/29 09:22:54, bulach wrote: > nit: out_fds and err_fds seem unused, maybe replace with _ and __ ? > also, spaces after the "," in the "select" arguments.. Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:133: (data_length, ) = struct.unpack('=L', data_length) On 2012/08/29 09:22:54, bulach wrote: > nit: no space after , Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:142: logging.info('Got port json data: %s' % port_json) On 2012/08/29 09:22:54, bulach wrote: > nit: logging can do the formatting itself, so use "," rather than "%" to let it > format. Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:151: """Generate the command lines based on the input arguments to run On 2012/08/29 09:22:54, bulach wrote: > nit: one line docstring Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:232: if self.process.poll() == None: On 2012/08/29 09:22:54, bulach wrote: > nit: "is None" Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:247: """Blocks until the loop has finished. This must be called in another On 2012/08/29 09:22:54, bulach wrote: > nit: one line Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:257: """A handler used to process http GET/POST request. On 2012/08/29 09:22:54, bulach wrote: > nit: one line Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:263: to client. On 2012/08/29 09:22:54, bulach wrote: > nit: one line Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:283: def StartTestServer(self): On 2012/08/29 09:22:54, bulach wrote: > nit: needs docstring Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:313: logging.info('Test server is running on port: %d.' % On 2012/08/29 09:22:54, bulach wrote: > nit: as above, s/%/,/ Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:321: def KillTestServer(self): On 2012/08/29 09:22:54, bulach wrote: > nit: need docstring Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:332: logging.info('Test server on port %d is killed' % port) On 2012/08/29 09:22:54, bulach wrote: > nit: s/%/,/ Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:341: logging.info('Action for POST method is: %s.' % action) On 2012/08/29 09:22:54, bulach wrote: > nit: s/%/,/ Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:346: logging.info('Encounter unknown request: %s.' % action) On 2012/08/29 09:22:54, bulach wrote: > nit: s/%/,/ Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:352: logging.info('Action for GET method is: %s.' % action) On 2012/08/29 09:22:54, bulach wrote: > nit: s/%/,/ Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:354: logging.info('%s=%s' % (param, params[param][0])) On 2012/08/29 09:22:54, bulach wrote: > nit: s/%/,/ Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:355: expect_exist = False On 2012/08/29 09:22:54, bulach wrote: > nit: unused.. Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:366: logging.info('Encounter unknown request: %s.' % action) On 2012/08/29 09:22:54, bulach wrote: > nit: s/%/,/ Done. http://codereview.chromium.org/10885005/diff/1/build/android/pylib/chrome_tes... build/android/pylib/chrome_test_server_spawner.py:370: """The class used to start/stop a http server. On 2012/08/29 09:22:54, bulach wrote: > nit: one line Done.
lgtm, just real tiny nits: http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... File build/android/pylib/chrome_test_server_spawner.py (right): http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... build/android/pylib/chrome_test_server_spawner.py:18: import sys nit: looks like sys is unused now.. http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... build/android/pylib/chrome_test_server_spawner.py:127: (in_fds, _, _) = select.select([self.pipe_in,], [], [], nit: s/,]/, ]/
Thanks again Marcus! Do we still need someone else's approval? The logic looks reasonably sane too me. http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... File build/android/pylib/chrome_test_server_spawner.py (right): http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... build/android/pylib/chrome_test_server_spawner.py:18: import sys On 2012/08/29 15:19:43, bulach wrote: > nit: looks like sys is unused now.. Done. http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... build/android/pylib/chrome_test_server_spawner.py:127: (in_fds, _, _) = select.select([self.pipe_in,], [], [], On 2012/08/29 15:19:43, bulach wrote: > nit: s/,]/, ]/ Done.
On 2012/08/30 11:44:45, Philippe wrote: > Thanks again Marcus! Do we still need someone else's approval? The logic looks > reasonably sane too me. > > http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... > File build/android/pylib/chrome_test_server_spawner.py (right): > > http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... > build/android/pylib/chrome_test_server_spawner.py:18: import sys > On 2012/08/29 15:19:43, bulach wrote: > > nit: looks like sys is unused now.. > > Done. > > http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... > build/android/pylib/chrome_test_server_spawner.py:127: (in_fds, _, _) = > select.select([self.pipe_in,], [], [], > On 2012/08/29 15:19:43, bulach wrote: > > nit: s/,]/, ]/ > > Done. I wrote some of the original changes and reviewed jnd's many functional changes over time downstream. Since the functionally matches downstream then I think it's fine to land.
On 2012/08/30 18:20:06, Yaron wrote: > On 2012/08/30 11:44:45, Philippe wrote: > > Thanks again Marcus! Do we still need someone else's approval? The logic looks > > reasonably sane too me. > > > > > http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... > > File build/android/pylib/chrome_test_server_spawner.py (right): > > > > > http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... > > build/android/pylib/chrome_test_server_spawner.py:18: import sys > > On 2012/08/29 15:19:43, bulach wrote: > > > nit: looks like sys is unused now.. > > > > Done. > > > > > http://codereview.chromium.org/10885005/diff/13001/build/android/pylib/chrome... > > build/android/pylib/chrome_test_server_spawner.py:127: (in_fds, _, _) = > > select.select([self.pipe_in,], [], [], > > On 2012/08/29 15:19:43, bulach wrote: > > > nit: s/,]/, ]/ > > > > Done. > > I wrote some of the original changes and reviewed jnd's many functional changes > over time downstream. Since the functionally matches downstream then I think > it's fine to land. Thanks Yaron.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10885005/14001
Change committed as 154429
On 2012/08/31 14:42:50, I haz the power (commit-bot) wrote: > Change committed as 154429 Sorry for missing this thread. Thank Philippe for landing this upstream!
On 2012/09/04 10:28:50, Johnny(Jianning) Ding wrote: > On 2012/08/31 14:42:50, I haz the power (commit-bot) wrote: > > Change committed as 154429 > > Sorry for missing this thread. Thank Philippe for landing this upstream! You're welcome Johnny! I haven't done much apart from copying the file we had downstream and addressing Marcus' comments. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
