Introduce UserStory & UserStorySet classes.

Introduce UserStory & UserStorySet classes.

Make Page subclasses UserStory & PageSet subclasses UserStorySet.

BUG=418278
Committed: https://crrev.com/7e4c78159e4eb78c28fd06894231fa4fb84c3d32
Cr-Commit-Position: refs/heads/master@{#297510}

[nednguyen, 2014-09-29 20:11:15]

Patchset #1 (id:1) has been deleted

[nednguyen, 2014-09-29 20:13:04]

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

[nednguyen, 2014-09-29 20:17:38]

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

[nednguyen, 2014-09-30 15:46:31]

Patchset #2 (id:40001) has been deleted

[nednguyen, 2014-09-30 15:46:37]

Patchset #1 (id:20001) has been deleted

[nednguyen, 2014-09-30 15:46:43]

Patchset #1 (id:60001) has been deleted

[nduca, 2014-09-30 16:30:52]

thoughts

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:199: def CreateUserStorySet(cls,
options):
why is this a classmethod? Please make this a regular method unless you have
good reason.

why do you have cls.CreatePageSet() and cls.PageSetlass() up on :196? This is
messy please make it cleaner...

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_set.py (right):

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:58: 
what about description, and name?

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_set.py (right):

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_set.py:11: 
extra space

[dtu, 2014-09-30 18:00:04]

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/benchmark.py (right):

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/benchmark.py:199: def CreateUserStorySet(cls,
options):
On 2014/09/30 16:30:51, nduca wrote:
> why is this a classmethod? Please make this a regular method unless you have
> good reason.
> 
> why do you have cls.CreatePageSet() and cls.PageSetlass() up on :196? This is
> messy please make it cleaner...

I can answer this, since these are not part of this patch but rather the
existing code. If I recall correctly...

classmethod: We want to get the page set without instantiating a Benchmark when
we record web pages. Also, I don't have any rule that states that
non-classmethods should be the default. A benchmark is more of a store of
configuration and doesn't have too much run-time logic, so most of its methods
are classmethods.

PageSetClass(): for consistency with PageTestClass(). Because people have found
it weird to override a field in the parent class, and found overriding methods
to be more intuitive. (People also think the options field is too magical.)

[nednguyen, 2014-09-30 18:21:08]

On 2014/09/30 18:00:04, dtu wrote:
>
https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/benchmark.py (right):
> 
>
https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/benchmark.py:199: def CreateUserStorySet(cls,
> options):
> On 2014/09/30 16:30:51, nduca wrote:
> > why is this a classmethod? Please make this a regular method unless you have
> > good reason.
> > 
> > why do you have cls.CreatePageSet() and cls.PageSetlass() up on :196? This
is
> > messy please make it cleaner...
> 
> I can answer this, since these are not part of this patch but rather the
> existing code. If I recall correctly...
> 
> classmethod: We want to get the page set without instantiating a Benchmark
when
> we record web pages. Also, I don't have any rule that states that
> non-classmethods should be the default. A benchmark is more of a store of
> configuration and doesn't have too much run-time logic, so most of its methods
> are classmethods.
> 
> PageSetClass(): for consistency with PageTestClass(). Because people have
found
> it weird to override a field in the parent class, and found overriding methods
> to be more intuitive. (People also think the options field is too magical.)

Since we can parameterize PageSet, the argument that creating PageSet is logic
free no longer applies. I think we can remove all the staticmethod related
constructing the pageset.

[nednguyen, 2014-09-30 18:31:03]

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_set.py (right):

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:58: 
On 2014/09/30 16:30:51, nduca wrote:
> what about description, and name?
Moved Name & Description to UserStorySet.

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/user_story/user_story_set.py (right):

https://codereview.chromium.org/612963002/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_set.py:11: 
On 2014/09/30 16:30:52, nduca wrote:
> extra space

Done.

[nduca, 2014-09-30 18:54:34]

lgtm

[nednguyen, 2014-09-30 20:03:54]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-09-30 20:05:18]

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

[commit-bot: I haz the power, 2014-09-30 20:34:49]

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

[commit-bot: I haz the power, 2014-09-30 20:34:50]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[nednguyen, 2014-09-30 20:37:46]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-09-30 20:39:44]

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

[commit-bot: I haz the power, 2014-09-30 21:47:22]

Committed patchset #3 (id:120001) as dcd8530594165b11eb0e351f17eed7df229484b6

[commit-bot: I haz the power, 2014-09-30 21:48:14]

Patchset 3 (id:??) landed as
https://crrev.com/7e4c78159e4eb78c28fd06894231fa4fb84c3d32
Cr-Commit-Position: refs/heads/master@{#297510}