Add key_idle_power_cases for ensuring idle activity on Android

Add key_idle_power_cases for ensuring idle activity on Android

Add a key_idle_power_cases pageset for monitoring activity for static
pages in the foreground and dynamic pages in the background. All such
cases should incur minimal recurring activity, monitored using a
new thread_times.key_power_cases benchmark.

BUG=466213, 470147
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect

Committed: https://crrev.com/040db4f47768b171ba2feced77bdedac341c5644
Cr-Commit-Position: refs/heads/master@{#330132}

[jdduke (slow), 2015-04-23 18:09:56]

jdduke@chromium.org changed reviewers:
+ rnephew@chromium.org

[jdduke (slow), 2015-04-23 18:09:57]

Randy would you mind taking a look at this before I send out for further review?

The screen on/off behavior probably needs some work. I was wondering if I should
make a new PageTest instance (derived from ThreadTimes) that would cleanup the
screen state, but I'm not really familiar enough with telemetry to know what the
best/canonical way to achieve something like this would be.

[rnephew (Reviews Here), 2015-04-23 18:28:05]

I'm also not familiar enough to know the preferred way of doing this, but I
agree the current way does feel wrong. Mostly just nits, and discussion on the
off/on behavior/location.

https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline.py
File tools/perf/metrics/timeline.py (right):

https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
tools/perf/metrics/timeline.py:214: self_time_result = \
nit: use (...) instead of \

https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
tools/perf/metrics/timeline.py:221: idle_time_result = \
Same

https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
tools/perf/metrics/timeline.py:264: model, [r.GetBounds() for r in
interaction_records], name,
nit: I know it was there before but it's under indented, should be 4.

https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_power_cases.py (right):

https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:27: 
Should the turn screen off/on step be performed here, since its an 'interaction'
with the page instead of a navigation? Or does the measurements require it to
happen before it runs the interactions? Either way, I think it should restore
screen state before it goes on to the next page in the test. This current
implementation has it turning the screen back on as the first step in
RunNavigateSteps.

https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:39: 'file://key_power_cases/blank.html',
I dont know enough about telemetry to know if it should use local files or not.
I know it typically uses WPR.

[jdduke (slow), 2015-04-24 16:10:52]

jdduke@chromium.org changed reviewers:
+ nednguyen@google.com

[jdduke (slow), 2015-04-24 16:10:52]

Hey Ned, maybe you have some thoughts on how best to structure this.

We'd like to use the existing ThreadTimes metric/measurement, but for certain
pages we'd like to run actions both before AND after the measurement is run. In
particular, for a subset of pages from the new pageset, we need to 1) make sure
the screen is on, 2) turn the screen off, 3) run the measurement, 4) restore the
screen state.

Most of this isn't a problem, but right now there's no good way for an
individual page to run a "cleanup" action. I considered adding a |RunCleanup|
step to each page. Another thought was to move the screen management up to the
PageTest level, so we'd basically subclass ThreadTimes and have that turn the
screen on/off, but it gets a little nasty when only a subset of the pageset
needs this to happen. I'm sure there's a nice structure in here somehow,
thoughts?

https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline.py
File tools/perf/metrics/timeline.py (right):

https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
tools/perf/metrics/timeline.py:214: self_time_result = \
On 2015/04/23 18:28:05, rnephew1 wrote:
> nit: use (...) instead of \

Done.

https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
tools/perf/metrics/timeline.py:221: idle_time_result = \
On 2015/04/23 18:28:05, rnephew1 wrote:
> Same

Done.

https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
tools/perf/metrics/timeline.py:264: model, [r.GetBounds() for r in
interaction_records], name,
On 2015/04/23 18:28:05, rnephew1 wrote:
> nit: I know it was there before but it's under indented, should be 4.

Done.

https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_power_cases.py (right):

