[Telemetry] Refactor mac power monitoring to use the base PowerMonitor class.

[Telemetry] Refactor mac power monitoring to use the base PowerMonitor class.

R=jeremy@chromium.org,tonyg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252042

[qsr, 2014-02-13 17:40:55]

Tony, Jeremy, is there any try bot that can check this change? I do not have
access to a Mac, so some of this is done a little bit too blindly for my taste.

[tonyg, 2014-02-13 17:53:10]

The mac_rel trybot runs telemetry_unittests. I've checked the box on this patch.

This change lg2m, but jeremy should provide the real review (at which point I'll
rubber stamp).

https://codereview.chromium.org/164133002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):

https://codereview.chromium.org/164133002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:31:
self.MAVERICKS =    platform_backend.OSVersion('mavericks',    10.9)
If it's all the same, I'd prefer to leave these constants where they were. You
can still access them in another file via module_name.CONSTANT_NAME.

[qsr, 2014-02-14 10:53:44]

https://codereview.chromium.org/164133002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):

https://codereview.chromium.org/164133002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/mac_platform_backend.py:31:
self.MAVERICKS =    platform_backend.OSVersion('mavericks',    10.9)
On 2014/02/13 17:53:10, tonyg wrote:
> If it's all the same, I'd prefer to leave these constants where they were. You
> can still access them in another file via module_name.CONSTANT_NAME.

 Having this a the module level creates issues with recursive module dependency.
I solved this by importing module inside the platform factory function. If
that's not allowed, I do not know how to solve the issue.

[tonyg, 2014-02-14 14:53:27]

On 2014/02/14 10:53:44, qsr wrote:
>
https://codereview.chromium.org/164133002/diff/1/tools/telemetry/telemetry/co...
> File tools/telemetry/telemetry/core/platform/mac_platform_backend.py (right):
> 
>
https://codereview.chromium.org/164133002/diff/1/tools/telemetry/telemetry/co...
> tools/telemetry/telemetry/core/platform/mac_platform_backend.py:31:
> self.MAVERICKS =    platform_backend.OSVersion('mavericks',    10.9)
> On 2014/02/13 17:53:10, tonyg wrote:
> > If it's all the same, I'd prefer to leave these constants where they were.
You
> > can still access them in another file via module_name.CONSTANT_NAME.
> 
>  Having this a the module level creates issues with recursive module
dependency.
> I solved this by importing module inside the platform factory function. If
> that's not allowed, I do not know how to solve the issue.

Ah, ok. Ignore me then :)

Jeremy, would you mind reviewing?

[qsr, 2014-02-14 15:52:03]

Not sure to understand what you mean by ignoring you :) Do you want me to revert
the last changes that put the import inside a function? In the end, I think it
is cleaner, because having platform import sub module of platform is a recipe
for disaster as it will trigger other recursive imports...

 Another solution would be to have the factory method in its own .py file...

[tonyg, 2014-02-14 16:14:18]

On 2014/02/14 15:52:03, qsr wrote:
> Not sure to understand what you mean by ignoring you :) Do you want me to
revert
> the last changes that put the import inside a function? In the end, I think it
> is cleaner, because having platform import sub module of platform is a recipe
> for disaster as it will trigger other recursive imports...
> 
>  Another solution would be to have the factory method in its own .py file...

Oh, I meant to ignore my initial comment and go back to your initial approach of
moving them into the class. We'd really like to avoid imports that aren't at the
top of the file.

[qsr, 2014-02-14 16:27:10]

On 2014/02/14 16:14:18, tonyg wrote:
> On 2014/02/14 15:52:03, qsr wrote:
> > Not sure to understand what you mean by ignoring you :) Do you want me to
> revert
> > the last changes that put the import inside a function? In the end, I think
it
> > is cleaner, because having platform import sub module of platform is a
recipe
> > for disaster as it will trigger other recursive imports...
> > 
> >  Another solution would be to have the factory method in its own .py file...
> 
> Oh, I meant to ignore my initial comment and go back to your initial approach
of
> moving them into the class. We'd really like to avoid imports that aren't at
the
> top of the file.

 What about moving the factory methdo to its own file?

