Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1004833003:
[Telemetry][Android] Add support for turning screen off. (Closed)
|
Created:
5 years, 9 months ago by rnephew (Reviews Here) Modified:
5 years, 9 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Telemetry][Android] Add support for turning screen off.
For testing background usage of Chrome on android devices.
BUG=
Committed: https://crrev.com/4c91b7d41b40faff7d4de868d5b10c4e8ac95ad3
Cr-Commit-Position: refs/heads/master@{#322254}
Patch Set 1 #
Total comments: 7
Patch Set 2 : use waitfor() #
Total comments: 5
Patch Set 3 : #
Total comments: 18
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Messages
Total messages: 19 (3 generated)
Tested and working on K and L. No existing benchmark will use these actions, new ones will need to be created to use them. https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/android_action_runner.py:123: else: Does telemetry have anything like the timeout/retry decorators used in build/android/pylib/device/device_utils.py ? It would be nice to be able to have it wait until the screen shows up as being off/on for these methods, but I run into race conditions where it will not register as off/on when looking the first time.
https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/android_action_runner.py:123: else: On 2015/03/24 20:38:07, rnephew wrote: > Does telemetry have anything like the timeout/retry decorators used in > build/android/pylib/device/device_utils.py ? > > It would be nice to be able to have it wait until the screen shows up as being > off/on for these methods, but I run into race conditions where it will not > register as off/on when looking the first time. It has util.WaitFor() https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Is this what you are looking for?
https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/android_platform_backend.py:639: def IsScreenOn(self): If this only works for android L & K, you should make an assertion of that fact here.
https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/android_action_runner.py:123: else: On 2015/03/24 20:55:14, nednguyen wrote: > On 2015/03/24 20:38:07, rnephew wrote: > > Does telemetry have anything like the timeout/retry decorators used in > > build/android/pylib/device/device_utils.py ? > > > > It would be nice to be able to have it wait until the screen shows up as being > > off/on for these methods, but I run into race conditions where it will not > > register as off/on when looking the first time. > > It has util.WaitFor() > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > Is this what you are looking for? That looks like what I need. Thanks! https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/android_platform_backend.py:639: def IsScreenOn(self): On 2015/03/24 20:56:29, nednguyen wrote: > If this only works for android L & K, you should make an assertion of that fact > here. I just dont have a J or earlier device handy to check that it works properly there. I will flash a device to J and test.
https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/android_platform_backend.py:639: def IsScreenOn(self): On 2015/03/24 21:01:18, rnephew wrote: > On 2015/03/24 20:56:29, nednguyen wrote: > > If this only works for android L & K, you should make an assertion of that > fact > > here. > > I just dont have a J or earlier device handy to check that it works properly > there. I will flash a device to J and test. It does work in J, using the mScreenOn value in input_method.
https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/1004833003/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/android_action_runner.py:123: else: On 2015/03/24 21:01:18, rnephew wrote: > On 2015/03/24 20:55:14, nednguyen wrote: > > On 2015/03/24 20:38:07, rnephew wrote: > > > Does telemetry have anything like the timeout/retry decorators used in > > > build/android/pylib/device/device_utils.py ? > > > > > > It would be nice to be able to have it wait until the screen shows up as > being > > > off/on for these methods, but I run into race conditions where it will not > > > register as off/on when looking the first time. > > > > It has util.WaitFor() > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > > > Is this what you are looking for? > > That looks like what I need. Thanks! Now using waitfor()
https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:120: """Turn screen on and unlock phone.""" A method should only do one thing. Also it's unclear what would happen if the phone has a screenlock. Please create another method called UnlockDevice(self) with documentation explaining what happens if unlock operation fails. https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:120: """Turn screen on and unlock phone.""" nits: s/phone/device since this API also works for tablet. https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:639: def IsScreenOn(self): If this isn't intended to be run on L & K, then just do s.t like: if not androidversion is L or K: raise UnimplementedError('Only supported in android L & K')
https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:120: """Turn screen on and unlock phone.""" On 2015/03/24 22:40:29, nednguyen wrote: > A method should only do one thing. Also it's unclear what would happen if the > phone has a screenlock. Please create another method called UnlockDevice(self) > with documentation explaining what happens if unlock operation fails. Done. https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:639: def IsScreenOn(self): On 2015/03/24 22:40:29, nednguyen wrote: > If this isn't intended to be run on L & K, then just do s.t like: > if not androidversion is L or K: > raise UnimplementedError('Only supported in android L & K') This isn't L/K specific. It works in other versions as well.
https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:121: If device fails to turn screen on, an exception is thrown. Can you add: If the screen is already on, this method returns immediately. If the screen is off and device fails to turn screen on, an exception is thrown. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:126: logging.warning('Screen on when expected off.') Add a return here to avoid the WaitFor below? https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:130: def TurnScreenOff(self): Make changes similar to TurnScreenOn https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:145: def UnlockScreen(self): Ditto https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:147: If device fails to unlock screen, an excpetion is thrown. nit: exception. Can you also document the type of exception? https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:640: """Determines if device screen is on.""" Please document the possible exceptions in the docstring here and other places follows style guide: https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:643: if 'mScreenOn' in line or 'mInteractive' in line: These are some non trivial parsing logic. Can you add unittest for this? Maybe create a static method like: _IsScreeOn(input_methods) so it's easier to unittest the parsing logic. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:658: for line in input_methods: Ditto https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:668: raise Exception('Unknown value for %s: %s' % (key, value)) From python style guide: "Modules or packages should define their own domain-specific base exception class, which should inherit from the built-in Exception class. The base exception for a module should be called Error." You can use https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... to define these exception.
https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_action_runner.py (right): https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:121: If device fails to turn screen on, an exception is thrown. On 2015/03/25 16:16:14, nednguyen wrote: > Can you add: > If the screen is already on, this method returns immediately. > If the screen is off and device fails to turn screen on, an exception is thrown. Done. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:126: logging.warning('Screen on when expected off.') On 2015/03/25 16:16:15, nednguyen wrote: > Add a return here to avoid the WaitFor below? Done. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:130: def TurnScreenOff(self): On 2015/03/25 16:16:14, nednguyen wrote: > Make changes similar to TurnScreenOn Done. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:145: def UnlockScreen(self): On 2015/03/25 16:16:15, nednguyen wrote: > Ditto Done. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_action_runner.py:147: If device fails to unlock screen, an excpetion is thrown. On 2015/03/25 16:16:14, nednguyen wrote: > nit: exception. > > Can you also document the type of exception? Done. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:640: """Determines if device screen is on.""" On 2015/03/25 16:16:15, nednguyen wrote: > Please document the possible exceptions in the docstring here and other places > follows style guide: > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments > Done. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:643: if 'mScreenOn' in line or 'mInteractive' in line: On 2015/03/25 16:16:15, nednguyen wrote: > These are some non trivial parsing logic. Can you add unittest for this? > Maybe create a static method like: _IsScreeOn(input_methods) so it's easier to > unittest the parsing logic. Done. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:658: for line in input_methods: On 2015/03/25 16:16:15, nednguyen wrote: > Ditto Done. https://codereview.chromium.org/1004833003/diff/40001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:668: raise Exception('Unknown value for %s: %s' % (key, value)) On 2015/03/25 16:16:15, nednguyen wrote: > From python style guide: > "Modules or packages should define their own domain-specific base exception > class, which should inherit from the built-in Exception class. The base > exception for a module should be called Error." > > You can use > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > to define these exception. Done. Note: There are other exceptions in this file using Exception(text).
lgtm https://codereview.chromium.org/1004833003/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:664: raise exceptions.AndroidDeviceParsingError() Maybe exceptions.AndroidDeviceParsingError(str(input_method)) so people can look at the debug log and figure out what went wrong? https://codereview.chromium.org/1004833003/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:697: raise exceptions.AndroidDeviecParsingError() ditto
https://codereview.chromium.org/1004833003/diff/60001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/android_platform_backend.py (right): https://codereview.chromium.org/1004833003/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:664: raise exceptions.AndroidDeviceParsingError() On 2015/03/25 21:05:40, nednguyen wrote: > Maybe exceptions.AndroidDeviceParsingError(str(input_method)) so people can look > at the debug log and figure out what went wrong? Done. https://codereview.chromium.org/1004833003/diff/60001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/android_platform_backend.py:697: raise exceptions.AndroidDeviecParsingError() On 2015/03/25 21:05:40, nednguyen wrote: > ditto Done.
The CQ bit was checked by rnephew@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1004833003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1004833003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4c91b7d41b40faff7d4de868d5b10c4e8ac95ad3 Cr-Commit-Position: refs/heads/master@{#322254}
Message was sent while issue was closed.
belated lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
