Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1057423002:
[Telemetry][Android] Change how power monitoring capability is decided (Closed)
|
Created:
5 years, 8 months ago by rnephew (Reviews Here) Modified:
5 years, 8 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] Change how power monitoring capability is decided
With recent changes, all known device types can be monitored. Update
CanMonitorPower() to reflect this.
BUG=
Committed: https://crrev.com/5f5118478a1994b5f02edc24cb17d74df64948eb
Cr-Commit-Position: refs/heads/master@{#323910}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Change CanMonitorPower() implementation #
Total comments: 2
Patch Set 3 : #Messages
Total messages: 19 (6 generated)
rnephew@google.com changed reviewers: + jdduke@chromium.org, rnephew@google.com, sullivan@chromium.org
The CQ bit was checked by jdduke@chromium.org
The CQ bit was unchecked by jdduke@chromium.org
On 2015/04/03 20:43:03, rnephew wrote: Trybot runs on n5 and n6 currently running: rnephew@rnephew0:~/chromium/clank/src$ ./tools/perf/run_benchmark --browser=trybot-android-nexus6 power.typical_10_mobile --also-run-disabled-tests Uploaded chromium try job to rietveld for android platform. View progress at https://codereview.chromium.org/1057213004 rnephew@rnephew0:~/chromium/clank/src$ ./tools/perf/run_benchmark --browser=trybot-android-nexus5 power.typical_10_mobile --also-run-disabled-tests Uploaded chromium try job to rietveld for android platform. View progress at https://codereview.chromium.org/1053363002
https://codereview.chromium.org/1057423002/diff/1/tools/telemetry/telemetry/c... File tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py (right): https://codereview.chromium.org/1057423002/diff/1/tools/telemetry/telemetry/c... tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:33: return True Nice, so this holds for all Android versions?
On 2015/04/03 20:57:02, jdduke wrote: > https://codereview.chromium.org/1057423002/diff/1/tools/telemetry/telemetry/c... > File > tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py > (right): > > https://codereview.chromium.org/1057423002/diff/1/tools/telemetry/telemetry/c... > tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:33: > return True > Nice, so this holds for all Android versions? The disabling of charging works on everything going back to JB. I never tried it on ICS. As for the getting power data, it requires L apis. How telemetry works is it determines if it can monitor power based on if it can disable charging, not if the dumpsys version is new enough to support the L api. Thinking about it, I could easily modify this to check the dumpsys version and better reflect reality. It would take a longer round trip to the device to establish if it can get data; since it has to get a dumpsys worth of data transferred from the phone, but overall if it cannot get power data it will have a lot less useless communication with the device since it will not turn power on/off if it can't get power data.
On 2015/04/03 21:02:33, rnephew wrote: > The disabling of charging works on everything going back to JB. I never tried it > on ICS. As for the getting power data, it requires L apis. How telemetry works > is it determines if it can monitor power based on if it can disable charging, > not if the dumpsys version is new enough to support the L api. Thinking about > it, I could easily modify this to check the dumpsys version and better reflect > reality. It would take a longer round trip to the device to establish if it can > get data; since it has to get a dumpsys worth of data transferred from the > phone, but overall if it cannot get power data it will have a lot less useless > communication with the device since it will not turn power on/off if it can't > get power data. Gotcha, thanks, I'll defer to your judgement here.
Updated to new version of CanMonitorPower.
Killed old trybot runs, new ones here: rnephew@rnephew0:~/chromium/clank/src$ ./tools/perf/run_benchmark --browser=trybot-android-nexus6 power.typical_10_mobile --also-run-disabled-tests Uploaded chromium try job to rietveld for android platform. View progress at https://codereview.chromium.org/1056193002 rnephew@rnephew0:~/chromium/clank/src$ ./tools/perf/run_benchmark --browser=trybot-android-nexus5 power.typical_10_mobile --also-run-disabled-tests Uploaded chromium try job to rietveld for android platform. View progress at https://codereview.chromium.org/1063513002 Sorry this is a bit spammy.
rnephew@google.com changed reviewers: + nednguyen@google.com
lgtm https://codereview.chromium.org/1057423002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py (right): https://codereview.chromium.org/1057423002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:33: if csvreader.next()[DUMP_VERSION_INDEX] in ['8', '9']: Can you add documentation explain why is this?
https://codereview.chromium.org/1057423002/diff/20001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py (right): https://codereview.chromium.org/1057423002/diff/20001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/power_monitor/android_dumpsys_power_monitor.py:33: if csvreader.next()[DUMP_VERSION_INDEX] in ['8', '9']: On 2015/04/06 16:00:28, nednguyen wrote: > Can you add documentation explain why is this? 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/1057423002/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1057423002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5f5118478a1994b5f02edc24cb17d74df64948eb Cr-Commit-Position: refs/heads/master@{#323910} |
