[System Health] Add browsing user stories for pinterest and tumblr.

[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images
and close them. Pinterest will also 'pin' every other image it clicks
on.

BUG=646392
Committed: https://crrev.com/022a7317076c57094cd6137ded6e9be08b666f12
Cr-Commit-Position: refs/heads/master@{#422886}

[rnephew (Reviews Here), 2016-09-30 20:26:41]

rnephew@chromium.org changed reviewers:
+ charliea@chromium.org, nednguyen@google.com, petrcermak@chromium.org

[petrcermak, 2016-10-03 09:01:23]

Looks good overall, but please don't shadow constants with local variables (see
my inline comments).

Thanks,
Petr

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/login_...
File tools/perf/page_sets/login_helpers/pinterest_login.py (right):

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/login_...
tools/perf/page_sets/login_helpers/pinterest_login.py:28: action_runner.Wait(1)
# Error page happens if this wait is not here.
Any chance this could be replaced with action_runner.WaitForElement?

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/system...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/browsing_stories.py:319:
self.ITEM_SELECTOR_INDEX += 1
This works, but it's very hacky. Please don't shadow a constant like this.
Instead, you should probably modify the abstract class _MediaBrowsingStory to
pass the iteration loop index to _NavigateToItem (instead of a constant). You'll
most probably need to modify the existing stories to keep their existing
behavior.

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/browsing_stories.py:357:
self.ITEM_SELECTOR_INDEX += 1
ditto: don't shadow a constant

[rnephew (Reviews Here), 2016-10-03 16:57:13]

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/login_...
File tools/perf/page_sets/login_helpers/pinterest_login.py (right):

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/login_...
tools/perf/page_sets/login_helpers/pinterest_login.py:28: action_runner.Wait(1)
# Error page happens if this wait is not here.
On 2016/10/03 09:01:23, petrcermak wrote:
> Any chance this could be replaced with action_runner.WaitForElement?

Nope. Its there, it just goes to an error page if you do it to fast for some
reason.

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/system...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/browsing_stories.py:319:
self.ITEM_SELECTOR_INDEX += 1
On 2016/10/03 09:01:23, petrcermak wrote:
> This works, but it's very hacky. Please don't shadow a constant like this.
> Instead, you should probably modify the abstract class _MediaBrowsingStory to
> pass the iteration loop index to _NavigateToItem (instead of a constant).
You'll
> most probably need to modify the existing stories to keep their existing
> behavior.

Done.

https://codereview.chromium.org/2383883002/diff/1/tools/perf/page_sets/system...
tools/perf/page_sets/system_health/browsing_stories.py:357:
self.ITEM_SELECTOR_INDEX += 1
On 2016/10/03 09:01:23, petrcermak wrote:
> ditto: don't shadow a constant

Done.

[nednguyen, 2016-10-03 17:27:24]

Nice work on the pinterest story!

However, for tumblr, I think we should leave it out until crbug.com/651909 is
fixed because changing a story will create a bunch of regressions.

[rnephew (Reviews Here), 2016-10-03 18:05:52]

On 2016/10/03 17:27:24, nednguyen wrote:
> Nice work on the pinterest story!
> 
> However, for tumblr, I think we should leave it out until crbug.com/651909 is
> fixed because changing a story will create a bunch of regressions.

I disabled it on all platforms.

[nednguyen, 2016-10-03 18:45:54]

lgtm but please wait for Charlie & Petr

[petrcermak, 2016-10-04 13:34:22]

LGTM with 2 more comments.

Maybe also add a short patch description explaining what the stories are doing?

Thanks,
Petr

https://codereview.chromium.org/2383883002/diff/40001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2383883002/diff/40001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/browsing_stories.py:239: INCREMENT_INDEX =
False
INCREMENT_INDEX_AFTER_EACH_ITEM might be easier to understand ;-)

https://codereview.chromium.org/2383883002/diff/40001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/browsing_stories.py:344: if
self.ITEM_SELECTOR_INDEX % 2 == 0:
ITEM_SELECTOR_INDEX is now a constant. You'll need to add an `index` argument to
_ViewMediaItem and use it here.

This suggests that you didn't manually test the stories. Please replay them
locally before you land the patch!

[rnephew (Reviews Here), 2016-10-04 15:30:29]

Description was changed from

==========
[System Health] Add browsing user stories for pinterest and tumblr.

BUG=646392
==========

to

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images and closes
them. Pinterest will also 'pin' every other image it clicks on.

BUG=646392
==========

[rnephew (Reviews Here), 2016-10-04 15:51:12]

