Add web page replay to chrome_remote_control.

Add web page replay to chrome_remote_control.

This adds an "archive_path" argument to the PageSet which specifies the
archive to record to or replay from. Specifying it there will allow for
sharding when page sets are broken into smaller subsets.

By default the benchmark still uses the live sites. If --record is passed, the
benchmark is run and web content is recorded to the archive. If an archive
exists, web content only comes from the archive.

BUG=None
TEST=Manual testing on linux

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

[tonyg, 2012-09-20 23:47:56]

This isn't done but is a good checkpoint as it works on desktop and doesn't
change the defaults.

[nduca, 2012-09-21 06:35:50]

So cool Tony! Left a ton of rambling thoughts, lmk if I lost you though! :)

https://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/c...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(right):

https://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/c...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:101:
pr = page_runner.PageRunner(ps, options)
i wonder if we should make the page runner manage this entire loop. Where,
effeictively, we say page_runner.Run(possible_browser) and make callers of this
pass in a possible_browser instead. That way page_runner can customize the
startup options of the browser and (if needed) tear it down during replay as
needed.

https://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/c...
File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right):

https://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/c...
tools/chrome_remote_control/chrome_remote_control/page_runner.py:10: def
__init__(self, page_set, options):
I'm hoping once we allow the page runner to control browser startup, then we can
actually remove the --record options from the lower level primitives. Then, we
can make a record.py <pageset> that instantiates just a page runner without the
multi_page_benchmark and uses that to record the archive.

https://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/c...
File tools/chrome_remote_control/chrome_remote_control/replay_server.py (right):

https://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/c...
tools/chrome_remote_control/chrome_remote_control/replay_server.py:10: # Get
chrome/test/functional scripts into our path.
Can we ad a TODO to not have a dependency here if we land this as-is?

Where will this eventually go, out of curiosity?

https://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/c...
tools/chrome_remote_control/chrome_remote_control/replay_server.py:17:
CHROME_FLAGS = webpagereplay.CHROME_FLAGS
Ah, I had forgotten is that in replay mode you need to start chrome up with
args. Do you still think that the page_runner infrastructure is the right way to
do this?

https://codereview.chromium.org/10965027/diff/1/tools/gpu/page_sets/Q32012.json
File tools/gpu/page_sets/Q32012.json (right):

https://codereview.chromium.org/10965027/diff/1/tools/gpu/page_sets/Q32012.js...
tools/gpu/page_sets/Q32012.json:3: "archive_path":
"data/page_cycler/webpagereplay/2012Q3.wpr",
aw whoops, i totally named this file backward didn't i? Whommmp. Lets just
rename this stupid file to match your better convention?

One open question I have is where in the chrome path to put these files, if
we're going to have them shared between a page cycler and, say, gpu tests. In a
standalone case, I'd say tools/gpu/data/webpagereplay/.

Finally, what path is the archive_path relative to? David sort of set up a
convention in file:// protocols where its relative to the json file.

[tonyg, 2012-09-25 17:29:29]

http://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py
(right):

http://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/multi_page_benchmark.py:101:
pr = page_runner.PageRunner(ps, options)
On 2012/09/21 06:35:50, nduca wrote:
> i wonder if we should make the page runner manage this entire loop. Where,
> effeictively, we say page_runner.Run(possible_browser) and make callers of
this
> pass in a possible_browser instead. That way page_runner can customize the
> startup options of the browser and (if needed) tear it down during replay as
> needed.

Done.

http://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/page_runner.py (right):

http://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/page_runner.py:10: def
__init__(self, page_set, options):
On 2012/09/21 06:35:50, nduca wrote:
> I'm hoping once we allow the page runner to control browser startup, then we
can
> actually remove the --record options from the lower level primitives. Then, we
> can make a record.py <pageset> that instantiates just a page runner without
the
> multi_page_benchmark and uses that to record the archive.

Thinking about this more, record.py vs --record just means an extra script to
know about and some additional boiler plate code. What do you think about
keeping --record.

http://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/ch...
File tools/chrome_remote_control/chrome_remote_control/replay_server.py (right):

http://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/replay_server.py:10: # Get
chrome/test/functional scripts into our path.
On 2012/09/21 06:35:50, nduca wrote:
> Can we ad a TODO to not have a dependency here if we land this as-is?
> 
> Where will this eventually go, out of curiosity?

Added TODO to sort this out.

http://codereview.chromium.org/10965027/diff/1/tools/chrome_remote_control/ch...
tools/chrome_remote_control/chrome_remote_control/replay_server.py:17:
CHROME_FLAGS = webpagereplay.CHROME_FLAGS
On 2012/09/21 06:35:50, nduca wrote:
> Ah, I had forgotten is that in replay mode you need to start chrome up with
> args. Do you still think that the page_runner infrastructure is the right way
to
> do this?

