Migrate infinite scrolls to system health stories

tools/perf/page_sets/system_health/browsing_stories.py

Index: tools/perf/page_sets/system_health/browsing_stories.py
diff --git a/tools/perf/page_sets/system_health/browsing_stories.py b/tools/perf/page_sets/system_health/browsing_stories.py
index 75ba50a20f98d63a565a72f0a2f8a61929710949..4b07d3889fb9ca72b17bc37a6fb666dc0e9aa792 100644
--- a/tools/perf/page_sets/system_health/browsing_stories.py
+++ b/tools/perf/page_sets/system_health/browsing_stories.py
@@ -7,6 +7,7 @@ from page_sets.system_health import platforms
 from page_sets.system_health import story_tags
 from page_sets.system_health import system_health_story
 
+from page_sets.login_helpers import facebook_login
 from page_sets.login_helpers import pinterest_login
 
 from telemetry import decorators
@@ -770,3 +771,130 @@ class GoogleEarthStory(_BrowsingStory):
     action_runner.RepeatableBrowserDrivenScroll(
         x_scroll_distance_ratio = 1, y_scroll_distance_ratio = 0,
         repeat_count=3, speed=500, timeout=120)
+
+
+##############################################################################
+# Browsing stories with infinite scrolling
+##############################################################################
+
+
+class _InfiniteScrollStory(system_health_story.SystemHealthStory):
+  SUPPORTED_PLATFORMS = platforms.ALL_PLATFORMS
+
+  SCROLL_DISTANCE = 25000
+  SCROLL_STEP = 1000
+  MAX_SCROLL_RETRIES = 3
+  TIME_BEFORE_SCROLL_RETRY_IN_SECONDS = 1
+  TIME_TO_WAIT_BEFORE_STARTING_IN_SECONDS = 5
+
+  def __init__(self, story_set, take_memory_measurement):
+    super(_InfiniteScrollStory, self).__init__(story_set,
+        take_memory_measurement)
+    self.script_to_evaluate_on_commit = '''
+        window.WebSocket = undefined;
+        window.Worker = undefined;
+        window.performance = undefined;'''
+
+  def RunPageInteractions(self, action_runner):

[perezju, 2017/05/19 09:40:35]

I don't thing we should override this method.

Could we do it by overriding _DidLoadDocument instead, like all other system
health stories do?

My worry is that this bypasses the system health mechanism for taking
measurements (e.g. cache flushes, measure memory but only in the corresponding
benchmark, etc.)

[nednguyen, 2017/05/24 21:08:49]

Oh, it's just me copy & paste the stories from v8 benchmarks without much
thinking. This is why we have code review :-)

Done.

+    with action_runner.CreateInteraction('Load'):

[perezju, 2017/05/19 09:40:36]

Do we still want to create all of these interaction records?

[nednguyen, 2017/05/24 21:08:49]

Nope.

+      action_runner.WaitForJavaScriptCondition(
+        'document.body != null && '
+        'document.body.scrollHeight > window.innerHeight && '
+        '!document.body.addEventListener("touchstart", function() {})')
+    with action_runner.CreateInteraction('Wait'):
+      action_runner.Wait(self.TIME_TO_WAIT_BEFORE_STARTING_IN_SECONDS)
+    with action_runner.CreateInteraction('GC'):
+      action_runner.ForceGarbageCollection()
+    with action_runner.CreateInteraction('Begin'):
+      action_runner.tab.browser.DumpMemory()
+    with action_runner.CreateInteraction('Scrolling'):
+      self._Scroll(action_runner, self.SCROLL_DISTANCE, self.SCROLL_STEP)
+    with action_runner.CreateInteraction('End'):
+      action_runner.tab.browser.DumpMemory()
+
+  def _Scroll(self, action_runner, distance, step_size):

[perezju, 2017/05/19 09:40:35]

Could we use some of the Scroll methods in action_runner? hjd@ spent a great
deal of effort making sure those worked properly.

[hjd, 2017/05/19 10:42:21]

This does use action_runner.ScrollPage underneath which is good, I think none of
the more involved scroll methods (ScrollPageToElement etc) quite match the logic
here sadly. However maybe we could improve/add to to the methods in
action_runner? Infinite scroll seems like a useful primitive and it's nice to
have all the scroll bugs concentrated in one place.

+    """ This function scrolls the webpage by the given scroll distance in
+    multiple steps, where each step (except the last one) has the given size.
+
+    If scrolling gets stuck, the functions retries scrolling MAX_SCROLL_RETRIES
+    times waiting TIME_BEFORE_SCROLL_RETRY_IN_SECONDS seconds between retries.
+    """
+    remaining = distance - action_runner.EvaluateJavaScript('window.scrollY')

[hjd, 2017/05/19 10:42:21]

I think scrollY can be overwritten in JavaScript to be (for example) a string
although this should be rare in practice.

[nednguyen, 2017/05/24 21:08:49]

Acknowledged.

+    retry_count = 0
+    # Scroll until the window.scrollY is within 1 pixel of the target distance.
+    while remaining > 1:
+      action_runner.ScrollPage(distance=min(remaining, step_size) + 1)
+      new_remaining = (distance -
+          action_runner.EvaluateJavaScript('window.scrollY'))
+      if remaining == new_remaining:

[hjd, 2017/05/19 10:42:21]

