Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1748173002:
New API for persistent ADB shell (Closed)
|
Created:
4 years, 9 months ago by alexandermont Modified:
4 years, 8 months ago Reviewers:
Zhen Wang, Chris Craik, jbudorick, aschulman, nednguyen, rnephew (Reviews Here), rnephew (Wrong account) CC:
catapult-reviews_chromium.org, charliea (OOO until 10-5) Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionNew API for persistent ADB shell
Implement a "persistent ADB shell", which allows for setting up an ADB shell in a separate process and passing commands to it. This is necessary to allow ADB communication with less overhead than creating a new ADB shell for each command entered.
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/87ef16864757739c019c49c2cc1a0a27e28fdc31
Patch Set 1 #Patch Set 2 : remove extra import #Patch Set 3 : add split_lines parameter to RunShellCommand #
Total comments: 11
Patch Set 4 : code review changes #
Total comments: 14
Patch Set 5 : #
Total comments: 9
Patch Set 6 : changes from rnephew code review #Patch Set 7 : add test case and fix filtering #
Total comments: 17
Patch Set 8 : changes from code review #Patch Set 9 : refactor RunCommand #
Total comments: 5
Patch Set 10 : more changes from code review #
Total comments: 4
Patch Set 11 : fix nits #
Messages
Total messages: 37 (12 generated)
alexandermont@chromium.org changed reviewers: + jbudorick@chromium.org, rnephew@google.com
Here is the new API for the persistent ADB shell. Note that the file pshell_test shows an example of using this API. If you run it while at least one device is connected via 'adb devices' it should send some commands and output them.
Add split_lines function to RunShellCommand
https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/dev... devil/devil/android/device_utils.py:716: timeout=None, retries=None, split_lines=True): I do think this is the best of several options that I'm not crazy about. Where will this be called from, though? https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/psh... File devil/devil/android/pshell_test.py (right): https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/psh... devil/devil/android/pshell_test.py:1: #!/usr/bin/env python This shouldn't be committed. https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:91: @contextmanager I think the _PersistentShell class should be a context manager (i.e., it should implement __enter__ and __exit__) rather than having this function be one. https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:121: self._process = subprocess.Popen(self._cmd, can this use cmd_helper.Popen instead? IIRC it specifically avoids some pitfalls around subprocess & pipes, though I forget the specifics. https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:131: self._process.stdin.write('echo START\n') Why is this using START instead of a blank echo? https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:136: def RunCommand(self, command, close=False): I want to review this function specifically in more depth. I'll find some time later this week to do so. Please don't commit this before that happens.
changes from code review https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/dev... devil/devil/android/device_utils.py:716: timeout=None, retries=None, split_lines=True): On 2016/03/07 at 19:27:05, jbudorick wrote: > I do think this is the best of several options that I'm not crazy about. Where will this be called from, though? This will be called from AtraceAgent._collect_trace_data, with cmd being the async-dump command. This will dump the output in one string. https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/psh... File devil/devil/android/pshell_test.py (right): https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/psh... devil/devil/android/pshell_test.py:1: #!/usr/bin/env python On 2016/03/07 at 19:27:05, jbudorick wrote: > This shouldn't be committed. Done https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:91: @contextmanager On 2016/03/07 at 19:27:05, jbudorick wrote: > I think the _PersistentShell class should be a context manager (i.e., it should implement __enter__ and __exit__) rather than having this function be one. Done https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:121: self._process = subprocess.Popen(self._cmd, On 2016/03/07 at 19:27:05, jbudorick wrote: > can this use cmd_helper.Popen instead? IIRC it specifically avoids some pitfalls around subprocess & pipes, though I forget the specifics. cmd_helper.Popen doesn't support the "stdin" keyword, so it won'd work here. Also, I looked at cmd_helper.Popen in more detail, and it does the following two things: 1. It sets the keyword argument "close_fds" in the popen call to true. According to the documentation, this closes all file descriptors except stdin, stdout, and stderr in the child process. 2. It sets the signal handler for SIGPIPE to the default (SIG_DFL) in the child process. (Note that, according to the documentation, the default is to ignore this signal.) It's unclear to me what the purpose of this is because it seems like by definition, if it's not set it should already be the default. Another thing to note is that it seems like it will make this not work on Windows since, according to the documentation, teh signal.signal function works with only a subset of the signals on Windows and SIGPIPE is not in this subset. (I don't know whether this is what cmd_helper.Popen is supposed to do. In any case, even if cmd_helper.Popen could be changed, that would be a separate CL.) https://codereview.chromium.org/1748173002/diff/40001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:131: self._process.stdin.write('echo START\n') On 2016/03/07 at 19:27:05, jbudorick wrote: > Why is this using START instead of a blank echo? Changed
zhenw@chromium.org changed reviewers: + aschulman@chromium.org
https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/dev... devil/devil/android/device_utils.py:833: if split_lines and single_line: I'm wondering about what single_line should do without split_lines. Currently, it's a no-op, and I don't think we want that. https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:105: def __exit__(self, exc_type, exc_value, tb): the linter should be warning about these three parameters... https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:111: raise RuntimeError, 'Persistent shell already running.' prefer raise RuntimeError('Persistent shell already running.') https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:140: if close: This function is entirely modal based on the value of close. Let's just split it into two functions. https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:148: if len(result[0]) == 0: nit: if not result[0] https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:159: send_cmd = '( %s ); echo DONE;\n' % command.rstrip() Instead of just using a flag here, let's do something similar to what we do in Shell and have it echo the exit code. I think we'd be interested in optionally checking that code. https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:169: if output_line[:6] != 'shell@': It'd be nice to merge some of this logic with what you've got for the close=True case above.
https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/dev... devil/devil/android/device_utils.py:833: if split_lines and single_line: On 2016/03/16 at 16:55:16, jbudorick wrote: > I'm wondering about what single_line should do without split_lines. Currently, it's a no-op, and I don't think we want that. It looks to me like the single_line parameter was basically there just as something where if you are expecting only a single line, then you can get just the string with that line in it rather than a one-element list with that string as the element. With the ability now to set split_lines = false, single_line is basically redundant since you can already get only a single string by setting split_lines = false. So there's no need to set split_lines = False and single_line = True. Theoretically we could just eliminate the single_line parameter completely. However, to avoid breaking existing functionality, it would be better to have single_line keep its current behavior and just not allow both split_lines = False and single_line = True. https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:105: def __exit__(self, exc_type, exc_value, tb): On 2016/03/16 at 16:55:16, jbudorick wrote: > the linter should be warning about these three parameters... __exit__ is supposed to have these three parameters. I will look into this separately to see if there is an issue with our lint configuration. https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:111: raise RuntimeError, 'Persistent shell already running.' On 2016/03/16 at 16:55:16, jbudorick wrote: > prefer > > raise RuntimeError('Persistent shell already running.') Done https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:140: if close: On 2016/03/16 at 16:55:16, jbudorick wrote: > This function is entirely modal based on the value of close. Let's just split it into two functions. Done https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:148: if len(result[0]) == 0: On 2016/03/16 at 16:55:16, jbudorick wrote: > nit: if not result[0] Done https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:159: send_cmd = '( %s ); echo DONE;\n' % command.rstrip() On 2016/03/16 at 16:55:16, jbudorick wrote: > Instead of just using a flag here, let's do something similar to what we do in Shell and have it echo the exit code. I think we'd be interested in optionally checking that code. Done https://codereview.chromium.org/1748173002/diff/60001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:169: if output_line[:6] != 'shell@': On 2016/03/16 at 16:55:16, jbudorick wrote: > It'd be nice to merge some of this logic with what you've got for the close=True case above. Done
alexandermont@chromium.org changed reviewers: + ccraik@google.com
alexandermont@chromium.org changed reviewers: + charliea@chromium.org
zhenw@chromium.org changed reviewers: + nednguyen@google.com, zhenw@chromium.org
https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:145: self._process.stdin.write(send_cmd) Why does this add 40 ms additonal latency? adb shell is already running, right? How is this different from RunCommandAndClose() below? https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:167: (output, _) = self._process.communicate( I think using communicate() is slow. It is better to just use self._process.stdin. Ned, can you confirm from your experience?
https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:167: (output, _) = self._process.communicate( On 2016/03/18 22:50:16, Zhen Wang wrote: > I think using communicate() is slow. It is better to just use > self._process.stdin. Ned, can you confirm from your experience? If the subprocess has some expensive operation before it closes, this will be slow.
rnephew@chromium.org changed reviewers: + rnephew@chromium.org
I think it would be prudent to add some unit tests for this before submitting. https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/dev... devil/devil/android/device_utils.py:819: raise ValueError('single_line=True is redundant with split_lines=False') Do you mean redundant or misleading? If its just redundant its probably a bit harsh to raise an exception and I would probably just have it issue a stern warning that its pointless. If its misleading then I agree with the exception. https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:125: output_line = self._process.stdout.readline() readline() blocks until it returns a line correct? Couldn't you just not have the while block? Or is there startup spew that we need to churn through?
https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/dev... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/dev... devil/devil/android/device_utils.py:819: raise ValueError('single_line=True is redundant with split_lines=False') On 2016/03/21 at 22:42:37, rnephew1 wrote: > Do you mean redundant or misleading? If its just redundant its probably a bit harsh to raise an exception and I would probably just have it issue a stern warning that its pointless. If its misleading then I agree with the exception. Done https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:125: output_line = self._process.stdout.readline() On 2016/03/21 at 22:42:37, rnephew1 wrote: > readline() blocks until it returns a line correct? Couldn't you just not have the while block? Or is there startup spew that we need to churn through? It does return other lines, like a repeat of the command "echo" and a line that's something like "shell $ echo", before returning the empty line that is the actual output of the echo command. https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:145: self._process.stdin.write(send_cmd) On 2016/03/18 at 22:50:16, Zhen Wang wrote: > Why does this add 40 ms additonal latency? adb shell is already running, right? How is this different from RunCommandAndClose() below? see discussion on buganizer https://codereview.chromium.org/1748173002/diff/80001/devil/devil/android/sdk... devil/devil/android/sdk/adb_wrapper.py:167: (output, _) = self._process.communicate( On 2016/03/18 at 23:14:03, nednguyen wrote: > On 2016/03/18 22:50:16, Zhen Wang wrote: > > I think using communicate() is slow. It is better to just use > > self._process.stdin. Ned, can you confirm from your experience? > > If the subprocess has some expensive operation before it closes, this will be slow. I haven't observed the subprocess having an expensive operation to close. In any case, until we fix the issue with Nagle (assuming that's what the problem is), we will need this in order to work around that problem.
https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/de... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/de... devil/devil/android/device_utils.py:717: timeout=None, retries=None, split_lines=True): If this change isn't used in this CL, it shouldn't be in this CL. Also, it should have a test when it shows up again. https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:90: class PersistentShell(object): Can you write a brief docstring for PersistentShell that gives an example of how to use it? https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:123: output_line = '.' is this non-empty just so the while loop condition below will accept it? That seems ... weird. https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:124: while output_line.rstrip() != "": nit: single quotes https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:128: """Runs an ADB command and returns the output. Note that there nit: Drop the second sentence into its own paragraph. https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:136: The command output, given as a list of lines., and the exit code nit: stray . https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:144: send_cmd = '( %s ); echo DONE:$?;\n' % command.rstrip() Can these two functions share this command-building and exit code handling with Shell? They really shouldn't all be implementing it individually. https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:155: if not ('/ # ' in output_line and send_cmd.rstrip() in output_line): What is '/ # ' looking for? It seems like it could be brittle. https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper_devicetest.py (right): https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_devicetest.py:52: serial = self._adb.device_serial # pylint: disable=protected-access self.GetDeviceSerial() https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper_devicetest.py:54: (res1, code1) = pshell.RunCommand('echo TEST') These two should be separate tests.
Description was changed from ========== New API for persistent ADB shell ========== to ========== New API for persistent ADB shell Implement a "persistent ADB shell", which allows for setting up an ADB shell in a separate process and passing commands to it. This is necessary to allow ADB communication with less overhead than creating a new ADB shell for each command entered. ==========
https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/de... File devil/devil/android/device_utils.py (right): https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/de... devil/devil/android/device_utils.py:717: timeout=None, retries=None, split_lines=True): On 2016/03/24 at 15:45:48, jbudorick wrote: > If this change isn't used in this CL, it shouldn't be in this CL. > > Also, it should have a test when it shows up again. Done https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:123: output_line = '.' On 2016/03/24 at 15:45:48, jbudorick wrote: > is this non-empty just so the while loop condition below will accept it? That seems ... weird. Done https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:124: while output_line.rstrip() != "": On 2016/03/24 at 15:45:48, jbudorick wrote: > nit: single quotes Done https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:128: """Runs an ADB command and returns the output. Note that there On 2016/03/24 at 15:45:48, jbudorick wrote: > nit: Drop the second sentence into its own paragraph. Done https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:136: The command output, given as a list of lines., and the exit code On 2016/03/24 at 15:45:48, jbudorick wrote: > nit: stray . Done https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:144: send_cmd = '( %s ); echo DONE:$?;\n' % command.rstrip() On 2016/03/24 at 15:45:49, jbudorick wrote: > Can these two functions share this command-building and exit code handling with Shell? They really shouldn't all be implementing it individually. The command-building code is actually different for each of the functions, because the persistent shell needs to have the extra "DONE" marker at the end while the rest of the code doesn't. Similarly, the exit code handling is also different because the exit code appears in a different place (e.g. a separate line). However, the code to get rid of the extraneous lines is basically the same between RunCommand and RunCommandAndClose, so this will be factored out into a separate function. https://codereview.chromium.org/1748173002/diff/120001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:155: if not ('/ # ' in output_line and send_cmd.rstrip() in output_line): On 2016/03/24 at 15:45:49, jbudorick wrote: > What is '/ # ' looking for? It seems like it could be brittle. See comment in IsExtraneousLine.
Moving myself to cc on this CL
Description was changed from ========== New API for persistent ADB shell Implement a "persistent ADB shell", which allows for setting up an ADB shell in a separate process and passing commands to it. This is necessary to allow ADB communication with less overhead than creating a new ADB shell for each command entered. ========== to ========== New API for persistent ADB shell Implement a "persistent ADB shell", which allows for setting up an ADB shell in a separate process and passing commands to it. This is necessary to allow ADB communication with less overhead than creating a new ADB shell for each command entered. ==========
charliea@chromium.org changed reviewers: - charliea@chromium.org
https://codereview.chromium.org/1748173002/diff/160001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/160001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:92: # Alternatively, we could try to detect the shell prompt lines and Remove all of these commented-out lines. https://codereview.chromium.org/1748173002/diff/160001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:141: self._cmd = ['adb', '-s', serial, 'shell'] use AdbWrapper.GetAdbPath() instead of 'adb' https://codereview.chromium.org/1748173002/diff/160001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:207: # remove extraneous blank line at beginning if there is one I think this should be the responsibility of the caller. These ten lines could consequently be collapsed to simply return (result[:-1], int(result[-1]))
https://codereview.chromium.org/1748173002/diff/160001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/160001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:92: # Alternatively, we could try to detect the shell prompt lines and On 2016/04/01 at 17:37:30, jbudorick wrote: > Remove all of these commented-out lines. Done https://codereview.chromium.org/1748173002/diff/160001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:141: self._cmd = ['adb', '-s', serial, 'shell'] On 2016/04/01 at 17:37:30, jbudorick wrote: > use AdbWrapper.GetAdbPath() instead of 'adb' Done
lgtm w/ nits https://codereview.chromium.org/1748173002/diff/180001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (left): https://codereview.chromium.org/1748173002/diff/180001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:35: nit: please leave this line. https://codereview.chromium.org/1748173002/diff/180001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/180001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:75: def IsExtraneousLine(line, send_cmd): nit: this should be module-private (i.e., prefixed by _) if not a local function in RunCommand
https://codereview.chromium.org/1748173002/diff/180001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (left): https://codereview.chromium.org/1748173002/diff/180001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:35: On 2016/04/01 at 18:05:46, jbudorick wrote: > nit: please leave this line. Done https://codereview.chromium.org/1748173002/diff/180001/devil/devil/android/sd... File devil/devil/android/sdk/adb_wrapper.py (right): https://codereview.chromium.org/1748173002/diff/180001/devil/devil/android/sd... devil/devil/android/sdk/adb_wrapper.py:75: def IsExtraneousLine(line, send_cmd): On 2016/04/01 at 18:05:46, jbudorick wrote: > nit: this should be module-private (i.e., prefixed by _) if not a local function in RunCommand Done
The CQ bit was checked by alexandermont@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1748173002/#ps200001 (title: "fix nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748173002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748173002/200001
Message was sent while issue was closed.
Description was changed from ========== New API for persistent ADB shell Implement a "persistent ADB shell", which allows for setting up an ADB shell in a separate process and passing commands to it. This is necessary to allow ADB communication with less overhead than creating a new ADB shell for each command entered. ========== to ========== New API for persistent ADB shell Implement a "persistent ADB shell", which allows for setting up an ADB shell in a separate process and passing commands to it. This is necessary to allow ADB communication with less overhead than creating a new ADB shell for each command entered. Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