yeah, page_runner worked well with the addition of browser_extra_args to
Create().

http://codereview.chromium.org/10965027/diff/1/tools/gpu/page_sets/Q32012.json
File tools/gpu/page_sets/Q32012.json (right):

http://codereview.chromium.org/10965027/diff/1/tools/gpu/page_sets/Q32012.jso...
tools/gpu/page_sets/Q32012.json:3: "archive_path":
"data/page_cycler/webpagereplay/2012Q3.wpr",
On 2012/09/21 06:35:50, nduca wrote:
> aw whoops, i totally named this file backward didn't i? Whommmp. Lets just
> rename this stupid file to match your better convention?

Done

> 
> One open question I have is where in the chrome path to put these files, if
> we're going to have them shared between a page cycler and, say, gpu tests. In
a
> standalone case, I'd say tools/gpu/data/webpagereplay/.

Seems pretty clean to just keep them in the page_sets dir. What do you think?

> 
> Finally, what path is the archive_path relative to? David sort of set up a
> convention in file:// protocols where its relative to the json file.

I made it relative to this file.

[nduca, 2012-09-25 18:51:13]

(You knew this was coming: unit tests?)

[nduca, 2012-09-25 19:04:50]

Wrt --record, I think the core problem is that we've got a script-per-benchmark.
I think we need my refactoring to have more of a ./run_benchmarks
<benchmarkname> api.

In the near term, we could land --record and then do followup patches to clean
it up.

Wrt the .wpr files, if we need to check those into a data dir from svn-internal,
won't we need to point at a different directory? E.g. data-internal/2012Q3.wpr?

Finally, does this patch set us up for a clean patch progression to a "multiple
WPR files per pageset" world? Recall the use case that bugs me: I add 1 site to
a pageset. We want to not have to re-download all 3000 sites in that case...

[tonyg, 2012-10-01 18:10:39]

Added a tests, should be ready for another look.

[nduca, 2012-10-01 23:42:11]

Niiice. LGTM with nits

http://codereview.chromium.org/10965027/diff/11001/tools/chrome_remote_contro...
File tools/chrome_remote_control/chrome_remote_control/replay_server.py (right):

http://codereview.chromium.org/10965027/diff/11001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/replay_server.py:26: def
__enter__(self):
the rest of our crc does construction inside __init__ where __enter__ is a nop.
I'd lean toward doing that here too, unless you see a strong reason to use this
approach.

http://codereview.chromium.org/10965027/diff/11001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/replay_server.py:27:
self._http_forwarder = self._browser.CreateForwarder(
Would you have any objection to keeping CreateForwarder a private method on
browser?

One way to do this would be to make a browser method called
CreateReplayServer(...) that internally creates the forwarder and then passes
that onto the ReplayServer.

[tonyg, 2012-10-02 16:47:29]

http://codereview.chromium.org/10965027/diff/11001/tools/chrome_remote_contro...
File tools/chrome_remote_control/chrome_remote_control/replay_server.py (right):

http://codereview.chromium.org/10965027/diff/11001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/replay_server.py:26: def
__enter__(self):
On 2012/10/01 23:42:11, nduca wrote:
> the rest of our crc does construction inside __init__ where __enter__ is a
nop.
> I'd lean toward doing that here too, unless you see a strong reason to use
this
> approach.

Done.

http://codereview.chromium.org/10965027/diff/11001/tools/chrome_remote_contro...
tools/chrome_remote_control/chrome_remote_control/replay_server.py:27:
self._http_forwarder = self._browser.CreateForwarder(
On 2012/10/01 23:42:11, nduca wrote:
> Would you have any objection to keeping CreateForwarder a private method on
> browser?
> 
> One way to do this would be to make a browser method called
> CreateReplayServer(...) that internally creates the forwarder and then passes
> that onto the ReplayServer.

Done.

[commit-bot: I haz the power, 2012-10-02 16:47:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tonyg@chromium.org/10965027/16001

[commit-bot: I haz the power, 2012-10-02 16:48:07]

Presubmit check for 10965027-16001 failed and returned exit status 1.


************* Module chrome_remote_control.replay_server
F0401: 13,0: Unable to import 'webpagereplay'
Running presubmit commit checks ...

** Presubmit ERRORS **
Fix pylint errors first.

Presubmit checks took 8.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[nduca, 2012-10-02 17:37:15]

Why'd we commit with pylint errors?

[tonyg, 2012-10-02 17:38:25]

Fix coming up.

On Tue, Oct 2, 2012 at 10:37 AM,  <nduca@chromium.org> wrote:
> Why'd we commit with pylint errors?
>
> http://codereview.chromium.org/10965027/