This could be remaining >= new_remaining to guarantee we're making forward
progress. Otherwise if the page js is trying to scroll us up at the same time
we're scrolling down we could end up stuck in a loop.

[nednguyen, 2017/05/24 21:08:49]

Done.

+        # Scrolling is stuck. This can happen if the page is loading
+        # resources. Give the page some time and retry scrolling.
+        if retry_count == self.MAX_SCROLL_RETRIES:
+          raise Exception('Scrolling stuck at %d' % remaining)
+        retry_count += 1
+        action_runner.Wait(self.TIME_BEFORE_SCROLL_RETRY_IN_SECONDS)
+      else:
+        retry_count = 0
+        remaining = new_remaining
+
+
+class DiscourseDesktopStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:forum:discourse'

[perezju, 2017/05/19 09:40:35]

I would call this: browse:social:discourse
And add some 'infinite_scroll' tag instead.

[ulan, 2017/05/19 12:59:00]

How about browse:infinite_scroll:discourse? I would like to keep all these
stories in the same group and avoid confusion with other browse stories.

[perezju, 2017/05/19 14:34:40]

I think "tags" should be the correct way to group together stories in system
health. (also see my other reply below.)

+  URL = ('https://meta.discourse.org/t/the-official-discourse-tags-plugin' +
+     '-discourse-tagging/26482')
+  SUPPORTED_PLATFORMS = platforms.DESKTOP_ONLY
+
+
+class DiscourseMobileStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:forum:discourse'

[perezju, 2017/05/19 09:40:35]

ditto

+  URL = ('https://meta.discourse.org/t/the-official-discourse-tags-plugin' +
+     '-discourse-tagging/26482')
+  SUPPORTED_PLATFORMS = platforms.MOBILE_ONLY
+  SCROLL_DISTANCE = 15000
+
+
+class FacebookScrollDesktopStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:social:facebook'

[perezju, 2017/05/19 09:40:35]

Could this replace our existing browse:social:facebook?

[ulan, 2017/05/19 12:59:00]

They execute different work loads and should be considered as different stories.

[perezju, 2017/05/19 14:34:40]

For system health, IMO, we should pick representative websites, and then pick a
representative workflow on that website.

There might be cases where we want to provide coverage for several workflows on
the same property, but that should be rare.

The driving principle should be to test a diverse set of representative websites
on representative workflows; but without doing the full Cartesian product of
both.

In my ideal world all stories should be "browse", and the rest of the name
should be "site_category:site_name".

Tags are then used to group stories by e.g. workflow or feature; providing a
convenient way for developers e.g. to run only a set of stories; or filter-by
when looking at dashboards.

Ned, what are your thoughts here?

+  URL = 'https://www.facebook.com/shakira'
+  SUPPORTED_PLATFORMS = platforms.DESKTOP_ONLY
+
+
+class FacebookScrollMobileStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:social:facebook'
+  URL = 'https://m.facebook.com/shakira'
+  SUPPORTED_PLATFORMS = platforms.MOBILE_ONLY
+
+  def RunNavigateSteps(self, action_runner):

[perezju, 2017/05/19 09:40:36]

We should be overriding _Login instead.

[nednguyen, 2017/05/24 21:08:49]

Done.

+    facebook_login.LoginWithMobileSite(
+        action_runner, 'facebook3', self.credentials_path)
+    super(FacebookScrollMobileStory, self).RunNavigateSteps(action_runner)
+
+
+class FlickrDesktopStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:media:flickr'

[perezju, 2017/05/19 09:40:35]

Call this just browse:media:flickr

[ulan, 2017/05/19 12:59:00]

How about browse:infinite_scroll:flickr?

+  URL = 'https://www.flickr.com/explore'
+  SUPPORTED_PLATFORMS = platforms.DESKTOP_ONLY
+
+
+class FlickrMobileStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:media:flickr'
+  URL = 'https://www.flickr.com/explore'
+  SUPPORTED_PLATFORMS = platforms.MOBILE_ONLY
+  SCROLL_DISTANCE = 10000
+
+
+class PinterestMobileStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:social:pinterest'

[perezju, 2017/05/19 09:40:36]

browse:social:pinterest

[ulan, 2017/05/19 12:59:00]

How about browse:infinite_scroll:pinterest?

+  URL = 'https://www.pinterest.com/all'
+  SUPPORTED_PLATFORMS = platforms.MOBILE_ONLY

[perezju, 2017/05/19 09:40:35]

No desktop version?

[nednguyen, 2017/05/24 21:08:49]

Nope

+
+
+class TumblrStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:social:tumblr'

[perezju, 2017/05/19 09:40:36]

browse:social:tumblr

[ulan, 2017/05/19 12:59:00]

How about browse:infinite_scroll:pinterest?

+  URL = 'http://techcrunch.tumblr.com/'

[hjd, 2017/05/19 10:42:21]

nit: maybe a comment that techcrunch.tumblr.com doesn't have https right now, so
later we don't think it was because of a wpr bug or something.

[nednguyen, 2017/05/24 21:08:49]

Done.

+
+
+class TwitterScrollDesktopStory(_InfiniteScrollStory):
+  NAME = 'browse_infinite_scroll:social:twitter'
+  URL = 'https://twitter.com/taylorswift13'
+  SUPPORTED_PLATFORMS = platforms.DESKTOP_ONLY