move set charging to powermonitorcontroller

move set charging to powermonitorcontroller

BUG=558106
Committed: https://crrev.com/699b68b3f14abf7957bbba3e130b88317f6f2646
Cr-Commit-Position: refs/heads/master@{#360994}

[alexandermont, 2015-11-17 23:59:56]

alexandermont@chromium.org changed reviewers:
+ nednguyen@google.com, rnephew@google.com

[alexandermont, 2015-11-17 23:59:56]

Move enabling/disabling of charging to PowerMonitorController.

[rnephew (Wrong account), 2015-11-18 00:40:02]

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:45:
def StopMonitoringPower(self, set_charge=True):
This could be set in the constructor, so that we don't have to keep passing
around keyword args everywhere. The android power monitors are created in
android_platform_backend.py. This would also make it so that monitors that do
not require disabling of charging (sys_fs_pm as an example) do not have to have
this.

Just a thought, not necessarily the correct way to do it.

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py
(right):

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py:44:
def StartMonitoringPower(self, browser, set_charge=True):
Only the android power monitors have charging enabling and disabling, so this
isn't really necessary for the non-android monitors.

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py
(right):

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:15:
class PowerMonitorController(power_monitor.PowerMonitor):
Since this is android specific now, it should probably be renamed to reflect
that reality.

[alexandermont, 2015-11-18 19:15:04]

Note that the presubmit checks found errors where some of the lines are too
long. For instance:

android_platform_backend.py 165, 168
android_dumpsys_power_monitor_unittest 95
andrpod_power_monitor_controller 14
android_power_monitor_controller_unittest 68

I am interested to see if you have suggestions for how to reduce the length of
these lines, since these lines consist of a few very long class and variable
names and there are no obvious places to break the lines into multiple lines.

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:45:
def StopMonitoringPower(self, set_charge=True):
On 2015/11/18 at 00:40:02, rnephew wrote:
> This could be set in the constructor, so that we don't have to keep passing
around keyword args everywhere. The android power monitors are created in
android_platform_backend.py. This would also make it so that monitors that do
not require disabling of charging (sys_fs_pm as an example) do not have to have
this.
> 
> Just a thought, not necessarily the correct way to do it.

Done.

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py
(right):

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/msr_power_monitor.py:44:
def StartMonitoringPower(self, browser, set_charge=True):
On 2015/11/18 at 00:40:02, rnephew wrote:
> Only the android power monitors have charging enabling and disabling, so this
isn't really necessary for the non-android monitors.

Done.

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py
(right):

https://codereview.chromium.org/1456803002/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/power_monitor/power_monitor_controller.py:15:
class PowerMonitorController(power_monitor.PowerMonitor):
On 2015/11/18 at 00:40:02, rnephew wrote:
> Since this is android specific now, it should probably be renamed to reflect
that reality.

Done.

[rnephew (Wrong account), 2015-11-18 19:33:26]

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/android_platform_backend.py:27: from
telemetry.internal.platform.power_monitor import
android_power_monitor_controller
Same linelength fix as the other import.

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/android_platform_backend.py:165:
self._power_monitor =
android_power_monitor_controller.AndroidPowerMonitorController([
self._power_monitor = (
    android_power_monitor_controller.AndroidPowerMonitorController([

and just close the parentheses at the end of creation of the
AndroidPowerMonitorController.
Similar things can be done for the other ones that are too long. More on line
length can be found at:
https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s...

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/android_platform_backend.py:168:
android_dumpsys_power_monitor.DumpsysPowerMonitor(self._battery, self,
set_charge=False),
         android_dumpsys_power_monitor.DumpsysPowerMonitor(
             self._battery, self, set_charge=False),

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py
(right):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:95:
pm = android_dumpsys_power_monitor.DumpsysPowerMonitor(battery, backend,
set_charge=False)
pm = android_dumpsys_power_monitor.DumpsysPowerMonitor(
    battery, backend, set_charge=False)

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller.py
(right):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller.py:14:
class
AndroidPowerMonitorController(android_power_monitor_base.AndroidPowerMonitorBase):
class AndroidPowerMonitorController(
    android_power_monitor_base.AndroidPowerMonitorBase):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller_unittest.py
(right):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller_unittest.py:8:
from telemetry.internal.platform.power_monitor import
android_power_monitor_controller
IIRC, the correct way to handle long imports is:
from telemetry.internal.platform.power_monitor import (
    android_power_monitor_controller)

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller_unittest.py:68:
android_power_monitor_controller.AndroidPowerMonitorController._MergePowerResults(
(android_power_monitor_controller.AndroidPowerMonitorController.
    _MergePowerResults(dict_one, dict_two))

or something similar to that.

[alexandermont, 2015-11-18 21:31:11]

Line lengths have been fixed.

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/android_platform_backend.py:27: from
telemetry.internal.platform.power_monitor import
android_power_monitor_controller
On 2015/11/18 at 19:33:26, rnephew wrote:
> Same linelength fix as the other import.

Donw.

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/android_platform_backend.py:165:
self._power_monitor =
android_power_monitor_controller.AndroidPowerMonitorController([
On 2015/11/18 at 19:33:26, rnephew wrote:
> self._power_monitor = (
>     android_power_monitor_controller.AndroidPowerMonitorController([
> 
> and just close the parentheses at the end of creation of the
AndroidPowerMonitorController.
> Similar things can be done for the other ones that are too long. More on line
length can be found at:
>
https://engdoc.corp.google.com/eng/doc/devguide/py/styleguide.shtml?cl=head#s...

Done.

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/android_platform_backend.py:168:
android_dumpsys_power_monitor.DumpsysPowerMonitor(self._battery, self,
set_charge=False),
On 2015/11/18 at 19:33:26, rnephew wrote:
> android_dumpsys_power_monitor.DumpsysPowerMonitor(
>              self._battery, self, set_charge=False),

Done.

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py
(right):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor_unittest.py:95:
pm = android_dumpsys_power_monitor.DumpsysPowerMonitor(battery, backend,
set_charge=False)
On 2015/11/18 at 19:33:26, rnephew wrote:
> pm = android_dumpsys_power_monitor.DumpsysPowerMonitor(
>     battery, backend, set_charge=False)

Done.

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller.py
(right):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller.py:14:
class
AndroidPowerMonitorController(android_power_monitor_base.AndroidPowerMonitorBase):
On 2015/11/18 at 19:33:26, rnephew wrote:
> class AndroidPowerMonitorController(
>     android_power_monitor_base.AndroidPowerMonitorBase):

Done.

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller_unittest.py
(right):

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller_unittest.py:8:
from telemetry.internal.platform.power_monitor import
android_power_monitor_controller
On 2015/11/18 at 19:33:26, rnephew wrote:
> IIRC, the correct way to handle long imports is:
> from telemetry.internal.platform.power_monitor import (
>     android_power_monitor_controller)

Done.

https://codereview.chromium.org/1456803002/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_controller_unittest.py:68:
android_power_monitor_controller.AndroidPowerMonitorController._MergePowerResults(
On 2015/11/18 at 19:33:26, rnephew wrote:
> (android_power_monitor_controller.AndroidPowerMonitorController.
>     _MergePowerResults(dict_one, dict_two))
> 
> or something similar to that.

Done.

[nednguyen, 2015-11-19 00:07:27]

Please file a bug & put the bug number in "BUG=..." to keep track of the effort
of landing this patch.

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:44:
self._ChargingOff(self._battery)
Hmh, I thought the idea was we gonna remove these calls since they are already
done in android_power_monitor_controller?

[rnephew (Wrong account), 2015-11-19 00:28:54]

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:44:
self._ChargingOff(self._battery)
On 2015/11/19 00:07:27, nednguyen wrote:
> Hmh, I thought the idea was we gonna remove these calls since they are already
> done in android_power_monitor_controller?

He implemented it in such a way that when you create the monitor you can pass it
set_charge=True/False.
If you pass it true, it will enable/disable charging, if you pass it false it
wont. That way it can be used by the controller and by itself.

[alexandermont, 2015-11-19 01:39:55]

Description was changed from

==========
move set charging to powermonitorcontroller
BUG=
==========

to

==========
move set charging to powermonitorcontroller

BUG=558106
==========

[rnephew (Wrong account), 2015-11-19 15:57:14]

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:16:
def __init__(self, battery, platform_backend, set_charge=True):
Me and Ned talked, and we both agree that these will never be used by something
other than the android_power_monitor_controller so its safe to just remove the
disabling logic from the individual power monitors completely and just have the
controller do it.

[alexandermont, 2015-11-19 17:53:29]

Removed the set_charge parameter.

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py
(right):

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:16:
def __init__(self, battery, platform_backend, set_charge=True):
On 2015/11/19 at 15:57:14, rnephew wrote:
> Me and Ned talked, and we both agree that these will never be used by
something other than the android_power_monitor_controller so its safe to just
remove the disabling logic from the individual power monitors completely and
just have the controller do it.

Done.

https://codereview.chromium.org/1456803002/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_dumpsys_power_monitor.py:44:
self._ChargingOff(self._battery)
On 2015/11/19 at 00:28:54, rnephew wrote:
> On 2015/11/19 00:07:27, nednguyen wrote:
> > Hmh, I thought the idea was we gonna remove these calls since they are
already
> > done in android_power_monitor_controller?
> 
> He implemented it in such a way that when you create the monitor you can pass
it set_charge=True/False.
> If you pass it true, it will enable/disable charging, if you pass it false it
wont. That way it can be used by the controller and by itself.

See above.

[nednguyen, 2015-11-19 18:12:42]

https://codereview.chromium.org/1456803002/diff/60001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py
(right):

https://codereview.chromium.org/1456803002/diff/60001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:39:
battery.SetCharging(False)
These methods also should be moved to android_power_monitor_controller.py?

[alexandermont, 2015-11-19 20:01:35]

https://codereview.chromium.org/1456803002/diff/60001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py
(right):

https://codereview.chromium.org/1456803002/diff/60001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/power_monitor/android_power_monitor_base.py:39:
battery.SetCharging(False)
On 2015/11/19 at 18:12:42, nednguyen wrote:
> These methods also should be moved to android_power_monitor_controller.py?

Done.

[nednguyen, 2015-11-19 20:05:33]

lgtm

[rnephew (Wrong account), 2015-11-20 22:09:05]

On 2015/11/19 20:05:33, nednguyen wrote:
> lgtm

lgtm

[alexandermont, 2015-11-20 22:27:28]

The CQ bit was checked by alexandermont@chromium.org

[commit-bot: I haz the power, 2015-11-20 22:29:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456803002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456803002/80001

[commit-bot: I haz the power, 2015-11-20 22:33:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-20 22:33:14]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexandermont, 2015-11-21 01:05:27]

The CQ bit was checked by alexandermont@chromium.org

[alexandermont, 2015-11-21 01:05:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, rnephew@google.com
Link to the patchset: https://codereview.chromium.org/1456803002/#ps100001
(title: "move setcharging to powermonitorcontroller")

[commit-bot: I haz the power, 2015-11-21 01:07:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456803002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456803002/100001

[commit-bot: I haz the power, 2015-11-21 02:38:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-21 02:38:29]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[nednguyen, 2015-11-21 02:51:37]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2015-11-21 02:52:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1456803002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1456803002/100001

[commit-bot: I haz the power, 2015-11-21 03:21:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-21 03:22:40]

Patchset 6 (id:??) landed as
https://crrev.com/699b68b3f14abf7957bbba3e130b88317f6f2646
Cr-Commit-Position: refs/heads/master@{#360994}