https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:27: 
On 2015/04/23 18:28:05, rnephew1 wrote:
> Should the turn screen off/on step be performed here, since its an
'interaction'
> with the page instead of a navigation? Or does the measurements require it to
> happen before it runs the interactions? Either way, I think it should restore
> screen state before it goes on to the next page in the test. This current
> implementation has it turning the screen back on as the first step in
> RunNavigateSteps. 

Right, we don't want the "interaction" to include only the time where the screen
is actually off.

Unfortunately there doesn't appear to be a good way for a given page to perform
a cleanup step. I started down the path of adding such a method, but it got
messy rather quickly. This made me think perhaps the screen on/off logic should
be done at a higher level (e.g., by the measurement class), but that also gets a
bit messy.

The other question I had was about lockscreen state. We want to make sure Chrome
is in the foreground before we turn the screen off, so do we also need to make
sure the screen is both on AND unlocked? How does our current power tests handle
this? Do we explicitly unlock the screen and turn it on? Or are those done with
the screen off (in which case I wonder if this is impacting how faithful the
power interaction metrics are).

https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:39: 'file://key_power_cases/blank.html',
On 2015/04/23 18:28:05, rnephew1 wrote:
> I dont know enough about telemetry to know if it should use local files or
not.
> I know it typically uses WPR.

Typically, yes, but these are (intentionally) synthetic examples. Other pagesets
also rely on local files, e.g., key_silk_cases.

[nednguyen, 2015-04-24 16:22:45]

On 2015/04/24 16:10:52, jdduke wrote:
> Hey Ned, maybe you have some thoughts on how best to structure this.
> 
> We'd like to use the existing ThreadTimes metric/measurement, but for certain
> pages we'd like to run actions both before AND after the measurement is run.
In
> particular, for a subset of pages from the new pageset, we need to 1) make
sure
> the screen is on, 2) turn the screen off, 3) run the measurement, 4) restore
the
> screen state.
> 
> Most of this isn't a problem, but right now there's no good way for an
> individual page to run a "cleanup" action. I considered adding a |RunCleanup|
> step to each page. Another thought was to move the screen management up to the
> PageTest level, so we'd basically subclass ThreadTimes and have that turn the
> screen on/off, but it gets a little nasty when only a subset of the pageset
> needs this to happen. I'm sure there's a nice structure in here somehow,
> thoughts?

How about creating your own Page subclass that does this:

def MyPowerTestCasePage(page.Page):

   def RunPageInteractions(self):
      self.PrepareScreen()  
      try:
          self.PerformPageInteractions()
      finally:
          self.CleanUp()
   
   def CleanUp():
      ...

   def PrepareScreen():
      ...

   def PerformPageInteractions():
     raise NotImplementedError

> 
>
https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline.py
> File tools/perf/metrics/timeline.py (right):
> 
>
https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
> tools/perf/metrics/timeline.py:214: self_time_result = \
> On 2015/04/23 18:28:05, rnephew1 wrote:
> > nit: use (...) instead of \
> 
> Done.
> 
>
https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
> tools/perf/metrics/timeline.py:221: idle_time_result = \
> On 2015/04/23 18:28:05, rnephew1 wrote:
> > Same
> 
> Done.
> 
>
https://codereview.chromium.org/1097463004/diff/1/tools/perf/metrics/timeline...
> tools/perf/metrics/timeline.py:264: model, [r.GetBounds() for r in
> interaction_records], name,
> On 2015/04/23 18:28:05, rnephew1 wrote:
> > nit: I know it was there before but it's under indented, should be 4.
> 
> Done.
> 
>
https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
> File tools/perf/page_sets/key_power_cases.py (right):
> 
>
https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
> tools/perf/page_sets/key_power_cases.py:27: 
> On 2015/04/23 18:28:05, rnephew1 wrote:
> > Should the turn screen off/on step be performed here, since its an
> 'interaction'
> > with the page instead of a navigation? Or does the measurements require it
to
> > happen before it runs the interactions? Either way, I think it should
restore
> > screen state before it goes on to the next page in the test. This current
> > implementation has it turning the screen back on as the first step in
> > RunNavigateSteps. 
> 
> Right, we don't want the "interaction" to include only the time where the
screen
> is actually off.
> 
> Unfortunately there doesn't appear to be a good way for a given page to
perform
> a cleanup step. I started down the path of adding such a method, but it got
> messy rather quickly. This made me think perhaps the screen on/off logic
should
> be done at a higher level (e.g., by the measurement class), but that also gets
a
> bit messy.
> 
> The other question I had was about lockscreen state. We want to make sure
Chrome
> is in the foreground before we turn the screen off, so do we also need to make
> sure the screen is both on AND unlocked? How does our current power tests
handle
> this? Do we explicitly unlock the screen and turn it on? Or are those done
with
> the screen off (in which case I wonder if this is impacting how faithful the
> power interaction metrics are).
> 
>
https://codereview.chromium.org/1097463004/diff/20001/tools/perf/page_sets/ke...
> tools/perf/page_sets/key_power_cases.py:39:
'file://key_power_cases/blank.html',
> On 2015/04/23 18:28:05, rnephew1 wrote:
> > I dont know enough about telemetry to know if it should use local files or
> not.
> > I know it typically uses WPR.
> 
> Typically, yes, but these are (intentionally) synthetic examples. Other
pagesets
> also rely on local files, e.g., key_silk_cases.

