Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1483063006:
Add android_skp_capture.py, JSON file for geniewidget (Closed)
DescriptionAdd android_skp_capture.py, JSON file for geniewidget
The script runs through the directory of JSON files, capturing SKPs from
the apps specified by the JSON files.
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/41cbf3f807d619861365112513655f5f00e16005
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address comments #Patch Set 3 : more #
Messages
Total messages: 14 (4 generated)
borenet@google.com changed reviewers: + djsollen@google.com, rmistry@google.com
concept looks good, but I'm not a great python reviewer. What are your thoughts on extending this to do more than just capture SKP data? Should the json file specify which type of tests it is (e.g. capture_skp or profile) or should that be done as a command line input?
On 2015/12/02 15:54:22, djsollen wrote: > concept looks good, but I'm not a great python reviewer. What are your thoughts > on extending this to do more than just capture SKP data? Should the json file > specify which type of tests it is (e.g. capture_skp or profile) or should that > be done as a command line input? Depends on whether we always want to profile the same things we want to capture SKPs for. If so, we can share the JSON files in which case I'd want to either make it switchable via command line or factor out most of this and have two separate scripts for capturing SKPs and profiling. I'm realizing now that a lot of the names aren't quite right if this is going to do both things.
non python lgtm
https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... File platform_tools/android/skp_gen/android_skp_capture.py (right): https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:48: tuple(action_dict['end']), What does it return if you do not specify tuple here? https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:63: self.run_component = '/'.join((self.package, self.activity)) [optional] I believe '%s/%s' % (self.package, self.activity) is more readable here but upto you. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:69: time.sleep(3) Make time to wait a parameter in the constructor. It could wary per app. Telemetry allows you to specify wait time in the JSON which might be useful here as well. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:87: split = output.splitlines() Call these lines or tokens or output_tokens instead? https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:102: def capture_skp(filename, package, device): call this skp_file instead? that is the argument you used while calling and it seems to fit better https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:106: adb_shell('rm %s' % remote_path) I am assuming that trying to remove a file that does not exist makes adb_shell show an exception which is why you have to make a separate call to check for existence. Another option is to pass in a consume_failures to adb_shell and set it to True when calling adb_shell rm. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:159: time.sleep(1) Could also either put this in the JSON or make it a global constant. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:164: print '' [optional] print with no arguments should also work.
Uploaded patch set 2. Unfortunately, now I'm getting errors from monkeyrunner not being able to talk to the device... https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... File platform_tools/android/skp_gen/android_skp_capture.py (right): https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:48: tuple(action_dict['end']), On 2015/12/02 20:49:18, rmistry wrote: > What does it return if you do not specify tuple here? A list. I'm not sure if monkeyrunner really cares, but the docs say that it's supposed to be a tuple, so that's what I used. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:63: self.run_component = '/'.join((self.package, self.activity)) On 2015/12/02 20:49:18, rmistry wrote: > [optional] I believe '%s/%s' % (self.package, self.activity) is more readable > here but upto you. Done. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:69: time.sleep(3) On 2015/12/02 20:49:18, rmistry wrote: > Make time to wait a parameter in the constructor. It could wary per app. > Telemetry allows you to specify wait time in the JSON which might be useful here > as well. Done. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:87: split = output.splitlines() On 2015/12/02 20:49:18, rmistry wrote: > Call these lines or tokens or output_tokens instead? Done. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:102: def capture_skp(filename, package, device): On 2015/12/02 20:49:18, rmistry wrote: > call this skp_file instead? that is the argument you used while calling and it > seems to fit better Done. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:106: adb_shell('rm %s' % remote_path) On 2015/12/02 20:49:18, rmistry wrote: > I am assuming that trying to remove a file that does not exist makes adb_shell > show an exception which is why you have to make a separate call to check for > existence. > Another option is to pass in a consume_failures to adb_shell and set it to True > when calling adb_shell rm. Yeah, so the problem with that is I'd want to actually error out if we failed to remove the file. So I'd really need to do something like this: try: adb_shell('rm %s' % remote_path) except: if remote_file_exists(remote_path): raise Would that be better? https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:159: time.sleep(1) On 2015/12/02 20:49:18, rmistry wrote: > Could also either put this in the JSON or make it a global constant. Done. https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:164: print '' On 2015/12/02 20:49:18, rmistry wrote: > [optional] print with no arguments should also work. Done.
lgtm https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... File platform_tools/android/skp_gen/android_skp_capture.py (right): https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:106: adb_shell('rm %s' % remote_path) On 2015/12/03 18:14:47, borenet wrote: > On 2015/12/02 20:49:18, rmistry wrote: > > I am assuming that trying to remove a file that does not exist makes adb_shell > > show an exception which is why you have to make a separate call to check for > > existence. > > Another option is to pass in a consume_failures to adb_shell and set it to > True > > when calling adb_shell rm. > > Yeah, so the problem with that is I'd want to actually error out if we failed to > remove the file. So I'd really need to do something like this: > > try: > adb_shell('rm %s' % remote_path) > except: > if remote_file_exists(remote_path): > raise > > Would that be better? You would be saving an exists call in a few places but upto you.
https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... File platform_tools/android/skp_gen/android_skp_capture.py (right): https://codereview.chromium.org/1483063006/diff/1/platform_tools/android/skp_... platform_tools/android/skp_gen/android_skp_capture.py:106: adb_shell('rm %s' % remote_path) On 2015/12/04 13:47:51, rmistry wrote: > On 2015/12/03 18:14:47, borenet wrote: > > On 2015/12/02 20:49:18, rmistry wrote: > > > I am assuming that trying to remove a file that does not exist makes > adb_shell > > > show an exception which is why you have to make a separate call to check for > > > existence. > > > Another option is to pass in a consume_failures to adb_shell and set it to > > True > > > when calling adb_shell rm. > > > > Yeah, so the problem with that is I'd want to actually error out if we failed > to > > remove the file. So I'd really need to do something like this: > > > > try: > > adb_shell('rm %s' % remote_path) > > except: > > if remote_file_exists(remote_path): > > raise > > > > Would that be better? > > You would be saving an exists call in a few places but upto you. Done.
The CQ bit was checked by borenet@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com, rmistry@google.com Link to the patchset: https://codereview.chromium.org/1483063006/#ps40001 (title: "more")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1483063006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1483063006/40001
Message was sent while issue was closed.
Description was changed from ========== Add android_skp_capture.py, JSON file for geniewidget The script runs through the directory of JSON files, capturing SKPs from the apps specified by the JSON files. BUG=skia: ========== to ========== Add android_skp_capture.py, JSON file for geniewidget The script runs through the directory of JSON files, capturing SKPs from the apps specified by the JSON files. BUG=skia: Committed: https://skia.googlesource.com/skia/+/41cbf3f807d619861365112513655f5f00e16005 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/41cbf3f807d619861365112513655f5f00e16005 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
