Ensure fixed amount of scrolling in infinite scroll stories.

Ensure fixed amount of scrolling in infinite scroll stories.

This patch changes the scrolling logic in the stories such that each
story does the same fixed amount of scrolling in any run.

Apart from that the stories are refactored and unified following the
convention of the system health stories.

PERF SHERIFF NOTE: expect regressions and/or improvements in
- v8.infinite_scroll_tbmv2 (and other variants of this benchmark).
- v8.mobile_infinite_scroll_tbmv2 (and other variants of this benchmark).

BUG=chromium:704124
Review-Url: https://codereview.chromium.org/2773483002
Cr-Commit-Position: refs/heads/master@{#462813}
Committed: https://chromium.googlesource.com/chromium/src/+/95e17ac4602dc38aaa6a44d4f6d2566920d6c75d

[ulan, 2017-03-22 19:48:31]

ulan@chromium.org changed reviewers:
+ hpayer@chromium.org, nednguyen@google.com

[ulan, 2017-03-22 19:48:31]

PTAL

[ulan, 2017-03-22 19:48:45]

Description was changed from

==========
Ensure fixed amount of scrolling in infinite scroll stories.

This patch changes the scrolling logic in the stories such that each
story does the same fixed amount of scrolling in any run.

Apart from the the stories are refactored and unified following the
convention of the system health stories.

PERF SHERIFF NOTE: expect regressions and/or improvements in
- v8.infinite_scroll_tbmv2 (and other variants of this benchmark).
- v8.mobile_infinite_scroll_tbmv2 (and other variants of this benchmark).

BUG=chromium:704124
==========

to

==========
Ensure fixed amount of scrolling in infinite scroll stories.

This patch changes the scrolling logic in the stories such that each
story does the same fixed amount of scrolling in any run.

Apart from that the stories are refactored and unified following the
convention of the system health stories.

PERF SHERIFF NOTE: expect regressions and/or improvements in
- v8.infinite_scroll_tbmv2 (and other variants of this benchmark).
- v8.mobile_infinite_scroll_tbmv2 (and other variants of this benchmark).

BUG=chromium:704124
==========

[nednguyen, 2017-03-24 15:12:13]

On 2017/03/22 19:48:31, ulan wrote:
> PTAL

Can you move the stories refactoring to a different CL to make the CL easier to
review?

[ulan, 2017-03-24 15:55:43]

On 2017/03/24 15:12:13, nednguyen wrote:
> On 2017/03/22 19:48:31, ulan wrote:
> > PTAL
> 
> Can you move the stories refactoring to a different CL to make the CL easier
to
> review?
The scroll logic is isolated in the _Scroll() function. Everything else is
refactoring. I believe the current CL is smallest possible change.

If I do the scroll change first and then refactoring, I will have to do
scrolling change twice (in infinite_scroll_cases.py and in
mobile_infinite_scroll_cases.py).

If I do refactoring first and then the scroll change, then I will have to
preserve the existing two different ways of scrolling (SCROLL_FAR, SCROLL_PAGE)
which will add more fields in each story class.

Which one would you prefer?

[Hannes Payer (out of office), 2017-03-30 11:42:02]

LGTM from my side

[nednguyen, 2017-03-30 13:09:46]

Ooops, I am very sorry for the delay here. Next time if I take more than a day
to reply to your code review request, feel free to ping me directly.

lgtm to unblock the fix

https://codereview.chromium.org/2773483002/diff/1/tools/perf/page_sets/infini...
File tools/perf/page_sets/infinite_scroll_cases.py (right):

https://codereview.chromium.org/2773483002/diff/1/tools/perf/page_sets/infini...
tools/perf/page_sets/infinite_scroll_cases.py:36: window.performance =
undefined;'''
Does this make any of the page's JS fail? Seems a bit scary to me, tbh

https://codereview.chromium.org/2773483002/diff/1/tools/perf/page_sets/infini...
tools/perf/page_sets/infinite_scroll_cases.py:48: with
action_runner.CreateInteraction('Begin'):
not in this CL but these interactions are only needed for the legacy TBM
benchmark. I think we can remove them to make the code cleaner now?

https://codereview.chromium.org/2773483002/diff/1/tools/perf/page_sets/infini...
tools/perf/page_sets/infinite_scroll_cases.py:55: def _Scroll(self,
action_runner, distance, step_size):
this method seems useful & common enough to be an action_runner API, like
action_runner.ScrollUntilExactDistanceReached(..). But feel free to do this in a
different CL

[ulan, 2017-03-30 14:35:09]

Thanks! No worries about delay.

https://codereview.chromium.org/2773483002/diff/1/tools/perf/page_sets/infini...
File tools/perf/page_sets/infinite_scroll_cases.py (right):

https://codereview.chromium.org/2773483002/diff/1/tools/perf/page_sets/infini...
tools/perf/page_sets/infinite_scroll_cases.py:36: window.performance =
undefined;'''
On 2017/03/30 13:09:46, nednguyen wrote:
> Does this make any of the page's JS fail? Seems a bit scary to me, tbh

This part did not change with this CL. At the time of recording there were no
errors.

https://codereview.chromium.org/2773483002/diff/1/tools/perf/page_sets/infini...
tools/perf/page_sets/infinite_scroll_cases.py:48: with
action_runner.CreateInteraction('Begin'):
On 2017/03/30 13:09:46, nednguyen wrote:
> not in this CL but these interactions are only needed for the legacy TBM
> benchmark. I think we can remove them to make the code cleaner now?

Yep, will do in separete CL.

https://codereview.chromium.org/2773483002/diff/1/tools/perf/page_sets/infini...
tools/perf/page_sets/infinite_scroll_cases.py:55: def _Scroll(self,
action_runner, distance, step_size):
On 2017/03/30 13:09:46, nednguyen wrote:
> this method seems useful & common enough to be an action_runner API, like
> action_runner.ScrollUntilExactDistanceReached(..). But feel free to do this in
a
> different CL
OK. I would wait until it is needed by another page set.

[ulan, 2017-04-07 08:00:45]

The CQ bit was checked by ulan@chromium.org

[commit-bot: I haz the power, 2017-04-07 08:01:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 09:00:07]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1491552045188660, "parent_rev":
"b63a4f94ae3e6b72ed0107f5525cc48615a0e118", "commit_rev":
"95e17ac4602dc38aaa6a44d4f6d2566920d6c75d"}

