[Telemetry] Move WPR life cycle from browser to platform

[Telemetry] Move WPR life cycle from browser to platform

Superseded by: https://codereview.chromium.org/1671903002/

In particular:
- Replay options are set at the platform, not the browser level.
- Replay is started/stopped by the shared state.
- The shared state may re-start the replay server to read from a
  different archive path.

BUG=404771

[perezju, 2015-12-02 16:02:43]

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

[perezju, 2015-12-02 16:02:44]

Ned, please have a look.

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/backends/chrome/cros_browser_backend.py
(left):

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/cros_browser_backend.py:35:
self.wpr_port_pairs.http.local_port)),
Note: http, and *not* htpps, on the remote port. Was this a typo?

This is moved to cros_platform_backend.py below.

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/platform/cros_platform_backend.py
(right):

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/cros_platform_backend.py:63:
default_port_pairs.http.local_port)),
Not sure if this was a typo, carried over from the old cros_browser_backend.py

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/platform/network_controller_backend_unittest.py
(left):

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/network_controller_backend_unittest.py:198:
def testSameBrowserUsesSamePorts(self):
this test case no longer applies.

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/platform_backend.py:124: dns=None)
These become the default settings for all platforms. Subclasses may
tweak/override these.

[perezju, 2015-12-02 18:05:24]

Started testing it on a few benchmarks, and found a few issues.

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:49:
self.platform_backend.InstallTestCa()
I've just noted these. If the certificate is installed by the browser it's
already too late; it won't be ready when WPR starts. Seems to be android only,
so I moved it to android_platform_backend.py

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:201:
self.platform_backend.RemoveTestCa()
Not sure where this should go. Maybe add a method to platform_backend that does
any necessary cleanup? And then call that method on the shared state tear down?

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:205:
self.platform_backend.StopForwardingHost(self._port)
Also, I've just noted these calls. Are they also related to WPR? Should they
also be moved to platform?

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/backends/mandoline/android_mandoline_backend.py
(left):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/backends/mandoline/android_mandoline_backend.py:153:
self.platform_backend.StopForwardingHost(self._port)
Same here, should this stop forwarding host be done by platform?

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/platform_backend.py:109: # be
accurate, so we also update it here.
wpr_modes should eventually be removed from browser_options, but it's still
referred to from many places. So I'm leaving it there for the moment, and
updating it with the same value as stored on the network_controller.

[nednguyen, 2015-12-08 19:09:58]

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

[nednguyen, 2015-12-08 19:09:59]

+Ethan: can you take a pass at this?

[eakuefner, 2015-12-22 19:18:56]

Sorry for the latency Juan; I've been a bit swamped lately. I've tried to
respond to all of your high-level points.

Thanks for doing this cleanup; it's difficult to motivate folks to do big
WPR-related changes :)

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:201:
self.platform_backend.RemoveTestCa()
On 2015/12/02 at 18:05:24, perezju wrote:
> Not sure where this should go. Maybe add a method to platform_backend that
does any necessary cleanup? And then call that method on the shared state tear
down?

I think the numerous calls to platform_backend could be condensed. As it is now,
I think this is a smell of unnecessary coupling between the browser and the
platform; you'll notice there's similar duplication in the mandoline one.

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:205:
self.platform_backend.StopForwardingHost(self._port)
On 2015/12/02 at 18:05:24, perezju wrote:
> Also, I've just noted these calls. Are they also related to WPR? Should they
also be moved to platform?

As above, yeah, I think we can move these into platform, privatize them, and
expose them through some kind of an API that we call not from the browser
backend, but from shared state. Not sure why KillBrowser is being done before
this; best guess is that it doesn't have to be.

This is not directly related to WPR, but it could profitably be moved as part of
this CL. That's to say, even if you use --use-live-sites, I'm pretty sure the
network traffic is still forwarded from the host.

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
File tools/telemetry/telemetry/internal/platform/cros_platform_backend.py
(right):

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/platform/cros_platform_backend.py:63:
default_port_pairs.http.local_port)),
On 2015/12/02 at 16:02:43, perezju wrote:
> Not sure if this was a typo, carried over from the old cros_browser_backend.py

It's bizarre that benchmarks on cros work, since this sure looks like a typo to
me. But let's not change it for now. Maybe we can consult with Achuith to see
what he thinks.

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/internal/platform/network_controller_backend_unittest.py
(right):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/network_controller_backend_unittest.py:186:
with tempfile.NamedTemporaryFile() as temp_file:
In general I think it would make sense to use pyfakefs in these tests instead of
real tempfiles.

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/platform_backend.py:109: # be
accurate, so we also update it here.
On 2015/12/02 at 18:05:24, perezju wrote:
> wpr_modes should eventually be removed from browser_options, but it's still
referred to from many places. So I'm leaving it there for the moment, and
updating it with the same value as stored on the network_controller.

As you noticed in 557641 we're working on refactoring options to surface some of
this configuration in a better way. We can probably move wpr_modes out of
browser_options as part of that.

[nednguyen, 2016-01-04 17:54:23]

