Use Histogram algorithm to calculate DNS timeout. Honor DnsConfig timeout if it exceeds 5s.

Use Histogram algorithm to calculate DNS timeout. 
Honor DnsConfig timeout if it exceeds 5s.

BUG=110197
TEST=net_unittests --gtest_filter=DnsSessionTest.HistogramTimeout*

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

[mef, 2013-07-25 20:48:04]

Hi Szymon, please take a look.

[szym, 2013-07-25 22:47:24]

lgtm

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcode94
net/dns/dns_session.cc:94: // Histogram-based method.
This comment can be misleading. Suggest undoing this change.

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcod...
net/dns/dns_session.cc:216: if (config_.timeout.InMilliseconds() >
kMaxTimeoutMs)
should it be >=?

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcod...
net/dns/dns_session.cc:218: // Use Histogram to calculate next timeout.
nit: superfluous comment

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session_unittest.cc
File net/dns/dns_session_unittest.cc (right):

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session_unittest....
net/dns/dns_session_unittest.cc:225: // Calculated timeout must be within 10ms
of config timeout.
Put comments on expectations just before TEST_F.

[szym, 2013-07-25 22:48:37]

Before committing, try it on some _valgrind bots (suggest linux_valgrind).


On Thu, Jul 25, 2013 at 6:47 PM, <szym@chromium.org> wrote:

> lgtm
>
>
>
>
>
https://codereview.chromium.**org/20482002/diff/1/net/dns/**dns_session.cc<ht...
> File net/dns/dns_session.cc (right):
>
> https://codereview.chromium.**org/20482002/diff/1/net/dns/**
>
dns_session.cc#newcode94<https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcode94>
> net/dns/dns_session.cc:94: // Histogram-based method.
> This comment can be misleading. Suggest undoing this change.
>
> https://codereview.chromium.**org/20482002/diff/1/net/dns/**
>
dns_session.cc#newcode216<https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcode216>
> net/dns/dns_session.cc:216: if (config_.timeout.**InMilliseconds() >
> kMaxTimeoutMs)
> should it be >=?
>
> https://codereview.chromium.**org/20482002/diff/1/net/dns/**
>
dns_session.cc#newcode218<https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcode218>
> net/dns/dns_session.cc:218: // Use Histogram to calculate next timeout.
> nit: superfluous comment
>
> https://codereview.chromium.**org/20482002/diff/1/net/dns/**
>
dns_session_unittest.cc<https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session_unittest.cc>
> File net/dns/dns_session_unittest.**cc (right):
>
> https://codereview.chromium.**org/20482002/diff/1/net/dns/**
>
dns_session_unittest.cc#**newcode225<https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session_unittest.cc#newcode225>
> net/dns/dns_session_unittest.**cc:225: // Calculated timeout must be
>
> within 10ms of config timeout.
> Put comments on expectations just before TEST_F.
>
>
https://codereview.chromium.**org/20482002/<https://codereview.chromium.org/2...
>

[mef, 2013-07-26 11:27:07]

Hi Szymon, I've addressed your comments and run linux_valgrind. It did fail, but
differently than a week ago (net vs unit), and seemingly unrelated to my change.
Could you take a look at the results?

thanks,
-m

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc
File net/dns/dns_session.cc (right):

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcode94
net/dns/dns_session.cc:94: // Histogram-based method.
On 2013/07/25 22:47:24, szym wrote:
> This comment can be misleading. Suggest undoing this change.

Done.

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcod...
net/dns/dns_session.cc:216: if (config_.timeout.InMilliseconds() >
kMaxTimeoutMs)
On 2013/07/25 22:47:24, szym wrote:
> should it be >=?

Done.

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session.cc#newcod...
net/dns/dns_session.cc:218: // Use Histogram to calculate next timeout.
On 2013/07/25 22:47:24, szym wrote:
> nit: superfluous comment

Done.

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session_unittest.cc
File net/dns/dns_session_unittest.cc (right):

https://codereview.chromium.org/20482002/diff/1/net/dns/dns_session_unittest....
net/dns/dns_session_unittest.cc:225: // Calculated timeout must be within 10ms
of config timeout.
On 2013/07/25 22:47:24, szym wrote:
> Put comments on expectations just before TEST_F.

Done.

[szym, 2013-07-26 20:30:56]

as we talked, the valgrind failure seemsunrelated. lgtm

[commit-bot: I haz the power, 2013-07-26 20:54:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/20482002/14001

[commit-bot: I haz the power, 2013-07-26 21:42:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/20482002/14001

[commit-bot: I haz the power, 2013-07-28 01:24:01]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-07-29 16:40:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/20482002/14001

[commit-bot: I haz the power, 2013-07-29 21:53:16]

Change committed as 214235