Add support for python pageset

Add support for python pageset

BUG=239179
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259819

[nduca, 2014-02-27 18:26:57]

https://codereview.chromium.org/180873008/diff/1/tools/perf/page_sets/top_10.py
File tools/perf/page_sets/top_10.py (right):

https://codereview.chromium.org/180873008/diff/1/tools/perf/page_sets/top_10....
tools/perf/page_sets/top_10.py:37: #top google property; a google tab is often
open
from our chat

class Top10(PageSet):
  def __init__(self):
    class SimplePage(page.Page):
      def smoothness(self, actions):
         actions.scroll(....)

    self.AddPage(SimplePage("http://techcrunch.com"))
    self.AddPage(SimplePage("http://plus.google.com", credeitals="google"))

    class Gmail(page.Page):
      def __init__(self):
         super(Gmail, self).__init__('http://gmail', credeitals='google')

      def navigate(self, actions):
        super(Gmail, self).navigate(actions)
        actions.wait.WaitUntilTrue(
                 expression='''window.gmonkey !== undefined &&
 48 document.getElementById('gb') !== null''')
  
      def scroll(self, actions)
        actions.scroll(....)        
    self.AddPage(Gmail())

[nednguyen, 2014-02-27 19:49:32]

https://codereview.chromium.org/180873008/diff/1/tools/perf/page_sets/top_10.py
File tools/perf/page_sets/top_10.py (right):

https://codereview.chromium.org/180873008/diff/1/tools/perf/page_sets/top_10....
tools/perf/page_sets/top_10.py:37: #top google property; a google tab is often
open
On 2014/02/27 18:26:57, nduca wrote:
> from our chat
> 
> class Top10(PageSet):
>   def __init__(self):
>     class SimplePage(page.Page):
>       def smoothness(self, actions):
>          actions.scroll(....)
> 
>     self.AddPage(SimplePage("http://techcrunch.com"))
>     self.AddPage(SimplePage("http://plus.google.com", credeitals="google"))
> 
>     class Gmail(page.Page):
>       def __init__(self):
>          super(Gmail, self).__init__('http://gmail', credeitals='google')
> 
>       def navigate(self, actions):
>         super(Gmail, self).navigate(actions)
>         actions.wait.WaitUntilTrue(
>                  expression='''window.gmonkey !== undefined &&
>  48 document.getElementById('gb') !== null''')
>   
>       def scroll(self, actions)
>         actions.scroll(....)        
>     self.AddPage(Gmail())

Done.

[nduca, 2014-03-04 03:29:28]

some thoughts. lets chat tomorrow?

https://codereview.chromium.org/180873008/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/page_action.py (right):

https://codereview.chromium.org/180873008/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/page_action.py:26: def
SetAttributes(self, attributes):
who calls this?

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

https://codereview.chromium.org/180873008/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:271: if
page_set.is_python_pageset:
how can we avoid this up front? I'd like this file to behave the same way each
time... if that means making more methods on the page_set base class then thats
fine. But we definitely want to avoid a big fork. I'd like the page_set object
to run exactly the same way from the runner point of view --- as you can see,
this file is rather large.

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

https://codereview.chromium.org/180873008/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test_runner.py:114: elif
page_set_arg.endswith('.py'):
i wonder, can you look into python's importlib and see if there's a way to get a
module from a file? We'd want to make sure we import it with the correct name,
not eval it, but I think there's a way to do that.... that way there're no edits
to this file...

ideally, PageSet.FromFile would do the right thing in all ases.

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

https://codereview.chromium.org/180873008/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/python_page_set.py:4: 
L:ets figure out how to not make this file not required.

Ideally, in this patch, PageSet.FromFile reads the json and then does a FromDict
call, then the FromDict automatically converts that to the same equivalent calls
that you'd see if the pageset was python. I'd like to avoid us having a new
class, even as an intermedaite state.

[nduca, 2014-03-05 01:10:43]

https://codereview.chromium.org/180873008/diff/80001/tools/perf/page_sets/top...
File tools/perf/page_sets/top_10.py (right):

https://codereview.chromium.org/180873008/diff/80001/tools/perf/page_sets/top...
tools/perf/page_sets/top_10.py:90: class Facebook(SimpleScrollPage):
mind hoisting these inner classes out to toplevel? general feedback is that its
clearer...

[nduca, 2014-03-05 03:00:56]

looking solid, more feedback :)

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/wait.py (right):

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/wait.py:21: def UntilTrue(self,
expression):
I think in the long term,  I'd rather we remove these and switch the page to
call stuff like:

action_runner.WaitUntilTrue(...)

