Removing Page.WaitToLoad and update all pagesets to use explicit wait actions

Removing Page.WaitToLoad and update all pagesets to use explicit wait actions

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

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

[edmundyan, 2013-08-20 23:02:40]

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

https://codereview.chromium.org/22883011/diff/1/tools/telemetry/telemetry/pag...
tools/telemetry/telemetry/page/page.py:57: self.navigate_actions =
[navigate.NavigateAction()]
This is to address the TODO in page_test to make a default action

https://codereview.chromium.org/22883011/diff/3001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page/page.py (right):

https://codereview.chromium.org/22883011/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/page.py:57: self.navigate_actions =
[navigate.NavigateAction()]
This is to address the TODO in page_test to make a default action

[dtu, 2013-08-20 23:57:39]

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

https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:82: "wait_for_href_change": true
These kinds of waits can also be replaced with wait actions.

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

https://codereview.chromium.org/22883011/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/page_test.py:212: page.navigate_actions =
GetCompoundActionFromPage(page, 'navigate_steps')
Why assign it to page if you're not using it anywhere else?

[tonyg, 2013-08-21 00:04:33]

https://codereview.chromium.org/22883011/diff/3001/content/test/gpu/page_sets...
File content/test/gpu/page_sets/pixel_tests.json (right):

https://codereview.chromium.org/22883011/diff/3001/content/test/gpu/page_sets...
content/test/gpu/page_sets/pixel_tests.json:9: { "action": "wait", "condition":
"duration", "seconds": 4 }
Should this be "condition": "seconds"?

https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/2012Q...
File tools/perf/page_sets/2012Q3.json (right):

https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/2012Q...
tools/perf/page_sets/2012Q3.json:8: {"action": "wait", "condition": "seconds",
"seconds": 1},
Or should this be "condition": "duration"?

https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/mobil...
File tools/perf/page_sets/mobile_memory.json (right):

https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/mobil...
tools/perf/page_sets/mobile_memory.json:8: {"action": "wait", "condition":
"seconds", "seconds": 15},
Another seconds vs duration question

[dtu, 2013-08-21 00:14:37]

On 2013/08/21 00:04:33, tonyg wrote:
>
https://codereview.chromium.org/22883011/diff/3001/content/test/gpu/page_sets...
> File content/test/gpu/page_sets/pixel_tests.json (right):
> 
>
https://codereview.chromium.org/22883011/diff/3001/content/test/gpu/page_sets...
> content/test/gpu/page_sets/pixel_tests.json:9: { "action": "wait",
"condition":
> "duration", "seconds": 4 }
> Should this be "condition": "seconds"?
> 
>
https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/2012Q...
> File tools/perf/page_sets/2012Q3.json (right):
> 
>
https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/2012Q...
> tools/perf/page_sets/2012Q3.json:8: {"action": "wait", "condition": "seconds",
> "seconds": 1},
> Or should this be "condition": "duration"?
> 
>
https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/mobil...
> File tools/perf/page_sets/mobile_memory.json (right):
> 
>
https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/mobil...
> tools/perf/page_sets/mobile_memory.json:8: {"action": "wait", "condition":
> "seconds", "seconds": 15},
> Another seconds vs duration question

Hurrr. Is it better to make "condition" an optional field in some cases?
(Implicit from having a "seconds" attribute)

[tonyg, 2013-08-21 00:16:20]

> Hurrr. Is it better to make "condition" an optional field in some cases?
> (Implicit from having a "seconds" attribute)

Love it. The current syntax is clunky as easy to get wrong.

[edmundyan, 2013-08-21 21:44:31]

Removed the "condition" option for javascript and duration, where were
redundant.

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

https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:82: "wait_for_href_change": true
On 2013/08/20 23:57:39, Dave Tu wrote:
> These kinds of waits can also be replaced with wait actions.

Done, but are you sure they are the same?  Won't this run the previous_action
twice?  Should we remove the wait_for_X attributes for click_element action
then?

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

