[chromedriver] Add Network Presets to Network Throttling feature

Add Network Presets to Network Throttling feature

BUG=

Committed: https://crrev.com/0d53d7a98e6fb3faf3a0865b830f783d7e60eb98
Cr-Commit-Position: refs/heads/master@{#320987}

[srawlins, 2015-03-12 19:38:33]

srawlins@google.com changed reviewers:
+ samuong@chromium.org

[srawlins, 2015-03-12 22:28:37]

Patchset #2 (id:20001) has been deleted

[srawlins, 2015-03-12 22:30:06]

OK Sam, this should be ready to go.

[samuong, 2015-03-13 22:30:51]

This looks pretty good, just a few small comments.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/network_conditions.cc (right):

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions.cc:9: #include
"chrome/test/chromedriver/chrome/network_conditions.h"
nit: the #include ".../network_conditions.h" should come first and have a blank
line afterwards

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions.cc:55: continue;
is there any reason not to use base::ListValue::Find() rather than iterating
over |networks|?

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions.cc:76: // |offline| is always
false for now.
is |offline| also false when network_name == "offline"?

also, as a workaround for crbug.com/456324, would it work if we set
throughput=0?

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:883: Status status =
FindPresetNetwork(network_name, &network_conditions);
I think your code would be simpler if you made FindPresetNetwork() take a raw
pointer, rather than a scoped_ptr.

Rather than allocate memory in network_conditions.cc:78 as well as
window_commands.cc:887, you could just do it once at window_commands.cc:880.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:904:
&network_conditions->download_throughput) ||
nit: unindent by 1 char

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:906:
&network_conditions->upload_throughput))
nit: unindent by 1 char

[srawlins, 2015-03-16 18:26:29]

Thanks for reviewing, Sam! Addressed comments.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/network_conditions.cc (right):

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions.cc:9: #include
"chrome/test/chromedriver/chrome/network_conditions.h"
On 2015/03/13 22:30:51, samuong wrote:
> nit: the #include ".../network_conditions.h" should come first and have a
blank
> line afterwards

Done.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions.cc:55: continue;
On 2015/03/13 22:30:51, samuong wrote:
> is there any reason not to use base::ListValue::Find() rather than iterating
> over |networks|?

I don't know how I would use ListValue::Find() here. I'm iterating and querying
each Value on it's "title" property.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions.cc:76: // |offline| is always
false for now.
On 2015/03/13 22:30:51, samuong wrote:
> is |offline| also false when network_name == "offline"?
> 
> also, as a workaround for crbug.com/456324, would it work if we set
> throughput=0?

No, I tried this before :( Still no communication from Chromedriver to Chrome.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:883: Status status =
FindPresetNetwork(network_name, &network_conditions);
On 2015/03/13 22:30:51, samuong wrote:
> I think your code would be simpler if you made FindPresetNetwork() take a raw
> pointer, rather than a scoped_ptr.
> 
> Rather than allocate memory in network_conditions.cc:78 as well as
> window_commands.cc:887, you could just do it once at window_commands.cc:880.

Good call. Done.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:904:
&network_conditions->download_throughput) ||
On 2015/03/13 22:30:51, samuong wrote:
> nit: unindent by 1 char

Done.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:906:
&network_conditions->upload_throughput))
On 2015/03/13 22:30:51, samuong wrote:
> nit: unindent by 1 char

Done.

[samuong, 2015-03-16 18:43:11]

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/network_conditions.cc (right):

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/network_conditions.cc:55: continue;
On 2015/03/16 18:26:28, srawlins wrote:
> On 2015/03/13 22:30:51, samuong wrote:
> > is there any reason not to use base::ListValue::Find() rather than iterating
> > over |networks|?
> 
> I don't know how I would use ListValue::Find() here. I'm iterating and
querying
> each Value on it's "title" property.

Oh right, yeah too bad this isn't a dictionary. OK let's just iterate through it
then.

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:883: Status status =
FindPresetNetwork(network_name, &network_conditions);
On 2015/03/16 18:26:28, srawlins wrote:
> On 2015/03/13 22:30:51, samuong wrote:
> > I think your code would be simpler if you made FindPresetNetwork() take a
raw
> > pointer, rather than a scoped_ptr.
> > 
> > Rather than allocate memory in network_conditions.cc:78 as well as
> > window_commands.cc:887, you could just do it once at window_commands.cc:880.
> 
> Good call. Done.

Oh, I meant just change FindPresetNetwork() to use a raw pointer, but in this
function you should continue to use a scoped_ptr. Otherwise we'll leak the
NetworkConditions object if we error out mid-way through (e.g. if the params are
not in the right format).

[srawlins, 2015-03-16 19:23:11]

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1004843002/diff/40001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:883: Status status =
FindPresetNetwork(network_name, &network_conditions);
On 2015/03/16 18:43:10, samuong wrote:
> On 2015/03/16 18:26:28, srawlins wrote:
> > On 2015/03/13 22:30:51, samuong wrote:
> > > I think your code would be simpler if you made FindPresetNetwork() take a
> raw
> > > pointer, rather than a scoped_ptr.
> > > 
> > > Rather than allocate memory in network_conditions.cc:78 as well as
> > > window_commands.cc:887, you could just do it once at
window_commands.cc:880.
> > 
> > Good call. Done.
> 
> Oh, I meant just change FindPresetNetwork() to use a raw pointer, but in this
> function you should continue to use a scoped_ptr. Otherwise we'll leak the
> NetworkConditions object if we error out mid-way through (e.g. if the params
are
> not in the right format).

Done.

[samuong, 2015-03-16 19:38:08]

thanks sam, lgtm with one nit

https://codereview.chromium.org/1004843002/diff/80001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1004843002/diff/80001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:881: network_conditions.reset(new
NetworkConditions());
nit: you can do this on one line:

scoped_ptr<NetworkConditions> network_conditions(new NetworkConditions());

[srawlins, 2015-03-17 20:38:13]

https://codereview.chromium.org/1004843002/diff/80001/chrome/test/chromedrive...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/1004843002/diff/80001/chrome/test/chromedrive...
chrome/test/chromedriver/window_commands.cc:881: network_conditions.reset(new
NetworkConditions());
On 2015/03/16 19:38:08, samuong wrote:
> nit: you can do this on one line:
> 
> scoped_ptr<NetworkConditions> network_conditions(new NetworkConditions());

Done.

[srawlins, 2015-03-17 20:38:16]

The CQ bit was checked by srawlins@google.com

[srawlins, 2015-03-17 20:38:17]

The patchset sent to the CQ was uploaded after l-g-t-m from samuong@chromium.org
Link to the patchset: https://codereview.chromium.org/1004843002/#ps100001
(title: "nits")

[commit-bot: I haz the power, 2015-03-17 20:38:37]

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

[commit-bot: I haz the power, 2015-03-17 21:53:48]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-03-17 21:54:39]

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