Handle the Web Page Replay related Chrome flags in one place.

Handle the Web Page Replay related Chrome flags in one place.

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

[slamm, 2014-09-26 16:54:30]

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

[slamm, 2014-09-26 16:54:31]

This includes one change from previous behavior.

With --android-dns, Telemetry currently runs Chrome with the following flags:
  --fixed-http-port=80
  --fixed-https-port=443
  (no host resolver rules)

With this change, the fixed port flags are dropped in that case since they are
the standard ports. They are dropped because the run is configured to use DNS.
My reasoning is that anytime the test uses WPR's DNS server, it has to be using
the standard ports.

[nednguyen, 2014-09-26 17:08:17]

nednguyen@google.com changed reviewers:
+ wuhu@google.com

[wuhu, 2014-09-26 17:55:46]

https://codereview.chromium.org/595213003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/wpr_server.py (right):

https://codereview.chromium.org/595213003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/wpr_server.py:24: flags =
['--ignore-certificate-errors']
Now that we are installing test CA onto the device, we should remove the
ignore-certificate-errors flag

https://codereview.chromium.org/595213003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/wpr_server.py:28: if not port_pairs.dns:
The http/https ports only need to be set when using non-RNDIS forwarder (ie.
neither rndis flag or netsim flag are set)

Relying on port_pairs.dns is not reliable as there can be situations where only
http/https proxy servers are used in the RNDIS mode.  In this case, you don't
want to change the ports from the default 80/443 as that will not work with the
iptables config.

[slamm, 2014-09-27 00:01:37]

I addressed the comments and added one more change:

I moved the flag setting into chrome_browser_backend. wpr_server.py should not
have to know about all the possible browser types.

https://codereview.chromium.org/595213003/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/wpr_server.py (right):

https://codereview.chromium.org/595213003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/wpr_server.py:24: flags =
['--ignore-certificate-errors']
On 2014/09/26 17:55:46, wuhu wrote:
> Now that we are installing test CA onto the device, we should remove the
> ignore-certificate-errors flag

I added a TODO since the root CA change has not landed yet.

https://codereview.chromium.org/595213003/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/wpr_server.py:28: if not port_pairs.dns:
On 2014/09/26 17:55:46, wuhu wrote:
> The http/https ports only need to be set when using non-RNDIS forwarder (ie.
> neither rndis flag or netsim flag are set)
> 
> Relying on port_pairs.dns is not reliable as there can be situations where
only
> http/https proxy servers are used in the RNDIS mode.  In this case, you don't
> want to change the ports from the default 80/443 as that will not work with
the
> iptables config.

I updated it to only use the fixed port flags if the report ports are not the
defaults.

[nednguyen, 2014-09-29 16:22:06]

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(left):

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:330:
if self.forwarder_factory.does_forwarder_override_dns:
This condition does not check for self.browser_options.netsim. Is this a bug of
current implementation?

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:142: if
http_remote_port != 80:
What are these magic numbers 80 & 443?

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend_unittest.py
(right):

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend_unittest.py:36:
assert self.forwarder_factory.does_forwarder_override_dns == False
nit: assert not self.forwarder_factory.does_forwarder_override_dns

Why do we need this assertion?

[chrishenry, 2014-09-29 17:04:07]

lgtm when Ned and Hubert are happy.

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:133:
use_host_resolver = (
Add a comment as to what these flags do, so it's easier for future readers to
understand why this is correct?

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend_unittest.py
(right):

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend_unittest.py:80:
)
nit: can you put the closing ) on previous line (and kill the trailing ,). Same
elsewhere. (The current way this is written makes it looks too much like a
tuple.)

[slamm, 2014-09-29 18:39:51]

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(left):

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:330:
if self.forwarder_factory.does_forwarder_override_dns:
On 2014/09/29 16:22:06, nednguyen wrote:
> This condition does not check for self.browser_options.netsim. Is this a bug
of
> current implementation?

The implementation works because of the dependencies among the options.

For android, 
self.forwarder_factory.does_forwarder_override_dns =
bool(self._rndis_configurator)

And, RNDIS is used for netsim.

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(right):

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:142: if
http_remote_port != 80:
On 2014/09/29 16:22:06, nednguyen wrote:
> What are these magic numbers 80 & 443?

80 & 443 are the default HTTP and HTTPS ports, respectively. I added a comment
explaining why Telemetry might use non-standard ports.

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend_unittest.py
(right):

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend_unittest.py:36:
assert self.forwarder_factory.does_forwarder_override_dns == False
On 2014/09/29 16:22:06, nednguyen wrote:
> nit: assert not self.forwarder_factory.does_forwarder_override_dns
> 
> Why do we need this assertion?

Wanted to check the default. However, I have changed it so the test always uses
a mock forwarder.

https://codereview.chromium.org/595213003/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend_unittest.py:80:
)
On 2014/09/29 17:04:07, chrishenry wrote:
> nit: can you put the closing ) on previous line (and kill the trailing ,).
Same
> elsewhere. (The current way this is written makes it looks too much like a
> tuple.)

Done.

[wuhu, 2014-09-29 20:05:31]

lgtm

[nednguyen, 2014-09-29 20:10:04]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2014-09-29 20:10:04]

lgtm

[commit-bot: I haz the power, 2014-09-29 20:11:40]

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

[commit-bot: I haz the power, 2014-09-29 21:23:26]

Committed patchset #5 (id:80001) as 1f470f65e9daf27bb180cb57f255f38131afda70

[commit-bot: I haz the power, 2014-09-29 21:25:38]

Patchset 5 (id:??) landed as
https://crrev.com/af110b914958fcfb2ebe4ea5f1c40c0f15de79e0
Cr-Commit-Position: refs/heads/master@{#297272}