Re: [Telemetry] [WIP] Remove PageTest.CanRunForPage. (issue 808893002 by slamm@chromium.org)

[sl...@chromium.org]

PTAL.


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

https://codereview.chromium.org/808893002/diff/120001/tools/perf/benchmarks/session_restore.py#newcode16
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/telemetry/user_story/user_story_set.py
File tools/telemetry/telemetry/user_story/user_story_set.py (right):

https://codereview.chromium.org/808893002/diff/120001/tools/telemetry/telemetry/user_story/user_story_set.py#newcode16
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/telemetry/user_story/user_story_set.py#newcode74
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/telemetry/user_story/user_story_set.py#newcode82
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.

https://codereview.chromium.org/808893002/

[sl...@chromium.org]

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

https://codereview.chromium.org/808893002/

[nedn...@google.com]

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

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

https://codereview.chromium.org/808893002/

[sl...@chromium.org]

I added a simple unit test.

https://codereview.chromium.org/808893002/

[nedn...@google.com]

lgtm



https://codereview.chromium.org/808893002/

[commi...@chromium.org]

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


https://codereview.chromium.org/808893002/

[commi...@chromium.org]

Committed patchset #6 (id:160001)

https://codereview.chromium.org/808893002/

[commi...@chromium.org]

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

https://codereview.chromium.org/808893002/