Juan: are you still actively pursuing this patch or are you blocking this on
crbug.com/570348?

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/cros_platform_backend.py
(right):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/cros_platform_backend.py:53: def
SetupNetworkController(self, finder_options):
ditto

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/platform_backend.py:106: def
SetupNetworkController(self, finder_options):
This should be an API of network_controller?

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/platform_backend.py:116:
browser_options.wpr_mode = wpr_modes.WPR_REPLAY
It's bad that we mutate the browser_options.wpr_mode here, but I also don't see
a clear way of fixing this now :-(

[perezju, 2016-01-13 16:08:28]

On 2016/01/04 17:54:23, nednguyen wrote:
> Juan: are you still actively pursuing this patch or are you blocking this on
> crbug.com/570348?
> 
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/internal/platform/cros_platform_backend.py
> (right):
> 
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> tools/telemetry/telemetry/internal/platform/cros_platform_backend.py:53: def
> SetupNetworkController(self, finder_options):
> ditto
> 
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):
> 
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> tools/telemetry/telemetry/internal/platform/platform_backend.py:106: def
> SetupNetworkController(self, finder_options):
> This should be an API of network_controller?
> 
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> tools/telemetry/telemetry/internal/platform/platform_backend.py:116:
> browser_options.wpr_mode = wpr_modes.WPR_REPLAY
> It's bad that we mutate the browser_options.wpr_mode here, but I also don't
see
> a clear way of fixing this now :-(

Hi, yes, still working on this. Had been busy on a few other things on my plate,
but will get back to this probably next week. I've seen some changes on option
handling, so I'll try to rebase around that.

[nednguyen, 2016-01-13 17:21:25]

On 2016/01/13 16:08:28, perezju wrote:
> On 2016/01/04 17:54:23, nednguyen wrote:
> > Juan: are you still actively pursuing this patch or are you blocking this on
> > crbug.com/570348?
> > 
> >
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/internal/platform/cros_platform_backend.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/internal/platform/cros_platform_backend.py:53: def
> > SetupNetworkController(self, finder_options):
> > ditto
> > 
> >
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/internal/platform/platform_backend.py
(right):
> > 
> >
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/internal/platform/platform_backend.py:106: def
> > SetupNetworkController(self, finder_options):
> > This should be an API of network_controller?
> > 
> >
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/internal/platform/platform_backend.py:116:
> > browser_options.wpr_mode = wpr_modes.WPR_REPLAY
> > It's bad that we mutate the browser_options.wpr_mode here, but I also don't
> see
> > a clear way of fixing this now :-(
> 
> Hi, yes, still working on this. Had been busy on a few other things on my
plate,
> but will get back to this probably next week. I've seen some changes on option
> handling, so I'll try to rebase around that.

Thanks for the update!

[perezju, 2016-01-19 13:02:51]

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/platform_backend.py:106: def
SetupNetworkController(self, finder_options):
On 2016/01/04 17:54:23, nednguyen wrote:
> This should be an API of network_controller?

I've been thinking about this. The issue is that some decisions for correctly
setting up WPR (e.g. whether test certificates are needed, and how to install
them), are down to each specific platform backend.

So I see two paths:

1. Provide a SetupNetworkController (similar to this) or maybe even a pair of
SetupPlaform/CleanupPlatform that each xyz_platform_backend subclass overrides
to configure the network controller, install test certificates, etc., as needed.
Then the clients, namely the shared page state, would then call
platform.SetupPlatform(finder_options) and platform.CleanupPlatform() when done.

2. Alternatively, expose as API of network_controller some
SetupReplay/StartReplay/StopReplay methods to be called directly by clients
(i.e. the shared page state). The implementation in
network_controller_backend.SetupReplay would then need to call several other
methods, say, platform_backed.GetWprPorPairs, platform_backend.NeedsTestCa,
platform_backend.InstallTestCa, etc. to perform each of those actions in a
platform specific way.

It seems to me that 2 gives too much knowledge to the network_controller about
platform specific setups that may or may not be required. For example if we go
for 1, then platform_backend.InstallTestCa could become private to
android_platform_backend, since only that particular platform would need to know
about it.

Ned, Ethan, what are your thoughts?

[perezju, 2016-01-19 14:52:54]

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
File
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/1491183003/diff/1/tools/telemetry/telemetry/i...
tools/telemetry/telemetry/internal/backends/chrome/android_browser_backend.py:205:
self.platform_backend.StopForwardingHost(self._port)
On 2015/12/22 19:18:56, eakuefner wrote:
> On 2015/12/02 at 18:05:24, perezju wrote:
> > Also, I've just noted these calls. Are they also related to WPR? Should they
> also be moved to platform?
> 
> As above, yeah, I think we can move these into platform, privatize them, and
> expose them through some kind of an API that we call not from the browser
> backend, but from shared state. Not sure why KillBrowser is being done before
> this; best guess is that it doesn't have to be.
> 
> This is not directly related to WPR, but it could profitably be moved as part
of
> this CL. That's to say, even if you use --use-live-sites, I'm pretty sure the
> network traffic is still forwarded from the host.

This one doesn't seem as straight-forward because, e.g. for android-chrome, the
port on the host is forwarded to the devtools port on the device and _that_
depends on the browser, not the platform. Does that sound right?

[nednguyen, 2016-01-19 16:15:20]

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/platform_backend.py:106: def
SetupNetworkController(self, finder_options):
On 2016/01/19 13:02:51, perezju wrote:
> On 2016/01/04 17:54:23, nednguyen wrote:
> > This should be an API of network_controller?
> 
> I've been thinking about this. The issue is that some decisions for correctly
> setting up WPR (e.g. whether test certificates are needed, and how to install
> them), are down to each specific platform backend.
> 
> So I see two paths:
> 
> 1. Provide a SetupNetworkController (similar to this) or maybe even a pair of
> SetupPlaform/CleanupPlatform that each xyz_platform_backend subclass overrides
> to configure the network controller, install test certificates, etc., as
needed.
> Then the clients, namely the shared page state, would then call
> platform.SetupPlatform(finder_options) and platform.CleanupPlatform() when
done.

To me, the pattern of SetupPlatform & CleanupPlatform does not make much sense
for singleton objects like the platform, hence we should get rid of them. The
call sites that invoke methods doX() that cause side effect on platform should
know to call unDoX() at the right time to clean up instead.

> 
> 2. Alternatively, expose as API of network_controller some
> SetupReplay/StartReplay/StopReplay methods to be called directly by clients
> (i.e. the shared page state). The implementation in
> network_controller_backend.SetupReplay would then need to call several other
> methods, say, platform_backed.GetWprPorPairs, platform_backend.NeedsTestCa,
> platform_backend.InstallTestCa, etc. to perform each of those actions in a
> platform specific way.
> 
> It seems to me that 2 gives too much knowledge to the network_controller about
> platform specific setups that may or may not be required. For example if we go
> for 1, then platform_backend.InstallTestCa could become private to
> android_platform_backend, since only that particular platform would need to
know
> about it.
> 
> Ned, Ethan, what are your thoughts?

Let's go with route 2. I think eventually we also may be able shave off the
stuffs from platform_backend that shouldn't be there.

[perezju, 2016-01-20 16:03:55]

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):

https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/internal/platform/platform_backend.py:106: def
SetupNetworkController(self, finder_options):
On 2016/01/19 16:15:20, nednguyen wrote:
> Let's go with route 2. I think eventually we also may be able shave off the
> stuffs from platform_backend that shouldn't be there.

Sounds good. Another thought, along the lines of 2, would it make sense to have
an android_network_controller, cros_network_controller, etc? So the network
specific bits are in the network controller and not necessarily in the platform
backend?

[nednguyen, 2016-01-20 16:12:01]

On 2016/01/20 16:03:55, perezju wrote:
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/internal/platform/platform_backend.py (right):
> 
>
https://codereview.chromium.org/1491183003/diff/20001/tools/telemetry/telemet...
> tools/telemetry/telemetry/internal/platform/platform_backend.py:106: def
> SetupNetworkController(self, finder_options):
> On 2016/01/19 16:15:20, nednguyen wrote:
> > Let's go with route 2. I think eventually we also may be able shave off the
> > stuffs from platform_backend that shouldn't be there.
> 
> Sounds good. Another thought, along the lines of 2, would it make sense to
have
> an android_network_controller, cros_network_controller, etc? So the network
> specific bits are in the network controller and not necessarily in the
platform
> backend?

That is nice but seems unnecessary in this case to me, unless there are
substantial difference between the implementation. I would just do s.t like the
below inside the network_controller:
if platform is android:
   ...
elif platform is cros:
   ...

[perezju, 2016-01-21 11:18:39]

Before I go wiring this up properly everywhere and fixing unit tests, please
have a look at the proposed network_controller API, a possible implementation in
network_controller_backend, and it's usage in the shared_page_state.

For easier viewing compare patchset 4 to 3.

[nednguyen, 2016-01-21 17:05:20]

The network_controller API lg2me with some comment

https://codereview.chromium.org/1491183003/diff/60001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/network_controller.py (right):

https://codereview.chromium.org/1491183003/diff/60001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/network_controller.py:23:
self._network_controller_backend.Start(finder_options)
Instead of finder_options, can you figure out the list of arguments
network_controller_backend exactly need? From my experience, passing the
monolithic finder_options object around has been a major pain for us :-(

https://codereview.chromium.org/1491183003/diff/60001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/network_controller.py:26: """Start (or reuse) a
replay server from a specific archive."""
If we call StartReplay on a new archive_path, it will stop the old replay, isn't
it? We should document this fact.

[perezju, 2016-02-05 15:29:15]

Description was changed from

==========
[Telemetry] Move WPR life cycle from browser to platform

In particular:
- Replay options are set at the platform, not the browser level.
- Replay is started/stopped by the shared state.
- The shared state may re-start the replay server to read from a
  different archive path.

BUG=404771
==========

to

==========
[Telemetry] Move WPR life cycle from browser to platform

Superseded by: https://codereview.chromium.org/1671903002/

In particular:
- Replay options are set at the platform, not the browser level.
- Replay is started/stopped by the shared state.
- The shared state may re-start the replay server to read from a
  different archive path.

BUG=404771
==========