Make credential_path a page attribute instead of page_set attribute.

Make credential_path a page attribute instead of page_set attribute.

Rationale:
+ This makes _RunPage --> SharedPageState refactoring easier.
+ This enables combining pages from different page_sets.

This patch also modifies page_set_smoke_test.py to make sure that the change doesn't break existing page sets.

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

[nednguyen, 2014-10-06 17:45:53]

Patchset #1 (id:1) has been deleted

[nednguyen, 2014-10-06 17:46:32]

nednguyen@google.com changed reviewers:
+ dtu@chromium.org, nduca@chromium.org, slamm@chromium.org, tonyg@chromium.org

[tonyg, 2014-10-06 18:51:19]

I defer to Dave on this.

[nednguyen, 2014-10-06 18:53:12]

Patchset #2 (id:40001) has been deleted

[nednguyen, 2014-10-06 19:32:16]

Patchset #3 (id:80001) has been deleted

[slamm, 2014-10-06 20:21:19]

Mostly nits.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/util.py (right):

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/util.py:149: """ Returns the absolute path for
|path| if it exists. If |path| is relative,
No space after the triple quotes.
First line of doc string should not wrap (internal Python style guide).

"""Returns the absolute path for |path| if it exists.

If |path| is relative, then it is relative to the directory of |base_dir|.
"""

When the doc string is longer than one line, put the closing quotes on their own
line.
Generally, if the doc string is longer than one line, I add the "Args" and
"Returns" sections too.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_runner_unittest.py (right):

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_runner_unittest.py:337: 'file://blank.html',
ps, base_dir=util.GetUnittestDataDir(),
indent

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/classes.py (right):

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:7: def IsDirectlyConstructable(cls):
Add a doc string.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:9: # Case class A: pass
# Case |class A: pass|.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:12: # Case class A(object): pass
# Case |class A(object): pass|.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:15: # Case class has __init__ method
which is not object.__init__.
# Case |class (object):| with |__init__| other than |object.__init__|.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:16: args, _1, _2, defaults =
inspect.getargspec(cls.__init__)
Does it not work to repeat an underscore variable?

args, _, _, defaults = ...

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:18: defaults = []
defaults is otherwise a tuple, so you can use an empty tuple here.

defaults = ()

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:20: len(args) == len(defaults) + 1)
A tighter check is that the first value of args is 'self'.
Would slicing be clearer?

# Return true if self is the only argument without a default value.
return args[0] == 'self' and len(args[1:]) == len(defaults)

[nednguyen, 2014-10-06 20:45:51]

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/util.py (right):

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/util.py:149: """ Returns the absolute path for
|path| if it exists. If |path| is relative,
On 2014/10/06 20:21:19, slamm wrote:
> No space after the triple quotes.
> First line of doc string should not wrap (internal Python style guide).
> 
> """Returns the absolute path for |path| if it exists.
> 
> If |path| is relative, then it is relative to the directory of |base_dir|.
> """
> 
> When the doc string is longer than one line, put the closing quotes on their
own
> line.
> Generally, if the doc string is longer than one line, I add the "Args" and
> "Returns" sections too.

Done.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/classes.py (right):

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:7: def IsDirectlyConstructable(cls):
On 2014/10/06 20:21:19, slamm wrote:
> Add a doc string.

Done.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:9: # Case class A: pass
On 2014/10/06 20:21:19, slamm wrote:
> # Case |class A: pass|.

Done.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:12: # Case class A(object): pass
On 2014/10/06 20:21:19, slamm wrote:
> # Case |class A(object): pass|.

Done.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:15: # Case class has __init__ method
which is not object.__init__.
On 2014/10/06 20:21:19, slamm wrote:
> # Case |class (object):| with |__init__| other than |object.__init__|.

Done.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:16: args, _1, _2, defaults =
inspect.getargspec(cls.__init__)
On 2014/10/06 20:21:19, slamm wrote:
> Does it not work to repeat an underscore variable?
> 
> args, _, _, defaults = ...

Done.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:18: defaults = []
On 2014/10/06 20:21:19, slamm wrote:
> defaults is otherwise a tuple, so you can use an empty tuple here.
> 
> defaults = ()

Done.

https://codereview.chromium.org/625123002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:20: len(args) == len(defaults) + 1)
On 2014/10/06 20:21:19, slamm wrote:
> A tighter check is that the first value of args is 'self'.
> Would slicing be clearer?
> 
> # Return true if self is the only argument without a default value.
> return args[0] == 'self' and len(args[1:]) == len(defaults)

I just do a quick check and turn out 'self' is just a conventional variable
name. One can totally substitute 'self' with 'foo' and it will still work the
same way.

