sandwich: Implements network condition on WPR server and browser.

sandwich: Implements network condition on WPR server and browser.

BUG=582080
Committed: https://crrev.com/45605a80083f18230d5359890f6b8735310b25af
Cr-Commit-Position: refs/heads/master@{#376731}

[gabadie, 2016-02-18 12:14:21]

gabadie@chromium.org changed reviewers:
+ lizeb@chromium.org, mattcary@chromium.org

[gabadie, 2016-02-18 12:14:22]

Got finally everything working.

lizeb@chromium.org & mattcary@chromium.org: PTAL. =)

[Benoit L, 2016-02-18 13:01:25]

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/devto...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:23: WEBSOCKET_TIMEOUT = 10 # seconds
This is not really a websocket timeout. It's rather the time after which we give
up if we don't get any new event.

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:68: # Websocket timeout of 1 minute to
avoid websocket timeout on slow
As above, need a better name (and comment).

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:302: options.OPTIONS.ParseArgs([])
Don't you want to pass the command line here?

[gabadie, 2016-02-18 13:21:58]

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/devto...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:23: WEBSOCKET_TIMEOUT = 10 # seconds
On 2016/02/18 13:01:25, Benoit L wrote:
> This is not really a websocket timeout. It's rather the time after which we
give
> up if we don't get any new event.

Done.

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:68: # Websocket timeout of 1 minute to
avoid websocket timeout on slow
On 2016/02/18 13:01:25, Benoit L wrote:
> As above, need a better name (and comment).

Done.

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:302: options.OPTIONS.ParseArgs([])
On 2016/02/18 13:01:25, Benoit L wrote:
> Don't you want to pass the command line here?

No. This is just a work around that has been added in the parent CL:
https://codereview.chromium.org/1707793002/

And I prefer to have 100% control over my command line parameter and its --help
rather than  having the lower level layers polluting my command line MAN message
with flags this script might not be ready for yet.

I personally think global variable usage like this guy must be banned...

[gabadie, 2016-02-19 12:38:34]

On 2016/02/18 13:21:58, gabadie wrote:
>
https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/devto...
> File tools/android/loading/devtools_monitor.py (right):
> 
>
https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/devto...
> tools/android/loading/devtools_monitor.py:23: WEBSOCKET_TIMEOUT = 10 # seconds
> On 2016/02/18 13:01:25, Benoit L wrote:
> > This is not really a websocket timeout. It's rather the time after which we
> give
> > up if we don't get any new event.
> 
> Done.
> 
>
https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
> File tools/android/loading/run_sandwich.py (right):
> 
>
https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
> tools/android/loading/run_sandwich.py:68: # Websocket timeout of 1 minute to
> avoid websocket timeout on slow
> On 2016/02/18 13:01:25, Benoit L wrote:
> > As above, need a better name (and comment).
> 
> Done.
> 
>
https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
> tools/android/loading/run_sandwich.py:302: options.OPTIONS.ParseArgs([])
> On 2016/02/18 13:01:25, Benoit L wrote:
> > Don't you want to pass the command line here?
> 
> No. This is just a work around that has been added in the parent CL:
> https://codereview.chromium.org/1707793002/
> 
> And I prefer to have 100% control over my command line parameter and its
--help
> rather than  having the lower level layers polluting my command line MAN
message
> with flags this script might not be ready for yet.
> 
> I personally think global variable usage like this guy must be banned...

New revision uploaded. PTAL. =)

[mattcary, 2016-02-19 15:40:17]

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707363002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:302: options.OPTIONS.ParseArgs([])
On 2016/02/18 13:21:58, gabadie wrote:
> On 2016/02/18 13:01:25, Benoit L wrote:
> > Don't you want to pass the command line here?
> 
> No. This is just a work around that has been added in the parent CL:
> https://codereview.chromium.org/1707793002/
> 
> And I prefer to have 100% control over my command line parameter and its
--help
> rather than  having the lower level layers polluting my command line MAN
message
> with flags this script might not be ready for yet.
> 
> I personally think global variable usage like this guy must be banned...


The problem though is that many of the other library tools assume specific
command line configuration. So you don't actually want 100% control over your
command line, as then you'd have to leak the rest of the library to configure it
correctly.

For example, you're disabling devtools port configuration here, and you probably
didn't intend that.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/d...
File tools/android/loading/device_setup.py (right):

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/d...
tools/android/loading/device_setup.py:126: logging.error('WPR cannot record
without a specified archive.')
Do you want to assert rather than just logging an error? In many situations it's
better to not work at all rather than work incorrectly.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/d...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/d...
tools/android/loading/devtools_monitor.py:209: timeout=DEFAULT_TIMEOUT):
this timeout should be configured as part of the devtools connection. It gets
very unmaintainable passing all these configuration details as method
parameters.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/r...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/r...
tools/android/loading/run_sandwich.py:302: options.OPTIONS.ParseArgs([])
As mentioned on other cl, integrate with OPTIONS so other libraries will work
correctly.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/r...
tools/android/loading/run_sandwich.py:345: device=device,
This would be the better place to put the timeout parameter.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/t...
File tools/android/loading/trace_recorder.py (right):

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/t...
tools/android/loading/trace_recorder.py:35:
timeout=devtools_monitor.DEFAULT_TIMEOUT):
As mentioned elsewhere, this timeout should really be a connection configuration
detail. That would avoid plumbing it around (the timeout is really a
characteristic of our particular connection and not something that MonitorUrl
needs to know about).

