[Telemetry] page_runner refactoring. Move all the logic of handling wpr_mode

[Telemetry] page_runner refactoring.

Move all the logic of handling wpr_mode into 1 place. This
also update page_runner_unittest.TestUseLiveSiteFlag to
assert the wpr_mode of network_controller instead of the browser_options.

BUG=418278
Committed: https://crrev.com/2243a22590179249c76e1f7a8d90d38f0f5e11fd
Cr-Commit-Position: refs/heads/master@{#302121}

[nednguyen, 2014-10-29 18:30:33]

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

[slamm, 2014-10-29 19:52:08]

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:41: def _PrepareWprMode(self,
finder_options, possible_browser, archive_path,
_PrepareWprMode -> _SetReplayArgs or _PrepareWpr

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:41: def _PrepareWprMode(self,
finder_options, possible_browser, archive_path,
You can drop finder_options from the args. Use possible_browser.finder_options
instead (see two lines down).

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:53: 
Drop the blank space. This line works together with line 54

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:55: wpr_mode =
wpr_modes.WPR_APPEND
Add a TODO to drop WPR_APPEND when replay life-cycle moves to the platform.
https://crbug.com/424777

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:71: # TODO(slamm): Update
life-cycle comment with https://crbug.com/424777 fix.
Move these four comment lines into the new method.

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner_unittest.py (right):

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner_unittest.py:583:
network_controller_backend._pending_replay_args['wpr_mode'])
The page_runner test should not depend on any network_controller_backend
implementation details. All the test needs to do is make sure SetReplayArgs was
passed the correct wpr_mode args. Perhaps mock out the
network_controller_backend.

[nednguyen, 2014-10-29 20:48:57]

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner.py (right):

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:41: def _PrepareWprMode(self,
finder_options, possible_browser, archive_path,
On 2014/10/29 19:52:07, slamm wrote:
> _PrepareWprMode -> _SetReplayArgs or _PrepareWpr

Done.

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:41: def _PrepareWprMode(self,
finder_options, possible_browser, archive_path,
On 2014/10/29 19:52:07, slamm wrote:
> You can drop finder_options from the args. Use possible_browser.finder_options
> instead (see two lines down).

That can apply for StartBrowserIfNeeded also. Let's address the finder_options
vs possible_browser.finder_options mess in another patch.

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:55: wpr_mode =
wpr_modes.WPR_APPEND
On 2014/10/29 19:52:07, slamm wrote:
> Add a TODO to drop WPR_APPEND when replay life-cycle moves to the platform.
> https://crbug.com/424777

Done.

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner.py:71: # TODO(slamm): Update
life-cycle comment with https://crbug.com/424777 fix.
On 2014/10/29 19:52:07, slamm wrote:
> Move these four comment lines into the new method.

Done.

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
File tools/telemetry/telemetry/page/page_runner_unittest.py (right):

https://codereview.chromium.org/671873003/diff/1/tools/telemetry/telemetry/pa...
tools/telemetry/telemetry/page/page_runner_unittest.py:583:
network_controller_backend._pending_replay_args['wpr_mode'])
On 2014/10/29 19:52:07, slamm wrote:
> The page_runner test should not depend on any network_controller_backend
> implementation details. All the test needs to do is make sure SetReplayArgs
was
> passed the correct wpr_mode args. Perhaps mock out the
> network_controller_backend.

It's not easy to mock out network_controller_backend at the moment :-(. I leave
a TODO.

[chrishenry, 2014-10-29 21:11:19]

Like, lgtm.

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

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:41: def _PrepareWpr(self,
finder_options, possible_browser, archive_path,
Should we pass finder_options and platform (or possibly network_controller)
instead of possible_browser? It makes what we need a bit more obvious.

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:42:
make_javascript_deterministic):
nit: alignment.

[slamm, 2014-10-29 23:33:26]

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

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:41: def _PrepareWpr(self,
finder_options, possible_browser, archive_path,
On 2014/10/29 21:11:19, chrishenry wrote:
> Should we pass finder_options and platform (or possibly network_controller)
> instead of possible_browser? It makes what we need a bit more obvious.

Yes, please.

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:43: browser_options =
possible_browser.finder_options.browser_options
browser_options = finder_options.browser_options

possible_browser.finder_options may be stale. See
https://codereview.chromium.org/688163003/

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:44: if
possible_browser.finder_options.use_live_sites:
if finder_options.use_live_sites:

possible_browser.finder_options may be stale. See
https://codereview.chromium.org/688163003/

[nednguyen, 2014-10-30 17:34:44]

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

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:41: def _PrepareWpr(self,
finder_options, possible_browser, archive_path,
On 2014/10/29 23:33:26, slamm wrote:
> On 2014/10/29 21:11:19, chrishenry wrote:
> > Should we pass finder_options and platform (or possibly network_controller)
> > instead of possible_browser? It makes what we need a bit more obvious.
> 
> Yes, please.

Good idea. This actually allows me faking out the network_controller for
testing.

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:42:
make_javascript_deterministic):
On 2014/10/29 21:11:19, chrishenry wrote:
> nit: alignment.

Done.

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:43: browser_options =
possible_browser.finder_options.browser_options
On 2014/10/29 23:33:26, slamm wrote:
> browser_options = finder_options.browser_options
> 
> possible_browser.finder_options may be stale. See
> https://codereview.chromium.org/688163003/

Done.

https://codereview.chromium.org/671873003/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:44: if
possible_browser.finder_options.use_live_sites:
On 2014/10/29 23:33:26, slamm wrote:
> if finder_options.use_live_sites:
> 
> possible_browser.finder_options may be stale. See
> https://codereview.chromium.org/688163003/

Done.

[slamm, 2014-10-30 17:54:38]

LGTM with small change.

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

https://codereview.chromium.org/671873003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:60: archive_path, wpr_mode,
finder_options.browser_options.netsim,
finder_options.browser_options -> browser_options

https://codereview.chromium.org/671873003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner.py:61:
finder_options.browser_options.extra_wpr_args,
here too.

https://codereview.chromium.org/671873003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/page/page_runner_unittest.py (right):

https://codereview.chromium.org/671873003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/page/page_runner_unittest.py:627: def
TestUseLiveSitesFlag(self, options, expected_wpr_mode):
Much easier to read.

[nednguyen, 2014-10-30 17:57:26]

The CQ bit was checked by nednguyen@google.com

[commit-bot: I haz the power, 2014-10-30 17:58:03]

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

[commit-bot: I haz the power, 2014-10-30 18:53:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-30 18:54:31]

Patchset 4 (id:??) landed as
https://crrev.com/2243a22590179249c76e1f7a8d90d38f0f5e11fd
Cr-Commit-Position: refs/heads/master@{#302121}