Refactoring scrolling_benchmark code into scrolling_interaction.py

Refactoring scrolling_benchmark code into scrolling_interaction.py

This is the "separate patch: mentioned in http://codereview.chromium.org/11366197/ to move scrolling logic into scrolling_interaction.py.

BUG=160149
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168610

[hartmanng, 2012-11-15 16:37:53]

Please take a look

[marja, 2012-11-15 17:24:55]

Wouldn't it be nice if the json defined the scrolling actions for the pages for
which they are applicable, in a way similar to stress_memory? And then all the
scrolling-related parameters could go inside those interactions...

Like:
"scroll_test": {
   "action": "scroll_interaction",
   "element_function": "function(callback) { ... }",
   "is_infinite": true
}

The scroll test would then be very similar to the memory_benchmark.py, so, the
interaction would be executed automagically by the runner, and the only thing
left is to take the measurements.

Disclaimer: I'm not sure in which direction you're going with the scrolling
benchmark, so feel free to ignore this comment.

[hartmanng, 2012-11-15 22:09:30]

On 2012/11/15 17:24:55, marja wrote:
> Wouldn't it be nice if the json defined the scrolling actions for the pages
for
> which they are applicable, in a way similar to stress_memory? And then all the
> scrolling-related parameters could go inside those interactions...
> 
> Like:
> "scroll_test": {
>    "action": "scroll_interaction",
>    "element_function": "function(callback) { ... }",
>    "is_infinite": true
> }
> 
> The scroll test would then be very similar to the memory_benchmark.py, so, the
> interaction would be executed automagically by the runner, and the only thing
> left is to take the measurements.
> 
> Disclaimer: I'm not sure in which direction you're going with the scrolling
> benchmark, so feel free to ignore this comment.

I like this idea, I've put up a patch to implement it. Let me know what you
think

[marja, 2012-11-15 22:16:03]

the generic structure lg, i'll have a closer look tomorrow.

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/scrolling_interaction.py (right):

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/scrolling_interaction.py:10: class
DidNotScrollException(multi_page_benchmark.MeasurementFailure):
Should this subclass the "interaction failed" exception instead?

[marja, 2012-11-16 12:40:33]

LGTM modulo the comment from my previous message (exception base class), and
this minor design question.

Please wait for nduca@'s review before committing.

https://codereview.chromium.org/11316017/diff/2002/tools/perf/page_sets/top_2...
File tools/perf/page_sets/top_25.json (right):

https://codereview.chromium.org/11316017/diff/2002/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:18: "scrolling": {
I wonder why this code review tool shows so much whitespace here (filed
crbug.com/161460). The file is fine.

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/scrolling_interaction.py (right):

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/scrolling_interaction.py:61: page.scroll_results =
rendering_stats_deltas
So, this is for transmitting the information to MeasurePage, right? Idk what
would be the most elegant way to do this.

Should MeasurePage do this

rendering_stats_deltas = tab.runtime.Evaluate(
         'window.__renderingStatsDeltas')

What are the pros and cons? I don't really have a strong opionion here. nduca@?

[marja, 2012-11-16 13:28:34]

One more comment:

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page.py (right):

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page.py:17: self.interactions = 'scroll'
Can this be removed, too? What is this for in the current state after this CL?

[hartmanng, 2012-11-16 16:19:23]

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page.py (right):

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page.py:17: self.interactions = 'scroll'
On 2012/11/16 13:28:34, marja wrote:
> Can this be removed, too? What is this for in the current state after this CL?

Done.

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/scrolling_interaction.py (right):

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/scrolling_interaction.py:10: class
DidNotScrollException(multi_page_benchmark.MeasurementFailure):
On 2012/11/15 22:16:03, marja wrote:
> Should this subclass the "interaction failed" exception instead?

Yep, you're right. However, if we're going to move
tab.runtime.Evaluate('window.__renderingStatsDeltas'), I feel like the
DidNotScrollException should go along with it, so for now I've moved it back to
scrolling_benchmark.py (in which case I think the MeasurementFailure is more
appropriate)

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/scrolling_interaction.py:61: page.scroll_results =
rendering_stats_deltas
On 2012/11/16 12:40:33, marja wrote:
> So, this is for transmitting the information to MeasurePage, right? Idk what
> would be the most elegant way to do this.
> 
> Should MeasurePage do this
> 
> rendering_stats_deltas = tab.runtime.Evaluate(
>          'window.__renderingStatsDeltas')
> 
> What are the pros and cons? I don't really have a strong opionion here.
nduca@?

Thanks for the suggestion, I like this better, this way we can keep it out of
page.py. I've changed it tentatively, but I'm interested in what nduca thinks.

[marja, 2012-11-19 09:47:09]

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/scrolling_interaction.py (right):

https://codereview.chromium.org/11316017/diff/2002/tools/telemetry/telemetry/...
tools/telemetry/telemetry/scrolling_interaction.py:10: class
DidNotScrollException(multi_page_benchmark.MeasurementFailure):
On 2012/11/16 16:19:24, hartmanng wrote:
> On 2012/11/15 22:16:03, marja wrote:
> > Should this subclass the "interaction failed" exception instead?
> 
> Yep, you're right. However, if we're going to move
> tab.runtime.Evaluate('window.__renderingStatsDeltas'), I feel like the
> DidNotScrollException should go along with it, so for now I've moved it back
to
> scrolling_benchmark.py (in which case I think the MeasurementFailure is more
> appropriate)

Yes, the right super class of this exception depends on who throws it, as you
said.

[nduca, 2012-11-19 18:08:03]

http://codereview.chromium.org/11316017/diff/12007/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/scrolling_interaction.py (right):

http://codereview.chromium.org/11316017/diff/12007/tools/telemetry/telemetry/...
tools/telemetry/telemetry/scrolling_interaction.py:12: 
why'd you not move scroll.js?

[hartmanng, 2012-11-19 18:16:25]

https://codereview.chromium.org/11316017/diff/12007/tools/telemetry/telemetry...
File tools/telemetry/telemetry/scrolling_interaction.py (right):

https://codereview.chromium.org/11316017/diff/12007/tools/telemetry/telemetry...
tools/telemetry/telemetry/scrolling_interaction.py:12: 
On 2012/11/19 18:08:03, nduca wrote:
> why'd you not move scroll.js?

I... don't know.

done.

[nduca, 2012-11-19 19:48:54]

lgtm

[commit-bot: I haz the power, 2012-11-19 19:50:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11316017/14001

[commit-bot: I haz the power, 2012-11-19 21:37:04]

Change committed as 168610