And then this file ends up exporting just a bunch of WaitUntil* methods, and
action_runner has a bunch of very simple WaitUntil methods that call into this
module.

Point being, I dont think there's any need for actions to have state, in the
end, except as functions.

As such, for the very very near term, lets just make actions work as they do,
meaning the python page set is going to be a bit ugly and have to make various
dicts... on the assumpiton that we'll do a followup to move the action runner to
be functions.

Wdyt?

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

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page.py:48: def AddCustomizeBrowserOptions(self,
options):
which flags do we end up having to add/customize from this call? If its all just
--enable-gpu-benchmarking, maybe we just turn it on all the time...

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

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:51: def Initialize(self, attributes):
_InitializeFromDict(self, dict) and only call from FromDict so that someone
can't do

ps = PageSet(...)
ps.Add(...)
ps.InitializeFromDict({})

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:117: @classmethod
@staticmethod? doesn't seem to need cls

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:141: setattr(page_class,
_RenamePageSubMethod(method_name), method)
I wonder, how about having the page_test file look for RunX all the time, and
then make the _Initialize method create method objects on the object for the
json backcompat path?

I think the idea would be to make a function that is a closure that runs the
action right away, then add that as an attribute on the class.

AddActionMethodToPageFromDict(page, initial_action_name, action_dict):
   def RunInitialAction(self, action_runner):
     # like the GetCompoundActionsRecursive stuff, but here its just run the
dictionary right awawy

  setattr(page, 'Run' + ToCamelCase(initial_action_name), RunInitialAction)

If thats not quite the exact way shout and I can work with you to figure out the
python voodoo.

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:206: def
AddCustomizeBrowserOptions(self, options):
lets try to avoid these and talk about the total set of corner cases that cause
us to need this set of entry points?

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

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:254: compound_action = getattr(page,
self._action_name_to_run)
compound_action -> run_method?

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:266: page.navigate_steps(runner)
method_name -> MethodName

and maybe just call this...
RunNavigate():

[nednguyen, 2014-03-05 04:54:04]

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/wait.py (right):

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/wait.py:21: def UntilTrue(self,
expression):
On 2014/03/05 03:00:57, nduca wrote:
> I think in the long term,  I'd rather we remove these and switch the page to
> call stuff like:
> 
> action_runner.WaitUntilTrue(...)
> 
> And then this file ends up exporting just a bunch of WaitUntil* methods, and
> action_runner has a bunch of very simple WaitUntil methods that call into this
> module.
> 
> Point being, I dont think there's any need for actions to have state, in the
> end, except as functions.
> 
> As such, for the very very near term, lets just make actions work as they do,
> meaning the python page set is going to be a bit ugly and have to make various
> dicts... on the assumpiton that we'll do a followup to move the action runner
to
> be functions.
> 
> Wdyt?

sgtm :)

[nednguyen, 2014-03-05 20:53:31]

https://codereview.chromium.org/180873008/diff/80001/tools/perf/page_sets/top...
File tools/perf/page_sets/top_10.py (right):

https://codereview.chromium.org/180873008/diff/80001/tools/perf/page_sets/top...
tools/perf/page_sets/top_10.py:90: class Facebook(SimpleScrollPage):
On 2014/03/05 01:10:43, nduca wrote:
> mind hoisting these inner classes out to toplevel? general feedback is that
its
> clearer...

Done.

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

https://codereview.chromium.org/180873008/diff/80001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_set.py:51: def Initialize(self, attributes):
On 2014/03/05 03:00:57, nduca wrote:
> _InitializeFromDict(self, dict) and only call from FromDict so that someone
> can't do
> 
> ps = PageSet(...)
> ps.Add(...)
> ps.InitializeFromDict({})

Done.

[nduca, 2014-03-06 22:41:45]

lmk on chat when you're ready for another review pass on this. i'm still a bit
confused whether we can get rid of commandline options

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

https://codereview.chromium.org/180873008/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_set.py:132: #pylint: disable=W0212
we typically put lint supressions on the same line, e.g.

foo._bar() # pylint: disable=everything

https://codereview.chromium.org/180873008/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_set.py:140: predicate=inspect.ismethod):
indentation

[nednguyen, 2014-03-06 23:43:31]

+jjtswan

[nduca, 2014-03-26 18:23:13]

lgtm

https://codereview.chromium.org/180873008/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page.py (right):

https://codereview.chromium.org/180873008/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page.py:25: if attributes:
can we remove the attributes argument and have it only be used by the fromdict
staticmethod?

[nednguyen, 2014-03-27 04:12:08]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-03-27 04:13:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nednguyen@google.com/180873008/260001

[commit-bot: I haz the power, 2014-03-27 07:55:19]

Change committed as 259819