Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1256353002:
Add android_run_app.go (Closed)
DescriptionAdd android_run_app.go
Launches an app, pipes its output, and exits when the app exits.
Loosely based on the old buildbot code in python:
https://skia.googlesource.com/buildbot/+/ac0663c599a443a4958c8cad5aefd25eb09eff58/slave/skia_slave_scripts/utils/android_utils.py
BUG=skia:4093
Committed: https://skia.googlesource.com/skia/+/850bbf1bb62e07b02e16627e522079f60333d857
Patch Set 1 #Patch Set 2 : Fix package names #
Total comments: 6
Patch Set 3 : Address comment #
Total comments: 6
Patch Set 4 : ... #Patch Set 5 : Use pointer receiver #Messages
Total messages: 13 (2 generated)
borenet@google.com changed reviewers: + rmistry@google.com, stephana@google.com
Friendly ping.
These are all nits and suggestions, the only thing that should change is that the term 'adb' is too overloaded. It's hard to keep track of what it refers to. https://codereview.chromium.org/1256353002/diff/20001/platform_tools/android/... File platform_tools/android/bin/android_run_app.go (right): https://codereview.chromium.org/1256353002/diff/20001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:37: func ADB(adb, serial string, log bool, args ...string) *exec.Cmd { Nit: I think "adbCommand" or adbCmd would be more descriptive. Also, it doesn't necessarily matter, but this and the other methods should be internal (lowercase) methods. https://codereview.chromium.org/1256353002/diff/20001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:51: return ADB(adb, serial, log, args...).Output() This is an unnecessary level of indirection. Instead one could just write: adbCmd(.....).Output() whenever RunADB(...) is called. https://codereview.chromium.org/1256353002/diff/20001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:62: output, err := RunADB(adb, serial, false, "shell", "ps") It might make sense to create a type 'ADB' instead of passing the same arguments around.
https://codereview.chromium.org/1256353002/diff/20001/platform_tools/android/... File platform_tools/android/bin/android_run_app.go (right): https://codereview.chromium.org/1256353002/diff/20001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:37: func ADB(adb, serial string, log bool, args ...string) *exec.Cmd { On 2015/07/30 13:37:42, stephana wrote: > Nit: I think "adbCommand" or adbCmd would be more descriptive. Also, it doesn't > necessarily matter, but this and the other methods should be internal > (lowercase) methods. I took the suggestion of making ADB a struct. The analogous method is now ADB.Cmd. https://codereview.chromium.org/1256353002/diff/20001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:51: return ADB(adb, serial, log, args...).Output() On 2015/07/30 13:37:42, stephana wrote: > This is an unnecessary level of indirection. Instead one could just write: > > adbCmd(.....).Output() > > > whenever RunADB(...) is called. Removed. https://codereview.chromium.org/1256353002/diff/20001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:62: output, err := RunADB(adb, serial, false, "shell", "ps") On 2015/07/30 13:37:42, stephana wrote: > It might make sense to create a type 'ADB' instead of passing the same arguments > around. I like this! Done.
My huge contribution to this review. https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... File platform_tools/android/bin/android_run_app.go (right): https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. 2015
https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... File platform_tools/android/bin/android_run_app.go (right): https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/07/30 17:39:13, rmistry wrote: > 2015 http://i.lvme.me/uwnawrl.jpg
https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... File platform_tools/android/bin/android_run_app.go (right): https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:55: func (adb ADB) Cmd(log bool, args ...string) *exec.Cmd { IMO the receiver should be *ADB instead of ADB. The performance hit is probably inconsequential, but it's inconsistent with "ADBFromFlags" which returns a pointer. https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:150: // Kill any running instances of the app. Is there a reason why RunApp is not part of the ADB?
https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... File platform_tools/android/bin/android_run_app.go (right): https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:55: func (adb ADB) Cmd(log bool, args ...string) *exec.Cmd { On 2015/07/30 17:49:48, stephana wrote: > IMO the receiver should be *ADB instead of ADB. The performance hit is probably > inconsequential, but it's inconsistent with "ADBFromFlags" which returns a > pointer. Done. https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... platform_tools/android/bin/android_run_app.go:150: // Kill any running instances of the app. On 2015/07/30 17:49:48, stephana wrote: > Is there a reason why RunApp is not part of the ADB? per live conversation, RunApp is a less generally-useful method than the others. If I were to add it to ADB, I'd want to generalize it a bit, and I don't think it's needed at the moment.
On 2015/07/30 17:55:55, borenet wrote: > https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... > File platform_tools/android/bin/android_run_app.go (right): > > https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... > platform_tools/android/bin/android_run_app.go:55: func (adb ADB) Cmd(log bool, > args ...string) *exec.Cmd { > On 2015/07/30 17:49:48, stephana wrote: > > IMO the receiver should be *ADB instead of ADB. The performance hit is > probably > > inconsequential, but it's inconsistent with "ADBFromFlags" which returns a > > pointer. > > Done. > > https://codereview.chromium.org/1256353002/diff/40001/platform_tools/android/... > platform_tools/android/bin/android_run_app.go:150: // Kill any running instances > of the app. > On 2015/07/30 17:49:48, stephana wrote: > > Is there a reason why RunApp is not part of the ADB? > > per live conversation, RunApp is a less generally-useful method than the others. > If I were to add it to ADB, I'd want to generalize it a bit, and I don't think > it's needed at the moment. lgtm
The CQ bit was checked by borenet@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1256353002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1256353002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/850bbf1bb62e07b02e16627e522079f60333d857 |