[jeremy, 2014-02-16 12:57:10]

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:239: from
telemetry.core.platform import win_platform_backend
nit: newline between imports and code.

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor.py
(right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor.py:1:
# Copyright 2014 The Chromium Authors. All rights reserved.
It's a bit unfortunate that there's no diff between this and the original code
in mac_platform_backend...

Is this just a copy/paste of the function bodies or did you make any functional
changes I should look at?

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py
(right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py:16:
def testCanCreateMonitor(self):
@test.Enabled('mac') ?

[qsr, 2014-02-17 12:38:05]

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:239: from
telemetry.core.platform import win_platform_backend
On 2014/02/16 12:57:11, jeremy wrote:
> nit: newline between imports and code.

 Refactored for not needing those imports.

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor.py
(right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor.py:1:
# Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/02/16 12:57:11, jeremy wrote:
> It's a bit unfortunate that there's no diff between this and the original code
> in mac_platform_backend...
> 
> Is this just a copy/paste of the function bodies or did you make any
functional
> changes I should look at?

 There is no functional change here.

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py
(right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py:16:
def testCanCreateMonitor(self):
On 2014/02/16 12:57:11, jeremy wrote:
> @test.Enabled('mac') ?

 Nope, I wrote this test especially because if was working even on non mac, to
check that my code was correct with respect to import etc.

 If you really don't like it, I can remove it altogether, it doesn't given
anything useful on a mac platform compared to the following tests.

[qsr, 2014-02-17 12:38:05]

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:239: from
telemetry.core.platform import win_platform_backend
On 2014/02/16 12:57:11, jeremy wrote:
> nit: newline between imports and code.

 Refactored for not needing those imports.

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor.py
(right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor.py:1:
# Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/02/16 12:57:11, jeremy wrote:
> It's a bit unfortunate that there's no diff between this and the original code
> in mac_platform_backend...
> 
> Is this just a copy/paste of the function bodies or did you make any
functional
> changes I should look at?

 There is no functional change here.

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py
(right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py:16:
def testCanCreateMonitor(self):
On 2014/02/16 12:57:11, jeremy wrote:
> @test.Enabled('mac') ?

 Nope, I wrote this test especially because if was working even on non mac, to
check that my code was correct with respect to import etc.

 If you really don't like it, I can remove it altogether, it doesn't given
anything useful on a mac platform compared to the following tests.

[jeremy, 2014-02-18 09:24:15]

lgtm++ with unit test removal

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py
(right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py:16:
def testCanCreateMonitor(self):
Getting PowerMetrics to work on non-Mac platforms is a non-goal, so I'd prefer
that you remove this test :)

[qsr, 2014-02-18 09:28:02]

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py
(right):

https://codereview.chromium.org/164133002/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/power_monitor/powermetrics_power_monitor_unittest.py:16:
def testCanCreateMonitor(self):
On 2014/02/18 09:24:16, jeremy wrote:
> Getting PowerMetrics to work on non-Mac platforms is a non-goal, so I'd prefer
> that you remove this test :)

 The aim was not to make it works, just making sure it ``compiles'' on a dynamic
language :) But no problem, I'll remove.

[qsr, 2014-02-18 09:28:07]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-02-18 09:28:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/164133002/490001

[commit-bot: I haz the power, 2014-02-18 09:32:20]

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

[commit-bot: I haz the power, 2014-02-18 09:32:20]

Try jobs failed on following builders: linux_chromium_rel

[qsr, 2014-02-18 09:59:08]

On 2014/02/18 09:32:20, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders: linux_chromium_rel

 Hum, tony -> missed that you didn't lgtm it. Can you look at it?

[tonyg, 2014-02-19 02:33:57]

lgtm

https://codereview.chromium.org/164133002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/factory.py (right):

https://codereview.chromium.org/164133002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/factory.py:11: def
CreatePlatformBackendForCurrentOS():
Should we add @decorators.Cache to this method and rename it
GetPlatformBackendForCurrentOS()?

[qsr, 2014-02-19 08:43:22]

https://codereview.chromium.org/164133002/diff/490001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/factory.py (right):

https://codereview.chromium.org/164133002/diff/490001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/factory.py:11: def
CreatePlatformBackendForCurrentOS():
On 2014/02/19 02:33:57, tonyg wrote:
> Should we add @decorators.Cache to this method and rename it
> GetPlatformBackendForCurrentOS()?

Done.

[qsr, 2014-02-19 08:43:58]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-02-19 08:44:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/164133002/640001

[commit-bot: I haz the power, 2014-02-19 14:50:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/164133002/640001

[commit-bot: I haz the power, 2014-02-19 15:26:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qsr@chromium.org/164133002/640001

[commit-bot: I haz the power, 2014-02-19 19:34:22]

Change committed as 252042