Handle replay args within replay module.

Handle replay args within replay module.

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

[slamm, 2014-09-30 22:12:56]

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

[slamm, 2014-09-30 22:12:56]

This drops support for environment variables, WPR_RECORD and WPR_ARCHIVE_PATH. I
added those before Telemetry was written and they are not necessary in the
Telemetry context.

[slamm, 2014-10-01 21:40:05]

Patchset #7 (id:120001) has been deleted

[slamm, 2014-10-01 23:34:22]

ping.

[wuhu, 2014-10-02 00:01:42]

AddReplayServerOptions() is used to pass --https_root_ca_cert_path in CL
572123002.  If we drop it, what is the recommended way of accomplishing the same
thing?

[slamm, 2014-10-02 00:11:47]

On 2014/10/02 00:01:42, wuhu wrote:
> AddReplayServerOptions() is used to pass --https_root_ca_cert_path in CL
> 572123002.  If we drop it, what is the recommended way of accomplishing the
same
> thing?

Thanks for pointing that out. I will think about it a bit.

[chromium-reviews, 2014-10-02 17:29:21]

In https://codereview.chromium.org/617353007/, replay's
--https_root_ca_cert_path get set by reading the
BrowserBackend.wpr_ca_cert_path.

On Wed, Oct 1, 2014 at 5:11 PM, <slamm@chromium.org> wrote:

> On 2014/10/02 00:01:42, wuhu wrote:
>
>> AddReplayServerOptions() is used to pass --https_root_ca_cert_path in CL
>> 572123002.  If we drop it, what is the recommended way of accomplishing
>> the
>>
> same
>
>> thing?
>>
>
> Thanks for pointing that out. I will think about it a bit.
>
> https://codereview.chromium.org/617043003/
>

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

[nednguyen, 2014-10-02 17:45:17]

On 2014/10/02 17:29:21, chromium-reviews wrote:
> In https://codereview.chromium.org/617353007/, replay's
> --https_root_ca_cert_path get set by reading the
> BrowserBackend.wpr_ca_cert_path.
> 
> On Wed, Oct 1, 2014 at 5:11 PM, <mailto:slamm@chromium.org> wrote:
> 
> > On 2014/10/02 00:01:42, wuhu wrote:
> >
> >> AddReplayServerOptions() is used to pass --https_root_ca_cert_path in CL
> >> 572123002.  If we drop it, what is the recommended way of accomplishing
> >> the
> >>
> > same
> >
> >> thing?
> >>
> >
> > Thanks for pointing that out. I will think about it a bit.
> >
> > https://codereview.chromium.org/617043003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Sorry for long waiting, please ping me after you rebase your change in 617353007

[slamm, 2014-10-02 20:15:02]

Patchset #8 (id:160001) has been deleted

[slamm, 2014-10-03 19:20:28]

Rebase.

[slamm, 2014-10-03 19:21:19]

Patchset #9 (id:200001) has been deleted

[slamm, 2014-10-03 19:22:25]

Rebase.

[slamm, 2014-10-03 19:23:35]

PTAL.

[slamm, 2014-10-06 21:40:45]

ping.

[nednguyen, 2014-10-06 22:07:50]

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(left):

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:439:
if not self.forwarder_factory.does_forwarder_override_dns:
What is the equivalent of this logic in your change?

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(left):

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:72:
else:
This says append "-no-dns_forwarding" if netsim option is not specified which is
totally different from the branching condition in
AndroidBrowserBackend.AddReplayServerOptions. How do you exactly unify the two
into replay module?

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/webpagereplay.py (right):

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/webpagereplay.py:72: WPR_REPLAY_DIR: path to
alternate Web Page Replay source.
Why not also remove this?