[mattcary, 2016-02-22 09:44:47]

lgtm

After offline discussion, we've decided that the options parsing needs some
reworking, as caching and device emulation need to be moved out of flag-defined
options so that they can be manipulated grammatically. So Guillaume will land
this without using options in the recommended way, then he & I will work
together to fix the API so that everything will work correctly.

[gabadie, 2016-02-22 10:05:01]

New revision uploaded. PTAL.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/d...
File tools/android/loading/device_setup.py (right):

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/d...
tools/android/loading/device_setup.py:126: logging.error('WPR cannot record
without a specified archive.')
On 2016/02/19 15:40:17, mattcary wrote:
> Do you want to assert rather than just logging an error? In many situations
it's
> better to not work at all rather than work incorrectly.

Done.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/d...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/d...
tools/android/loading/devtools_monitor.py:209: timeout=DEFAULT_TIMEOUT):
On 2016/02/19 15:40:17, mattcary wrote:
> this timeout should be configured as part of the devtools connection. It gets
> very unmaintainable passing all these configuration details as method
> parameters.

Agreed. But to me only few dispatch calls needed a longer timeout. Since this
custom timeout is significantly higher, it was making sens to not apply it to
every call and have a per call granularity.

To solve the maintainability, the TraceRecorder proposal was my solution,
because he could take car e of this. "Oh you are running on android with a 2g
emulated network on the browser side, let me change this timeout silently for
you so that you don't have to deal with it in your upper layers".

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/r...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/r...
tools/android/loading/run_sandwich.py:302: options.OPTIONS.ParseArgs([])
On 2016/02/19 15:40:17, mattcary wrote:
> As mentioned on other cl, integrate with OPTIONS so other libraries will work
> correctly.

Acknowledged.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/r...
tools/android/loading/run_sandwich.py:345: device=device,
On 2016/02/19 15:40:17, mattcary wrote:
> This would be the better place to put the timeout parameter.

Please see the other comment about this.

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/t...
File tools/android/loading/trace_recorder.py (right):

https://codereview.chromium.org/1707363002/diff/20001/tools/android/loading/t...
tools/android/loading/trace_recorder.py:35:
timeout=devtools_monitor.DEFAULT_TIMEOUT):
On 2016/02/19 15:40:17, mattcary wrote:
> As mentioned elsewhere, this timeout should really be a connection
configuration
> detail. That would avoid plumbing it around (the timeout is really a
> characteristic of our particular connection and not something that MonitorUrl
> needs to know about).

Please see the other comment about this issue.

[Benoit L, 2016-02-22 14:11:26]

lgtm

https://codereview.chromium.org/1707363002/diff/60001/tools/android/loading/d...
File tools/android/loading/device_setup.py (right):

https://codereview.chromium.org/1707363002/diff/60001/tools/android/loading/d...
tools/android/loading/device_setup.py:115: network_condition_name: Network
condition name available in
tiny nit: the ordering of arguments should match the ordering in the parameter
list. Also, add doc for disable_script_injection.

https://codereview.chromium.org/1707363002/diff/60001/tools/android/loading/d...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/1707363002/diff/60001/tools/android/loading/d...
tools/android/loading/devtools_monitor.py:23: DEFAULT_TIMEOUT = 10 # seconds
I usually prefer to have the unit in the name, such as DEFAULT_TIMEOUT_S.
But up to you.

[gabadie, 2016-02-22 14:46:46]

On 2016/02/22 14:11:26, Benoit L wrote:
> lgtm
> 
>
https://codereview.chromium.org/1707363002/diff/60001/tools/android/loading/d...
> File tools/android/loading/device_setup.py (right):
> 
>
https://codereview.chromium.org/1707363002/diff/60001/tools/android/loading/d...
> tools/android/loading/device_setup.py:115: network_condition_name: Network
> condition name available in
> tiny nit: the ordering of arguments should match the ordering in the parameter
> list. Also, add doc for disable_script_injection.
> 
>
https://codereview.chromium.org/1707363002/diff/60001/tools/android/loading/d...
> File tools/android/loading/devtools_monitor.py (right):
> 
>
https://codereview.chromium.org/1707363002/diff/60001/tools/android/loading/d...
> tools/android/loading/devtools_monitor.py:23: DEFAULT_TIMEOUT = 10 # seconds
> I usually prefer to have the unit in the name, such as DEFAULT_TIMEOUT_S.
> But up to you.

Thanks! Committing once dependencies are landed.

[gabadie, 2016-02-22 15:15:40]

The CQ bit was checked by gabadie@chromium.org

[gabadie, 2016-02-22 15:15:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org
Link to the patchset: https://codereview.chromium.org/1707363002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2016-02-22 15:16:01]

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

[commit-bot: I haz the power, 2016-02-22 15:36:30]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-22 15:37:52]

Description was changed from

==========
sandwich: Implements network condition on WPR server and browser.

BUG=582080
==========

to

==========
sandwich: Implements network condition on WPR server and browser.

BUG=582080

Committed: https://crrev.com/45605a80083f18230d5359890f6b8735310b25af
Cr-Commit-Position: refs/heads/master@{#376731}
==========

[commit-bot: I haz the power, 2016-02-22 15:37:53]

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