Allow defining custom navigation on a per-page basis.

Allow defining custom navigation on a per-page basis.

Create explicit 'navigate' and 'javascript' actions

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

[edmundyan, 2013-07-29 23:40:51]

Initial patch, PTAL.

For some reason presubmit is copying+modifying javascript/navigate.py from
existing actions.  They should be brand new files.

javascript.py is a response to the "post_navigate_javascript_to_execute" I
posted on the crbug

This has some similar code to https://codereview.chromium.org/19518009/ in its
current state (just spliiting out the navigation from PreparePage).  However, I
believe they can be worked on in parallel.

I plan to update all the pagesets/recordings to use the new 'navigate_steps' (or
whatever it ends up being called) in a different CL after this one.

https://codereview.chromium.org/21174002/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/21174002/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/page_test.py:188: def _RunCompoundAction(self,
page, tab, actions, run_setup_methods=True):
I'm not entirely sure the reasons, but when running smoothness, the
SmoothnessMetric error'd out.  Thus, I had to skip these pre/post actions when
running the navigation actions.

Are some metrics not available until a page has been navigated to?

[dtu, 2013-07-31 22:08:49]

https://codereview.chromium.org/21174002/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/actions/javascript.py (right):

https://codereview.chromium.org/21174002/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/actions/javascript.py:8: class
JavascriptAction(page_action.PageAction):
I think a JavaScript Action is too general and easy for people to abuse, so I
don't like it. Unfortunately, I don't have a better solution for you at the
moment :\

post_navigate_javascript_to_execute is similar to another Test feature,
script_to_evaluate_on_commit, so maybe there's something you can do there?
Although I think it's undefined whether post_navigate_javascript_to_execute runs
before or after page load, and some tests might rely on it being after, not
sure..

https://codereview.chromium.org/21174002/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/actions/navigate.py (right):

https://codereview.chromium.org/21174002/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/actions/navigate.py:13:
tab.Navigate(page.target_side_url, page.script_to_evaluate_on_commit)
You can determine target_side_url here instead of storing it on the page.

https://codereview.chromium.org/21174002/diff/1/tools/telemetry/telemetry/pag...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/21174002/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/page_test.py:188: def _RunCompoundAction(self,
page, tab, actions, run_setup_methods=True):
On 2013/07/29 23:40:51, edmundyan wrote:
> I'm not entirely sure the reasons, but when running smoothness, the
> SmoothnessMetric error'd out.  Thus, I had to skip these pre/post actions when
> running the navigation actions.
> 
> Are some metrics not available until a page has been navigated to?

Yes, that is possibly true. And we don't want to run Will/DidRunAction more than
once per page iteration, so doing this makes sense.

[edmundyan, 2013-08-13 18:26:25]

Now that skip_navigate_on_repeat is landed, I've rebased this CL and addressed
Dave's changes.

I am also unsure if using "script_to_evaluate_on_commit" is the same, but
everything is still backward compatible so it should be fine.  We could also
just make 'post_navigate_javascript_to_execute' an attribute for the Navigate
action.  Which might be better actually.

[edmundyan, 2013-08-13 21:31:45]

Actually, I don't think re-using "script_to_evaluate_on_commit" is a good idea
as it could be defined elsewhere (not in the pageset) like in blink_perf.py.  I
don't know how you can run both JS scripts without some hacky concat or decide
which of the two to run?

Adding the old "post_navigate_javascript_to_execute" as a Navigate attribute
now.  Only issue I might see is that the JS is evaluated before the "wait"
actions, which might make some cases not work.  Uploaded the new patch.

[dennis_jeffrey, 2013-08-14 18:37:53]

I'll leave this for Dave to comment on the appropriateness of the structural
changes to Telemetry, but I do have some style nits.

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/actions/navigate_unittest.py (right):

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/navigate_unittest.py:1: # Copyright (c)
2012 The Chromium Authors. All rights reserved.
2012 --> 2013

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/navigate_unittest.py:4: import os
add a blank line above this

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/navigate_unittest.py:14: '..', '..',
'..', 'unittest_data')
replace .. with os.pardir

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/navigate_unittest.py:18: None # In this
test, we don't need a page set.
add one more space before the comment #

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/navigate_unittest.py:23: '..', '..',
'..', 'unittest_data')
.. --> os.pardir

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/navigate_unittest.py:26: page =
self.CreatePageFromUnittestDataDir("blank.html")
prefer single quotes in strings

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/page_runner.py:139: # how to get code coverage of
this
Dave, any insight into this?

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/page_test.py:225: run it as a compound action."""
move the """ down to the next line

[dtu, 2013-08-14 18:42:32]

On 2013/08/13 21:31:45, edmundyan wrote:
> Actually, I don't think re-using "script_to_evaluate_on_commit" is a good idea
> as it could be defined elsewhere (not in the pageset) like in blink_perf.py. 
I
> don't know how you can run both JS scripts without some hacky concat or decide
> which of the two to run?
> 
> Adding the old "post_navigate_javascript_to_execute" as a Navigate attribute
> now.  Only issue I might see is that the JS is evaluated before the "wait"
> actions, which might make some cases not work.  Uploaded the new patch.

