[Telemetry] Remove session_restore's use of PageTest.CanRunForPage.

Remove one use of PageTest.CanRunForPage.

After this only the startup benchmarks will have a dependency. The goal is to remove CanRunForPage entirely.

session_restore has used CanRunForPage to select one page from the typical_25. The URL is not used because the navigation action is skipped. The only reason it is there is because there is a test when setting up Web Page Replay that the archive has the URL.

BUG=440101
Committed: https://crrev.com/bfe184253c64158970e3f3a7cf281c563c10b169
Cr-Commit-Position: refs/heads/master@{#317734}

[sullivan, 2014-12-17 00:35:06]

sullivan@chromium.org changed reviewers:
+ sullivan@chromium.org

[sullivan, 2014-12-17 00:35:06]

lgtm

[slamm, 2014-12-17 00:45:52]

On 2014/12/17 00:35:06, sullivan wrote:
> lgtm
 
Oops, I sent the wrong CL for review. This one still needs work. The CL that is
ready is https://codereview.chromium.org/809653004/

[slamm, 2015-01-08 01:04:19]

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

[slamm, 2015-01-08 01:04:31]

slamm@chromium.org changed reviewers:
- sullivan@chromium.org

[nednguyen, 2015-01-08 01:54:13]

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

[nednguyen, 2015-01-08 01:54:14]

https://codereview.chromium.org/808893002/diff/20001/tools/perf/benchmarks/se...
File tools/perf/benchmarks/session_restore.py (right):

https://codereview.chromium.org/808893002/diff/20001/tools/perf/benchmarks/se...
tools/perf/benchmarks/session_restore.py:15: page_set =
session_restore_page_set.SessionRestorePageSet
I think this will lead to naming change.

[slamm, 2015-01-08 18:30:14]

Simpler approach.

[slamm, 2015-01-08 18:30:58]

Simpler approach.

[slamm, 2015-01-09 00:02:26]

Patchset #3 (id:40001) has been deleted

[slamm, 2015-01-09 00:02:38]

Patchset #3 (id:60001) has been deleted

[slamm, 2015-01-09 00:02:46]

Patchset #3 (id:80001) has been deleted

[slamm, 2015-01-09 00:22:50]

On 2015/01/08 18:30:58, slamm wrote:
> Simpler approach.

Ned, now this creates an instance of the Typical25PageSet and then changes its
user_stories member. A bit of an interface violation, I am sure.  However, is it
acceptable?

[nednguyen, 2015-01-09 00:29:13]

On 2015/01/09 00:22:50, slamm wrote:
> On 2015/01/08 18:30:58, slamm wrote:
> > Simpler approach.
> 
> Ned, now this creates an instance of the Typical25PageSet and then changes its
> user_stories member. A bit of an interface violation, I am sure.  However, is
it
> acceptable?

How about?
@classmethod
def CreatePageSet(cls, ..):
   session_restore_pageset = page_set.PageSet(cls.page_set.file_path,
cls..page_set.archive_data_file, ...)
   session_restore_pageset.AddUserStory(cls.page_set()[1])

[nednguyen, 2015-01-09 17:10:32]

On 2015/01/09 00:29:13, nednguyen wrote:
> On 2015/01/09 00:22:50, slamm wrote:
> > On 2015/01/08 18:30:58, slamm wrote:
> > > Simpler approach.
> > 
> > Ned, now this creates an instance of the Typical25PageSet and then changes
its
> > user_stories member. A bit of an interface violation, I am sure.  However,
is
> it
> > acceptable?
> 
> How about?
> @classmethod
> def CreatePageSet(cls, ..):
>    session_restore_pageset = page_set.PageSet(cls.page_set.file_path,
> cls..page_set.archive_data_file, ...)
>    session_restore_pageset.AddUserStory(cls.page_set()[1])

Also is the purpose of this refactoring is getting rid of CanRunForPage in
session_restore page_test? Can you also remove it in this same patch?

[slamm, 2015-01-09 22:10:06]

Add UserStorySet.RemoveUserStory

[qyearsley, 2015-01-28 19:39:08]

Just taking a quick look at this CL and adding a couple optional comments about
docstrings.

Where is CanRunForPage used for session_restore, and how is it not used after
this CL? I don't see any code being removed in this CL.

https://codereview.chromium.org/808893002/diff/120001/tools/perf/benchmarks/s...
File tools/perf/benchmarks/session_restore.py (right):

https://codereview.chromium.org/808893002/diff/120001/tools/perf/benchmarks/s...
tools/perf/benchmarks/session_restore.py:16: """Create a session restore test
that is either "warm" or "cold".
1. Possible alternative phrasing: "Base Benchmark class for session restore
benchmarks."

2. Could be re-named _SesssionRestoreTypical25 since the page set is now set in
the base class.

What is done before measurement in a "warm" test? Is it that the browser is
launched and a session restore is done first, then the tabs are closed without
closing the browser, and the session is restored again?

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/user_story_set.py (right):

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/user_story_set.py:16: AddUserStory for each
UserStory..
Extra ".".

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/user_story_set.py:74: """Remove a user
story.
1. The verb in the docstring should be third-person singular, i.e. "Removes...".

2. Could say UserStory instead of "user story", since we're expecting the
argument to be a UserStory instance.

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/user_story_set.py:82: """ Returns the
string name of this UserStorySet.
Outside the scope of this CL:
There's an extra space here, and a blank line should be added after the first
line of the docstring.

[slamm, 2015-02-23 23:46:35]

PTAL.

https://codereview.chromium.org/808893002/diff/120001/tools/perf/benchmarks/s...
File tools/perf/benchmarks/session_restore.py (right):

https://codereview.chromium.org/808893002/diff/120001/tools/perf/benchmarks/s...
tools/perf/benchmarks/session_restore.py:16: """Create a session restore test
that is either "warm" or "cold".
On 2015/01/28 19:39:08, qyearsley wrote:
> 1. Possible alternative phrasing: "Base Benchmark class for session restore
> benchmarks."
> 
> 2. Could be re-named _SesssionRestoreTypical25 since the page set is now set
in
> the base class.
> 
> What is done before measurement in a "warm" test? Is it that the browser is
> launched and a session restore is done first, then the tabs are closed without
> closing the browser, and the session is restored again?

Done.

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/user_story_set.py (right):

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/user_story_set.py:16: AddUserStory for each
UserStory..
On 2015/01/28 19:39:08, qyearsley wrote:
> Extra ".".

Done.

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/user_story_set.py:74: """Remove a user
story.
On 2015/01/28 19:39:08, qyearsley wrote:
> 1. The verb in the docstring should be third-person singular, i.e.
"Removes...".
> 
> 2. Could say UserStory instead of "user story", since we're expecting the
> argument to be a UserStory instance.

Done.

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/user_story_set.py:82: """ Returns the
string name of this UserStorySet.
On 2015/01/28 19:39:08, qyearsley wrote:
> Outside the scope of this CL:
> There's an extra space here, and a blank line should be added after the first
> line of the docstring.

Done.

These extra spaces are all over the place.

[slamm, 2015-02-23 23:48:30]

On 2015/02/23 23:46:35, slamm wrote:
> PTAL.
> 
>
https://codereview.chromium.org/808893002/diff/120001/tools/perf/benchmarks/s...
> File tools/perf/benchmarks/session_restore.py (right):
> 
>
https://codereview.chromium.org/808893002/diff/120001/tools/perf/benchmarks/s...
> tools/perf/benchmarks/session_restore.py:16: """Create a session restore test
> that is either "warm" or "cold".
> On 2015/01/28 19:39:08, qyearsley wrote:
> > 1. Possible alternative phrasing: "Base Benchmark class for session restore
> > benchmarks."
> > 
> > 2. Could be re-named _SesssionRestoreTypical25 since the page set is now set
> in
> > the base class.
> > 
> > What is done before measurement in a "warm" test? Is it that the browser is
> > launched and a session restore is done first, then the tabs are closed
without
> > closing the browser, and the session is restored again?
> 
> Done.
> 
>
https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/user_story/user_story_set.py (right):
> 
>
https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
> tools/telemetry/telemetry/user_story/user_story_set.py:16: AddUserStory for
each
> UserStory..
> On 2015/01/28 19:39:08, qyearsley wrote:
> > Extra ".".
> 
> Done.
> 
>
https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
> tools/telemetry/telemetry/user_story/user_story_set.py:74: """Remove a user
> story.
> On 2015/01/28 19:39:08, qyearsley wrote:
> > 1. The verb in the docstring should be third-person singular, i.e.
> "Removes...".
> > 
> > 2. Could say UserStory instead of "user story", since we're expecting the
> > argument to be a UserStory instance.
> 
> Done.
> 
>
https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemet...
> tools/telemetry/telemetry/user_story/user_story_set.py:82: """ Returns the
> string name of this UserStorySet.
> On 2015/01/28 19:39:08, qyearsley wrote:
> > Outside the scope of this CL:
> > There's an extra space here, and a blank line should be added after the
first
> > line of the docstring.
> 
> Done.
> 
> These extra spaces are all over the place.

Quinten, I really appreciate the extra comments -- especially about using
third-person singular. I have never been clear on that one.

[nednguyen, 2015-02-23 23:48:41]

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

[nednguyen, 2015-02-23 23:48:42]

https://codereview.chromium.org/808893002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/user_story/user_story_set.py (right):

https://codereview.chromium.org/808893002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/user_story/user_story_set.py:73: def
RemoveUserStory(self, user_story):
Can you add unittest coverage for this new method?

[slamm, 2015-02-24 00:37:17]

Add unittest'

[slamm, 2015-02-24 00:38:12]

I added a simple unit test.

[nednguyen, 2015-02-24 00:43:54]

lgtm

[slamm, 2015-02-24 00:44:23]

The CQ bit was checked by slamm@chromium.org

[slamm, 2015-02-24 00:44:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org
Link to the patchset: https://codereview.chromium.org/808893002/#ps160001
(title: "Add unittest")

[commit-bot: I haz the power, 2015-02-24 00:45:49]

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

[commit-bot: I haz the power, 2015-02-24 01:55:38]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-02-24 01:56:56]

Patchset 6 (id:??) landed as
https://crrev.com/bfe184253c64158970e3f3a7cf281c563c10b169
Cr-Commit-Position: refs/heads/master@{#317734}