[chrishenry, 2014-10-07 23:13:59]

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(left):

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:440:
extra_wpr_args.append('--no-dns_forwarding')
Where's the code that sets dns_port? Are they using the same conditionals?

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:442:
extra_wpr_args.append('--net=%s' % self.browser_options.netsim)
Where's --net being set?

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/webpagereplay.py (left):

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/webpagereplay.py:72: WPR_ARCHIVE_PATH: path to
alternate archive file (e.g. '/tmp/foo.wpr').
To make this patch a bit more focused, perhaps extract this deletion to a
separate patch.

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/webpagereplay.py (right):

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/webpagereplay.py:113: def _CommandLine(replay_py,
wpr_args, archive_path, host_ip,
The method name is really not obvious to me. Can you just name it CreateCommand
or something like that?

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/webpagereplay.py:257: logging.warning('DNS
nameserver configuration might not be restored!')
Can you give more context to this warning? I can see people be really confused
by the warning message...

[slamm, 2014-10-13 20:46:19]

Rebase. Address comments. Fix CrOs --host-resolver-rules

[nednguyen, 2014-10-13 20:57:45]

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

[nednguyen, 2014-10-13 20:57:46]

+achuith for changes to cros.

[achuithb, 2014-10-13 21:06:59]

On 2014/10/13 20:57:46, nednguyen wrote:
> +achuith for changes to cros.

lgtm

[chromium-reviews, 2014-10-13 21:16:54]

I decided to put those changes in their own CL with their own bug:

https://codereview.chromium.org/652793004/
https://code.google.com/p/chromium/issues/detail?id=423092

On Mon, Oct 13, 2014 at 2:06 PM, <achuith@chromium.org> wrote:

> On 2014/10/13 20:57:46, nednguyen wrote:
>
>> +achuith for changes to cros.
>>
>
> lgtm
>
> https://codereview.chromium.org/617043003/
>

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

[slamm, 2014-10-13 21:22:44]

Use separate CL for --host-resolver-rules fix (652793004).

[slamm, 2014-10-13 21:45:55]

I went back and double-checked everything.

I wrote a spreadsheet to map the logic:

https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_2qwVqlf...

The only change in behavior with this change is that the Replay DNS server is
never started with CrOS. That is fine because CrOS never uses it anyway.

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(left):

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:439:
if not self.forwarder_factory.does_forwarder_override_dns:
On 2014/10/06 22:07:50, nednguyen wrote:
> What is the equivalent of this logic in your change?

In webpagereplay, --no-dns_forwarding is added when the DNS port is None.

I created a spreadsheet to show make sure that matches up.

https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_2qwVqlf...

A possible improvement after this CL would be setting the ports in one place
based on a few parameters. As it is now, the ports are set in various browser
backend initializers.

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
(left):

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:72:
else:
On 2014/10/06 22:07:50, nednguyen wrote:
> This says append "-no-dns_forwarding" if netsim option is not specified which
is
> totally different from the branching condition in
> AndroidBrowserBackend.AddReplayServerOptions. How do you exactly unify the two
> into replay module?

If netsim is True, chrome_browser_backend sets the DNS port (in __init__), so
this ends up being the same as the logic in webpagereplay.

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/webpagereplay.py (right):

https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/webpagereplay.py:72: WPR_REPLAY_DIR: path to
alternate Web Page Replay source.
On 2014/10/06 22:07:50, nednguyen wrote:
> Why not also remove this?

Done.

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(left):

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:440:
extra_wpr_args.append('--no-dns_forwarding')
On 2014/10/07 23:13:59, chrishenry wrote:
> Where's the code that sets dns_port? Are they using the same conditionals?

Right now it is spread among the browser backend __init__ methods.  The logic is
the same.

I made a spreadsheet to validate: 

https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_2qwVqlf...

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:442:
extra_wpr_args.append('--net=%s' % self.browser_options.netsim)
On 2014/10/07 23:13:59, chrishenry wrote:
> Where's --net being set?

wpr_server.ReplayServer.__init__

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/webpagereplay.py (left):

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/webpagereplay.py:72: WPR_ARCHIVE_PATH: path to
alternate archive file (e.g. '/tmp/foo.wpr').
On 2014/10/07 23:13:59, chrishenry wrote:
> To make this patch a bit more focused, perhaps extract this deletion to a
> separate patch.

Done.

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/webpagereplay.py (right):

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/webpagereplay.py:113: def _CommandLine(replay_py,
wpr_args, archive_path, host_ip,
On 2014/10/07 23:13:59, chrishenry wrote:
> The method name is really not obvious to me. Can you just name it
CreateCommand
> or something like that?

Done.

https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/webpagereplay.py:257: logging.warning('DNS
nameserver configuration might not be restored!')
On 2014/10/07 23:13:59, chrishenry wrote:
> Can you give more context to this warning? I can see people be really confused
> by the warning message...

Moved to separate CL: https://codereview.chromium.org/625123002/

[nednguyen, 2014-10-14 20:34:03]

lgtm

[nednguyen, 2014-10-14 20:38:54]

On 2014/10/13 21:45:55, slamm wrote:
> I went back and double-checked everything.
> 
> I wrote a spreadsheet to map the logic:
> 
>
https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_2qwVqlf...

What about ios_browser_backend?

> 
> The only change in behavior with this change is that the Replay DNS server is
> never started with CrOS. That is fine because CrOS never uses it anyway.
> 
>
https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
> (left):
> 
>
https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:439:
> if not self.forwarder_factory.does_forwarder_override_dns:
> On 2014/10/06 22:07:50, nednguyen wrote:
> > What is the equivalent of this logic in your change?
> 
> In webpagereplay, --no-dns_forwarding is added when the DNS port is None.
> 
> I created a spreadsheet to show make sure that matches up.
> 
>
https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_2qwVqlf...
> 
> A possible improvement after this CL would be setting the ports in one place
> based on a few parameters. As it is now, the ports are set in various browser
> backend initializers.
> 
>
https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
> (left):
> 
>
https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py:72:
> else:
> On 2014/10/06 22:07:50, nednguyen wrote:
> > This says append "-no-dns_forwarding" if netsim option is not specified
which
> is
> > totally different from the branching condition in
> > AndroidBrowserBackend.AddReplayServerOptions. How do you exactly unify the
two
> > into replay module?
> 
> If netsim is True, chrome_browser_backend sets the DNS port (in __init__), so
> this ends up being the same as the logic in webpagereplay.
> 
>
https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/webpagereplay.py (right):
> 
>
https://codereview.chromium.org/617043003/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/webpagereplay.py:72: WPR_REPLAY_DIR: path to
> alternate Web Page Replay source.
> On 2014/10/06 22:07:50, nednguyen wrote:
> > Why not also remove this?
> 
> Done.
> 
>
https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
> (left):
> 
>
https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:440:
> extra_wpr_args.append('--no-dns_forwarding')
> On 2014/10/07 23:13:59, chrishenry wrote:
> > Where's the code that sets dns_port? Are they using the same conditionals?
> 
> Right now it is spread among the browser backend __init__ methods.  The logic
is
> the same.
> 
> I made a spreadsheet to validate: 
> 
>
https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_2qwVqlf...
> 
>
https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:442:
> extra_wpr_args.append('--net=%s' % self.browser_options.netsim)
> On 2014/10/07 23:13:59, chrishenry wrote:
> > Where's --net being set?
> 
> wpr_server.ReplayServer.__init__
> 
>
https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/webpagereplay.py (left):
> 
>
https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/webpagereplay.py:72: WPR_ARCHIVE_PATH: path to
> alternate archive file (e.g. '/tmp/foo.wpr').
> On 2014/10/07 23:13:59, chrishenry wrote:
> > To make this patch a bit more focused, perhaps extract this deletion to a
> > separate patch.
> 
> Done.
> 
>
https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/webpagereplay.py (right):
> 
>
https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/webpagereplay.py:113: def
_CommandLine(replay_py,
> wpr_args, archive_path, host_ip,
> On 2014/10/07 23:13:59, chrishenry wrote:
> > The method name is really not obvious to me. Can you just name it
> CreateCommand
> > or something like that?
> 
> Done.
> 
>
https://codereview.chromium.org/617043003/diff/240001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/webpagereplay.py:257: logging.warning('DNS
> nameserver configuration might not be restored!')
> On 2014/10/07 23:13:59, chrishenry wrote:
> > Can you give more context to this warning? I can see people be really
confused
> > by the warning message...
> 
> Moved to separate CL: https://codereview.chromium.org/625123002/

[chromium-reviews, 2014-10-14 21:31:53]

It does not have a forwarder that I can see. AddReplayServerOptions only
warns "Not implemented", so netsim was not working there.

On Tue, Oct 14, 2014 at 1:38 PM, <nednguyen@google.com> wrote:

> On 2014/10/13 21:45:55, slamm wrote:
>
>> I went back and double-checked everything.
>>
>
>  I wrote a spreadsheet to map the logic:
>>
>
>
> https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_
> 2qwVqlfmFWAMfWILjuT7XMKd0y3g/edit?usp=sharing
>
> What about ios_browser_backend?
>
>
>
>  The only change in behavior with this change is that the Replay DNS
>> server is
>> never started with CrOS. That is fine because CrOS never uses it anyway.
>>
>
>
> https://codereview.chromium.org/617043003/diff/220001/
> tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
>
>> File tools/telemetry/telemetry/core/backends/chrome/android_
>> browser_backend.py
>> (left):
>>
>
>
> https://codereview.chromium.org/617043003/diff/220001/
> tools/telemetry/telemetry/core/backends/chrome/android_
> browser_backend.py#oldcode439
>
>> tools/telemetry/telemetry/core/backends/chrome/android_
>> browser_backend.py:439:
>> if not self.forwarder_factory.does_forwarder_override_dns:
>> On 2014/10/06 22:07:50, nednguyen wrote:
>> > What is the equivalent of this logic in your change?
>>
>
>  In webpagereplay, --no-dns_forwarding is added when the DNS port is None.
>>
>
>  I created a spreadsheet to show make sure that matches up.
>>
>
>
> https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_
> 2qwVqlfmFWAMfWILjuT7XMKd0y3g/edit#gid=0
>
>  A possible improvement after this CL would be setting the ports in one
>> place
>> based on a few parameters. As it is now, the ports are set in various
>> browser
>> backend initializers.
>>
>
>
> https://codereview.chromium.org/617043003/diff/220001/
> tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py
>
>> File tools/telemetry/telemetry/core/backends/chrome/chrome_
>> browser_backend.py
>> (left):
>>
>
>
> https://codereview.chromium.org/617043003/diff/220001/
> tools/telemetry/telemetry/core/backends/chrome/chrome_
> browser_backend.py#oldcode72
>
>> tools/telemetry/telemetry/core/backends/chrome/chrome_
>> browser_backend.py:72:
>> else:
>> On 2014/10/06 22:07:50, nednguyen wrote:
>> > This says append "-no-dns_forwarding" if netsim option is not specified
>>
> which
>
>> is
>> > totally different from the branching condition in
>> > AndroidBrowserBackend.AddReplayServerOptions. How do you exactly unify
>> the
>>
> two
>
>> > into replay module?
>>
>
>  If netsim is True, chrome_browser_backend sets the DNS port (in
>> __init__), so
>> this ends up being the same as the logic in webpagereplay.
>>
>
>
> https://codereview.chromium.org/617043003/diff/220001/
> tools/telemetry/telemetry/core/webpagereplay.py
>
>> File tools/telemetry/telemetry/core/webpagereplay.py (right):
>>
>
>
> https://codereview.chromium.org/617043003/diff/220001/
> tools/telemetry/telemetry/core/webpagereplay.py#newcode72
>
>> tools/telemetry/telemetry/core/webpagereplay.py:72: WPR_REPLAY_DIR: path
>> to
>> alternate Web Page Replay source.
>> On 2014/10/06 22:07:50, nednguyen wrote:
>> > Why not also remove this?
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.org/617043003/diff/240001/
> tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
>
>> File tools/telemetry/telemetry/core/backends/chrome/android_
>> browser_backend.py
>> (left):
>>
>
>
> https://codereview.chromium.org/617043003/diff/240001/
> tools/telemetry/telemetry/core/backends/chrome/android_
> browser_backend.py#oldcode440
>
>> tools/telemetry/telemetry/core/backends/chrome/android_
>> browser_backend.py:440:
>> extra_wpr_args.append('--no-dns_forwarding')
>> On 2014/10/07 23:13:59, chrishenry wrote:
>> > Where's the code that sets dns_port? Are they using the same
>> conditionals?
>>
>
>  Right now it is spread among the browser backend __init__ methods.  The
>> logic
>>
> is
>
>> the same.
>>
>
>  I made a spreadsheet to validate:
>>
>
>
> https://docs.google.com/a/chromium.org/spreadsheets/d/14fNT8Sd-bO3ZbD_
> 2qwVqlfmFWAMfWILjuT7XMKd0y3g/edit?usp=sharing
>
>
> https://codereview.chromium.org/617043003/diff/240001/
> tools/telemetry/telemetry/core/backends/chrome/android_
> browser_backend.py#oldcode442
>
>> tools/telemetry/telemetry/core/backends/chrome/android_
>> browser_backend.py:442:
>> extra_wpr_args.append('--net=%s' % self.browser_options.netsim)
>> On 2014/10/07 23:13:59, chrishenry wrote:
>> > Where's --net being set?
>>
>
>  wpr_server.ReplayServer.__init__
>>
>
>
> https://codereview.chromium.org/617043003/diff/240001/
> tools/telemetry/telemetry/core/webpagereplay.py
>
>> File tools/telemetry/telemetry/core/webpagereplay.py (left):
>>
>
>
> https://codereview.chromium.org/617043003/diff/240001/
> tools/telemetry/telemetry/core/webpagereplay.py#oldcode72
>
>> tools/telemetry/telemetry/core/webpagereplay.py:72: WPR_ARCHIVE_PATH:
>> path to
>> alternate archive file (e.g. '/tmp/foo.wpr').
>> On 2014/10/07 23:13:59, chrishenry wrote:
>> > To make this patch a bit more focused, perhaps extract this deletion to
>> a
>> > separate patch.
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.org/617043003/diff/240001/
> tools/telemetry/telemetry/core/webpagereplay.py
>
>> File tools/telemetry/telemetry/core/webpagereplay.py (right):
>>
>
>
> https://codereview.chromium.org/617043003/diff/240001/
> tools/telemetry/telemetry/core/webpagereplay.py#newcode113
>
>> tools/telemetry/telemetry/core/webpagereplay.py:113: def
>>
> _CommandLine(replay_py,
>
>> wpr_args, archive_path, host_ip,
>> On 2014/10/07 23:13:59, chrishenry wrote:
>> > The method name is really not obvious to me. Can you just name it
>> CreateCommand
>> > or something like that?
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.org/617043003/diff/240001/
> tools/telemetry/telemetry/core/webpagereplay.py#newcode257
>
>> tools/telemetry/telemetry/core/webpagereplay.py:257: logging.warning('DNS
>> nameserver configuration might not be restored!')
>> On 2014/10/07 23:13:59, chrishenry wrote:
>> > Can you give more context to this warning? I can see people be really
>>
> confused
>
>> > by the warning message...
>>
>
>  Moved to separate CL: https://codereview.chromium.org/625123002/
>>
>
>
>
> https://codereview.chromium.org/617043003/
>

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

[slamm, 2014-10-14 21:54:02]

slamm@chromium.org changed reviewers:
+ baxley@chromium.org

[slamm, 2014-10-14 23:39:28]

Rebase.

[slamm, 2014-10-15 18:11:11]

The CQ bit was checked by slamm@chromium.org

[commit-bot: I haz the power, 2014-10-15 18:13:07]

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

[chrishenry, 2014-10-15 18:29:47]

lgtm

[commit-bot: I haz the power, 2014-10-15 18:33:31]

Committed patchset #13 (id:400001)

[commit-bot: I haz the power, 2014-10-15 18:34:35]

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