Chrome remote control multipage tests: Add interactions.

Chrome remote control multipage tests: Add interactions.

The page set .json description defines what kind of interactions (e.g., link
clicks) are possible for a page. The corresponding Web Page Replay .wpr file
must contain replies to the requests sent when the interaction is executed.

This will be used in memory tests (see
http://codereview.chromium.org/11273081/ ).

The top_25_usage.json is a proof of concept page set description adding a couple
of interactions. When it's more complete, the existing tests will be migrated to
use it.

Parts of the code are stolen from nduca@chromium.org (see
http://codereview.chromium.org/11114020/ ).

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

[marja, 2012-11-05 14:46:43]

This is the interaction part of http://codereview.chromium.org/11273081/

nduca, tonyg: could you review this? Thanks!

[tonyg, 2012-11-06 00:28:20]

lgtm, but Nat should approve

https://codereview.chromium.org/11369075/diff/3001/tools/perf/page_sets/top_2...
File tools/perf/page_sets/top_25_usage.json (right):

https://codereview.chromium.org/11369075/diff/3001/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25_usage.json:12: "interactions": [
What do you think about adding this to top_25.json rather than creating a new
set? Am I missing a strong reason to have a new one?

[nduca, 2012-11-06 05:03:27]

I think this needs a few more iterations.

http://codereview.chromium.org/11369075/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/interaction.py (right):

http://codereview.chromium.org/11369075/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/interaction.py:4: 
interaction -> page_interaction? PageInteraction, etc?

http://codereview.chromium.org/11369075/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/interaction_clickLink.js
(right):

http://codereview.chromium.org/11369075/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/interaction_clickLink.js:2: //
Use of this source code is governed by a BSD-style license that can be
non normative file name. click_link_interaction.js ... and, where's the .py
file? And the unit test for the interaction to show that it is working?

http://codereview.chromium.org/11369075/diff/3001/tools/perf/page_sets/top_25...
File tools/perf/page_sets/top_25_usage.json (right):

http://codereview.chromium.org/11369075/diff/3001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:12: "interactions": [
Agreed, I think this is a must have--- top25 should be shared.

Not loving the verbosity of having to specify every different interaction with
an object. What about

  interactions: ["click_link", "scroll"]
  clickable_links: ["Women for Obama", "About", "Home"]?

If i read this right, you're supposed to do each one in sequence, right? E.g.
click women, then wait, then click about, then wait, then click home? If thats
the case, then you really may want a better name. Like "click_sequence", where
you have this instead:

interactions: ["scroll", "click_sequence"],
click_sequence: [
  {action: "click_link", link_text: "foo"},
  {action: "wait_for_navigation_to_complete"},
}

which brings up the point, should you make the action between each click
explicit?

finally, yno use a selector instead of link text? E.g a[text=blah]?

[nduca, 2012-11-06 05:17:22]

Also, I was thinking about how the goal here is to have some pages support a
"stress_memory" interaction. Maybe the way to factor this is the following.

CompoundPageInteractionBase: PageInteraction
  # Represenets a sequence of things you might do to a page. This isn't
  # regsitered with the pageinteraction code. Rather, you subclass it when you
want
  # to build an interaction for a specific purpose, e.g. memory stress test.
  def __init__(self, interaction_list)
    # where step is an array of interaction names or something

  def performInteraction(*args):
    for i in self.child_interactoins:
      getInteractionNamed(i).performInteraction(args)

class ClickLinkInteraction
   takes link to click AND "how to know when the interaction is done" parameters
  # Represenets a sequence of things you might do to a page. This isn't
  # regsitered with the pageinteraction code. Rather, you subclass it when you
want
  # to build an interaction for a specific purpose, e.g. memory stress test.


class StressMemoryInteraction(CompoundPageInteraction):
   # Actually registered with the interaction manager. A memory test would
create on of these, e.g.
   interactions: ["stress_memory"],
   stress_memory: [
      {interaction: "scroll"},
      {interaction: "click", link_text: "Vote Tomorrow", click_done:
"navigation_happened"}
   ]

Wdyt?

[marja, 2012-11-06 16:01:20]

Thanks for comments!

I'm adding this work-in-progress patch set so that we can discuss it today; I'll
add tests as soon as we've agreed on the approach to take.

http://codereview.chromium.org/11369075/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/interaction.py (right):

http://codereview.chromium.org/11369075/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/interaction.py:4: 
On 2012/11/06 05:03:27, nduca wrote:
> interaction -> page_interaction? PageInteraction, etc?

Done.

http://codereview.chromium.org/11369075/diff/3001/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/interaction_clickLink.js
(right):

http://codereview.chromium.org/11369075/diff/3001/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/interaction_clickLink.js:2: //
Use of this source code is governed by a BSD-style license that can be
On 2012/11/06 05:03:27, nduca wrote:
> non normative file name. click_link_interaction.js ... and, where's the .py
> file? And the unit test for the interaction to show that it is working?

There is no .py; test_helper_interaction.py (which I renamed to
javascript_runner_interaction.py) dispatches to all these files, and thus the
non-standard file name.

"clickLink" in the .json -> load interaction_clickLink.js and call
window.__clickLink(json_object).

But I see in another comment that you already used "click_link" as a name for
the action, so I can instead dispatch like this:

"click_link" in the .json -> load interaction_click_link.js and call
window.__click_link(json_object).

That'll create a nonstandard js function name, but I think it's nicer to have
that than to have the "click_link" <-> "clickLink" dispatching step somewhere.
Wdyt?

http://codereview.chromium.org/11369075/diff/3001/tools/perf/page_sets/top_25...
File tools/perf/page_sets/top_25_usage.json (right):

http://codereview.chromium.org/11369075/diff/3001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:12: "interactions": [
On 2012/11/06 05:03:27, nduca wrote:
> Agreed, I think this is a must have--- top25 should be shared.
> 
> Not loving the verbosity of having to specify every different interaction with
> an object. What about
> 
>   interactions: ["click_link", "scroll"]
>   clickable_links: ["Women for Obama", "About", "Home"]?
> 
> If i read this right, you're supposed to do each one in sequence, right? E.g.
> click women, then wait, then click about, then wait, then click home? If thats
> the case, then you really may want a better name. Like "click_sequence", where
> you have this instead:
> 
> interactions: ["scroll", "click_sequence"],
> click_sequence: [
>   {action: "click_link", link_text: "foo"},
>   {action: "wait_for_navigation_to_complete"},
> }
> 
> which brings up the point, should you make the action between each click
> explicit?
> 
> finally, yno use a selector instead of link text? E.g a[text=blah]?

Merged this with the top_25.json. This means that when I update the .wpr file in
the repository, the scroll benchmark results won't be comparable to the previous
version (because the pages will contain different data).

Yes, it's a sequence, so, "About" is a link on the "Women for Obama" page.

I'd have just one sequence of interactions, where each interaction can be e.g.,
click or scroll. So we could define a test which clicks a link, scrolls the
page, clicks another link, etc. I wouldn't put "stress_memory" into the name,
since that action sequence is generic, and after executing the actions, we can
measure multiple things, not only memory.

Your comments had a couple of ideas how to note down the action sequence and how
to add the completion condition, the latest patch set has:
"interaction_sequence": [
    {
        "action": "click_link",
        "selector": "a[href=\\\"http://www.facebook.com/WomenforObama\\\"]",
	"action_done": "navigation_complete"
    }, ...
}

Selectors... you meant document.querySelector(), right? Hmm, I can't pass
a[text="foobar"] to it, since "text" is not an attribute of the "a".

So this is a bit ugly:
"selector": "a[href=\\\"http://www.facebook.com/WomenforObama\\\"]"

[marja, 2012-11-06 16:36:07]

(Simplification below)

http://codereview.chromium.org/11369075/diff/3001/tools/perf/page_sets/top_25...
File tools/perf/page_sets/top_25_usage.json (right):

http://codereview.chromium.org/11369075/diff/3001/tools/perf/page_sets/top_25...
tools/perf/page_sets/top_25_usage.json:12: "interactions": [
On 2012/11/06 16:01:21, marja wrote:
> On 2012/11/06 05:03:27, nduca wrote:
> > Agreed, I think this is a must have--- top25 should be shared.
> > 
> > Not loving the verbosity of having to specify every different interaction
with
> > an object. What about
> > 
> >   interactions: ["click_link", "scroll"]
> >   clickable_links: ["Women for Obama", "About", "Home"]?
> > 
> > If i read this right, you're supposed to do each one in sequence, right?
E.g.
> > click women, then wait, then click about, then wait, then click home? If
thats
> > the case, then you really may want a better name. Like "click_sequence",
where
> > you have this instead:
> > 
> > interactions: ["scroll", "click_sequence"],
> > click_sequence: [
> >   {action: "click_link", link_text: "foo"},
> >   {action: "wait_for_navigation_to_complete"},
> > }
> > 
> > which brings up the point, should you make the action between each click
> > explicit?
> > 
> > finally, yno use a selector instead of link text? E.g a[text=blah]?
> 
> Merged this with the top_25.json. This means that when I update the .wpr file
in
> the repository, the scroll benchmark results won't be comparable to the
previous
> version (because the pages will contain different data).
> 
> Yes, it's a sequence, so, "About" is a link on the "Women for Obama" page.
> 
> I'd have just one sequence of interactions, where each interaction can be
e.g.,
> click or scroll. So we could define a test which clicks a link, scrolls the
> page, clicks another link, etc. I wouldn't put "stress_memory" into the name,
> since that action sequence is generic, and after executing the actions, we can
> measure multiple things, not only memory.
> 
> Your comments had a couple of ideas how to note down the action sequence and
how
> to add the completion condition, the latest patch set has:
> "interaction_sequence": [
>     {
>         "action": "click_link",
>         "selector": "a[href=\\\"http://www.facebook.com/WomenforObama\\\"]",
> 	"action_done": "navigation_complete"
>     }, ...
> }
> 
> Selectors... you meant document.querySelector(), right? Hmm, I can't pass
> a[text="foobar"] to it, since "text" is not an attribute of the "a".
> 
> So this is a bit ugly:
> "selector": "a[href=\\\"http://www.facebook.com/WomenforObama\\\"]"

I simplified this to
"selector": "a[href=\"http://www.facebook.com/WomenforObama\"]"
and also eliminated this unnecessary round trip of passing the interaction as a
json string instead of an object and converting it back on the js side.

[marja, 2012-11-06 19:51:28]

I refactored this based on discussions with nduca@.

nduca: could you have another look? This CL doesn't yet contain a test, but is
does the code (which we agreed will be in the first CL) look ok?

[nduca, 2012-11-07 04:13:49]

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/click_element.js (right):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element.js:3: // found
in the LICENSE file.
how about click_element_interaction.js? we'd of course rename scroll.js to
scroll_interaction.js to match the py file

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element.js:8: function
clickElement(interaction) {
I'm kind of skeptical why youre even injecting javascript code here at all. This
is very simple, if you do this straight in python: 

def PerformInteraction(self, page, tab):
  assert page['selector']
 
tab.runtime.ExecuteJavascript("document.querySelector(page['selector']).click());

Done. No code injection required. No javascript required.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File
tools/chrome_remote_control/chrome_remote_control/click_element_interaction.py
(right):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element_interaction.py:8:
javascript_runner_interaction.JavaScriptRunnerInteraction):
how about movign these to interaction.py but as methods, like

InjectJavascriptIfNeeded(filename, tab)?

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element_interaction.py:14:
test.WillPerformInteraction(self, page, tab)
I think the runner should call this, not the interaction itself.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element_interaction.py:17:
self.InjectCode(tab)
self.InjectodeIfNeeded(tab, "click_element.js")

Why is MakeJSCall() needed? I'm not comfortable with this part of code being in
the base class...

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/inspector_page.py (left):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/inspector_page.py:19:
logging.debug('Notification: %s', json.dumps(msg, indent=2))
dont forget a unit test for the new method in this file.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/inspector_page.py
(right):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/inspector_page.py:29: def
PerformActionAndWaitForNavigate(self, action_function, timeout=60):
Love it, but, add some doc strings to this, explaining the action function.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/inspector_page.py:37:
action_function()
if action_function raises an exception, then we're going to leave Page.enable
on. Catch, Page.disable, then raise?

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py
(right):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:9:
class JavaScriptRunnerInteraction(page_interaction.PageInteraction):
Once you do the chagnes below, you should be able to move the
InjectScriptIfNeeded down to the PageInteraction class.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:19:
def PerformInteraction(self, page, tab, test):
Would ratehr this not be implemented.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:29:
def InjectCode(self, tab):
How about making this be InjectFileIfNeeded(self, tab, filename)? THen you dont
need to pass _js_foilename to the constructor.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:34:
def MakeJSCall(self, tab):
I dont think this helper is needed. We should make the call directly in the
parent. Then you dont need to store data on the base class at all.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:41:
# For simple waiting conditions, subclasses can just override this
I'd rather the place that makes the call just does the WaitForCompletion logic
directly.

[marja, 2012-11-07 10:52:27]

Thanks for comments, can you have a look at the simplified new version?

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/click_element.js (right):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element.js:3: // found
in the LICENSE file.
On 2012/11/07 04:13:49, nduca wrote:
> how about click_element_interaction.js? we'd of course rename scroll.js to
> scroll_interaction.js to match the py file

This file is removed, and I renamed the python file to
click_to_navigate_interaction.py, since it's not only about links but arbitrary
HTML elements, and it can only handle clicks that result in a navigation
(because it waits for the navigation to happen).

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element.js:8: function
clickElement(interaction) {
On 2012/11/07 04:13:49, nduca wrote:
> I'm kind of skeptical why youre even injecting javascript code here at all.
This
> is very simple, if you do this straight in python: 
> 
> def PerformInteraction(self, page, tab):
>   assert page['selector']
>  
>
tab.runtime.ExecuteJavascript("document.querySelector(page['selector']).click());
> 
> Done. No code injection required. No javascript required.

Ok, this makes sense since the abstraction "all interactions are just loading a
js file and calling a function on it" is gone, and I also removed the
JavaScriptRunnerInteraction (it's not needed atm).

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File
tools/chrome_remote_control/chrome_remote_control/click_element_interaction.py
(right):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element_interaction.py:8:
javascript_runner_interaction.JavaScriptRunnerInteraction):
On 2012/11/07 04:13:49, nduca wrote:
> how about movign these to interaction.py but as methods, like
> 
> InjectJavascriptIfNeeded(filename, tab)?

This is removed for now (I can add it in the next CL if it simplifies the
scrolling interaction).

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element_interaction.py:14:
test.WillPerformInteraction(self, page, tab)
On 2012/11/07 04:13:49, nduca wrote:
> I think the runner should call this, not the interaction itself.

Done. (This pattern was adopted from your CL,
http://codereview.chromium.org/11114020/ .)

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/click_element_interaction.py:17:
self.InjectCode(tab)
On 2012/11/07 04:13:49, nduca wrote:
> self.InjectodeIfNeeded(tab, "click_element.js")
> 
> Why is MakeJSCall() needed? I'm not comfortable with this part of code being
in
> the base class...

This code is now removed.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/inspector_page.py (left):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/inspector_page.py:19:
logging.debug('Notification: %s', json.dumps(msg, indent=2))
On 2012/11/07 04:13:49, nduca wrote:
> dont forget a unit test for the new method in this file.

Done.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File tools/chrome_remote_control/chrome_remote_control/inspector_page.py
(right):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/inspector_page.py:29: def
PerformActionAndWaitForNavigate(self, action_function, timeout=60):
On 2012/11/07 04:13:49, nduca wrote:
> Love it, but, add some doc strings to this, explaining the action function.

Done.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/inspector_page.py:37:
action_function()
On 2012/11/07 04:13:49, nduca wrote:
> if action_function raises an exception, then we're going to leave Page.enable
> on. Catch, Page.disable, then raise?

Done.

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
File
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py
(right):

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:9:
class JavaScriptRunnerInteraction(page_interaction.PageInteraction):
On 2012/11/07 04:13:49, nduca wrote:
> Once you do the chagnes below, you should be able to move the
> InjectScriptIfNeeded down to the PageInteraction class.

(This file has been removed. I can add the helpers to the PageInteraction class
when I'm refactoring the scroll interaction in  the next CL.)

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:19:
def PerformInteraction(self, page, tab, test):
On 2012/11/07 04:13:49, nduca wrote:
> Would ratehr this not be implemented.

(This file has been removed.)

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:29:
def InjectCode(self, tab):
On 2012/11/07 04:13:49, nduca wrote:
> How about making this be InjectFileIfNeeded(self, tab, filename)? THen you
dont
> need to pass _js_foilename to the constructor.

(This file has been removed.)

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:34:
def MakeJSCall(self, tab):
On 2012/11/07 04:13:49, nduca wrote:
> I dont think this helper is needed. We should make the call directly in the
> parent. Then you dont need to store data on the base class at all.

(This file has been removed.)

http://codereview.chromium.org/11369075/diff/3002/tools/chrome_remote_control...
tools/chrome_remote_control/chrome_remote_control/javascript_runner_interaction.py:41:
# For simple waiting conditions, subclasses can just override this
On 2012/11/07 04:13:49, nduca wrote:
> I'd rather the place that makes the call just does the WaitForCompletion logic
> directly.

(This file has been removed.)

[nduca, 2012-11-07 18:55:57]

http://codereview.chromium.org/11369075/diff/15009/tools/chrome_remote_contro...
File
tools/chrome_remote_control/chrome_remote_control/click_to_navigate_interaction.py
(right):

http://codereview.chromium.org/11369075/diff/15009/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/click_to_navigate_interaction.py:7:
def __init__(self, data):
what is data here? Is it the page object? Or some part of the page object? Lets
have a better name than "data" :)

http://codereview.chromium.org/11369075/diff/15009/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/click_to_navigate_interaction.py:12:
pass
return True?

http://codereview.chromium.org/11369075/diff/15009/tools/perf/page_sets/top_2...
File tools/perf/page_sets/top_25.json (right):

http://codereview.chromium.org/11369075/diff/15009/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:63: "interaction_sequence": [
so this part doesn't make sense without a bit more code. Can you make the
compound interaction as we discussed before?

I'd like this file to say this:
"interactions": ["scroll", "compound"]
"compound_interaction": [
  {"interaction": "click_to_navigate",
   "selector": ...},
]

[marja, 2012-11-08 10:24:00]

Hi again and thanks for comments!

In addition to the comments, I simplified the interactions a bit:

I removed SupportedForPage(), since in the current system there is no use case
for it; if the page set describes an interaction, it's also supported for the
page. (In your CL, the interaction was relying on some external data being
present, but in this CL, all the data is in the interaction itself.)

I removed WillPerformInteraction and DidPerformInteraction from page_test.py,
since there's no use for them either; if the page test runs the interactions, it
can call whatever functions it wants before and after running them. It would've
been anyway responsible of calling WillPerform and DidPerform, since you didn't
want the interaction to call them itself, as they do in your original CL. So
they're just unnecessary.

http://codereview.chromium.org/11369075/diff/15009/tools/chrome_remote_contro...
File
tools/chrome_remote_control/chrome_remote_control/click_to_navigate_interaction.py
(right):

http://codereview.chromium.org/11369075/diff/15009/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/click_to_navigate_interaction.py:7:
def __init__(self, data):
On 2012/11/07 18:55:57, nduca wrote:
> what is data here? Is it the page object? Or some part of the page object?
Lets
> have a better name than "data" :) 

I changed this to follow the same pattern as page.py, so, the variable is called
"attributes" and it sets the attributes of the current object based on the data.

http://codereview.chromium.org/11369075/diff/15009/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/click_to_navigate_interaction.py:12:
pass
On 2012/11/07 18:55:57, nduca wrote:
> return True?

Done. (Oops!)

http://codereview.chromium.org/11369075/diff/15009/tools/perf/page_sets/top_2...
File tools/perf/page_sets/top_25.json (right):

http://codereview.chromium.org/11369075/diff/15009/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:63: "interaction_sequence": [
On 2012/11/07 18:55:57, nduca wrote:
> so this part doesn't make sense without a bit more code. Can you make the
> compound interaction as we discussed before?
> 
> I'd like this file to say this:
> "interactions": ["scroll", "compound"]
> "compound_interaction": [
>   {"interaction": "click_to_navigate",
>    "selector": ...},
> ]

Ok, for this I needed to add compound interactions (compound interaction is also
a PageInteraction, and the json description of it must obey the same format...)
and the compound interaction runner.

So, I added this:
"tests": {
    "stress_memory": {
        "action": "compound",
        "actions": [
        {
            "action": "click_to_navigate",
            "selector": ...
         }, ...

So, every test is one interaction (which can be a compound interaction), and in
the next CL, I can describe the scroll interaction in the same format. Ok?

And now the memory test runner is very simple:
interaction_data = page.tests['stress_memory']
i =
page_interaction.FindClassWithName(interaction_data['action'])(interaction_data)
i.PerformInteraction(page, tab)
# get the mem histograms

[nduca, 2012-11-08 19:23:17]

lgtm with nit

http://codereview.chromium.org/11369075/diff/11004/tools/perf/page_sets/top_2...
File tools/perf/page_sets/top_25.json (right):

http://codereview.chromium.org/11369075/diff/11004/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:63: "tests": {
lets take out the "tests:" bit

[marja, 2012-11-08 19:45:26]

Thanks for review! I'll commit this now.

http://codereview.chromium.org/11369075/diff/11004/tools/perf/page_sets/top_2...
File tools/perf/page_sets/top_25.json (right):

http://codereview.chromium.org/11369075/diff/11004/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:63: "tests": {
On 2012/11/08 19:23:17, nduca wrote:
> lets take out the "tests:" bit

Done.

[marja, 2012-11-08 19:53:17]

Note: run_tests for telemetry doesn't pass, but the same tests are failing
before and after this CL.

[dtu, 2012-11-08 20:35:03]

I'll take a look at the run_tests failures. They're not related to this CL.