Telemetry: Move Web Page Replay to the platform.

Telemetry: Step 1 of moving Web Page Replay to the platform.

In this step, we move WPR down to platform without making
any changes to the lifecycle nor where the input comes from.
API and lifecycle cleanup to follow in a follow up patch.

BUG=404771
Committed: https://crrev.com/2b4f9c4f7c8c47f6e19daac2845c47d3d1f3cd4e
Cr-Commit-Position: refs/heads/master@{#301578}

[slamm, 2014-10-16 17:25:09]

checkpoint

[slamm, 2014-10-16 18:38:59]

checkpoint

[slamm, 2014-10-16 20:56:29]

Keep existing replay lifecycle. (Will change in later CL.)

[slamm, 2014-10-16 20:59:55]

slamm@chromium.org changed reviewers:
+ chrishenry@google.com

[slamm, 2014-10-16 20:59:55]

I am still going to add docs and NetworkControllerTest. However, the CL now
works with "Step 1" that we talked about this morning.

[chrishenry, 2014-10-16 22:06:52]

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/browser.py:40:
self._platform_backend.platform.network_controller.StartReplay(
Can you add a TODO for what we discussed offline? Can you file a bug (or use
existing bug) to point out what are the steps you plan to do here and link from
the TODO?

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/__init__.py:100: def
network_controller(self):
Pydoc please. Say what NetworkController does briefly.

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/network_controller.py (right):

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/network_controller.py:28:
make_javascript_deterministic=make_javascript_deterministic)
Don't this need to update wpr server too if one is already running? Remember
that browser start/stop is not tied to page run today.

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/network_controller.py:31: """StartReplay
with the previously bound args.
This is not the important bit here. Just say:

"Start Web Page Replay server.

Web Page Replay server is [describe what it does for telemetry].

[Describe what happen if SetReplayArchivePath is called, in case: (a) when WPR
server is already running, (b) if it hasn't run yet.]"

TODO(slamm): The browser_backend argument..."

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/network_controller.py:37: assert
self._pending_replay_args
Looks like you're now also enforcing that SetReplayArchivePath must be called
once. In particular, this is because you need to get wpr_mode, right? Should
wpr_mode be passed to StartReplay instead?

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

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:48: wpr_mode =
wpr_modes.WPR_APPEND,
what's the trailing , for?

[slamm, 2014-10-17 18:08:21]

Address review comments.

[slamm, 2014-10-17 18:08:48]

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/browser.py (right):

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/browser.py:40:
self._platform_backend.platform.network_controller.StartReplay(
On 2014/10/16 22:06:52, chrishenry wrote:
> Can you add a TODO for what we discussed offline? Can you file a bug (or use
> existing bug) to point out what are the steps you plan to do here and link
from
> the TODO?

Done.

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/__init__.py:100: def
network_controller(self):
On 2014/10/16 22:06:52, chrishenry wrote:
> Pydoc please. Say what NetworkController does briefly.

Done.

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/network_controller.py (right):

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/network_controller.py:28:
make_javascript_deterministic=make_javascript_deterministic)
On 2014/10/16 22:06:52, chrishenry wrote:
> Don't this need to update wpr server too if one is already running? Remember
> that browser start/stop is not tied to page run today.

Replay is started (or updated) in UpdateReplay (formerly StartReplay). I
expected that method to go away when the browser_backend dependencies move to
platform.

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/network_controller.py:31: """StartReplay
with the previously bound args.
On 2014/10/16 22:06:52, chrishenry wrote:
> This is not the important bit here. Just say:
> 
> "Start Web Page Replay server.
> 
> Web Page Replay server is [describe what it does for telemetry].
> 
> [Describe what happen if SetReplayArchivePath is called, in case: (a) when WPR
> server is already running, (b) if it hasn't run yet.]"
> 
> TODO(slamm): The browser_backend argument..."

Done.

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/network_controller.py:37: assert
self._pending_replay_args
On 2014/10/16 22:06:52, chrishenry wrote:
> Looks like you're now also enforcing that SetReplayArchivePath must be called
> once. In particular, this is because you need to get wpr_mode, right? Should
> wpr_mode be passed to StartReplay instead?

I am asserting that every SetReplayArgs call (formerly SetReplayArchivePath) is
followed by only one call to UpdateReplay (formerly StartReplay).

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

https://codereview.chromium.org/656303003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:48: wpr_mode =
wpr_modes.WPR_APPEND,
On 2014/10/16 22:06:52, chrishenry wrote:
> what's the trailing , for?

The trailing comma is for introducing a bug. Removed.

[slamm, 2014-10-17 22:39:50]

Set the stage for moving Replay's browser_backend deps to the platform.

[slamm, 2014-10-17 22:44:02]

Remove UpdateReplay (unused) from network_controller API.

[slamm, 2014-10-17 23:01:30]

Move StopReplay to platform_backend (still tied to closing the browser.

[slamm, 2014-10-17 23:13:18]

Update a comment.

[chrishenry, 2014-10-17 23:26:51]

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/network_controller_backend.py
(right):

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/network_controller_backend.py:1: #
Copyright 2014 The Chromium Authors. All rights reserved.
No unit test?

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/platform_backend.py (right):

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:104: #    get moved
to platform. https://crbug.com/423962
nit: I think TODO comment continuation doesn't need to have extra 4-space align
(in fact you do 3). Same elsewhere.

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:105:
self._network_controller_backend.UpdateReplay(browser_backend)
Is this correct? Does browser not need wpr ports changes during creation?

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/wpr_server.py (right):

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/wpr_server.py:1: # Copyright 2012 The Chromium
Authors. All rights reserved.
You still plan to merge this to network_controller right?

[slamm, 2014-10-17 23:50:54]

Add unittest stub. TODO comment indent.

[slamm, 2014-10-17 23:51:10]

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/network_controller_backend.py
(right):

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/network_controller_backend.py:1: #
Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/10/17 23:26:51, chrishenry wrote:
> No unit test?

Next on the list. Added a stub.

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/platform_backend.py (right):

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:104: #    get moved
to platform. https://crbug.com/423962
On 2014/10/17 23:26:51, chrishenry wrote:
> nit: I think TODO comment continuation doesn't need to have extra 4-space
align
> (in fact you do 3). Same elsewhere.

Done.

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:105:
self._network_controller_backend.UpdateReplay(browser_backend)
On 2014/10/17 23:26:51, chrishenry wrote:
> Is this correct? Does browser not need wpr ports changes during creation?

That part of the parameter passing has not changed.

wpr_server sets browser_backend.wpr_port_pairs and that is read in
chrome_browser_backend.GetReplayBrowserStartupArgs.

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/wpr_server.py (right):

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/wpr_server.py:1: # Copyright 2012 The Chromium
Authors. All rights reserved.
On 2014/10/17 23:26:51, chrishenry wrote:
> You still plan to merge this to network_controller right?

network_controller_backend said to wpr_server, "This Town Ain't Big Enough for
Both of Us".

Seems like a good thing to do in a later step.

[chrishenry, 2014-10-18 00:00:34]

Looks fine to me, please ping after you have test, and I'll take a (hopefully)
final look.

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/platform_backend.py (right):

https://codereview.chromium.org/656303003/diff/160001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/platform_backend.py:105:
self._network_controller_backend.UpdateReplay(browser_backend)
On 2014/10/17 23:51:10, slamm wrote:
> On 2014/10/17 23:26:51, chrishenry wrote:
> > Is this correct? Does browser not need wpr ports changes during creation?
> 
> That part of the parameter passing has not changed.
> 
> wpr_server sets browser_backend.wpr_port_pairs and that is read in
> chrome_browser_backend.GetReplayBrowserStartupArgs.

Ah yeah, sorry, I thought this is DidStartBrowser.

[slamm, 2014-10-20 22:22:28]

Add network_controller_backend_unittest.

[slamm, 2014-10-20 22:24:32]

slamm@chromium.org changed reviewers:
+ nednguyen@google.com

[slamm, 2014-10-20 22:24:33]

Added unit tests (and fixed a bug).
PTAL.

[chrishenry, 2014-10-20 22:49:36]

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/network_controller.py (right):

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/network_controller.py:1: # Copyright
2014 The Chromium Authors. All rights reserved.
As mentioned offline, let's see if we can get rid of this class and stick with
just network_controller_backend. I see no reason to ever expose
network_controller to users (and when we do, we can create network_controller).

[slamm, 2014-10-20 23:18:14]

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/network_controller.py (right):

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/network_controller.py:1: # Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/10/20 22:49:36, chrishenry wrote:
> As mentioned offline, let's see if we can get rid of this class and stick with
> just network_controller_backend. I see no reason to ever expose
> network_controller to users (and when we do, we can create
network_controller).

That seems like a refactor better left to another CL (unless you see an easy way
to accomplish what you are asking).

[chrishenry, 2014-10-20 23:50:22]

lgtm

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/network_controller.py (right):

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/network_controller.py:22: def
StopReplay(self):
Can we at least move this to NetworkControllerBackend? It doesn't seem to be
used from PageRunner?

[slamm, 2014-10-21 02:13:32]

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/network_controller.py (right):

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/network_controller.py:22: def
StopReplay(self):
On 2014/10/20 23:50:22, chrishenry wrote:
> Can we at least move this to NetworkControllerBackend? It doesn't seem to be
> used from PageRunner?

I planned to have page_runner call StopReplay when the life-cycle changes
(https://crbug.com/424777). There currently is no mechanism to shutdown
everything that is running on the platform.

[nednguyen, 2014-10-21 02:19:52]

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/network_controller.py (right):

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/network_controller.py:22: def
StopReplay(self):
On 2014/10/21 02:13:32, slamm wrote:
> On 2014/10/20 23:50:22, chrishenry wrote:
> > Can we at least move this to NetworkControllerBackend? It doesn't seem to be
> > used from PageRunner?
> 
> I planned to have page_runner call StopReplay when the life-cycle changes
> (https://crbug.com/424777). There currently is no mechanism to shutdown
> everything that is running on the platform.

I see you make the call NetworkControllerBackend::StopReplay in
platform_backend's WillCloseBrowser(). 

Exposing this may allow people to call this without knowing that
platform_backend will call StopReplay before the browser is closed anyway,
leading to StopReplay called twice?

[slamm, 2014-10-23 23:08:28]

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/network_controller.py (right):

https://codereview.chromium.org/656303003/diff/200001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/network_controller.py:22: def
StopReplay(self):
On 2014/10/21 02:19:52, nednguyen wrote:
> On 2014/10/21 02:13:32, slamm wrote:
> > On 2014/10/20 23:50:22, chrishenry wrote:
> > > Can we at least move this to NetworkControllerBackend? It doesn't seem to
be
> > > used from PageRunner?
> > 
> > I planned to have page_runner call StopReplay when the life-cycle changes
> > (https://crbug.com/424777). There currently is no mechanism to shutdown
> > everything that is running on the platform.
> 
> I see you make the call NetworkControllerBackend::StopReplay in
> platform_backend's WillCloseBrowser(). 
> 
> Exposing this may allow people to call this without knowing that
> platform_backend will call StopReplay before the browser is closed anyway,
> leading to StopReplay called twice?

The StopReplay will move from WillCloseBrowser to the page_runner. However, for
now, I removed it from the NetworkController interface. (Stopping twice would
not hurt anything.)

[nednguyen, 2014-10-24 15:03:33]

LGTM

https://codereview.chromium.org/656303003/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_runner.py (left):

https://codereview.chromium.org/656303003/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_runner.py:92: if page.archive_path and
self._last_archive_path != page.archive_path:
Where does this logic go? 

It maybe fine that we don't need to handle the case WPR path changing now since
all page in the page_set will have same archive path defined in page_set
(https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...).
 

However, the future we are heading to is each page's archive_path should be
separate, and since people may mistakenly override @archive_path in page, you
may want to explode this if archive_path change or add a test in
page_set_smoke_test to prevent them from doing so.

[slamm, 2014-10-27 16:39:59]

https://codereview.chromium.org/656303003/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/page/page_runner.py (left):

https://codereview.chromium.org/656303003/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/page/page_runner.py:92: if page.archive_path and
self._last_archive_path != page.archive_path:
On 2014/10/24 15:03:33, nednguyen wrote:
> Where does this logic go? 
> 
> It maybe fine that we don't need to handle the case WPR path changing now
since
> all page in the page_set will have same archive path defined in page_set
>
(https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...).
>  
> 
> However, the future we are heading to is each page's archive_path should be
> separate, and since people may mistakenly override @archive_path in page, you
> may want to explode this if archive_path change or add a test in
> page_set_smoke_test to prevent them from doing so.

This logic moved to the network_controller_backend (line 75). If the WPR
parameters match what is already running, the network_controller_backend leaves
replay running. If there is a WPR parameter mismatch, replay is stopped and a
new one is started.

In the future, we can change WPR to be able to update its configuration on the
fly.

[slamm, 2014-10-27 20:10:45]

I have added a method to update replay when the browser is already existing. 

This CL does not add logic to make sure the remote ports are consistent. That
will come in a follow-up CL. This CL is only trying to replicate the existing
behavior.

[slamm, 2014-10-27 20:47:38]

Fix bug with NetworkControllerBackend._ReplayServer

[slamm, 2014-10-27 20:49:59]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-10-27 20:52:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656303003/260001

[slamm, 2014-10-27 22:22:57]

Allow the browser to be used without replay.

[slamm, 2014-10-27 22:23:31]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-10-27 22:27:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656303003/280001

[commit-bot: I haz the power, 2014-10-27 23:41:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-27 23:41:27]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chromium-reviews, 2014-10-27 23:56:51]

I am working on fixing the broken unit test. The page_runner unit test is
accessing a private browser property -- a private browser property that my
CL removes.

On Mon, Oct 27, 2014 at 4:41 PM, <commit-bot@chromium.org> wrote:

> Try jobs failed on following builders:
>   linux_chromium_rel on tryserver.chromium.linux
> (http://build.chromium.org/p/tryserver.chromium.linux/
> builders/linux_chromium_rel/builds/3724)
>
> https://codereview.chromium.org/656303003/
>

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

[slamm, 2014-10-28 00:33:19]

Update --use-live-sites tests to not use private browser property.

[slamm, 2014-10-28 00:33:26]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-10-28 00:36:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656303003/300001

[slamm, 2014-10-28 04:25:12]

The CQ bit was unchecked by slamm@chromium.org

[slamm, 2014-10-28 04:25:13]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-10-28 04:25:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656303003/300001

[commit-bot: I haz the power, 2014-10-28 06:50:10]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2014-10-28 06:50:58]

Patchset 16 (id:??) landed as
https://crrev.com/2b4f9c4f7c8c47f6e19daac2845c47d3d1f3cd4e
Cr-Commit-Position: refs/heads/master@{#301578}

[Sébastien Marchand, 2014-11-05 17:31:02]

sebmarchand@chromium.org changed reviewers:
+ sebmarchand@chromium.org

[Sébastien Marchand, 2014-11-05 17:31:03]

I got the following stack trace if I try this CL locally:
Traceback (most recent call last):
  <module> at tools\perf\run_benchmark:18
    sys.exit(benchmark_runner.main())
  main at tools\telemetry\telemetry\benchmark_runner.py:336
    return command().Run(options)
  Run at tools\telemetry\telemetry\benchmark_runner.py:169
    return min(255, self._benchmark().Run(args))
  Run at tools\telemetry\telemetry\benchmark.py:106
    page_runner.Run(pt, us, expectations, finder_options, results)
  Run at tools\telemetry\telemetry\page\page_runner.py:404
    test, page_set, expectations, page, results, state)
  _RunPageAndRetryRunIfNeeded at
tools\telemetry\telemetry\page\page_runner.py:306
    state.WillRunPage(page)
  WillRunPage at tools\telemetry\telemetry\page\page_runner.py:66
    self._StartProfiling(self._current_page)
  _StartProfiling at tools\telemetry\telemetry\page\page_runner.py:219
    page.file_safe_name)
AttributeError: 'NoneType' object has no attribute 'file_safe_name'

[Sébastien Marchand, 2014-11-05 17:33:06]

Sorry, replied on the wrong CL...