Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1057563002:
[Android] Convert to BatteryUtils (Closed)
|
Created:
5 years, 8 months ago by rnephew (Reviews Here) Modified:
5 years, 8 months ago CC:
chromium-reviews, telemetry-reviews_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Convert to BatteryUtils
Switch all battery related device interaction from
DeviceUtils to BatteryUtils. Currently they reside in device
utils, which has a lot of functionality around a lot of
different device interactions. This will consolidate all
current and future power related phone interactions into one location.
BUG=472763
Committed: https://crrev.com/58368d3ea55f26f5eb98a76cb7985f580f23ff8c
Cr-Commit-Position: refs/heads/master@{#323940}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Total comments: 4
Patch Set 9 : #
Messages
Total messages: 46 (9 generated)
rnephew@google.com changed reviewers: + jbudorick@chromium.org, nednguyen@chromium.org, perezju@chromium.org, rnephew@google.com
For the telemetry changes, trybot is running now link: https://codereview.chromium.org/1057563002
rnephew@google.com changed reviewers: + nednguyen@google.com - nednguyen@chromium.org
https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... File build/android/provision_devices.py (right): https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... build/android/provision_devices.py:213: battery = battery_utils.BatteryUtils(device) Let's just move this into ChargeDeviceToLevel.
On 2015/04/01 17:28:00, rnephew wrote: > For the telemetry changes, trybot is running now link: > https://codereview.chromium.org/1057563002 Helps if I c/p the right link. https://codereview.chromium.org/1057593002
https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... File build/android/provision_devices.py (right): https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... build/android/provision_devices.py:213: battery = battery_utils.BatteryUtils(device) On 2015/04/01 17:29:47, jbudorick wrote: > Let's just move this into ChargeDeviceToLevel. Done.
On 2015/04/01 17:42:54, rnephew wrote: > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > File build/android/provision_devices.py (right): > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > build/android/provision_devices.py:213: battery = > battery_utils.BatteryUtils(device) > On 2015/04/01 17:29:47, jbudorick wrote: > > Let's just move this into ChargeDeviceToLevel. > > Done. Can you run some benchmark with android trybot with this change?
On 2015/04/01 18:01:27, nednguyen wrote: > On 2015/04/01 17:42:54, rnephew wrote: > > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > > File build/android/provision_devices.py (right): > > > > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > > build/android/provision_devices.py:213: battery = > > battery_utils.BatteryUtils(device) > > On 2015/04/01 17:29:47, jbudorick wrote: > > > Let's just move this into ChargeDeviceToLevel. > > > > Done. > > Can you run some benchmark with android trybot with this change? See link above.
On 2015/04/01 18:04:26, rnephew wrote: > On 2015/04/01 18:01:27, nednguyen wrote: > > On 2015/04/01 17:42:54, rnephew wrote: > > > > > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > > > File build/android/provision_devices.py (right): > > > > > > > > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > > > build/android/provision_devices.py:213: battery = > > > battery_utils.BatteryUtils(device) > > > On 2015/04/01 17:29:47, jbudorick wrote: > > > > Let's just move this into ChargeDeviceToLevel. > > > > > > Done. > > > > Can you run some benchmark with android trybot with this change? > > See link above. Trybot passed: https://codereview.chromium.org/1057593002
On 2015/04/01 18:24:48, rnephew wrote: > On 2015/04/01 18:04:26, rnephew wrote: > > On 2015/04/01 18:01:27, nednguyen wrote: > > > On 2015/04/01 17:42:54, rnephew wrote: > > > > > > > > > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > > > > File build/android/provision_devices.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > > > > build/android/provision_devices.py:213: battery = > > > > battery_utils.BatteryUtils(device) > > > > On 2015/04/01 17:29:47, jbudorick wrote: > > > > > Let's just move this into ChargeDeviceToLevel. > > > > > > > > Done. > > > > > > Can you run some benchmark with android trybot with this change? > > > > See link above. > > Trybot passed: https://codereview.chromium.org/1057593002 Can you also put some context of this change in the description? I feel like this is part of an effort to moving battery's related APIs in device to pylib.device.battery_utils. Do we have bug filed for that?
On 2015/04/01 20:21:03, nednguyen wrote: > On 2015/04/01 18:24:48, rnephew wrote: > > On 2015/04/01 18:04:26, rnephew wrote: > > > On 2015/04/01 18:01:27, nednguyen wrote: > > > > On 2015/04/01 17:42:54, rnephew wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > > > > > File build/android/provision_devices.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1057563002/diff/1/build/android/provision_dev... > > > > > build/android/provision_devices.py:213: battery = > > > > > battery_utils.BatteryUtils(device) > > > > > On 2015/04/01 17:29:47, jbudorick wrote: > > > > > > Let's just move this into ChargeDeviceToLevel. > > > > > > > > > > Done. > > > > > > > > Can you run some benchmark with android trybot with this change? > > > > > > See link above. > > > > Trybot passed: https://codereview.chromium.org/1057593002 > > Can you also put some context of this change in the description? I feel like > this is part of an effort to moving battery's related APIs in device to > pylib.device.battery_utils. Do we have bug filed for that? I've been meaning to make a tracking bug for this, but hadn't. It can be found at: crbug.com/472763
https://codereview.chromium.org/1057563002/diff/20001/build/android/provision... File build/android/provision_devices.py (left): https://codereview.chromium.org/1057563002/diff/20001/build/android/provision... build/android/provision_devices.py:211: device.SetCharging(True) This removal looks like a functional change. https://codereview.chromium.org/1057563002/diff/20001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1057563002/diff/20001/build/android/provision... build/android/provision_devices.py:167: def ChargeDeviceToLevel(device, level): Shouldn't this be a method of BatteryUtils class?
https://codereview.chromium.org/1057563002/diff/20001/build/android/provision... File build/android/provision_devices.py (left): https://codereview.chromium.org/1057563002/diff/20001/build/android/provision... build/android/provision_devices.py:211: device.SetCharging(True) On 2015/04/01 21:22:15, nednguyen wrote: > This removal looks like a functional change. It was moved into ChargeDeviceToLevel instead of being here. The action still takes place. https://codereview.chromium.org/1057563002/diff/20001/build/android/provision... File build/android/provision_devices.py (right): https://codereview.chromium.org/1057563002/diff/20001/build/android/provision... build/android/provision_devices.py:167: def ChargeDeviceToLevel(device, level): On 2015/04/01 21:22:16, nednguyen wrote: > Shouldn't this be a method of BatteryUtils class? Good idea, moved.
https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:250: def ChargeDeviceToLevel(self, level): Maybe this needn't be addressed in this patch but I find this API awkward because it's unsure what would happen to the charging state after the device has reached the specified level. I think this probably should be changed to WaitForDeviceToCharge(self, level) method which throw exception if the charging bit is False. https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:256: self.SetCharging(True) Assume that the device's charging state is off, and it is at level 90%. Making the call ChargeDeviceToLevel(70) just turns on the charging state is a very surprising.
https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:250: def ChargeDeviceToLevel(self, level): On 2015/04/01 22:17:39, nednguyen wrote: > Maybe this needn't be addressed in this patch but I find this API awkward > because it's unsure what would happen to the charging state after the device has > reached the specified level. Why does this matter? > > I think this probably should be changed to WaitForDeviceToCharge(self, level) > method which throw exception if the charging bit is False. Strongly disagree with this. Easier to make a mistake for no benefit. https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:256: self.SetCharging(True) On 2015/04/01 22:17:39, nednguyen wrote: > Assume that the device's charging state is off, and it is at level 90%. Making > the call ChargeDeviceToLevel(70) just turns on the charging state is a very > surprising. I don't see why a user asking for a device to be charged to some level would be surprised that a device is charging.
https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:250: def ChargeDeviceToLevel(self, level): On 2015/04/01 22:32:21, jbudorick wrote: > On 2015/04/01 22:17:39, nednguyen wrote: > > Maybe this needn't be addressed in this patch but I find this API awkward > > because it's unsure what would happen to the charging state after the device > has > > reached the specified level. > > Why does this matter? Actually, this comment should just make it explicit that this function: - enables charging - waits for the device to charge to the level > > > > > I think this probably should be changed to WaitForDeviceToCharge(self, level) > > method which throw exception if the charging bit is False. > > Strongly disagree with this. Easier to make a mistake for no benefit.
https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:250: def ChargeDeviceToLevel(self, level): On 2015/04/01 22:34:15, jbudorick wrote: > On 2015/04/01 22:32:21, jbudorick wrote: > > On 2015/04/01 22:17:39, nednguyen wrote: > > > Maybe this needn't be addressed in this patch but I find this API awkward > > > because it's unsure what would happen to the charging state after the device > > has > > > reached the specified level. > > > > Why does this matter? > > Actually, this comment should just make it explicit that this function: > - enables charging > - waits for the device to charge to the level > > > > > > > > > I think this probably should be changed to WaitForDeviceToCharge(self, > level) > > > method which throw exception if the charging bit is False. > > > > Strongly disagree with this. Easier to make a mistake for no benefit. > Just looking at the method name, it's unclear what this will do in case the charge level you are asking for is lower the the actual charged level of the device. Another question comes into to mind is whether this will stop charging the device when the device has reached specified level. Based on the function name, I expect it to stop charging, but it doesn't based on the implementation.
https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:250: def ChargeDeviceToLevel(self, level): On 2015/04/01 22:49:06, nednguyen wrote: > On 2015/04/01 22:34:15, jbudorick wrote: > > On 2015/04/01 22:32:21, jbudorick wrote: > > > On 2015/04/01 22:17:39, nednguyen wrote: > > > > Maybe this needn't be addressed in this patch but I find this API awkward > > > > because it's unsure what would happen to the charging state after the > device > > > has > > > > reached the specified level. > > > > > > Why does this matter? > > > > Actually, this comment should just make it explicit that this function: > > - enables charging > > - waits for the device to charge to the level > > > > > > > > > > > > > I think this probably should be changed to WaitForDeviceToCharge(self, > > level) > > > > method which throw exception if the charging bit is False. > > > > > > Strongly disagree with this. Easier to make a mistake for no benefit. > > > > Just looking at the method name, it's unclear what this will do in case the > charge level you are asking for is lower the the actual charged level of the > device. > > Another question comes into to mind is whether this will stop charging the > device when the device has reached specified level. Based on the function name, > I expect it to stop charging, but it doesn't based on the implementation. We generally want our devices in a charging state unless explicitly disabled. This is doing _exactly_ what I think we want it to do, and I think we'd rather make any possible ambiguity clear in the comment than have a function name like "EnableChargingAndChargeDeviceToLevel" If a caller has a strong preference about the charging state coming out of this, fine. They can set the charging state. Otherwise, let's leave the device in the default charging state -- i.e., with charging enabled.
https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1057563002/diff/40001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:250: def ChargeDeviceToLevel(self, level): On 2015/04/01 22:58:51, jbudorick wrote: > On 2015/04/01 22:49:06, nednguyen wrote: > > On 2015/04/01 22:34:15, jbudorick wrote: > > > On 2015/04/01 22:32:21, jbudorick wrote: > > > > On 2015/04/01 22:17:39, nednguyen wrote: > > > > > Maybe this needn't be addressed in this patch but I find this API > awkward > > > > > because it's unsure what would happen to the charging state after the > > device > > > > has > > > > > reached the specified level. > > > > > > > > Why does this matter? > > > > > > Actually, this comment should just make it explicit that this function: > > > - enables charging > > > - waits for the device to charge to the level > > > > > > > > > > > > > > > > > I think this probably should be changed to WaitForDeviceToCharge(self, > > > level) > > > > > method which throw exception if the charging bit is False. > > > > > > > > Strongly disagree with this. Easier to make a mistake for no benefit. > > > > > > > Just looking at the method name, it's unclear what this will do in case the > > charge level you are asking for is lower the the actual charged level of the > > device. > > > > Another question comes into to mind is whether this will stop charging the > > device when the device has reached specified level. Based on the function > name, > > I expect it to stop charging, but it doesn't based on the implementation. > > We generally want our devices in a charging state unless explicitly disabled. > This is doing _exactly_ what I think we want it to do, and I think we'd rather > make any possible ambiguity clear in the comment than have a function name like > "EnableChargingAndChargeDeviceToLevel" > > If a caller has a strong preference about the charging state coming out of this, > fine. They can set the charging state. Otherwise, let's leave the device in the > default charging state -- i.e., with charging enabled. Added comment saying it enabled charging.
lgtm
lgtm w/ nits https://codereview.chromium.org/1057563002/diff/60001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1057563002/diff/60001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:251: """ Enables charging and waits for device to be charged to given level. nit: no space before Enables https://codereview.chromium.org/1057563002/diff/60001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:255: wait_period: time to wait between checking. nit: specify the units, i.e., "time in seconds ..."
https://codereview.chromium.org/1057563002/diff/60001/build/android/pylib/dev... File build/android/pylib/device/battery_utils.py (right): https://codereview.chromium.org/1057563002/diff/60001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:251: """ Enables charging and waits for device to be charged to given level. On 2015/04/01 23:14:20, jbudorick wrote: > nit: no space before Enables Done. https://codereview.chromium.org/1057563002/diff/60001/build/android/pylib/dev... build/android/pylib/device/battery_utils.py:255: wait_period: time to wait between checking. On 2015/04/01 23:14:20, jbudorick wrote: > nit: specify the units, i.e., "time in seconds ..." Done.
The CQ bit was checked by rnephew@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1057563002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1057563002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
A bit late to the party, but lgtm too, when the try jobs do pass.
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1057563002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/04/02 14:04:11, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Telemetry uses some different kind of mock for their unit tests to replace DeviceUtils. I don't remember the exact details, but this bot failure is not a flake.
On 2015/04/02 14:05:17, jbudorick wrote: > On 2015/04/02 14:04:11, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > Telemetry uses some different kind of mock for their unit tests to replace > DeviceUtils. I don't remember the exact details, but this bot failure is not a > flake. If you need to modify the unittest, you can switch the existing mock to telemetry/third_party/mock. We used to have a homegrown mock/stub thing and trying to get rid of it. https://groups.google.com/a/chromium.org/forum/#!topic/telemetry/3Tde7spyXVE
On 2015/04/02 15:35:44, nednguyen wrote: > On 2015/04/02 14:05:17, jbudorick wrote: > > On 2015/04/02 14:04:11, I haz the power (commit-bot) wrote: > > > Try jobs failed on following builders: > > > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > > > > Telemetry uses some different kind of mock for their unit tests to replace > > DeviceUtils. I don't remember the exact details, but this bot failure is not a > > flake. > > If you need to modify the unittest, you can switch the existing mock to > telemetry/third_party/mock. We used to have a homegrown mock/stub thing and > trying to get rid of it. > > https://groups.google.com/a/chromium.org/forum/#!topic/telemetry/3Tde7spyXVE Mocked out battery_utils in android_platform_backend_unittests. I am not overly familiar with how the python mock library work, and I do not like how I have to disable unused-variables in this so if anyone has any suggestions on how to better use mock, let me know.
> Mocked out battery_utils in android_platform_backend_unittests. I am not overly > familiar with how the python mock library work, and I do not like how I have to > disable unused-variables in this so if anyone has any suggestions on how to > better use mock, let me know. Changed to context manager version of mock to get rid of disabling of pylint.
https://codereview.chromium.org/1057563002/diff/120001/tools/telemetry/teleme... File tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py (right): https://codereview.chromium.org/1057563002/diff/120001/tools/telemetry/teleme... tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py:37: with mock.patch.object(battery_utils, 'BatteryUtils'): Can we move this to setUp & cleanup it up teardown so you don't have to repeat this everywhere?
https://codereview.chromium.org/1057563002/diff/120001/tools/telemetry/teleme... File tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py (right): https://codereview.chromium.org/1057563002/diff/120001/tools/telemetry/teleme... tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py:37: with mock.patch.object(battery_utils, 'BatteryUtils'): On 2015/04/06 17:46:27, nednguyen wrote: > Can we move this to setUp & cleanup it up teardown so you don't have to repeat > this everywhere? Done.
lgtm https://codereview.chromium.org/1057563002/diff/140001/tools/telemetry/teleme... File tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py (right): https://codereview.chromium.org/1057563002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py:31: battery_utils.BatteryUtils.__init__ = mock.Mock(return_value=None) nit: you can use: self.battery_patcher = mock.patch.object(battery_utils, 'BatteryUtils') self.battery_patcher.start() https://codereview.chromium.org/1057563002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py:36: battery_utils.BatteryUtils.__init__ = self._actual_battery_utils_init ditto: self.battery_patcher.stop()
https://codereview.chromium.org/1057563002/diff/140001/tools/telemetry/teleme... File tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py (right): https://codereview.chromium.org/1057563002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py:31: battery_utils.BatteryUtils.__init__ = mock.Mock(return_value=None) On 2015/04/06 18:42:25, nednguyen wrote: > nit: you can use: > > self.battery_patcher = mock.patch.object(battery_utils, 'BatteryUtils') > self.battery_patcher.start() Done. https://codereview.chromium.org/1057563002/diff/140001/tools/telemetry/teleme... tools/telemetry/telemetry/core/platform/android_platform_backend_unittest.py:36: battery_utils.BatteryUtils.__init__ = self._actual_battery_utils_init On 2015/04/06 18:42:25, nednguyen wrote: > ditto: self.battery_patcher.stop() Done.
The CQ bit was checked by rnephew@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from jbudorick@chromium.org, perezju@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1057563002/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1057563002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/58368d3ea55f26f5eb98a76cb7985f580f23ff8c Cr-Commit-Position: refs/heads/master@{#323940} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