[jdduke (slow), 2015-04-24 16:26:25]

On 2015/04/24 16:22:45, nednguyen wrote:
> How about creating your own Page subclass that does this:
> 
> def MyPowerTestCasePage(page.Page):
> 
>    def RunPageInteractions(self):
>       self.PrepareScreen()  
>       try:
>           self.PerformPageInteractions()
>       finally:
>           self.CleanUp()
>    
>    def CleanUp():
>       ...
> 
>    def PrepareScreen():
>       ...
> 
>    def PerformPageInteractions():
>      raise NotImplementedError

The problem is that everything within |RunPageInteractions| gets pulled into the
ThreadTimes metric. We don't want the turning on of the screen to be included
there, as it will impact the "idleness" that we're trying to measure.

[nednguyen, 2015-04-24 16:31:36]

On 2015/04/24 16:26:25, jdduke wrote:
> On 2015/04/24 16:22:45, nednguyen wrote:
> > How about creating your own Page subclass that does this:
> > 
> > def MyPowerTestCasePage(page.Page):
> > 
> >    def RunPageInteractions(self):
> >       self.PrepareScreen()  
> >       try:
> >           self.PerformPageInteractions()
> >       finally:
> >           self.CleanUp()
> >    
> >    def CleanUp():
> >       ...
> > 
> >    def PrepareScreen():
> >       ...
> > 
> >    def PerformPageInteractions():
> >      raise NotImplementedError
> 
> The problem is that everything within |RunPageInteractions| gets pulled into
the
> ThreadTimes metric. We don't want the turning on of the screen to be included
> there, as it will impact the "idleness" that we're trying to measure.

I see, then subclassing SharedPageState may be what you are looking for. You can
look at the documentation of SharedUserStoryState
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

[rnephew (Reviews Here), 2015-04-24 17:10:03]

> Right, we don't want the "interaction" to include only the time where the
screen
> is actually off.
> 
> Unfortunately there doesn't appear to be a good way for a given page to
perform
> a cleanup step. I started down the path of adding such a method, but it got
> messy rather quickly. This made me think perhaps the screen on/off logic
should
> be done at a higher level (e.g., by the measurement class), but that also gets
a
> bit messy.
> 
> The other question I had was about lockscreen state. We want to make sure
Chrome
> is in the foreground before we turn the screen off, so do we also need to make
> sure the screen is both on AND unlocked? How does our current power tests
handle
> this? Do we explicitly unlock the screen and turn it on? Or are those done
with
> the screen off (in which case I wonder if this is impacting how faithful the
> power interaction metrics are).
> 