[slamm_google, 2014-10-06 21:02:59]

slamm@google.com changed reviewers:
+ slamm@google.com

[slamm_google, 2014-10-06 21:03:01]

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/unittest/page_set_smoke_test.py (right):

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/unittest/page_set_smoke_test.py:113: raise
Why is it okay to continue for classes that are not directly constructable?
Add a comment?

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/classes.py (right):

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:9: """ Returns True if instance of
|cls| can be construct without arguments. """
No spaces immediately inside the triple quotes.

"""Returns True if instance of |cls| can be construct without arguments."""

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:12: if not hasattr(cls, '__init__'):
Might be better to put case comments inside the if statements:

if not hasattr(cls, '__init__'):
  # Case |class A: pass|.
  return True
if cls.__init__ is object.__init__:
  # Case |class A(object): pass|.
  return True
# Case |class (object):| with |__init__| other than |object.__init__|.
...

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:21: return len(args) == len(defaults)
+ 1
A comment would help here.

# Return true if |self| is only arg without a default.
return len(args) == len(defaults) + 1

[dtu, 2014-10-06 21:30:22]

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/util.py (right):

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/util.py:148: def GetAbsPathIfExist(path,
base_dir):
Is this the same as os.path.realpath(os.path.relpath(path, base_dir))?

