Move WPR-related attributes to UserStorySet.

Move WPR-related attributes to UserStorySet.
Make wpr_archive_info property effectively immutable.
Move cloud storage bucket validation logic to page_set_archive_info module.

BUG=439512
Committed: https://crrev.com/743c93574d3f9c2205cf53346f888a110ff8308c
Cr-Commit-Position: refs/heads/master@{#307148}

[chrishenry, 2014-12-05 22:34:56]

Patchset #1 (id:1) has been deleted

[chrishenry, 2014-12-05 22:44:47]

Patchset #1 (id:20001) has been deleted

[chrishenry, 2014-12-05 22:45:13]

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

[nednguyen, 2014-12-05 22:52:25]

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

https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:33: if file_path is None:
Do we have any page set that defines this file_path? I wonder whether we can
deprecate this.

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

https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_set.py:90: def
WprFilePathForPage(self, page):
WprFilePathForUserStory. Or better I think user_story can use it handle to
user_story_set to retrieve this.

[slamm, 2014-12-05 23:03:39]

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

https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:33: if file_path is None:
On 2014/12/05 22:52:25, nednguyen wrote:
> Do we have any page set that defines this file_path? I wonder whether we can
> deprecate this.

Found some:

tools/perf:
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/perf.*%5C.py%...

tools/telemetry:
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/telemetry.*%5...

[nednguyen, 2014-12-05 23:10:35]

On 2014/12/05 23:03:39, slamm wrote:
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/page_set.py (right):
> 
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page_set.py:33: if file_path is None:
> On 2014/12/05 22:52:25, nednguyen wrote:
> > Do we have any page set that defines this file_path? I wonder whether we can
> > deprecate this.
> 
> Found some:
> 
> tools/perf:
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/perf.*%5C.py%...
> 
> tools/telemetry:
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/telemetry.*%5...

Looks like we still need it, thanks Steve.

[nednguyen, 2014-12-05 23:12:22]

On 2014/12/05 23:10:35, nednguyen wrote:
> On 2014/12/05 23:03:39, slamm wrote:
> >
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> > File tools/telemetry/telemetry/page/page_set.py (right):
> > 
> >
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> > tools/telemetry/telemetry/page/page_set.py:33: if file_path is None:
> > On 2014/12/05 22:52:25, nednguyen wrote:
> > > Do we have any page set that defines this file_path? I wonder whether we
can
> > > deprecate this.
> > 
> > Found some:
> > 
> > tools/perf:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/perf.*%5C.py%...
> > 
> > tools/telemetry:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/telemetry.*%5...
> 
> Looks like we still need it, thanks Steve.

Can you also update the code in user_story_runner:
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

[chrishenry, 2014-12-05 23:35:01]

On 2014/12/05 23:12:22, nednguyen wrote:
> On 2014/12/05 23:10:35, nednguyen wrote:
> > On 2014/12/05 23:03:39, slamm wrote:
> > >
> >
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> > > File tools/telemetry/telemetry/page/page_set.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> > > tools/telemetry/telemetry/page/page_set.py:33: if file_path is None:
> > > On 2014/12/05 22:52:25, nednguyen wrote:
> > > > Do we have any page set that defines this file_path? I wonder whether we
> can
> > > > deprecate this.
> > > 
> > > Found some:
> > > 
> > > tools/perf:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/perf.*%5C.py%...
> > > 
> > > tools/telemetry:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/telemetry.*%5...
> > 
> > Looks like we still need it, thanks Steve.
> 
> Can you also update the code in user_story_runner:
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

Not quite ready for this change. Looks like we need to move serving_dir as
well...

[nednguyen, 2014-12-05 23:41:57]

On 2014/12/05 23:35:01, chrishenry wrote:
> On 2014/12/05 23:12:22, nednguyen wrote:
> > On 2014/12/05 23:10:35, nednguyen wrote:
> > > On 2014/12/05 23:03:39, slamm wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> > > > File tools/telemetry/telemetry/page/page_set.py (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> > > > tools/telemetry/telemetry/page/page_set.py:33: if file_path is None:
> > > > On 2014/12/05 22:52:25, nednguyen wrote:
> > > > > Do we have any page set that defines this file_path? I wonder whether
we
> > can
> > > > > deprecate this.
> > > > 
> > > > Found some:
> > > > 
> > > > tools/perf:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/perf.*%5C.py%...
> > > > 
> > > > tools/telemetry:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/telemetry.*%5...
> > > 
> > > Looks like we still need it, thanks Steve.
> > 
> > Can you also update the code in user_story_runner:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
> 
> Not quite ready for this change. Looks like we need to move serving_dir as
> well...

Ok, let's do it another patch if you prefer. LGTM 
Please fix WprFilePathForPage issue

[chrishenry, 2014-12-05 23:42:00]

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

https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:33: if file_path is None:
On 2014/12/05 23:03:39, slamm wrote:
> On 2014/12/05 22:52:25, nednguyen wrote:
> > Do we have any page set that defines this file_path? I wonder whether we can
> > deprecate this.
> 
> Found some:
> 
> tools/perf:
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/perf.*%5C.py%...
> 
> tools/telemetry:
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/telemetry.*%5...

Yep indeed. That said, most of them can be removed IIUC (with 2-3 exceptions).

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

https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/user_story/user_story_set.py:90: def
WprFilePathForPage(self, page):
On 2014/12/05 22:52:25, nednguyen wrote:
> WprFilePathForUserStory. Or better I think user_story can use it handle to
> user_story_set to retrieve this. 

Actually user_story doesn't have a handle to user_story_set. My current thinking
is that this is a good thing. We should just update user_story_runner to get
archive_path from user_story_set instead. Will have to look into this more.

Updated the method name tho.

[nednguyen, 2014-12-05 23:58:30]

On 2014/12/05 23:42:00, chrishenry wrote:
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/page_set.py (right):
> 
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page_set.py:33: if file_path is None:
> On 2014/12/05 23:03:39, slamm wrote:
> > On 2014/12/05 22:52:25, nednguyen wrote:
> > > Do we have any page set that defines this file_path? I wonder whether we
can
> > > deprecate this.
> > 
> > Found some:
> > 
> > tools/perf:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/perf.*%5C.py%...
> > 
> > tools/telemetry:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=f:tools/telemetry.*%5...
> 
> Yep indeed. That said, most of them can be removed IIUC (with 2-3 exceptions).
> 
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/user_story/user_story_set.py (right):
> 
>
https://codereview.chromium.org/779383002/diff/40001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/user_story/user_story_set.py:90: def
> WprFilePathForPage(self, page):
> On 2014/12/05 22:52:25, nednguyen wrote:
> > WprFilePathForUserStory. Or better I think user_story can use it handle to
> > user_story_set to retrieve this. 
> 
> Actually user_story doesn't have a handle to user_story_set. My current
thinking
> is that this is a good thing. We should just update user_story_runner to get
> archive_path from user_story_set instead. Will have to look into this more.
> 
> Updated the method name tho.

KK

[chrishenry, 2014-12-06 01:32:28]

The CQ bit was checked by chrishenry@google.com

[commit-bot: I haz the power, 2014-12-06 01:33:37]

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

[commit-bot: I haz the power, 2014-12-06 03:08:57]

Committed patchset #3 (id:70012)

[commit-bot: I haz the power, 2014-12-06 03:09:37]

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