Currently no telemetry tests use the ability to turn the screen off, so the
current power tests are always run with the screen on with chrome in the
foreground.

[rnephew (Reviews Here), 2015-04-24 19:49:30]

https://codereview.chromium.org/1097463004/diff/60001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_power_cases.py (right):

https://codereview.chromium.org/1097463004/diff/60001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:16:
shared_page_state_class=(android_screen_restoration_shared_state\
\ not needed do to implicit line joining inside ()'s

[jdduke (slow), 2015-04-24 22:01:23]

Randy, do we need to worry about perf bots having screen lock? Are those
disabled for test perf bots?

Ned, is this about what you had in mind? Mind taking a look for owner review?

https://codereview.chromium.org/1097463004/diff/60001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_power_cases.py (right):

https://codereview.chromium.org/1097463004/diff/60001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:16:
shared_page_state_class=(android_screen_restoration_shared_state\
On 2015/04/24 19:49:29, rnephew1 wrote:
> \ not needed do to implicit line joining inside ()'s

Done.

[jbudorick, 2015-04-24 23:32:49]

On 2015/04/24 22:01:23, jdduke wrote:
> Randy, do we need to worry about perf bots having screen lock? Are those
> disabled for test perf bots?

build/android/provision_devices.py will attempt to disable the screen lock on
userdebug builds. IIRC the perf bots are all on userdebug builds, so you should
be ok.

> 
> Ned, is this about what you had in mind? Mind taking a look for owner review?
> 
>
https://codereview.chromium.org/1097463004/diff/60001/tools/perf/page_sets/ke...
> File tools/perf/page_sets/key_power_cases.py (right):
> 
>
https://codereview.chromium.org/1097463004/diff/60001/tools/perf/page_sets/ke...
> tools/perf/page_sets/key_power_cases.py:16:
> shared_page_state_class=(android_screen_restoration_shared_state\
> On 2015/04/24 19:49:29, rnephew1 wrote:
> > \ not needed do to implicit line joining inside ()'s
> 
> Done.

[nednguyen, 2015-04-27 17:47:49]

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/an...
File tools/perf/page_sets/android_screen_restoration_shared_state.py (right):

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/an...
tools/perf/page_sets/android_screen_restoration_shared_state.py:13: 
You will want to override GetTestExpectationAndSkipValue() to sepcify the logic
that this shared state should be skipped on platform other than android.

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_power_cases.py (right):

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:25:
action_runner._tab.browser.platform.android_action_runner.TurnScreenOff()
This is an API violation. I am working enable a single Run method that allow you
to have direct access to your shared state instance, and pull out the
android_action_runner.

https://code.google.com/p/chromium/issues/detail?id=470147

[jdduke (slow), 2015-04-28 21:30:29]

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/an...
File tools/perf/page_sets/android_screen_restoration_shared_state.py (right):

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/an...
tools/perf/page_sets/android_screen_restoration_shared_state.py:13: 
On 2015/04/27 17:47:49, nednguyen wrote:
> You will want to override GetTestExpectationAndSkipValue() to sepcify the
logic
> that this shared state should be skipped on platform other than android.

Done. (overrode CanRunOnBrowser).

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_power_cases.py (right):

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:25:
action_runner._tab.browser.platform.android_action_runner.TurnScreenOff()
On 2015/04/27 17:47:49, nednguyen wrote:
> This is an API violation. I am working enable a single Run method that allow
you
> to have direct access to your shared state instance, and pull out the
> android_action_runner.
> 
> https://code.google.com/p/chromium/issues/detail?id=470147

OK, how should we proceed? Wait on that?

[nednguyen, 2015-04-28 23:03:24]

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/ke...
File tools/perf/page_sets/key_power_cases.py (right):

https://codereview.chromium.org/1097463004/diff/80001/tools/perf/page_sets/ke...
tools/perf/page_sets/key_power_cases.py:25:
action_runner._tab.browser.platform.android_action_runner.TurnScreenOff()
On 2015/04/28 21:30:29, jdduke wrote:
> On 2015/04/27 17:47:49, nednguyen wrote:
> > This is an API violation. I am working enable a single Run method that allow
> you
> > to have direct access to your shared state instance, and pull out the
> > android_action_runner.
> > 
> > https://code.google.com/p/chromium/issues/detail?id=470147
> 
> OK, how should we proceed? Wait on that?

Yes. Or you can go ahead with this change and add a TODO in the code to clean
this up & update crbug/470147 with your change.

[jdduke (slow), 2015-04-28 23:14:58]

On 2015/04/28 23:03:24, nednguyen wrote:
> Yes. Or you can go ahead with this change and add a TODO in the code to clean
> this up & update crbug/470147 with your change.

Sounds good, added a TODO with a link to the bug.

[nednguyen, 2015-04-29 16:25:45]

On 2015/04/28 23:14:58, jdduke wrote:
> On 2015/04/28 23:03:24, nednguyen wrote:
> > Yes. Or you can go ahead with this change and add a TODO in the code to
clean
> > this up & update crbug/470147 with your change.
> 
> Sounds good, added a TODO with a link to the bug.

This patch is a bit too much to review. Can you move the changes to timeline &
thread_times to a separate CL? I would also want to see unittest for the new
functionality change.

[jdduke (slow), 2015-04-29 17:19:09]

On 2015/04/29 16:25:45, nednguyen wrote:
> On 2015/04/28 23:14:58, jdduke wrote:
> > On 2015/04/28 23:03:24, nednguyen wrote:
> > > Yes. Or you can go ahead with this change and add a TODO in the code to
> clean
> > > this up & update crbug/470147 with your change.
> > 
> > Sounds good, added a TODO with a link to the bug.
> 
> This patch is a bit too much to review. Can you move the changes to timeline &
> thread_times to a separate CL? I would also want to see unittest for the new
> functionality change.

Sure, I'll split it out and add some test coverage.

When trying to run the tests locally (tools/perf/run_tests
LoadTimesTimelineMetric.testMeasureTotalThreadTime), I'm seeing:

Failed to import test module: internal.gws.page_sets.page_set_unittest
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests
    module = self._get_module_from_name(name)
  File "/usr/lib/python2.7/unittest/loader.py", line 232, in
_get_module_from_name
    __import__(name)
  File
"/usr/local/google/code/clankium-master/src/tools/perf/internal/gws/page_sets/page_set_unittest.py",
line 7, in <module>
    from telemetry.unittest import page_set_smoke_test
ImportError: No module named unittest

Look at all familiar?

[jdduke (slow), 2015-04-29 17:33:35]

On 2015/04/29 17:19:09, jdduke wrote:
> On 2015/04/29 16:25:45, nednguyen wrote:
> > On 2015/04/28 23:14:58, jdduke wrote:
> > > On 2015/04/28 23:03:24, nednguyen wrote:
> > > > Yes. Or you can go ahead with this change and add a TODO in the code to
> > clean
> > > > this up & update crbug/470147 with your change.
> > > 
> > > Sounds good, added a TODO with a link to the bug.
> > 
> > This patch is a bit too much to review. Can you move the changes to timeline
&
> > thread_times to a separate CL? I would also want to see unittest for the new
> > functionality change.
> 
> Sure, I'll split it out and add some test coverage.
> 
> When trying to run the tests locally (tools/perf/run_tests
> LoadTimesTimelineMetric.testMeasureTotalThreadTime), I'm seeing:
> 
> Failed to import test module: internal.gws.page_sets.page_set_unittest
> Traceback (most recent call last):
>   File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests
>     module = self._get_module_from_name(name)
>   File "/usr/lib/python2.7/unittest/loader.py", line 232, in
> _get_module_from_name
>     __import__(name)
>   File
>
"/usr/local/google/code/clankium-master/src/tools/perf/internal/gws/page_sets/page_set_unittest.py",
> line 7, in <module>
>     from telemetry.unittest import page_set_smoke_test
> ImportError: No module named unittest
> 
> Look at all familiar?

Weird, I went ahead and deleted the tools/perf/internal directory and
everything's peachy now.

[nednguyen, 2015-04-29 18:15:29]

On 2015/04/29 17:33:35, jdduke wrote:
> On 2015/04/29 17:19:09, jdduke wrote:
> > On 2015/04/29 16:25:45, nednguyen wrote:
> > > On 2015/04/28 23:14:58, jdduke wrote:
> > > > On 2015/04/28 23:03:24, nednguyen wrote:
> > > > > Yes. Or you can go ahead with this change and add a TODO in the code
to
> > > clean
> > > > > this up & update crbug/470147 with your change.
> > > > 
> > > > Sounds good, added a TODO with a link to the bug.
> > > 
> > > This patch is a bit too much to review. Can you move the changes to
timeline
> &
> > > thread_times to a separate CL? I would also want to see unittest for the
new
> > > functionality change.
> > 
> > Sure, I'll split it out and add some test coverage.
> > 
> > When trying to run the tests locally (tools/perf/run_tests
> > LoadTimesTimelineMetric.testMeasureTotalThreadTime), I'm seeing:
> > 
> > Failed to import test module: internal.gws.page_sets.page_set_unittest
> > Traceback (most recent call last):
> >   File "/usr/lib/python2.7/unittest/loader.py", line 254, in _find_tests
> >     module = self._get_module_from_name(name)
> >   File "/usr/lib/python2.7/unittest/loader.py", line 232, in
> > _get_module_from_name
> >     __import__(name)
> >   File
> >
>
"/usr/local/google/code/clankium-master/src/tools/perf/internal/gws/page_sets/page_set_unittest.py",
> > line 7, in <module>
> >     from telemetry.unittest import page_set_smoke_test
> > ImportError: No module named unittest
> > 
> > Look at all familiar?
> 
> Weird, I went ahead and deleted the tools/perf/internal directory and
> everything's peachy now.

Yeah, we have a bug opened for cleaning up the perf/internal here:
https://code.google.com/p/chromium/issues/detail?id=480686

[jdduke (slow), 2015-05-12 16:26:25]

I've gone ahead and rebased the change to the recently landed timeline metric
change, and I also renamed the benchmark/pageset to "key_idle_power_cases".

[nednguyen, 2015-05-15 00:43:25]

lgtm with minor question

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/platform/android_action_runner.py (right):

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/platform/android_action_runner.py:130:
util.WaitFor(self._platform_backend.IsScreenOn, 5)
Why do we need this wait?

[jdduke (slow), 2015-05-15 15:11:09]

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/platform/android_action_runner.py (right):

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/platform/android_action_runner.py:130:
util.WaitFor(self._platform_backend.IsScreenOn, 5)
On 2015/05/15 00:43:24, nednguyen (slow review) wrote:
> Why do we need this wait?

You'd have to ask rnephew@ (I used the same timeout he has below). I expect we
want to make sure the screen is actually on before proceeding, and there can be
a slight delay?

[nednguyen, 2015-05-15 15:18:47]

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/platform/android_action_runner.py (right):

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/platform/android_action_runner.py:130:
util.WaitFor(self._platform_backend.IsScreenOn, 5)
On 2015/05/15 15:11:09, jdduke wrote:
> On 2015/05/15 00:43:24, nednguyen (slow review) wrote:
> > Why do we need this wait?
> 
> You'd have to ask rnephew@ (I used the same timeout he has below). I expect we
> want to make sure the screen is actually on before proceeding, and there can
be
> a slight delay?

I would want to see s.t like WaitForScreenOn() rather than a random 5s wait like
this. These wait time usually don't work at Chrome scale, and cause test
flakiness.

[rnephew (Reviews Here), 2015-05-15 15:27:13]

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/platform/android_action_runner.py (right):

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/platform/android_action_runner.py:130:
util.WaitFor(self._platform_backend.IsScreenOn, 5)
On 2015/05/15 15:18:47, nednguyen (slow review) wrote:
> On 2015/05/15 15:11:09, jdduke wrote:
> > On 2015/05/15 00:43:24, nednguyen (slow review) wrote:
> > > Why do we need this wait?
> > 
> > You'd have to ask rnephew@ (I used the same timeout he has below). I expect
we
> > want to make sure the screen is actually on before proceeding, and there can
> be
> > a slight delay?
> 
> I would want to see s.t like WaitForScreenOn() rather than a random 5s wait
like
> this. These wait time usually don't work at Chrome scale, and cause test
> flakiness.

Doesn't the WaitFor in telemetry/util wait for either the timeout to occur, or
the return value of the IsScreenOn to return true? Looking at it, it looks like
that's the case; and if it is wouldn't a function like WaitForScreenOn() just
call util.WaitFor()?

[nednguyen, 2015-05-15 15:30:22]

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
File tools/telemetry/telemetry/core/platform/android_action_runner.py (right):

https://codereview.chromium.org/1097463004/diff/160001/tools/telemetry/teleme...
tools/telemetry/telemetry/core/platform/android_action_runner.py:130:
util.WaitFor(self._platform_backend.IsScreenOn, 5)
On 2015/05/15 15:27:13, rnephew1 wrote:
> On 2015/05/15 15:18:47, nednguyen (slow review) wrote:
> > On 2015/05/15 15:11:09, jdduke wrote:
> > > On 2015/05/15 00:43:24, nednguyen (slow review) wrote:
> > > > Why do we need this wait?
> > > 
> > > You'd have to ask rnephew@ (I used the same timeout he has below). I
expect
> we
> > > want to make sure the screen is actually on before proceeding, and there
can
> > be
> > > a slight delay?
> > 
> > I would want to see s.t like WaitForScreenOn() rather than a random 5s wait
> like
> > this. These wait time usually don't work at Chrome scale, and cause test
> > flakiness.
> 
> Doesn't the WaitFor in telemetry/util wait for either the timeout to occur, or
> the return value of the IsScreenOn to return true? Looking at it, it looks
like
> that's the case; and if it is wouldn't a function like WaitForScreenOn() just
> call util.WaitFor()?

Err, didn't read this carefully and thought that this is 5s wait. nvm, please
ignore my silly comment :P

[jdduke (slow), 2015-05-15 15:37:13]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-05-15 15:37:13]

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/1097463004/#ps180001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-15 15:37:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097463004/180001

[commit-bot: I haz the power, 2015-05-15 16:02:28]

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

[commit-bot: I haz the power, 2015-05-15 16:02:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jdduke (slow), 2015-05-15 16:08:19]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-05-15 16:10:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097463004/180001

[commit-bot: I haz the power, 2015-05-15 16:23:05]

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

[commit-bot: I haz the power, 2015-05-15 16:23:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nednguyen, 2015-05-15 16:42:58]

https://codereview.chromium.org/1097463004/diff/180001/tools/perf/page_sets/k...
File tools/perf/page_sets/key_idle_power_cases.py (right):

https://codereview.chromium.org/1097463004/diff/180001/tools/perf/page_sets/k...
tools/perf/page_sets/key_idle_power_cases.py:26: # exposed here,
crbug.com/470147.
Add: # pylint: disable=protected-access

[jdduke (slow), 2015-05-15 16:49:44]

The CQ bit was checked by jdduke@chromium.org

[jdduke (slow), 2015-05-15 16:49:44]

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/1097463004/#ps200001
(title: "lint")

[commit-bot: I haz the power, 2015-05-15 16:51:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1097463004/200001

[commit-bot: I haz the power, 2015-05-15 18:11:28]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-05-15 18:12:13]

Patchset 11 (id:??) landed as
https://crrev.com/040db4f47768b171ba2feced77bdedac341c5644
Cr-Commit-Position: refs/heads/master@{#330132}