Add perf tests for starting Chrome with URL

Add perf tests for starting Chrome with URL

This makes the --url option generally available, and adds tests for
starting Chrome with a URL, with and without a simultaneously
restoring a session.

Note:- As part of this I have added to telemetry the ability
to specify different browser options for each page, and to
specify the startup URL in the page set (this could easily
be extended to other browser options). This was needed to
avoid having to create 6 separate benchmarks to test
different combinations of URLs and sessions to be restored.

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

[aberent, 2013-12-13 19:11:58]

Jeremy - please review complete CL
David - Please approve as owner, although since this makes changes to the
page_runner etc. you may also want to do a wider review.

[jeremy, 2013-12-15 13:03:35]

Very nice!

A couple of questions:
* Do we want to run these tests on desktop?
* Jus to be clear, is starting with a URL on Android the same codepath as
starting via an Intent?
* This CL doesn't include plumbing for running a set of Telemetry tests together
right?m

https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
File tools/perf/benchmarks/session_restore_with_url.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
tools/perf/benchmarks/session_restore_with_url.py:10: """Measure time to start
Chrome cold with various pages"""
How about something like, also in other file - the part I don't like is "various
pages":
"""Measure Cold Chrome session restore starting to a URL."""

https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
tools/perf/benchmarks/session_restore_with_url.py:18: """Measure time to start
Chrome cold with various pages"""
s/cold/warm/

https://codereview.chromium.org/113563004/diff/20001/tools/perf/measurements/...
File tools/perf/measurements/session_restore_with_url.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/perf/measurements/...
tools/perf/measurements/session_restore_with_url.py:14: # Unlike the session
restore tests, these can run with any page set
nit:"set."

Can you explain why in the comment?

https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
File tools/perf/page_sets/startup_pages.json (right):

https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
tools/perf/page_sets/startup_pages.json:3: "description": "Use record_wpr
startup_pages_record to record data for this test",
Why 2 description keys?

https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
File tools/perf/page_sets/startup_pages_record.json (right):