https://codereview.chromium.org/2383883002/diff/40001/tools/perf/page_sets/sy...
File tools/perf/page_sets/system_health/browsing_stories.py (right):

https://codereview.chromium.org/2383883002/diff/40001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/browsing_stories.py:239: INCREMENT_INDEX =
False
On 2016/10/04 13:34:21, petrcermak wrote:
> INCREMENT_INDEX_AFTER_EACH_ITEM might be easier to understand ;-)

Done.

https://codereview.chromium.org/2383883002/diff/40001/tools/perf/page_sets/sy...
tools/perf/page_sets/system_health/browsing_stories.py:344: if
self.ITEM_SELECTOR_INDEX % 2 == 0:
On 2016/10/04 13:34:21, petrcermak wrote:
> ITEM_SELECTOR_INDEX is now a constant. You'll need to add an `index` argument
to
> _ViewMediaItem and use it here.
> 
> This suggests that you didn't manually test the stories. Please replay them
> locally before you land the patch!

I did replay them, I just didn't pay close enough attention to what was going on
on screen. It doesn't fail spectacularly. I watched more closely this time, and
it works as expected.

[petrcermak, 2016-10-04 15:53:28]

LGTM with a few description nits:

* Wrap description at 72 characters please
* s/closes/close/

Thanks,
Petr

[rnephew (Reviews Here), 2016-10-04 16:57:41]

Description was changed from

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images and closes
them. Pinterest will also 'pin' every other image it clicks on.

BUG=646392
==========

to

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images 
and closes them. Pinterest will also 'pin' every other image it clicks on.

BUG=646392
==========

[rnephew (Reviews Here), 2016-10-04 16:57:52]

Description was changed from

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images 
and closes them. Pinterest will also 'pin' every other image it clicks on.

BUG=646392
==========

to

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images 
and close them. Pinterest will also 'pin' every other image it clicks on.

BUG=646392
==========

[rnephew (Reviews Here), 2016-10-04 16:58:37]

Description was changed from

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images 
and close them. Pinterest will also 'pin' every other image it clicks on.

BUG=646392
==========

to

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images and
close them. Pinterest will also 'pin' every other image it clicks on.

BUG=646392
==========

[rnephew (Reviews Here), 2016-10-04 16:58:57]

The CQ bit was checked by rnephew@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 16:59:24]

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

[rnephew (Reviews Here), 2016-10-04 17:11:03]

Description was changed from

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images and
close them. Pinterest will also 'pin' every other image it clicks on.

BUG=646392
==========

to

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images
and close them. Pinterest will also 'pin' every other image it clicks
on.

BUG=646392
==========

[rnephew (Reviews Here), 2016-10-04 17:45:54]

Forgot to make pinterest desktop only. Done with new patchset.

[rnephew (Reviews Here), 2016-10-04 17:48:31]

The CQ bit was checked by rnephew@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 17:49:08]

Dry run: 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, 2016-10-04 18:46:13]

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

[commit-bot: I haz the power, 2016-10-04 18:46:14]

Dry run: This issue passed the CQ dry run.

[rnephew (Reviews Here), 2016-10-04 19:28:43]

The CQ bit was checked by rnephew@chromium.org

[rnephew (Reviews Here), 2016-10-04 19:28:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, petrcermak@chromium.org
Link to the patchset: https://codereview.chromium.org/2383883002/#ps80001
(title: "make pinterest desktop only")

[commit-bot: I haz the power, 2016-10-04 19:29:14]

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, 2016-10-04 19:35:44]

Description was changed from

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images
and close them. Pinterest will also 'pin' every other image it clicks
on.

BUG=646392
==========

to

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images
and close them. Pinterest will also 'pin' every other image it clicks
on.

BUG=646392
==========

[commit-bot: I haz the power, 2016-10-04 19:35:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-04 19:41:51]

Description was changed from

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images
and close them. Pinterest will also 'pin' every other image it clicks
on.

BUG=646392
==========

to

==========
[System Health] Add browsing user stories for pinterest and tumblr.

The stories go to their respective sites and open a series of images
and close them. Pinterest will also 'pin' every other image it clicks
on.

BUG=646392

Committed: https://crrev.com/022a7317076c57094cd6137ded6e9be08b666f12
Cr-Commit-Position: refs/heads/master@{#422886}
==========

[commit-bot: I haz the power, 2016-10-04 19:41:53]

Patchset 5 (id:??) landed as
https://crrev.com/022a7317076c57094cd6137ded6e9be08b666f12
Cr-Commit-Position: refs/heads/master@{#422886}