https://codereview.chromium.org/22883011/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/page_test.py:212: page.navigate_actions =
GetCompoundActionFromPage(page, 'navigate_steps')
On 2013/08/20 23:57:39, Dave Tu wrote:
> Why assign it to page if you're not using it anywhere else?
I think your previous comment had said to make it a page attribute, and I
thought it made more sense to have the "default" navigate action be defined
inside page.py rather than here.

[dtu, 2013-08-21 23:09:18]

lgtm

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

https://codereview.chromium.org/22883011/diff/3001/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:82: "wait_for_href_change": true
On 2013/08/21 21:44:31, edmundyan wrote:
> On 2013/08/20 23:57:39, Dave Tu wrote:
> > These kinds of waits can also be replaced with wait actions.
> 
> Done, but are you sure they are the same?  Won't this run the previous_action
> twice?  Should we remove the wait_for_X attributes for click_element action
> then?

Yes, _RunCompoundAction does some wonky magic to make this work. Yes, we can
remove the wait_for_X from click_element!

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

https://codereview.chromium.org/22883011/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/page_test.py:212: page.navigate_actions =
GetCompoundActionFromPage(page, 'navigate_steps')
On 2013/08/21 21:44:31, edmundyan wrote:
> On 2013/08/20 23:57:39, Dave Tu wrote:
> > Why assign it to page if you're not using it anywhere else?
> I think your previous comment had said to make it a page attribute, and I
> thought it made more sense to have the "default" navigate action be defined
> inside page.py rather than here.

Okay, either way. I think my original idea was to have page do
self.navigate_steps = {'action': 'navigate'}, but whatever works :)

[edmundyan, 2013-08-22 23:05:08]

Oops, I converted the wait_for_href and wait_for_navigate actions wrong on
top_25.

Also, there was a bug in wait.py for WillRunAction that I fixed.

Removed wait_for_X in click_element, but that also required an update to
unittests, so PTAL.

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

https://codereview.chromium.org/22883011/diff/3001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page/page_test.py:212: page.navigate_actions =
GetCompoundActionFromPage(page, 'navigate_steps')
On 2013/08/21 23:09:18, Dave Tu wrote:
> On 2013/08/21 21:44:31, edmundyan wrote:
> > On 2013/08/20 23:57:39, Dave Tu wrote:
> > > Why assign it to page if you're not using it anywhere else?
> > I think your previous comment had said to make it a page attribute, and I
> > thought it made more sense to have the "default" navigate action be defined
> > inside page.py rather than here.
> 
> Okay, either way. I think my original idea was to have page do
> self.navigate_steps = {'action': 'navigate'}, but whatever works :)

Oh heh. Done that way. I like defining the default in page.py

[dtu, 2013-08-22 23:26:28]

Cool, still lgtm

[commit-bot: I haz the power, 2013-08-26 20:22:44]

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

[edmundyan, 2013-08-26 21:10:44]

Ken, could you take a look at gpu/ stuff?

[Ken Russell (switch to Gerrit), 2013-08-26 21:18:15]

content/test/gpu LGTM so long as the changes have been tested. +skyostil as FYI.

[commit-bot: I haz the power, 2013-08-26 21:33:50]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-08-26 22:02:45]

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

[commit-bot: I haz the power, 2013-08-27 03:13:00]

Retried try job too often on linux_rel for step(s) browser_tests,
content_browsertests, nacl_integration, sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-27 03:23:43]

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

[commit-bot: I haz the power, 2013-08-27 03:28:15]

Change committed as 219675

[edmundyan, 2013-08-27 19:11:37]

Dtu mind taking another look?  This was breaking image_decoding.  We need to
define the default navigate_steps inside the page_set object rather than the
page.

[dtu, 2013-08-27 20:48:29]

lgtm

[commit-bot: I haz the power, 2013-08-27 21:22:37]

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

[commit-bot: I haz the power, 2013-08-28 00:42:04]

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

[commit-bot: I haz the power, 2013-08-28 07:52:18]

Change committed as 219955