https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
tools/perf/page_sets/startup_pages_record.json:2: "description": "Pages to
record data for testing starting Chrome with a URL",
Can you expand this description please, not clear on the difference between this
and startup_pages.json :(

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/set_startup_url.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/set_startup_url.py:7: class
SetStartupUrlAction(page_action.PageAction):
Class comment plz

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/set_startup_url.py:13: assert
hasattr(self,'startup_url')
Also assert if this is a blank string?

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/gtest_test_results.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/gtest_test_results.py:67: self._timestamp =
time.time()
What is this change needed for?

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

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page.py:61: self._scheme == 'about')
self.scheme in ['file', 'chrome', 'about'] ?

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

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:216: # options just before
starting the browser
nit: indent comment 2 spaces.

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:306: if not
test.AlwaysRestartBrowserForEachRun():
Does the above if mean that we restart the browser for each page individually
rather than each run?

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

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:111: """ Should be browser be
restarted after each run - browser independent.
nit: "Should the..."

And if this returns false that means that the browser gets restarted after the
whole pageset?

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:120: has to restart the browser for
some browsers but not all."""
Not clear what "browser dependent" means in this context :( Can you please
expand/rephrase the comment?

esp. the difference between this and the previous function.

[aberent, 2013-12-16 11:23:17]

On 2013/12/15 13:03:35, jeremy wrote:
> Very nice!
> 
> A couple of questions:
> * Do we want to run these tests on desktop?
I thought I would leave that up to you! They make sense for the desktop and I
have tested them on Linux (I don't have an easy way of testing them on Windows
or Mac), so I think we probably should.

> * Jus to be clear, is starting with a URL on Android the same codepath as
> starting via an Intent?
On Android Chrome (or any application) is always started via an intent. There is
no command line, except for adb, which is only intended for debugging, and the
only way to start Chrome is by sending it an intent. The Chrome intent may, or
may not, include a URL. It will not do so if Chrome is started by tapping the
Chrome icon, but will if it is started by, for example, tapping a URL in an
email.

> * This CL doesn't include plumbing for running a set of Telemetry tests
together
> right?m
Yes; for Chrome on Android this is in the Clank repository (separate CL to
follow), and I decided to leave the desktop side to you! I haven't looked into
how the list of tests to be run is set up on the Chromium bots, and didn't want
to delay this CL to do so. I don't think these tests make sense for upstream
Android (Chromium TestShell or Content Shell) since they don't support tabs.

Other answers will follow as I go through your code comments.

> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
> File tools/perf/benchmarks/session_restore_with_url.py (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
> tools/perf/benchmarks/session_restore_with_url.py:10: """Measure time to start
> Chrome cold with various pages"""
> How about something like, also in other file - the part I don't like is
"various
> pages":
> """Measure Cold Chrome session restore starting to a URL."""
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
> tools/perf/benchmarks/session_restore_with_url.py:18: """Measure time to start
> Chrome cold with various pages"""
> s/cold/warm/
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/measurements/...
> File tools/perf/measurements/session_restore_with_url.py (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/measurements/...
> tools/perf/measurements/session_restore_with_url.py:14: # Unlike the session
> restore tests, these can run with any page set
> nit:"set."
> 
> Can you explain why in the comment?
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
> File tools/perf/page_sets/startup_pages.json (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
> tools/perf/page_sets/startup_pages.json:3: "description": "Use record_wpr
> startup_pages_record to record data for this test",
> Why 2 description keys?
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
> File tools/perf/page_sets/startup_pages_record.json (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
> tools/perf/page_sets/startup_pages_record.json:2: "description": "Pages to
> record data for testing starting Chrome with a URL",
> Can you expand this description please, not clear on the difference between
this
> and startup_pages.json :(
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/actions/set_startup_url.py (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/actions/set_startup_url.py:7: class
> SetStartupUrlAction(page_action.PageAction):
> Class comment plz
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/actions/set_startup_url.py:13: assert
> hasattr(self,'startup_url')
> Also assert if this is a blank string?
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/gtest_test_results.py (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/gtest_test_results.py:67: self._timestamp =
> time.time()
> What is this change needed for?
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/page.py (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page.py:61: self._scheme == 'about')
> self.scheme in ['file', 'chrome', 'about'] ?
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/page_runner.py (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page_runner.py:216: # options just before
> starting the browser
> nit: indent comment 2 spaces.
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page_runner.py:306: if not
> test.AlwaysRestartBrowserForEachRun():
> Does the above if mean that we restart the browser for each page individually
> rather than each run?
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/page_test.py (right):
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page_test.py:111: """ Should be browser be
> restarted after each run - browser independent.
> nit: "Should the..."
> 
> And if this returns false that means that the browser gets restarted after the
> whole pageset?
> 
>
https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page_test.py:120: has to restart the browser
for
> some browsers but not all."""
> Not clear what "browser dependent" means in this context :( Can you please
> expand/rephrase the comment?
> 
> esp. the difference between this and the previous function.

[aberent, 2013-12-16 17:07:42]

https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
File tools/perf/benchmarks/session_restore_with_url.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
tools/perf/benchmarks/session_restore_with_url.py:10: """Measure time to start
Chrome cold with various pages"""
On 2013/12/15 13:03:36, jeremy wrote:
> How about something like, also in other file - the part I don't like is
"various
> pages":
> """Measure Cold Chrome session restore starting to a URL."""

Done.

https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
tools/perf/benchmarks/session_restore_with_url.py:18: """Measure time to start
Chrome cold with various pages"""
On 2013/12/15 13:03:36, jeremy wrote:
> s/cold/warm/

Done.

https://codereview.chromium.org/113563004/diff/20001/tools/perf/benchmarks/se...
tools/perf/benchmarks/session_restore_with_url.py:23: 'pageset_repeat_iters': 2}
Count left low from test testing, will fix on next upload.

https://codereview.chromium.org/113563004/diff/20001/tools/perf/measurements/...
File tools/perf/measurements/session_restore_with_url.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/perf/measurements/...
tools/perf/measurements/session_restore_with_url.py:14: # Unlike the session
restore tests, these can run with any page set
On 2013/12/15 13:03:36, jeremy wrote:
> nit:"set."
> 
> Can you explain why in the comment?

Done.

https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
File tools/perf/page_sets/startup_pages.json (right):

https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
tools/perf/page_sets/startup_pages.json:3: "description": "Use record_wpr
startup_pages_record to record data for this test",
On 2013/12/15 13:03:36, jeremy wrote:
> Why 2 description keys?

A mistake :-(. However JSON doesn't allow multiline strings, so I have had to
make the description into a list of strings to make it readable. I don't think
the description is used anywhere in the code, so this should be okay.

https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
File tools/perf/page_sets/startup_pages_record.json (right):

https://codereview.chromium.org/113563004/diff/20001/tools/perf/page_sets/sta...
tools/perf/page_sets/startup_pages_record.json:2: "description": "Pages to
record data for testing starting Chrome with a URL",
On 2013/12/15 13:03:36, jeremy wrote:
> Can you expand this description please, not clear on the difference between
this
> and startup_pages.json :(

Done. Had to turn the description into a list of strings, since JSON
doesn't allow multiline strings. I can't see that the description is read
anywhere in the code, so this should be fine.

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/actions/set_startup_url.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/set_startup_url.py:7: class
SetStartupUrlAction(page_action.PageAction):
On 2013/12/15 13:03:36, jeremy wrote:
> Class comment plz

Done.

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/actions/set_startup_url.py:13: assert
hasattr(self,'startup_url')
On 2013/12/15 13:03:36, jeremy wrote:
> Also assert if this is a blank string?

My feeling is not. To get a blank string the JSON file would have to contain

"startup_url" : ""

and this doesn't seem particularly more likely than any other URL error.

In addition a blank string will actually work, and start the browser without a
URL, so this might make sense as a way of explicitly specifying that the test
doesn't want a startup URL, overriding anything on the benchmark's command line
or any default.

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/gtest_test_results.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/gtest_test_results.py:67: self._timestamp =
time.time()
On 2013/12/15 13:03:36, jeremy wrote:
> What is this change needed for?

Actually nothing to do with this CL. I hit a crash during testing on the next
line when _GetMs was called, because _GetMs does arithmetic on self.timestamp
(hence crashes if it is None). Not sure why this hasn't been hit before, nor am
I sure if addSkip is ever called after the timestamp is set, but it certainly
can be called before the timestamp is set.

The specific case I had was that I was trying to run record_wpr on a page set
including a local file, and record_wpr crashed when it tried to skip the local
file.

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

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page.py:61: self._scheme == 'about')
On 2013/12/15 13:03:36, jeremy wrote:
> self.scheme in ['file', 'chrome', 'about'] ?
Done. Agreed, much nicer.

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

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:216: # options just before
starting the browser
On 2013/12/15 13:03:36, jeremy wrote:
> nit: indent comment 2 spaces.

Done.

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:306: if not
test.AlwaysRestartBrowserForEachRun():
On 2013/12/15 13:03:36, jeremy wrote:
> Does the above if mean that we restart the browser for each page individually
> rather than each run?

Yes. Renamed to make this clearer.

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

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:111: """ Should be browser be
restarted after each run - browser independent.
On 2013/12/15 13:03:36, jeremy wrote:
> nit: "Should the..."
> 
> And if this returns false that means that the browser gets restarted after the
> whole pageset?

Done.

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:120: has to restart the browser for
some browsers but not all."""
On 2013/12/15 13:03:36, jeremy wrote:
> Not clear what "browser dependent" means in this context :( Can you please
> expand/rephrase the comment?
> 
> esp. the difference between this and the previous function.

I have renamed this to more accurately reflect what it does, and extended the
comments. The reason for this function is that image_decoding.py needs to call a
browser function to decide whether to restart the browser.

[jeremy, 2013-12-17 11:29:44]

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/gtest_test_results.py (right):

https://codereview.chromium.org/113563004/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/gtest_test_results.py:67: self._timestamp =
time.time()
Makes sense, can you ask dtu@ or someone else familiar with this code to review
this part?

https://codereview.chromium.org/113563004/diff/60001/tools/perf/benchmarks/se...
File tools/perf/benchmarks/session_restore_with_url.py (right):

https://codereview.chromium.org/113563004/diff/60001/tools/perf/benchmarks/se...
tools/perf/benchmarks/session_restore_with_url.py:10: """Measure Chrome cold
session restore with startup URLs"""
nit: period at end of comment, also below.

https://codereview.chromium.org/113563004/diff/60001/tools/perf/measurements/...
File tools/perf/measurements/session_restore_with_url.py (right):

https://codereview.chromium.org/113563004/diff/60001/tools/perf/measurements/...
tools/perf/measurements/session_restore_with_url.py:14: # The session restore
tests ignore the contents of the page set, so only
nit s/so/so they/

https://codereview.chromium.org/113563004/diff/60001/tools/perf/measurements/...
tools/perf/measurements/session_restore_with_url.py:17: # between the pages so
it makes sense to run it for every page in the page
nit: How about changing this to something like -

pages. The test needs to be run for every page in the set to pick up the
individual attributes.

https://codereview.chromium.org/113563004/diff/60001/tools/perf/page_sets/sta...
File tools/perf/page_sets/startup_pages.json (right):

https://codereview.chromium.org/113563004/diff/60001/tools/perf/page_sets/sta...
tools/perf/page_sets/startup_pages.json:6: "Instead use record_wpr
startup_pages_record to record data for this test"],
nit: s/test/test./

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

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page.py:60: return (self._scheme in ['file',
'chrome', 'about'])
Don't think you need the outer parens now?

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

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:216: # options just before
starting the browser
nit: period

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:313:
possible_browser.finder_options)
Consider changing this to stick/nonsticky command line options with appropriate
comment per offline discussion.

Or on second thoughts, have CustomizeBrowserOptions take the pageset as an
argument - that actually makes more sense to me...

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

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:110: def
AlwaysRestartBrowserForEachPage(self):
Any reason not to name this: RestartBrowserForEachPage() ?

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:121: def
NeedsBrowserRestartBeforeNextPage(self, browser):  # pylint: disable=W0613
nit: rename to RestartBrowserBeforeNextPage() ?

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:149: this function will be called
for each page individually before the browser
nit:this this

[aberent, 2013-12-17 17:31:27]

https://codereview.chromium.org/113563004/diff/60001/tools/perf/benchmarks/se...
File tools/perf/benchmarks/session_restore_with_url.py (right):

https://codereview.chromium.org/113563004/diff/60001/tools/perf/benchmarks/se...
tools/perf/benchmarks/session_restore_with_url.py:10: """Measure Chrome cold
session restore with startup URLs"""
On 2013/12/17 11:29:44, jeremy wrote:
> nit: period at end of comment, also below.

Done.

https://codereview.chromium.org/113563004/diff/60001/tools/perf/measurements/...
File tools/perf/measurements/session_restore_with_url.py (right):

https://codereview.chromium.org/113563004/diff/60001/tools/perf/measurements/...
tools/perf/measurements/session_restore_with_url.py:14: # The session restore
tests ignore the contents of the page set, so only
On 2013/12/17 11:29:44, jeremy wrote:
> nit s/so/so they/

Actually changing the function to check that the page has a startup url, hence
rewriting the comment completely.

https://codereview.chromium.org/113563004/diff/60001/tools/perf/measurements/...
tools/perf/measurements/session_restore_with_url.py:17: # between the pages so
it makes sense to run it for every page in the page
On 2013/12/17 11:29:44, jeremy wrote:
> nit: How about changing this to something like -
> 
> pages. The test needs to be run for every page in the set to pick up the
> individual attributes.

See above

https://codereview.chromium.org/113563004/diff/60001/tools/perf/page_sets/sta...
File tools/perf/page_sets/startup_pages.json (right):

https://codereview.chromium.org/113563004/diff/60001/tools/perf/page_sets/sta...
tools/perf/page_sets/startup_pages.json:6: "Instead use record_wpr
startup_pages_record to record data for this test"],
On 2013/12/17 11:29:44, jeremy wrote:
> nit: s/test/test./

Done.

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

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page.py:60: return (self._scheme in ['file',
'chrome', 'about'])
On 2013/12/17 11:29:44, jeremy wrote:
> Don't think you need the outer parens now?

Done.

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

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:216: # options just before
starting the browser
On 2013/12/17 11:29:44, jeremy wrote:
> nit: period

Done.

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:313:
possible_browser.finder_options)
On 2013/12/17 11:29:44, jeremy wrote:
> Consider changing this to stick/nonsticky command line options with
appropriate
> comment per offline discussion.
> 
> Or on second thoughts, have CustomizeBrowserOptions take the pageset as an
> argument - that actually makes more sense to me...
I don't think we can do quite this. CustomizeBrowserOptions is overridden in
many of the tests, and we don't want to lose the default behaviour in cases
where it is overridden.

After some experiments the best option I have found is a variation of the
sticky/nonsticky solution. Let me know what you think of the next patch.

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

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:110: def
AlwaysRestartBrowserForEachPage(self):
On 2013/12/17 11:29:44, jeremy wrote:
> Any reason not to name this: RestartBrowserForEachPage() ?

Done.

https://codereview.chromium.org/113563004/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_test.py:121: def
NeedsBrowserRestartBeforeNextPage(self, browser):  # pylint: disable=W0613
On 2013/12/17 11:29:44, jeremy wrote:
> nit: rename to RestartBrowserBeforeNextPage() ?

Done.

[jeremy, 2013-12-29 09:39:07]

Lets discuss over vc tomorrow :)

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

https://codereview.chromium.org/113563004/diff/90001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:307:
test.CustomizePageSetBrowserOptions(page_set,
CustomizeBrowserOptionsForPageSet()?

[Anthony Berent, 2013-12-29 11:29:12]

Happy to discuss over vc, but still on holiday, back Thursday.
On 29 Dec 2013 09:39, <jeremy@chromium.org> wrote:

> Lets discuss over vc tomorrow :)
>
>
> https://codereview.chromium.org/113563004/diff/90001/
> tools/telemetry/telemetry/page/page_runner.py
> File tools/telemetry/telemetry/page/page_runner.py (right):
>
> https://codereview.chromium.org/113563004/diff/90001/
> tools/telemetry/telemetry/page/page_runner.py#newcode307
> tools/telemetry/telemetry/page/page_runner.py:307:
> test.CustomizePageSetBrowserOptions(page_set,
> CustomizeBrowserOptionsForPageSet()?
>
> https://codereview.chromium.org/113563004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dtu, 2014-01-07 21:44:49]

lgtm

[aberent, 2014-01-09 10:03:49]

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

https://codereview.chromium.org/113563004/diff/90001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:307:
test.CustomizePageSetBrowserOptions(page_set,
On 2013/12/29 09:39:07, jeremy wrote:
> CustomizeBrowserOptionsForPageSet()?

Done.

[aberent, 2014-01-09 10:06:16]

On 2014/01/09 10:03:49, aberent wrote:
>
https://codereview.chromium.org/113563004/diff/90001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/page/page_runner.py (right):
> 
>
https://codereview.chromium.org/113563004/diff/90001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/page/page_runner.py:307:
> test.CustomizePageSetBrowserOptions(page_set,
> On 2013/12/29 09:39:07, jeremy wrote:
> > CustomizeBrowserOptionsForPageSet()?
> 
> Done.

Landing. If anybody thinks of a way of cleaning up setting browser options we
can do this as a separate CL.

[commit-bot: I haz the power, 2014-01-09 10:07:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aberent@chromium.org/113563004/160001

[commit-bot: I haz the power, 2014-01-09 11:50:40]

Change committed as 243849