[commit-bot: I haz the power, 2017-04-07 09:00:58]

Description was changed from

==========
Ensure fixed amount of scrolling in infinite scroll stories.

This patch changes the scrolling logic in the stories such that each
story does the same fixed amount of scrolling in any run.

Apart from that the stories are refactored and unified following the
convention of the system health stories.

PERF SHERIFF NOTE: expect regressions and/or improvements in
- v8.infinite_scroll_tbmv2 (and other variants of this benchmark).
- v8.mobile_infinite_scroll_tbmv2 (and other variants of this benchmark).

BUG=chromium:704124
==========

to

==========
Ensure fixed amount of scrolling in infinite scroll stories.

This patch changes the scrolling logic in the stories such that each
story does the same fixed amount of scrolling in any run.

Apart from that the stories are refactored and unified following the
convention of the system health stories.

PERF SHERIFF NOTE: expect regressions and/or improvements in
- v8.infinite_scroll_tbmv2 (and other variants of this benchmark).
- v8.mobile_infinite_scroll_tbmv2 (and other variants of this benchmark).

BUG=chromium:704124

Review-Url: https://codereview.chromium.org/2773483002
Cr-Commit-Position: refs/heads/master@{#462813}
Committed:
https://chromium.googlesource.com/chromium/src/+/95e17ac4602dc38aaa6a44d4f6d2...
==========

[commit-bot: I haz the power, 2017-04-07 09:00:59]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/95e17ac4602dc38aaa6a44d4f6d2...