I walked with Nat a bit, and he thinks having a JavaScript Action wouldn't be
the worst thing. There are also some other nice side effects of having a JS
action. So let's go with that route. Sorry! :$

[dtu, 2013-08-14 18:43:11]

On 2013/08/14 18:42:32, Dave Tu wrote:
> On 2013/08/13 21:31:45, edmundyan wrote:
> > Actually, I don't think re-using "script_to_evaluate_on_commit" is a good
idea
> > as it could be defined elsewhere (not in the pageset) like in blink_perf.py.

> I
> > don't know how you can run both JS scripts without some hacky concat or
decide
> > which of the two to run?
> > 
> > Adding the old "post_navigate_javascript_to_execute" as a Navigate attribute
> > now.  Only issue I might see is that the JS is evaluated before the "wait"
> > actions, which might make some cases not work.  Uploaded the new patch.
> 
> I walked with Nat a bit, and he thinks having a JavaScript Action wouldn't be
> the worst thing. There are also some other nice side effects of having a JS
> action. So let's go with that route. Sorry! :$

talked*

[edmundyan, 2013-08-14 22:06:12]

Done and added back Javascript page action. PTAL

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/page_runner.py:139: # how to get code coverage of
this
On 2013/08/14 18:37:54, dennis_jeffrey wrote:
> Dave, any insight into this?
Actually, I think this is only used by the tab_switching measurement.  Is this
being used?  It seems to accept weird pagesets, which should be used?

[dennis_jeffrey, 2013-08-14 22:21:04]

LGTM with a few minor comments.  Please wait for Dave to approve too.

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/actions/navigate_unittest.py (right):

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/navigate_unittest.py:20: None  # In this
test, we don't need a page set.
indent the above 2 lines by 2 more spaces each, since there's a hanging indent
due to line 18

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/page_test.py:230: for action in
navigate_actions):
interesting - i would have thought that we'd need to enclose the parameter to
any() inside square brackets since it's a list comprehension, but I checked in a
python interpreter and it looks like this works too.

[dtu, 2013-08-14 22:21:38]

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/21174002/diff/27001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/page_runner.py:139: # how to get code coverage of
this
On 2013/08/14 22:06:12, edmundyan wrote:
> On 2013/08/14 18:37:54, dennis_jeffrey wrote:
> > Dave, any insight into this?
> Actually, I think this is only used by the tab_switching measurement.  Is this
> being used?  It seems to accept weird pagesets, which should be used?

Yes, it's a hack that was fairly recently added to make that test work. In the
future, we're going to have a better solution for tests that use multiple tabs.

[dtu, 2013-08-14 23:04:32]

Looks good! Sorry again about the review mess. Some smaller comments.

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/actions/javascript.py (right):

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/javascript.py:13: assert hasattr(self,
'execute')
s/execute/expression

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/javascript.py:14:
tab.EvaluateJavaScript(self.execute)
ExecuteJavaScript (Evaluate may have some additional overhead in piping the
result back over the DevTools socket and will fail if the result is recursive or
too large)

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/page_test.py:234: else:
Instead of having a special case here, we should define this as the default
navigate_steps on the page object. I think that'll require converting the page
sets to use navigate_steps instead of WaitToLoad, so that can be TODO'd.

[edmundyan, 2013-08-14 23:37:58]

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/actions/javascript.py (right):

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/javascript.py:13: assert hasattr(self,
'execute')
On 2013/08/14 23:04:33, Dave Tu wrote:
> s/execute/expression

Done.

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/javascript.py:14:
tab.EvaluateJavaScript(self.execute)
On 2013/08/14 23:04:33, Dave Tu wrote:
> ExecuteJavaScript (Evaluate may have some additional overhead in piping the
> result back over the DevTools socket and will fail if the result is recursive
or
> too large)

Done.

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/actions/navigate_unittest.py (right):

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/actions/navigate_unittest.py:20: None  # In this
test, we don't need a page set.
On 2013/08/14 22:21:04, dennis_jeffrey wrote:
> indent the above 2 lines by 2 more spaces each, since there's a hanging indent
> due to line 18

Done.

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/page_test.py (right):

https://codereview.chromium.org/21174002/diff/39001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/page_test.py:234: else:
On 2013/08/14 23:04:33, Dave Tu wrote:
> Instead of having a special case here, we should define this as the default
> navigate_steps on the page object. I think that'll require converting the page
> sets to use navigate_steps instead of WaitToLoad, so that can be TODO'd.

Done.

[dtu, 2013-08-15 00:35:44]

lgtm

[commit-bot: I haz the power, 2013-08-15 00:42:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/edmundyan@chromium.org/21174002/50001

[commit-bot: I haz the power, 2013-08-15 07:23:57]

Change committed as 217762