Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
899573003:
Add android_action_runner to the android platform. (Closed)
DescriptionAdd android_action_runner to the android platform.
This allows us to run the 'adb shell input X' commands.
See for details:
https://android.googlesource.com/platform/frameworks/base/+/cd92588/cmds/input/src/com/android/commands/input/Input.java
BUG=
Committed: https://crrev.com/479a27623945529fa8c7dcac8a0d9c66a65c70b0
Cr-Commit-Position: refs/heads/master@{#314433}
Patch Set 1 #
Total comments: 4
Patch Set 2 : add SmoothScrollBy #
Total comments: 6
Patch Set 3 : address comments #
Messages
Total messages: 18 (5 generated)
ariblue@google.com changed reviewers: + nednguyen@google.com
This is a quick first pass at a wrapper for the 'adb shell input X' commands.
https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/android_action_runner.py:21: self.InputSwipe(100, 800, 100, 300, 100) Why do we hard code the value here? Maybe the API should look a bit similar to action_runner.ScrollPage (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...)
https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/android_action_runner.py:21: self.InputSwipe(100, 800, 100, 300, 100) On 2015/02/03 03:05:09, nednguyen wrote: > Why do we hard code the value here? Maybe the API should look a bit similar to > action_runner.ScrollPage > (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) I could probably kill this function entirely or use them as default values in InputSwipe below. I'm not sure that we can make the API similar to that of action_runner (since we can't specify an exact number of pixels we want to scroll, just where and how fast).
https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/android_action_runner.py:21: self.InputSwipe(100, 800, 100, 300, 100) On 2015/02/03 03:09:17, ariblue wrote: > On 2015/02/03 03:05:09, nednguyen wrote: > > Why do we hard code the value here? Maybe the API should look a bit similar to > > action_runner.ScrollPage > > > (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) > > I could probably kill this function entirely or use them as default values in > InputSwipe below. I'm not sure that we can make the API similar to that of > action_runner (since we can't specify an exact number of pixels we want to > scroll, just where and how fast). I think if the one of the input parameter is the number of pixels we want to scroll, we should be able to compute the the left_end & top_end based on the start coordinates?
On 2015/02/03 03:14:24, nednguyen wrote: > https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): > > https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/platform/android_action_runner.py:21: > self.InputSwipe(100, 800, 100, 300, 100) > On 2015/02/03 03:09:17, ariblue wrote: > > On 2015/02/03 03:05:09, nednguyen wrote: > > > Why do we hard code the value here? Maybe the API should look a bit similar > to > > > action_runner.ScrollPage > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) > > > > I could probably kill this function entirely or use them as default values in > > InputSwipe below. I'm not sure that we can make the API similar to that of > > action_runner (since we can't specify an exact number of pixels we want to > > scroll, just where and how fast). > > I think if the one of the input parameter is the number of pixels we want to > scroll, we should be able to compute the the left_end & top_end based on the > start coordinates? Sort of. If we slow down the scroll, then the math is simple: distance scrolled is the delta between start and end coordinates. However, if we scroll more quickly, then we will scroll further.
https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/android_action_runner.py:21: self.InputSwipe(100, 800, 100, 300, 100) On 2015/02/03 03:14:24, nednguyen wrote: > On 2015/02/03 03:09:17, ariblue wrote: > > On 2015/02/03 03:05:09, nednguyen wrote: > > > Why do we hard code the value here? Maybe the API should look a bit similar > to > > > action_runner.ScrollPage > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) > > > > I could probably kill this function entirely or use them as default values in > > InputSwipe below. I'm not sure that we can make the API similar to that of > > action_runner (since we can't specify an exact number of pixels we want to > > scroll, just where and how fast). > > I think if the one of the input parameter is the number of pixels we want to > scroll, we should be able to compute the the left_end & top_end based on the > start coordinates? Ah, I see. Maybe the API should contain: SmoothScrollBy(left_start_coord, top_start_coord, direction, scroll_distance). In the implementation, we set the pixel_per_second_velocity to a constant so that the over scroll effect doesn't happen.
Patchset #2 (id:20001) has been deleted
On 2015/02/03 03:48:01, nednguyen wrote: > https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): > > https://codereview.chromium.org/899573003/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/platform/android_action_runner.py:21: > self.InputSwipe(100, 800, 100, 300, 100) > On 2015/02/03 03:14:24, nednguyen wrote: > > On 2015/02/03 03:09:17, ariblue wrote: > > > On 2015/02/03 03:05:09, nednguyen wrote: > > > > Why do we hard code the value here? Maybe the API should look a bit > similar > > to > > > > action_runner.ScrollPage > > > > > > > > > > (https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) > > > > > > I could probably kill this function entirely or use them as default values > in > > > InputSwipe below. I'm not sure that we can make the API similar to that of > > > action_runner (since we can't specify an exact number of pixels we want to > > > scroll, just where and how fast). > > > > I think if the one of the input parameter is the number of pixels we want to > > scroll, we should be able to compute the the left_end & top_end based on the > > start coordinates? > > Ah, I see. Maybe the API should contain: > SmoothScrollBy(left_start_coord, top_start_coord, direction, scroll_distance). > > In the implementation, we set the pixel_per_second_velocity to a constant so > that the over scroll effect doesn't happen. SmoothScrollBy added.
nednguyen@google.com changed reviewers: + nduca@chromium.org, sullivan@chromium.org
LGTM +Annie, Nat: do you think this is a good way to handle a solution that we won't support in the long term? https://codereview.chromium.org/899573003/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/899573003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_action_runner.py:6: nits: extra blank line here https://codereview.chromium.org/899573003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_action_runner.py:9: nits: extra blank line here https://codereview.chromium.org/899573003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_action_runner.py:10: class AndroidActionRunner(object): Can you add some documentation that none of the gesture here is guaranteed to be performant for telemetry test -> no official support, and add a TODO saying that we may replace this API with a different API with better implementation?
New patchsets have been uploaded after l-g-t-m from nednguyen@google.com
https://codereview.chromium.org/899573003/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/899573003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_action_runner.py:6: On 2015/02/03 19:55:08, nednguyen wrote: > nits: extra blank line here Done. https://codereview.chromium.org/899573003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_action_runner.py:9: On 2015/02/03 19:55:08, nednguyen wrote: > nits: extra blank line here Done. https://codereview.chromium.org/899573003/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/android_action_runner.py:10: class AndroidActionRunner(object): On 2015/02/03 19:55:08, nednguyen wrote: > Can you add some documentation that none of the gesture here is guaranteed to be > performant for telemetry test -> no official support, and add a TODO saying that > we may replace this API with a different API with better implementation? Done.
The CQ bit was checked by nednguyen@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/899573003/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/479a27623945529fa8c7dcac8a0d9c66a65c70b0 Cr-Commit-Position: refs/heads/master@{#314433}
Message was sent while issue was closed.
On 2015/02/03 19:55:08, nednguyen wrote: > LGTM > > +Annie, Nat: do you think this is a good way to handle a solution that we won't > support in the long term? I think this makes sense with the comments that were added in the pydoc. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
