Randomize proxy timeout on Android over a range.

Randomize proxy timeout on Android over a range.

BUG=237876
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199288

[mmenke, 2013-05-08 16:50:08]

https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc#newco...
net/proxy/proxy_list.cc:201: TimeDelta::FromMilliseconds(base::RandInt(1 * 60 *
1000, 5 * 60 * 1000));
const Blah kBlah is generally only used for values that are always constant.  I
don't think I've ever seen it used for a randomized value, so this seems a
little weird.

I'm open to suggestions of prettier ways to do this.  If not, I'll think about
it and get back to you later (Probably today, possibly tomorrow).

[cbentzel, 2013-05-08 16:57:18]

https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc#newco...
net/proxy/proxy_list.cc:201: TimeDelta::FromMilliseconds(base::RandInt(1 * 60 *
1000, 5 * 60 * 1000));
On 2013/05/08 16:50:08, mmenke wrote:
> const Blah kBlah is generally only used for values that are always constant. 
I
> don't think I've ever seen it used for a randomized value, so this seems a
> little weird.
> 
> I'm open to suggestions of prettier ways to do this.  If not, I'll think about
> it and get back to you later (Probably today, possibly tomorrow).

proxy_retry_delay?

I don't think the const buys us much here.

[mmenke, 2013-05-08 16:58:58]

https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc#newco...
net/proxy/proxy_list.cc:201: TimeDelta::FromMilliseconds(base::RandInt(1 * 60 *
1000, 5 * 60 * 1000));
On 2013/05/08 16:57:18, cbentzel wrote:
> On 2013/05/08 16:50:08, mmenke wrote:
> > const Blah kBlah is generally only used for values that are always constant.

> I
> > don't think I've ever seen it used for a randomized value, so this seems a
> > little weird.
> > 
> > I'm open to suggestions of prettier ways to do this.  If not, I'll think
about
> > it and get back to you later (Probably today, possibly tomorrow).
> 
> proxy_retry_delay?
> 
> I don't think the const buys us much here.

The nice thing about the const is that it draws attention to "Thar be hard-coded
constants here!"

I'm fine with that solution, though.

[bengr, 2013-05-08 17:23:11]

https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc
File net/proxy/proxy_list.cc (right):

https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc#newco...
net/proxy/proxy_list.cc:201: TimeDelta::FromMilliseconds(base::RandInt(1 * 60 *
1000, 5 * 60 * 1000));
On 2013/05/08 16:58:58, mmenke wrote:
> On 2013/05/08 16:57:18, cbentzel wrote:
> > On 2013/05/08 16:50:08, mmenke wrote:
> > > const Blah kBlah is generally only used for values that are always
constant.
> 
> > I
> > > don't think I've ever seen it used for a randomized value, so this seems a
> > > little weird.
> > > 
> > > I'm open to suggestions of prettier ways to do this.  If not, I'll think
> about
> > > it and get back to you later (Probably today, possibly tomorrow).
> > 
> > proxy_retry_delay?
> > 
> > I don't think the const buys us much here.
> 
> The nice thing about the const is that it draws attention to "Thar be
hard-coded
> constants here!"
> 
> I'm fine with that solution, though.

Yeah, I thought about three ways of writing this: (1) declare some constants,
which seemed liked overkill, considering the retry delay on line 203 isn't
declared that way; (2) use TimeDelta as a way of calling out that this is a
constant:
const TimeDelta kProxyRetryDelay =
    TimeDelta::FromMilliseconds(base::RandInt(
        TimeDelta::FromMinutes(1).InMilliseconds(),
        TimeDelta::FromMinutes(5).InMilliseconds()));
(3) what's in this CL.

[mmenke, 2013-05-08 18:24:05]

On 2013/05/08 17:23:11, bengr1 wrote:
> https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc
> File net/proxy/proxy_list.cc (right):
> 
>
https://codereview.chromium.org/14864016/diff/1/net/proxy/proxy_list.cc#newco...
> net/proxy/proxy_list.cc:201: TimeDelta::FromMilliseconds(base::RandInt(1 * 60
*
> 1000, 5 * 60 * 1000));
> On 2013/05/08 16:58:58, mmenke wrote:
> > On 2013/05/08 16:57:18, cbentzel wrote:
> > > On 2013/05/08 16:50:08, mmenke wrote:
> > > > const Blah kBlah is generally only used for values that are always
> constant.
> > 
> > > I
> > > > don't think I've ever seen it used for a randomized value, so this seems
a
> > > > little weird.
> > > > 
> > > > I'm open to suggestions of prettier ways to do this.  If not, I'll think
> > about
> > > > it and get back to you later (Probably today, possibly tomorrow).
> > > 
> > > proxy_retry_delay?
> > > 
> > > I don't think the const buys us much here.
> > 
> > The nice thing about the const is that it draws attention to "Thar be
> hard-coded
> > constants here!"
> > 
> > I'm fine with that solution, though.
> 
> Yeah, I thought about three ways of writing this: (1) declare some constants,
> which seemed liked overkill, considering the retry delay on line 203 isn't
> declared that way; (2) use TimeDelta as a way of calling out that this is a
> constant:
> const TimeDelta kProxyRetryDelay =
>     TimeDelta::FromMilliseconds(base::RandInt(
>         TimeDelta::FromMinutes(1).InMilliseconds(),
>         TimeDelta::FromMinutes(5).InMilliseconds()));
> (3) what's in this CL.

I think we should just go with Chris's suggestion.

Could add constants at the top of the file instead (min/max on Android, and just
one value elsewhere, and code here to randomize on android), but don't think we
get enough out of it to be worth the effort.

[bengr, 2013-05-08 21:05:15]

PTAL.

[mmenke, 2013-05-09 14:51:05]

On 2013/05/08 21:05:15, bengr1 wrote:
> PTAL.

LGTM.

[commit-bot: I haz the power, 2013-05-09 15:34:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/14864016/8001

[commit-bot: I haz the power, 2013-05-09 15:53:27]

Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-09 20:32:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bengr@chromium.org/14864016/8001

[commit-bot: I haz the power, 2013-05-09 22:21:07]

Change committed as 199288

[Matt Welsh, 2013-05-10 18:28:25]

https://chromiumcodereview.appspot.com/14864016/diff/8001/net/proxy/proxy_lis...
File net/proxy/proxy_list.cc (right):

https://chromiumcodereview.appspot.com/14864016/diff/8001/net/proxy/proxy_lis...
net/proxy/proxy_list.cc:198: #if defined(OS_ANDROID)
Shouldn't this also be the case for iOS?

[mmenke, 2013-05-10 18:34:01]

On 2013/05/10 18:28:25, Matt Welsh wrote:
>
https://chromiumcodereview.appspot.com/14864016/diff/8001/net/proxy/proxy_lis...
> File net/proxy/proxy_list.cc (right):
> 
>
https://chromiumcodereview.appspot.com/14864016/diff/8001/net/proxy/proxy_lis...
> net/proxy/proxy_list.cc:198: #if defined(OS_ANDROID)
> Shouldn't this also be the case for iOS?

Gah!  Can't believe I missed that.