If you decide to keep this function, it should go in telemetry.util.path.

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/__init__.py (right):

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/__init__.py:29: logging.error('Invalid
credentials path: %s' % credentials_path)
We need to do _UpdateCredentials before we do this check. Either move this to
PageRunner or move that here. (Yes, I think both of these solutions are not very
clean.)

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/page_set_smoke_test.py (right):

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/page_set_smoke_test.py:111: except TypeError:
What is this except block for in the first place? I just ran perf and gpu unit
tests without it, and they still pass.

[nednguyen, 2014-10-07 20:11:10]

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/util.py (right):

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/util.py:148: def GetAbsPathIfExist(path,
base_dir):
On 2014/10/06 21:30:22, dtu wrote:
> Is this the same as os.path.realpath(os.path.relpath(path, base_dir))?

Not really, this returns path if path is already an absolute path.
> 
> If you decide to keep this function, it should go in telemetry.util.path.

Done.

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/__init__.py (right):

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/__init__.py:29: logging.error('Invalid
credentials path: %s' % credentials_path)
On 2014/10/06 21:30:22, dtu wrote:
> We need to do _UpdateCredentials before we do this check. Either move this to
> PageRunner or move that here. (Yes, I think both of these solutions are not
very
> clean.)

Done. But how does cloud_storage take care of relative path vs absolute path?

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/page_set_smoke_test.py (right):

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/page_set_smoke_test.py:111: except TypeError:
On 2014/10/06 21:30:22, dtu wrote:
> What is this except block for in the first place? I just ran perf and gpu unit
> tests without it, and they still pass.

There are some page_set that aren't directly constructable in perf/internal. I
guess Tony added this except to ignore them all. 
However, when I remove credentials_path argument from PageSet.__init__, this try
catch block make this test still pass even if I don't remove the
crendentials_path call in super(PageSetX, self).__init__(...)

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/unittest/page_set_smoke_test.py (right):

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/unittest/page_set_smoke_test.py:113: raise
On 2014/10/06 21:03:00, slamm_google wrote:
> Why is it okay to continue for classes that are not directly constructable?
> Add a comment?

Done.

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/classes.py (right):

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:9: """ Returns True if instance of
|cls| can be construct without arguments. """
On 2014/10/06 21:03:01, slamm_google wrote:
> No spaces immediately inside the triple quotes.
> 
> """Returns True if instance of |cls| can be construct without arguments."""

Done.

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:12: if not hasattr(cls, '__init__'):
On 2014/10/06 21:03:01, slamm_google wrote:
> Might be better to put case comments inside the if statements:
> 
> if not hasattr(cls, '__init__'):
>   # Case |class A: pass|.
>   return True
> if cls.__init__ is object.__init__:
>   # Case |class A(object): pass|.
>   return True
> # Case |class (object):| with |__init__| other than |object.__init__|.
> ...

Done.

https://codereview.chromium.org/625123002/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/classes.py:21: return len(args) == len(defaults)
+ 1
On 2014/10/06 21:03:01, slamm_google wrote:
> A comment would help here.
> 
> # Return true if |self| is only arg without a default.
> return len(args) == len(defaults) + 1

Done.

[slamm_google, 2014-10-07 22:45:36]

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/unittest/page_set_smoke_test.py (right):

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/unittest/page_set_smoke_test.py:111: # don't know what
arguments to put for the constructor.
Thanks for adding the comment.

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/path.py (right):

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/path.py:47: """ Returns the absolute path for
|path| if it exists.
No space after the triple quotes.

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/path.py:50: path: a string that specify either a
relative or absolute path.
How about the following?

path: a relative or absolute path as a string.

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/path.py:52: which it's relative to.
Indent 4 spaces.

Also,
...is the directory to which it is relative.

Do not use a preposition to end a sentence with. :-)

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/path.py:55: The absolute and canonical path of
|path| if it exists. None if the path
The absolute, canonical path of |path|, if it exists; otherwise, None.

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/path.py:62: abs_path = os.path.join(base_dir,
path)
It just dawned on me that os.path.join returns |path| if it is absolute, so the
whole function reduces to the following:

    abs_path = os.path.realpath(os.path.join(base_dir, path))
    return abs_path if os.path.exists(abs_path) else None

https://docs.python.org/2/library/os.path.html#os.path.join
    [...] If any component is an absolute path, all previous components
    (on Windows, including the previous drive letter, if there was one)
    are thrown away, and joining continues.

[slamm_google, 2014-10-07 22:55:11]

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/path_unittest.py (right):

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/path_unittest.py:28: 
You can keep the setUp/tearDown specific to these new tests:

class GetAbsPathIfExistTest(unittest.TestCase):
  def setUp(self):
    self.test_directory = tempfile.mkdtemp()

  def tearDown(self):
    shutil.rmtree(self.test_directory)

It makes room for more descriptive test names too:

  def testAbsPathThatExistsReturnsSamePath(self):
     ...

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/path_unittest.py:33: f.name,
self.test_directory))
A little easier to read:

self.assertEquals(
    f.name,
    path_module.GetAbsPathIfExist(f.name, base_dir=self.test_directory))

[nednguyen, 2014-10-08 02:04:46]

nednguyen@google.com changed reviewers:
- nduca@chromium.org, tonyg@chromium.org

[nednguyen, 2014-10-08 02:04:47]

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/util/path.py (right):

https://codereview.chromium.org/625123002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/util/path.py:62: abs_path = os.path.join(base_dir,
path)
On 2014/10/07 22:45:34, slamm_google wrote:
> It just dawned on me that os.path.join returns |path| if it is absolute, so
the
> whole function reduces to the following:
> 
>     abs_path = os.path.realpath(os.path.join(base_dir, path))
>     return abs_path if os.path.exists(abs_path) else None
> 
> https://docs.python.org/2/library/os.path.html#os.path.join
>     [...] If any component is an absolute path, all previous components
>     (on Windows, including the previous drive letter, if there was one)
>     are thrown away, and joining continues.

Hmh, interesting, I would remove this function then.

[nednguyen, 2014-10-08 15:28:28]

Patchset #6 (id:160001) has been deleted

[dtu, 2014-10-08 21:34:19]

lgtm

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/util.py (right):

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/util.py:148: def GetAbsPathIfExist(path,
base_dir):
On 2014/10/07 20:11:09, nednguyen wrote:
> On 2014/10/06 21:30:22, dtu wrote:
> > Is this the same as os.path.realpath(os.path.relpath(path, base_dir))?
> 
> Not really, this returns path if path is already an absolute path.
> > 
> > If you decide to keep this function, it should go in telemetry.util.path.
> 
> Done.

This is true of realpath(relpath()) as well, but if os.path.join works, that's
also great.

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/unittest/page_set_smoke_test.py (right):

https://codereview.chromium.org/625123002/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/unittest/page_set_smoke_test.py:111: except TypeError:
On 2014/10/07 20:11:09, nednguyen wrote:
> On 2014/10/06 21:30:22, dtu wrote:
> > What is this except block for in the first place? I just ran perf and gpu
unit
> > tests without it, and they still pass.
> 
> There are some page_set that aren't directly constructable in perf/internal. I
> guess Tony added this except to ignore them all. 
> However, when I remove credentials_path argument from PageSet.__init__, this
try
> catch block make this test still pass even if I don't remove the
> crendentials_path call in super(PageSetX, self).__init__(...)

Interesting, okay.

[nednguyen, 2014-10-08 21:51:42]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-10-08 21:57:05]

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

[slamm, 2014-10-08 22:46:17]

lgtm

[commit-bot: I haz the power, 2014-10-09 04:55:43]

Committed patchset #6 (id:180001) as a42e7312e1147153bb0eb1dece71191bb031b837

[commit-bot: I haz the power, 2014-10-09 04:56:55]

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