Display DNS probe results.

Display DNS probe results.

1. Modify the browser-side NetErrorTabHelper to send extra messages when it
   starts or declines to start a DNS probe.

2. Create a new error domain, "dnsprobe", with errors for "might run a DNS
   probe", "currently running a DNS probe", and all of the possible probe
   results.

3. Modify ChromeContentRendererClient to give the renderer-side NetErrorHelper
   a chance to choose the error strings before we call LocalizedError directly.

4. Catch DNS errors and provide the strings for the "might run a DNS probe"
   pseudo-error instead.

5. Add a function to neterror.html that lets us re-render the template with a
   new set of strings.

6. When we get a "probe started" message, replace the strings with those for
   the "currently running a DNS probe" pseudo-error.

7. When we get a "probe finished" message, replace the strings with those for
   the probe result.

8. When we get a "probe not run" message, replace the strings with those for
   the original error we received.

BUG=156415
TEST=DnsProbeBrowserTest

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

[Deprecated (see juliatuttle), 2013-03-29 19:57:49]

PTAL, mmenke.

[mmenke, 2013-04-01 17:44:12]

https://codereview.chromium.org/13270005/diff/1/chrome/browser/net/net_error_...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/13270005/diff/1/chrome/browser/net/net_error_...
chrome/browser/net/net_error_tab_helper.cc:239: {
nit:  "{" should go on previous line.

https://codereview.chromium.org/13270005/diff/1/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://codereview.chromium.org/13270005/diff/1/chrome/chrome_common.gypi#new...
chrome/chrome_common.gypi:640: 'common/net/net_error_info.cc',
Your upload seems to be missing this file.

https://codereview.chromium.org/13270005/diff/1/chrome/common/net/net_error_i...
File chrome/common/net/net_error_info.h (right):

https://codereview.chromium.org/13270005/diff/1/chrome/common/net/net_error_i...
chrome/common/net/net_error_info.h:21: // page.
nit:  Suggest comments on the line before the status.  It's more common, and
matches the style below.

https://codereview.chromium.org/13270005/diff/1/chrome/common/net/net_error_i...
chrome/common/net/net_error_info.h:34: enum DnsProbeError {
I think separating this out from DnsProbeResult is counter-intuitive and a bit
on the ugly side.

I'd just combine them into DnsProbeStatus or somesuch.

https://codereview.chromium.org/13270005/diff/1/chrome/common/net/net_error_i...
chrome/common/net/net_error_info.h:43: // added to it before passing to
GetErrorStrings.
nit:  Missing close parens.

https://codereview.chromium.org/13270005/diff/1/chrome/common/net/net_error_i...
chrome/common/net/net_error_info.h:44: DNS_PROBE_ERR_FINISHED, // +
DnsProbeResult
nit:  Two spaces between code and comment.

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:7: #include <string>
This should be in the header

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:10: #include "base/stringprintf.h"
This doesn't seem to be used.

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:24: #include
"grit/generated_resources.h"
Don't think these are used.

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:33: #include "ui/base/l10n/l10n_util.h"
Not used

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:82:
ALLOW_THIS_IN_INITIALIZER_LIST(tracker_(
It looks like we're now completely ignoring the results returned by the tracker.
 Should we go back to using it?  If not, we should get rid of it.

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:91: // the tab helper should start a DNS
probe after receiving it.)
Function level documentation should be with the function declaration, not its
implementation.

Also...This doesn't seem to return anything.

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:93:
tracker_.OnStartProvisionalLoad(GetFrameType(frame), GetPageType(frame));
state_ should be reset if this isn't an error page load, right?  Isn't that what
tracker_'s for?

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:104: LOG(WARNING) << "DNS error in
unexpected state " << state_;
DVLOG?

Also...Why isn't this expected?  Reload an error page before the probe finishes,
and it seems like you'd end up here.

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:129: LOG(WARNING) << "Finish load in
unexpected state " << state_;
DVLOG?

https://codereview.chromium.org/13270005/diff/1/chrome/renderer/net/net_error...
chrome/renderer/net/net_error_helper.cc:195: NOTREACHED();
This seems reachable:  If the error page is reloaded when we're in
AWAITING_PROBE_RESULT, then we can get one of the other messages.

[Deprecated (see juliatuttle), 2013-04-09 20:35:49]

PTAL, mmenke.

Still have todos, in the issue description.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/browser/net/net...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/browser/net/net...
chrome/browser/net/net_error_tab_helper.cc:239: {
On 2013/04/01 17:44:12, mmenke wrote:
> nit:  "{" should go on previous line.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/chrome_common.gypi
File chrome/chrome_common.gypi (right):

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/chrome_common.g...
chrome/chrome_common.gypi:640: 'common/net/net_error_info.cc',
On 2013/04/01 17:44:12, mmenke wrote:
> Your upload seems to be missing this file.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/common/net/net_...
File chrome/common/net/net_error_info.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/common/net/net_...
chrome/common/net/net_error_info.h:21: // page.
On 2013/04/01 17:44:12, mmenke wrote:
> nit:  Suggest comments on the line before the status.  It's more common, and
> matches the style below.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/common/net/net_...
chrome/common/net/net_error_info.h:34: enum DnsProbeError {
On 2013/04/01 17:44:12, mmenke wrote:
> I think separating this out from DnsProbeResult is counter-intuitive and a bit
> on the ugly side.
> 
> I'd just combine them into DnsProbeStatus or somesuch.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/common/net/net_...
chrome/common/net/net_error_info.h:43: // added to it before passing to
GetErrorStrings.
On 2013/04/01 17:44:12, mmenke wrote:
> nit:  Missing close parens.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/common/net/net_...
chrome/common/net/net_error_info.h:44: DNS_PROBE_ERR_FINISHED, // +
DnsProbeResult
On 2013/04/01 17:44:12, mmenke wrote:
> nit:  Two spaces between code and comment.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:7: #include <string>
On 2013/04/01 17:44:12, mmenke wrote:
> This should be in the header

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:10: #include "base/stringprintf.h"
On 2013/04/01 17:44:12, mmenke wrote:
> This doesn't seem to be used.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:24: #include
"grit/generated_resources.h"
On 2013/04/01 17:44:12, mmenke wrote:
> Don't think these are used.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:33: #include "ui/base/l10n/l10n_util.h"
On 2013/04/01 17:44:12, mmenke wrote:
> Not used

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:82:
ALLOW_THIS_IN_INITIALIZER_LIST(tracker_(
On 2013/04/01 17:44:12, mmenke wrote:
> It looks like we're now completely ignoring the results returned by the
tracker.
>  Should we go back to using it?  If not, we should get rid of it.

Went back to using it.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:91: // the tab helper should start a DNS
probe after receiving it.)
On 2013/04/01 17:44:12, mmenke wrote:
> Function level documentation should be with the function declaration, not its
> implementation.
> 
> Also...This doesn't seem to return anything.

It belonged with IsDnsError, which is declared above.  I've put it in the right
place.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:93:
tracker_.OnStartProvisionalLoad(GetFrameType(frame), GetPageType(frame));
On 2013/04/01 17:44:12, mmenke wrote:
> state_ should be reset if this isn't an error page load, right?  Isn't that
what
> tracker_'s for?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:104: LOG(WARNING) << "DNS error in
unexpected state " << state_;
On 2013/04/01 17:44:12, mmenke wrote:
> DVLOG?
> 
> Also...Why isn't this expected?  Reload an error page before the probe
finishes,
> and it seems like you'd end up here.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:129: LOG(WARNING) << "Finish load in
unexpected state " << state_;
On 2013/04/01 17:44:12, mmenke wrote:
> DVLOG?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper.cc:195: NOTREACHED();
On 2013/04/01 17:44:12, mmenke wrote:
> This seems reachable:  If the error page is reloaded when we're in
> AWAITING_PROBE_RESULT, then we can get one of the other messages.

We'd see the DidStart and DidCommit before we got another message.

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/resour...
File chrome/renderer/resources/neterror.html (right):

https://chromiumcodereview.appspot.com/13270005/diff/1/chrome/renderer/resour...
chrome/renderer/resources/neterror.html:166: function updateForDnsProbe(strings)
{
TODO(ttuttle): Update just the summary/suggestions so the page flickers less.

[mmenke, 2013-04-10 13:16:16]

I'll get to this this afternoon.

[mmenke, 2013-04-10 16:56:58]

Bunch more comments.  I still want to take a closer look at this later today, so
expect more comments later today.

https://codereview.chromium.org/13270005/diff/7001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/13270005/diff/7001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9458: +        can't be found, because the
DNS lookup failed.  Please hold on a moment while
Maybe "DNS" -> "host name"?  Less technical for the average user.

https://codereview.chromium.org/13270005/diff/7001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9459: +        <ph
name="PRODUCT_NAME">&lt;span jscontent="productName"&gt;&lt;/span&gt;<ex>Google
Chrome</ex></ph>
Substituting the product name in strings like this is now considered a bad idea,
since nouns change form depending on usage in some languages.  Instead, we
either shouldn't use the product name, or have two copies of the string, with
the name inlined - one each in chromium_resources.grd and
google_chrome_resouces.grd (I may have the file names wrong - they're in the
same directory as generated_resources.grd).

https://codereview.chromium.org/13270005/diff/7001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:9535: +      </message>
Something to think about:  May make sense just to use the ERR_NAME_NOT_RESOLVED
string here in all cases, to reduce flicker.

https://codereview.chromium.org/13270005/diff/7001/chrome/browser/net/dns_pro...
File chrome/browser/net/dns_probe_service.cc (right):

https://codereview.chromium.org/13270005/diff/7001/chrome/browser/net/dns_pro...
chrome/browser/net/dns_probe_service.cc:130: result_ =
chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN;
Should this actually be DNS_PROBE_MAX?  Seems like it would provide some
additional DCHECKS protecting against treating the value set here as a real
result.

https://codereview.chromium.org/13270005/diff/7001/chrome/browser/net/dns_pro...
chrome/browser/net/dns_probe_service.cc:184: DCHECK_NE(result_, kMaxStatus);
DCHECK_LT?

https://codereview.chromium.org/13270005/diff/7001/chrome/browser/net/dns_pro...
chrome/browser/net/dns_probe_service.cc:223: case
chrome_common_net::DNS_PROBE_MAX:
Maybe just a "default:"?

https://codereview.chromium.org/13270005/diff/7001/chrome/browser/net/net_err...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/13270005/diff/7001/chrome/browser/net/net_err...
chrome/browser/net/net_error_tab_helper.cc:193: dns_probe_result_ = result;
I don't think we gain anything from having |dns_probe_result_|.  This is now a
sufficiently simple state machine that I think we're adding complexity with no
real benefit.

My suggestion:  Get rid of it and MaybeSendStatus as well.

Put the logic for when to send the other two statuses in
MaybePostStartDnsProbeTask.  Can also get rid of DNS_PROBE_FINISHED.

https://codereview.chromium.org/13270005/diff/7001/chrome/browser/net/net_err...
chrome/browser/net/net_error_tab_helper.cc:217:
SendInfo(chrome_common_net::DNS_PROBE_NOT_RUN);
Think sending this on all tracker events is a bit too aggressive.  Really only
matters when we'd other send a message, except for the fact that ProbesAllowed()
is false.

https://codereview.chromium.org/13270005/diff/7001/chrome/common/localized_er...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/13270005/diff/7001/chrome/common/localized_er...
chrome/common/localized_error.cc:365: SUGGEST_NONE,
All the other results have "SUGGEST_RELOAD", and it's also handled above fold. 
Suggest using it here, too.

https://codereview.chromium.org/13270005/diff/7001/chrome/common/localized_er...
chrome/common/localized_error.cc:377: SUGGEST_RELOAD | SUGGEST_NONE,
"| SUGGEST_NONE" doesn't make any sense.  :)

https://codereview.chromium.org/13270005/diff/7001/chrome/common/localized_er...
chrome/common/localized_error.cc:377: SUGGEST_RELOAD | SUGGEST_NONE,
Do we need a PROBE_STARTED page?  I don't think we want / need separate pages
for PROBE_POSSIBLE and PROVE_STARTED.

https://codereview.chromium.org/13270005/diff/7001/chrome/common/localized_er...
chrome/common/localized_error.cc:381: // original error, which might be one of
several DNS-related errors.
Erm....  DNS_PROBE_FINISHED_UNKNOWN is here.

https://codereview.chromium.org/13270005/diff/7001/chrome/common/localized_er...
chrome/common/localized_error.cc:557: error_string =
ASCIIToUTF16(ascii_error_string);
Per other comment, we may just want to use the ERR_NAME_NOT_RESOLVED error stuff
here.

https://codereview.chromium.org/13270005/diff/7001/chrome/common/net/net_erro...
File chrome/common/net/net_error_info.cc (right):

https://codereview.chromium.org/13270005/diff/7001/chrome/common/net/net_erro...
chrome/common/net/net_error_info.cc:35: bool DnsProbesEnabledByFieldTrial() {
Since this isn't being used in renderer/, don't think it belongs in common/

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/chrome_con...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/chrome_con...
chrome/renderer/chrome_content_renderer_client.cc:872:
RenderThread::Get()->GetLocale());
Shouldn't call GetLocale() again when we have it in a local variable.

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/net/net_er...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/net/net_er...
chrome/renderer/net/net_error_helper.cc:194: last_status_ = status;
Don't think last_status_ serves any purpose - can just pass it along directly.

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/net/net_er...
chrome/renderer/net/net_error_helper.cc:199: WebKit::WebURLError
NetErrorHelper::MakeUpdatedError() {
Doesn't always make a WebKitError.  Suggest GetFinalError().

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/net/net_er...
chrome/renderer/net/net_error_helper.cc:199: WebKit::WebURLError
NetErrorHelper::MakeUpdatedError() {
This can be const.

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/net/net_er...
chrome/renderer/net/net_error_helper.cc:202: || last_status_ ==
chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN)
nit:  Put "||" on previous line.

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/net/net_er...
chrome/renderer/net/net_error_helper.cc:202: || last_status_ ==
chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN)
nit:  Use braces in if's where the condition uses multiple lines.

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/net/net_er...
File chrome/renderer/net/net_error_helper.h (right):

https://codereview.chromium.org/13270005/diff/7001/chrome/renderer/net/net_er...
chrome/renderer/net/net_error_helper.h:55: NetErrorTracker::DnsErrorPageState
state);
nit:  Does this fit on single line?

[Deprecated (see juliatuttle), 2013-04-10 23:42:31]

PTAL, mmenke.

It looks like the suggestions do work; the problem was that the case I was
testing (NXDOMAIN) doesn't *have* any.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/app/generate...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/app/generate...
chrome/app/generated_resources.grd:9458: +        can't be found, because the
DNS lookup failed.  Please hold on a moment while
On 2013/04/10 16:56:58, mmenke wrote:
> Maybe "DNS" -> "host name"?  Less technical for the average user.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/app/generate...
chrome/app/generated_resources.grd:9459: +        <ph
name="PRODUCT_NAME">&lt;span jscontent="productName"&gt;&lt;/span&gt;<ex>Google
Chrome</ex></ph>
On 2013/04/10 16:56:58, mmenke wrote:
> Substituting the product name in strings like this is now considered a bad
idea,
> since nouns change form depending on usage in some languages.  Instead, we
> either shouldn't use the product name, or have two copies of the string, with
> the name inlined - one each in chromium_resources.grd and
> google_chrome_resouces.grd (I may have the file names wrong - they're in the
> same directory as generated_resources.grd).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/app/generate...
chrome/app/generated_resources.grd:9535: +      </message>
On 2013/04/10 16:56:58, mmenke wrote:
> Something to think about:  May make sense just to use the
ERR_NAME_NOT_RESOLVED
> string here in all cases, to reduce flicker.

I'm okay with a tiny grey message flickering, and I want the info there if, say,
the prober breaks and never returns results, so users can report that it's stuck
in this state.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/browser/net/...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/browser/net/...
chrome/browser/net/dns_probe_service.cc:130: result_ =
chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN;
On 2013/04/10 16:56:58, mmenke wrote:
> Should this actually be DNS_PROBE_MAX?  Seems like it would provide some
> additional DCHECKS protecting against treating the value set here as a real
> result.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/browser/net/...
chrome/browser/net/dns_probe_service.cc:184: DCHECK_NE(result_, kMaxStatus);
On 2013/04/10 16:56:58, mmenke wrote:
> DCHECK_LT?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/browser/net/...
chrome/browser/net/dns_probe_service.cc:223: case
chrome_common_net::DNS_PROBE_MAX:
On 2013/04/10 16:56:58, mmenke wrote:
> Maybe just a "default:"?

I didn't use default: to preserve the warning if we add another DNS probe
result.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/browser/net/...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/browser/net/...
chrome/browser/net/net_error_tab_helper.cc:193: dns_probe_result_ = result;
On 2013/04/10 16:56:58, mmenke wrote:
> I don't think we gain anything from having |dns_probe_result_|.  This is now a
> sufficiently simple state machine that I think we're adding complexity with no
> real benefit.
> 
> My suggestion:  Get rid of it and MaybeSendStatus as well.
> 
> Put the logic for when to send the other two statuses in
> MaybePostStartDnsProbeTask.  Can also get rid of DNS_PROBE_FINISHED.

The state machine is still complicated.  We're simultaneously waiting for the
page to load and maybe waiting for a probe to finish.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/browser/net/...
chrome/browser/net/net_error_tab_helper.cc:217:
SendInfo(chrome_common_net::DNS_PROBE_NOT_RUN);
On 2013/04/10 16:56:58, mmenke wrote:
> Think sending this on all tracker events is a bit too aggressive.  Really only
> matters when we'd other send a message, except for the fact that
ProbesAllowed()
> is false.

We don't.  We only send it if the page has just loaded.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/common/local...
File chrome/common/localized_error.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/common/local...
chrome/common/localized_error.cc:365: SUGGEST_NONE,
On 2013/04/10 16:56:58, mmenke wrote:
> All the other results have "SUGGEST_RELOAD", and it's also handled above fold.

> Suggest using it here, too.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/common/local...
chrome/common/localized_error.cc:377: SUGGEST_RELOAD | SUGGEST_NONE,
On 2013/04/10 16:56:58, mmenke wrote:
> "| SUGGEST_NONE" doesn't make any sense.  :)

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/common/local...
chrome/common/localized_error.cc:377: SUGGEST_RELOAD | SUGGEST_NONE,
On 2013/04/10 16:56:58, mmenke wrote:
> Do we need a PROBE_STARTED page?  I don't think we want / need separate pages
> for PROBE_POSSIBLE and PROVE_STARTED.

It's there in case we want to show a spinner or different text.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/common/local...
chrome/common/localized_error.cc:381: // original error, which might be one of
several DNS-related errors.
On 2013/04/10 16:56:58, mmenke wrote:
> Erm....  DNS_PROBE_FINISHED_UNKNOWN is here.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/common/local...
chrome/common/localized_error.cc:557: error_string =
ASCIIToUTF16(ascii_error_string);
On 2013/04/10 16:56:58, mmenke wrote:
> Per other comment, we may just want to use the ERR_NAME_NOT_RESOLVED error
stuff
> here.

I'd rather have the detail, in case things break.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/common/net/n...
File chrome/common/net/net_error_info.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/common/net/n...
chrome/common/net/net_error_info.cc:35: bool DnsProbesEnabledByFieldTrial() {
On 2013/04/10 16:56:58, mmenke wrote:
> Since this isn't being used in renderer/, don't think it belongs in common/

It's now checked in NetErrorHelper also.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:872:
RenderThread::Get()->GetLocale());
On 2013/04/10 16:56:58, mmenke wrote:
> Shouldn't call GetLocale() again when we have it in a local variable.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/net...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/net...
chrome/renderer/net/net_error_helper.cc:194: last_status_ = status;
On 2013/04/10 16:56:58, mmenke wrote:
> Don't think last_status_ serves any purpose - can just pass it along directly.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/net...
chrome/renderer/net/net_error_helper.cc:199: WebKit::WebURLError
NetErrorHelper::MakeUpdatedError() {
On 2013/04/10 16:56:58, mmenke wrote:
> Doesn't always make a WebKitError.  Suggest GetFinalError().

GetUpdatedError(), since it's not necessarily the final error.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/net...
chrome/renderer/net/net_error_helper.cc:199: WebKit::WebURLError
NetErrorHelper::MakeUpdatedError() {
On 2013/04/10 16:56:58, mmenke wrote:
> This can be const.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/net...
chrome/renderer/net/net_error_helper.cc:202: || last_status_ ==
chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN)
On 2013/04/10 16:56:58, mmenke wrote:
> nit:  Put "||" on previous line.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/net...
chrome/renderer/net/net_error_helper.cc:202: || last_status_ ==
chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN)
On 2013/04/10 16:56:58, mmenke wrote:
> nit:  Use braces in if's where the condition uses multiple lines.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/net...
File chrome/renderer/net/net_error_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/7001/chrome/renderer/net...
chrome/renderer/net/net_error_helper.h:55: NetErrorTracker::DnsErrorPageState
state);
On 2013/04/10 16:56:58, mmenke wrote:
> nit:  Does this fit on single line?

Done.

[mmenke, 2013-04-11 13:48:04]

Sorry I didn't get around to taking a more thorough look yesterday.  I have a
pretty meeting-heavy day today, though I'll get back to you as soon as I can
manage.

[mmenke, 2013-04-12 14:27:12]

On 2013/04/11 13:48:04, mmenke wrote:
> Sorry I didn't get around to taking a more thorough look yesterday.  I have a
> pretty meeting-heavy day today, though I'll get back to you as soon as I can
> manage.

Been forgetting about this, but we really should have a bunch of browser tests
before finally hooking everything up.  The time required to get them right is a
concern for getting this into M28.

[Deprecated (see juliatuttle), 2013-05-22 21:41:44]

PTAL, mmenke.

[mmenke, 2013-05-23 16:19:55]

On 2013/05/22 21:41:44, ttuttle wrote:
> PTAL, mmenke.

Between another big review and investigating the whole ERR_ABORTED spike, may
not get to this until late tomorrow.

[mmenke, 2013-05-28 18:20:13]

Still reading through the code, but wanted to get you some comments today.  All
of these but the first are pretty minor.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:91: class MockDnsProbeService :
public DnsProbeService {
I'd rather use the real DnsProbeService.  Also think it's much cleaner if we
mock out the same class in both sets of tests.  My suggestion is to have the
JobFactory manage the probes - creates them and owns them, and passes both their
results back to the probe service in a single function.

In that case, JobManager may be a better name.

WDYT?

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:165: protected:
optional:  Most test fixtures don't use protected, since they aren't subclassed
and can't be accessed from other files.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:169: void WaitForProbe();
I think "WaitForProbeStart" would be clearer.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:182: TestCompletionCallback
probe_started_callback_;
I think having two variables in this file called probe_started_callback_, one of
which is a closure and the other a TestCompletionCallback is a bit confusion.

Suggest renaming this one and adding a comment.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:233: io_thread_done.WaitForCall();
Waiting on this does not seem to be needed.  Any task we later post to the IO
thread will necessarily happen after SetUpOnIOThread is run.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:236: void
DnsProbeBrowserTestIOThreadHelper::SetUpOnIOThread(
DnsProbeBrowserTestIOThreadHelper's functions should be defined right under the
definition of DnsProbeBrowserTestIOThreadHelper.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:277: io_thread_done.WaitForCall();
Rather than waiting on a callback, how about having
DnsProbeBrowserTestIOThreadHelper destroyed on the IO thread, in the function
called from CleanUpOnMainThread.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:346: bool
DnsProbeBrowserTest::TitleContains(const std::string& expected) {
Is there a reason not to use TitleWatcher?

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:363: std::string textContent;
text_content.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:366:
browser()->tab_strip_model()->GetActiveWebContents();
optional:  Suggest you just inline this, to reduce code length a bit (For
readability).

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_browsertest.cc:378: void
DnsProbeBrowserTest::SetLinkDoctorNetError(int net_error) {
Instead of this, can we just change the link doctor URL?  I seem to recall that
didn't work for some reason, but just double checking.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
File chrome/browser/net/dns_probe_service.h (right):

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_service.h:28: virtual ~JobFactory() { }
nit:  Google C++ style guide says not to use a space in empty function bodies.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_service.h:42: virtual ~DnsProbeService() { }
nit:  Google C++ style guide says not to use a space in empty function bodies.

https://codereview.chromium.org/13270005/diff/72001/chrome/browser/net/dns_pr...
chrome/browser/net/dns_probe_service.h:49: virtual void ExpireResultForTesting()
{ }
nit:  Google C++ style guide says not to use a space in empty function bodies.

https://codereview.chromium.org/13270005/diff/72001/chrome/renderer/net/net_e...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/13270005/diff/72001/chrome/renderer/net/net_e...
chrome/renderer/net/net_error_helper.cc:142: // Get the strings for a fake "DNS
probe possible" error
nit:  period.

https://codereview.chromium.org/13270005/diff/72001/chrome/renderer/net/net_e...
chrome/renderer/net/net_error_helper.cc:208: const {
nit:  const should go on previous line.

https://codereview.chromium.org/13270005/diff/72001/chrome/renderer/resources...
File chrome/renderer/resources/neterror.html (right):

https://codereview.chromium.org/13270005/diff/72001/chrome/renderer/resources...
chrome/renderer/resources/neterror.html:258: </style>
Don't have strong opinions about whether we should have a linebreak  above the
close tag or not, but seems like we should be consistent with the open tag.

https://codereview.chromium.org/13270005/diff/72001/chrome/renderer/resources...
chrome/renderer/resources/neterror.html:285: var details =
document.getElementById('details');
Why?  This seems very regression prone.  If it matters that we only populate
these two, then we should fail otherwise.  

Since the page will only be redrawn once the entire script has run, why is it
better to only to update two sections?

https://codereview.chromium.org/13270005/diff/72001/chrome/renderer/resources...
chrome/renderer/resources/neterror.html:296: </script>
Same comment as for style tag applies.

[Deprecated (see juliatuttle), 2013-06-05 21:01:46]

PTAL, mmenke.

(I'm working on NetError{,Tab}Helper unittests; any feedback you can provide in
parallel would be helpful for getting this in by Friday.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:91: class MockDnsProbeService :
public DnsProbeService {
On 2013/05/28 18:20:13, mmenke wrote:
> I'd rather use the real DnsProbeService.  Also think it's much cleaner if we
> mock out the same class in both sets of tests.  My suggestion is to have the
> JobFactory manage the probes - creates them and owns them, and passes both
their
> results back to the probe service in a single function.
> 
> In that case, JobManager may be a better name.
> 
> WDYT?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:165: protected:
On 2013/05/28 18:20:13, mmenke wrote:
> optional:  Most test fixtures don't use protected, since they aren't
subclassed
> and can't be accessed from other files.

The test cases are implemented as subclasses, I thought.  I use protected for
stuff that can be called from test cases and private for stuff that's only
accessed through the test fixture itself.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:169: void WaitForProbe();
On 2013/05/28 18:20:13, mmenke wrote:
> I think "WaitForProbeStart" would be clearer.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:182: TestCompletionCallback
probe_started_callback_;
On 2013/05/28 18:20:13, mmenke wrote:
> I think having two variables in this file called probe_started_callback_, one
of
> which is a closure and the other a TestCompletionCallback is a bit confusion.
> 
> Suggest renaming this one and adding a comment.

Obsolete.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:233: io_thread_done.WaitForCall();
On 2013/05/28 18:20:13, mmenke wrote:
> Waiting on this does not seem to be needed.  Any task we later post to the IO
> thread will necessarily happen after SetUpOnIOThread is run.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:236: void
DnsProbeBrowserTestIOThreadHelper::SetUpOnIOThread(
On 2013/05/28 18:20:13, mmenke wrote:
> DnsProbeBrowserTestIOThreadHelper's functions should be defined right under
the
> definition of DnsProbeBrowserTestIOThreadHelper.

My goal was to arrange them in the order of what actually happens, since methods
from the two are interleaved.  I can separate them if you want.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:277: io_thread_done.WaitForCall();
On 2013/05/28 18:20:13, mmenke wrote:
> Rather than waiting on a callback, how about having
> DnsProbeBrowserTestIOThreadHelper destroyed on the IO thread, in the function
> called from CleanUpOnMainThread.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:346: bool
DnsProbeBrowserTest::TitleContains(const std::string& expected) {
On 2013/05/28 18:20:13, mmenke wrote:
> Is there a reason not to use TitleWatcher?

I don't want to wait for it; I expect it to have this title right now.

(I could move to TitleWatcher if you want, though.)

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:363: std::string textContent;
On 2013/05/28 18:20:13, mmenke wrote:
> text_content.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:366:
browser()->tab_strip_model()->GetActiveWebContents();
On 2013/05/28 18:20:13, mmenke wrote:
> optional:  Suggest you just inline this, to reduce code length a bit (For
> readability).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_browsertest.cc:378: void
DnsProbeBrowserTest::SetLinkDoctorNetError(int net_error) {
On 2013/05/28 18:20:13, mmenke wrote:
> Instead of this, can we just change the link doctor URL?  I seem to recall
that
> didn't work for some reason, but just double checking.

We can't change it once the tab is created.  That's why I have to do the weird
thing where we either serve the actual mock link doctor page or a redirect to a
mock error.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
File chrome/browser/net/dns_probe_service.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_service.h:28: virtual ~JobFactory() { }
On 2013/05/28 18:20:13, mmenke wrote:
> nit:  Google C++ style guide says not to use a space in empty function bodies.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_service.h:42: virtual ~DnsProbeService() { }
On 2013/05/28 18:20:13, mmenke wrote:
> nit:  Google C++ style guide says not to use a space in empty function bodies.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/browser/net...
chrome/browser/net/dns_probe_service.h:49: virtual void ExpireResultForTesting()
{ }
On 2013/05/28 18:20:13, mmenke wrote:
> nit:  Google C++ style guide says not to use a space in empty function bodies.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/renderer/ne...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/renderer/ne...
chrome/renderer/net/net_error_helper.cc:142: // Get the strings for a fake "DNS
probe possible" error
On 2013/05/28 18:20:13, mmenke wrote:
> nit:  period.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/renderer/ne...
chrome/renderer/net/net_error_helper.cc:208: const {
On 2013/05/28 18:20:13, mmenke wrote:
> nit:  const should go on previous line.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/renderer/re...
File chrome/renderer/resources/neterror.html (right):

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/renderer/re...
chrome/renderer/resources/neterror.html:258: </style>
On 2013/05/28 18:20:13, mmenke wrote:
> Don't have strong opinions about whether we should have a linebreak  above the
> close tag or not, but seems like we should be consistent with the open tag.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/renderer/re...
chrome/renderer/resources/neterror.html:285: var details =
document.getElementById('details');
On 2013/05/28 18:20:13, mmenke wrote:
> Why?  This seems very regression prone.  If it matters that we only populate
> these two, then we should fail otherwise.  
> 
> Since the page will only be redrawn once the entire script has run, why is it
> better to only to update two sections?

We want to avoid re-rendering the stuff that's outside the "Details" section so
it doesn't flicker.  The hope is that if we just update those two, we're
updating things the user can't actually see, since they haven't clicked Details.

I'll remove the fallback case.

https://chromiumcodereview.appspot.com/13270005/diff/72001/chrome/renderer/re...
chrome/renderer/resources/neterror.html:296: </script>
On 2013/05/28 18:20:13, mmenke wrote:
> Same comment as for style tag applies.

Done.

[Deprecated (see juliatuttle), 2013-06-06 21:10:55]

Bringing in jhawkins to get OWNERS review started.

Please look at everything but chrome/browser/net and chrome/renderer/net (mmenke
can handle OWNERS approval for those).

I would, of course, not mind feedback if you see any obvious issues elsewhere.

[szym, 2013-06-07 19:31:27]

Only reviewed dns_probe_runner*
I suggest splitting it for public/system, passing DnsClient into DnsProbeRunner
(DnsProbeRunnerImpl) ctor, and adding a pure virtual interface 
if you need to mock its results in tests.

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.cc (right):

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:91: InitializeSystemClient();
Note that this does not affect any pending transactions, so the they might call
you back with obsolete result.

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:112: ProbeInfo(type, callback)));
Instead bind ProbeInfo as an argument to OnTransactionComplete above. It's
probably fine to pass it by copy.

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:233: if (!async) {
I'm not sure you need this. The net_error returned synchronously from
DnsTransaction is already covered in DidReceiveDnsResponse.

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.h (right):

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:22: class DnsProbeRunner : public
net::NetworkChangeNotifier::DNSObserver {
Add a short class-level comment, e.g., "Runs DnsTransactions against the
system-configured and known public DNS server. Used to diagnose connectivity
issues." or something.

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:45: void RunProbe(Type type, const
ProbeCallback& callback);
Why does it need |type|? Should it run both? If it doesn't run both, and
processes system and public results separately, then you should make one
DnsProbeRunner for system and one for public.

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:48: void SetResultsForTesting(Result
system_result, Result public_result);
Could you avoid doing this by creating:

class DnsProbeRunner {
 public:
  virtual void RunProbe(Type, ProbeCallback) = 0;
};

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:67: typedef
std::map<net::DnsTransaction*,ProbeInfo> TransactionMap;
This is strange. I think you could accomplish the same thing by using callback
currying.

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:77: Result EvaluateResponse(bool async,
Needs comment.

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.h (right):

https://codereview.chromium.org/13270005/diff/123001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.h:35: virtual void OnDNSChanged() OVERRIDE;
You have it both here and in DnsProbeRunner.

In DnsProbeRunner::OnDNSChanged you just reset the "system" DnsClient. I'd
suggest replacing it with DnsProbeRunner::SetSystemClient and
DnsProbeRunner::SetPublicClient, and moving the client initialization entirely
to DnsProbeService.

In fact, I think DnsProbeRunner could just accept scoped_ptr<DnsClient>. This
way, when DnsClient needs to be changed, DnsProbeRunner will die together with
any DnsTransactions it owns.

https://codereview.chromium.org/13270005/diff/123001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.h (right):

https://codereview.chromium.org/13270005/diff/123001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.h:23: class NetErrorHelper : public
content::RenderViewObserver {
I think a short class-level or file-level comment would help.

[szym, 2013-06-07 19:40:27]

On second thought, don't create pure-virtual DnsProbeRunner. I think
DnsProbeRunner::SetClient(scoped_ptr<DnsClient>) and
DnsProbeService::SetSystemClient(scoped_ptr<DnsClient>) will suffice. DnsClient
is already mockable, and we already have MockDnsClient.

[cbentzel, 2013-06-10 17:10:00]

Tiny driveby nits.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:20: }
Should this be in chrome_browser_net namespace?

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.h (left):

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.h:21: namespace chrome_browser_net {
Why did you remove the namespace?

[cbentzel, 2013-06-10 17:13:09]

On 2013/06/10 17:10:00, cbentzel wrote:
> Tiny driveby nits.
> 
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> File chrome/browser/net/dns_probe_runner.h (right):
> 
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> chrome/browser/net/dns_probe_runner.h:20: }
> Should this be in chrome_browser_net namespace?
> 
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> File chrome/browser/net/dns_probe_service.h (left):
> 
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> chrome/browser/net/dns_probe_service.h:21: namespace chrome_browser_net {
> Why did you remove the namespace?

Also high-level question: Is there a single boolean/flag which controls the
behavior (so it can be disabled easily)?

[cbentzel, 2013-06-10 17:13:11]

On 2013/06/10 17:10:00, cbentzel wrote:
> Tiny driveby nits.
> 
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> File chrome/browser/net/dns_probe_runner.h (right):
> 
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> chrome/browser/net/dns_probe_runner.h:20: }
> Should this be in chrome_browser_net namespace?
> 
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> File chrome/browser/net/dns_probe_service.h (left):
> 
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> chrome/browser/net/dns_probe_service.h:21: namespace chrome_browser_net {
> Why did you remove the namespace?

Also high-level question: Is there a single boolean/flag which controls the
behavior (so it can be disabled easily)?

[Deprecated (see juliatuttle), 2013-06-11 01:07:14]

On 2013/06/10 17:13:11, cbentzel wrote:
> On 2013/06/10 17:10:00, cbentzel wrote:
> > Tiny driveby nits.
> > 
> >
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> > File chrome/browser/net/dns_probe_runner.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> > chrome/browser/net/dns_probe_runner.h:20: }
> > Should this be in chrome_browser_net namespace?
> > 
> >
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> > File chrome/browser/net/dns_probe_service.h (left):
> > 
> >
>
https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
> > chrome/browser/net/dns_probe_service.h:21: namespace chrome_browser_net {
> > Why did you remove the namespace?
> 
> Also high-level question: Is there a single boolean/flag which controls the
> behavior (so it can be disabled easily)?

There is the DnsProbe-Enable field trial.

[Deprecated (see juliatuttle), 2013-06-11 01:07:33]

PTAL mmenke (and whoever else is interested).

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:91: InitializeSystemClient();
On 2013/06/07 19:31:28, szym wrote:
> Note that this does not affect any pending transactions, so the they might
call
> you back with obsolete result.

That's okay; error pages can be obsolete for other reasons, so I'm not
super-worried if the DNS config changes *while* we're probing it.  The results
will hopefully give an idea of the original issue that caused the error.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:112: ProbeInfo(type, callback)));
On 2013/06/07 19:31:28, szym wrote:
> Instead bind ProbeInfo as an argument to OnTransactionComplete above. It's
> probably fine to pass it by copy.

Obsolete.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:233: if (!async) {
On 2013/06/07 19:31:28, szym wrote:
> I'm not sure you need this. The net_error returned synchronously from
> DnsTransaction is already covered in DidReceiveDnsResponse.

I don't *need* it, but I want to be explicit about it in case we end up with
other errors in the future.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:20: }
On 2013/06/10 17:10:00, cbentzel wrote:
> Should this be in chrome_browser_net namespace?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:45: void RunProbe(Type type, const
ProbeCallback& callback);
On 2013/06/07 19:31:28, szym wrote:
> Why does it need |type|? Should it run both? If it doesn't run both, and
> processes system and public results separately, then you should make one
> DnsProbeRunner for system and one for public. 

Done.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:48: void SetResultsForTesting(Result
system_result, Result public_result);
On 2013/06/07 19:31:28, szym wrote:
> Could you avoid doing this by creating:
> 
> class DnsProbeRunner {
>  public:
>   virtual void RunProbe(Type, ProbeCallback) = 0;
> };

Obsolete.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:67: typedef
std::map<net::DnsTransaction*,ProbeInfo> TransactionMap;
On 2013/06/07 19:31:28, szym wrote:
> This is strange. I think you could accomplish the same thing by using callback
> currying.

I could, but I needed the DnsTransactions anyway to delete them when the
DnsProbeRunner was deleted.

Obsolete, though.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:77: Result EvaluateResponse(bool async,
On 2013/06/07 19:31:28, szym wrote:
> Needs comment.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.h (left):

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.h:21: namespace chrome_browser_net {
On 2013/06/10 17:10:00, cbentzel wrote:
> Why did you remove the namespace?

Obsolete.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.h:35: virtual void OnDNSChanged() OVERRIDE;
On 2013/06/07 19:31:28, szym wrote:
> You have it both here and in DnsProbeRunner.
> 
> In DnsProbeRunner::OnDNSChanged you just reset the "system" DnsClient. I'd
> suggest replacing it with DnsProbeRunner::SetSystemClient and
> DnsProbeRunner::SetPublicClient, and moving the client initialization entirely
> to DnsProbeService.
> 
> In fact, I think DnsProbeRunner could just accept scoped_ptr<DnsClient>. This
> way, when DnsClient needs to be changed, DnsProbeRunner will die together with
> any DnsTransactions it owns.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/123001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:23: class NetErrorHelper : public
content::RenderViewObserver {
On 2013/06/07 19:31:28, szym wrote:
> I think a short class-level or file-level comment would help.

Done.

[mmenke, 2013-06-11 16:15:35]

A bunch of minor comments.  Still working my way through the code.  I'm going to
continue reviewing after lunch, so no need to respond / upload a new CL until
I'm done.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:8: #include "base/memory/scoped_ptr.h"
This is included in header (As is bind, though I gave another suggestion there).

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:47: // Returns true if the given
net_error indicates that we received a response
nit:  |net_error| (References to arguments should be in ||'s, per Google style
guide).  Same for the next line.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:54: case
net::ERR_DNS_SERVER_REQUIRES_TCP:
Think it's worth noting that this shouldn't happen, since the DnsTransaction
supports TCP fallback.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:57: return true;
completely optional nit:  Just to make the behavior match the name, may want to
make OK return true, too (And update comment).  Or can just rename the function
name to "DidReceiveDnsError" or DidReceiveDnsFailingResponse" or somesuch.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:69:
bound_net_log_(BoundNetLog::Make(NULL, NetLog::SOURCE_DNS_PROBER)) {
Per other comment, not needed.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:81: "google.com",
Suggest putting this up top, with the public DNS IPs, just to make it more
obvious.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:86: bound_net_log_);
BoundNetLog()

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:116: callback_ = ProbeCallback();
To avoid the possibility of any re-entrancy issues, I suggest clearing callback_
and storing it in the local before calling it.  This admittedly isn't fool proof
- could still get infinite recursion and run out of stack space with a bunch of
sync failures in a row - but still feel better with at least some protection in
place.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:124: if (!async) {
Do we need this case / parameter?  Seems like we'd just be getting a net_error
in this case, anyways, and the next code block would handle it.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.h (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:7: #define
CHROME_BROWSER_NET_DNS_PROBE_RUNNER_H
nit:  Blank line not needed between these two lines.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:9: #include <list>
Not needed.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:25: // Runs DNS probes using a single
DnsClient and evaluates the responses.
nit:  responses -> response

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:25: // Runs DNS probes using a single
DnsClient and evaluates the responses.
Think it's worth mentioning that we use google.com for testing.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:32: INCORRECT,
Think it's worth commenting that any response that includes an IP address is
considered correct.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:33: FAILING,
Also think it's worth mentioning this means we received a response, but it was a
malformed response.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:45: bool is_running() const;
If you use this naming scheme, the function should be inlined - so should either
change naming scheme, or inline the functions.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:53: // Evaluates the result of a
DnsTransaction (whether the response was
nit:  Think this comment could be clearer.  Suggest:

"Evaluates the result of a DnsTransaction, based on whether the response..."

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:61: const net::DnsResponse* response);
Seems like this doesn't need to be declared here - can just go in a private
namespace in the cc file.  Then you can also get rid of a forward declaration.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:64: net::BoundNetLog bound_net_log_;
No need for this, or the include in this file.  You can just use BoundNetLog()
whereever you need the BoundNetLog.  Passing in a NULL NetLog, like you're doing
in the other file, effectively makes the BoundNetLog do nothing.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:67: scoped_ptr<net::DnsTransaction>
transaction_;
DISALLOW_COPY_AND_ASSIGN (And also the correspoding include).

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:31:
base::WeakPtrFactory<TestDnsProbeRunnerCallback> weak_factory_;
I don't think we need this.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:69: MessageLoopForIO
message_loop_;
There was a recent post to Chromium-dev announcing TestBrowserThreadBundle, and
it recommended using it instead of creating individual message loops, even when
only one is needed.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:80: run_loop.RunUntilIdle();
Think it's more common to just use:

RunLoop().RunUntilIdle();

And inline it.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:88:
SetRule(MockDnsClientRule::OK);
I suggest turning this into a function, along the lines of:

RunDnsClientTest(MockDnsClientRule::OK, <async>, DnsProbeRunner::CORRECT);

And then call it once with each of the MockDnsClientRules, and once having it
fail synchronously.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:92: TestDnsProbeRunnerCallback
cb;
nit:  Google style discourages abbreviations, suggest just callback.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:10: #include
"chrome/browser/net/dns_probe_runner.h"
nit:  Already used in header.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:11: #include
"chrome/common/net/net_error_info.h"
nit:  Already used in header.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:14: #include
"net/base/network_change_notifier.h"
nit:  Already used in header.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:62: }
This function is not currently being used, nor is this being used elsewhere. 
Did we ever decide what to do here?

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:122: system_config.randomize_ports =
false;
Think these two behaviors are worth mentioning in the header somewhere.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:133:
public_config.nameservers.push_back(MakeDnsEndPoint(kGooglePublicDns1));
kGooglePublicDns1 -> kGooglePublicDns2

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:152: DCHECK_NE(STATE_PROBE_RUNNING,
state_);
Think you can DCHECK_EQ(STATE_NO_RESULTS, state_); here.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:160:
public_runner_.RunProbe(base::Bind(callback, PUBLIC));
Suggest a comment that it's theoretically possible for the probe to have
finished by the time we're here.  Easy to overlook the (rare) sync case.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:169: case SYSTEM:
DCHECK(system_runner_.is_running())?

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:172: case PUBLIC:
DCHECK(public_runner_.is_running());

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.h (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.h:65: chrome_common_net::DnsProbeStatus
EvaluateResults() const;
Think a couple of these may be woth comments.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.h:75: chrome_common_net::DnsProbeStatus
result_;
cached_result_, maybe?

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.h:80: DnsProbeRunner::Result
public_result_;
Think these are worth comments.

[mmenke, 2013-06-12 19:17:12]

More mostly nit-level comments - Still reviewing the renderer-side stuff, and
haven't yet touched the browser code, but the design looks reasonable to me, so
far.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:88:
SetRule(MockDnsClientRule::OK);
On 2013/06/11 16:15:35, mmenke wrote:
> I suggest turning this into a function, along the lines of:
> 
> RunDnsClientTest(MockDnsClientRule::OK, <async>, DnsProbeRunner::CORRECT);
> 
> And then call it once with each of the MockDnsClientRules, and once having it
> fail synchronously.

Also, seems like one test should runs two probes with the same runner.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:38: const char kGooglePublicDns1[]   =
"8.8.8.8";
nit:  Remove extra spaces.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:231: if
(NetworkChangeNotifier::IsOffline()) {
Random comment that can be ignored for now:  Long term, we may want to remove
this - it's been known to cause jank on windows.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:251:
UMA_HISTOGRAM_MEDIUM_TIMES("DnsProbe.Elapsed_BadConfig",
These should all pretty much represent the same thing as the old histograms,
right?  Other than the fact we're removing that field trial.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.h (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.h:70: bool ResultsExpired();
nit:  const.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:41: run_loop.RunUntilIdle();
Per earlier comment, suggest inlining this everywhere, and merging into one
line.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:73: MessageLoopForIO
message_loop_;
Other comment about how to handle message loops applies here as well.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:88: // TODO(ttuttle): More test
cases?
Suggestions:  Sync failure, all other different results, and maybe a case with
two callbacks called together?  Could also have a callback that triggers another
probe, but don't think that's too important.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:96: // Cached NXDOMAIN result
should persist.
Shouldn't you change the result, so if the result is not cached, we get a new
result?

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:101: RunUntilIdle();
Not needed, is it?

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:117: Probe();
Should change the result for the second probe.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_test_util.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_test_util.cc:7: #include "base/memory/scoped_ptr.h"
Not needed, appears in header.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_test_util.cc:8: #include "net/dns/dns_client.h"
Not needed, if this appears in header (Of course, if it's removed from the
header...).

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_test_util.cc:11: #include "net/dns/dns_test_util.h"
Not needed, appears in header.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_test_util.h (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_test_util.h:9: #include "net/dns/dns_client.h"
I don't believe this is needed.  If it is, should get rid of the forward
declaration.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.h (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.h:70: // (Resent whenever an error page
commits, in case it is in a new renderer.)
I think this comment is hard to follow.  What you mean is if there is a new
renderer, sending it before commit would be too early.  What it sounds like to
me is if it's and old renderer, we don't even need to send anything.

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:74: );
This should go on the previous line (x3).

https://codereview.chromium.org/13270005/diff/140001/chrome/common/localized_...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/common/localized_...
chrome/common/localized_error.cc:373: const LocalizedErrorMap
dns_probe_error_options[] = {
Hmm...Naming scheme used for these is wrong, for consts.  Not a big change, but
this CL is already kinda huge without fixing that.

https://codereview.chromium.org/13270005/diff/140001/chrome/common/localized_...
chrome/common/localized_error.cc:447: error_code);
We should DCHECK that this succeeds.

https://codereview.chromium.org/13270005/diff/140001/chrome/common/net/net_er...
File chrome/common/net/net_error_info.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/common/net/net_er...
chrome/common/net/net_error_info.cc:37: && status < DNS_PROBE_MAX;
nit:  Put "&&" on the previous line.

https://codereview.chromium.org/13270005/diff/140001/chrome/common/net/net_er...
chrome/common/net/net_error_info.cc:42: static const char
kDnsProbeFieldTrialEnableGroupName[] = "enable";
I believe it was mentioned on the mailing list that this creates bad code on
some platforms, as it does the whole initialize-static-on-first-call thing. 
"const char" is the way to go (Or const char?  Can't remember if one was
preferred over the other).

https://codereview.chromium.org/13270005/diff/140001/chrome/common/net/net_er...
chrome/common/net/net_error_info.cc:45: == kDnsProbeFieldTrialEnableGroupName;
nit:  Put "==" on previous line, if it fits.

https://codereview.chromium.org/13270005/diff/140001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:25: #include
"third_party/WebKit/public/platform/WebURLError.h"
Again, should remove duplicates also found in the header file.

https://codereview.chromium.org/13270005/diff/140001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.h (right):

https://codereview.chromium.org/13270005/diff/140001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.h:12: #include "googleurl/src/gurl.h"
Don't think this is needed.

https://codereview.chromium.org/13270005/diff/140001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.h:53: // us to unit-test the state tracking
without having to mock out WebFrames
nit:  Many reviewers don't like we/us in comments.  Life is easiest if we just
appease them.  Suggest "this allows unit testings without having to...."

https://codereview.chromium.org/13270005/diff/140001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.h:69: // True if the last provisional load
start the helper saw was an error page.
nit:  Suggest "True if the last started load was an error page."  (I think using
"start" as a noun makes the comment a little harder to follow).

[Deprecated (see juliatuttle), 2013-06-13 14:37:03]

Fixed everything suggested so far.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:8: #include "base/memory/scoped_ptr.h"
On 2013/06/11 16:15:35, mmenke wrote:
> This is included in header (As is bind, though I gave another suggestion
there).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:47: // Returns true if the given
net_error indicates that we received a response
On 2013/06/11 16:15:35, mmenke wrote:
> nit:  |net_error| (References to arguments should be in ||'s, per Google style
> guide).  Same for the next line.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:54: case
net::ERR_DNS_SERVER_REQUIRES_TCP:
On 2013/06/11 16:15:35, mmenke wrote:
> Think it's worth noting that this shouldn't happen, since the DnsTransaction
> supports TCP fallback.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:57: return true;
On 2013/06/11 16:15:35, mmenke wrote:
> completely optional nit:  Just to make the behavior match the name, may want
to
> make OK return true, too (And update comment).  Or can just rename the
function
> name to "DidReceiveDnsError" or DidReceiveDnsFailingResponse" or somesuch.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:69:
bound_net_log_(BoundNetLog::Make(NULL, NetLog::SOURCE_DNS_PROBER)) {
On 2013/06/11 16:15:35, mmenke wrote:
> Per other comment, not needed.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:81: "google.com",
On 2013/06/11 16:15:35, mmenke wrote:
> Suggest putting this up top, with the public DNS IPs, just to make it more
> obvious.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:86: bound_net_log_);
On 2013/06/11 16:15:35, mmenke wrote:
> BoundNetLog()

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:116: callback_ = ProbeCallback();
On 2013/06/11 16:15:35, mmenke wrote:
> To avoid the possibility of any re-entrancy issues, I suggest clearing
callback_
> and storing it in the local before calling it.  This admittedly isn't fool
proof
> - could still get infinite recursion and run out of stack space with a bunch
of
> sync failures in a row - but still feel better with at least some protection
in
> place.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:124: if (!async) {
On 2013/06/11 16:15:35, mmenke wrote:
> Do we need this case / parameter?  Seems like we'd just be getting a net_error
> in this case, anyways, and the next code block would handle it.

I like having it explicit, but I'll drop it if you want.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:7: #define
CHROME_BROWSER_NET_DNS_PROBE_RUNNER_H
On 2013/06/11 16:15:35, mmenke wrote:
> nit:  Blank line not needed between these two lines.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:9: #include <list>
On 2013/06/11 16:15:35, mmenke wrote:
> Not needed.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:25: // Runs DNS probes using a single
DnsClient and evaluates the responses.
On 2013/06/11 16:15:35, mmenke wrote:
> Think it's worth mentioning that we use http://google.com for testing.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:25: // Runs DNS probes using a single
DnsClient and evaluates the responses.
On 2013/06/11 16:15:35, mmenke wrote:
> nit:  responses -> response

Disagree; a DnsProbeRunner can run multiple probes over the course of its
lifetime, and it evaluates all of those responses.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:32: INCORRECT,
On 2013/06/11 16:15:35, mmenke wrote:
> Think it's worth commenting that any response that includes an IP address is
> considered correct.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:33: FAILING,
On 2013/06/11 16:15:35, mmenke wrote:
> Also think it's worth mentioning this means we received a response, but it was
a
> malformed response.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:45: bool is_running() const;
On 2013/06/11 16:15:35, mmenke wrote:
> If you use this naming scheme, the function should be inlined - so should
either
> change naming scheme, or inline the functions.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:53: // Evaluates the result of a
DnsTransaction (whether the response was
On 2013/06/11 16:15:35, mmenke wrote:
> nit:  Think this comment could be clearer.  Suggest:
> 
> "Evaluates the result of a DnsTransaction, based on whether the response..."

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:61: const net::DnsResponse* response);
On 2013/06/11 16:15:35, mmenke wrote:
> Seems like this doesn't need to be declared here - can just go in a private
> namespace in the cc file.  Then you can also get rid of a forward declaration.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:64: net::BoundNetLog bound_net_log_;
On 2013/06/11 16:15:35, mmenke wrote:
> No need for this, or the include in this file.  You can just use BoundNetLog()
> whereever you need the BoundNetLog.  Passing in a NULL NetLog, like you're
doing
> in the other file, effectively makes the BoundNetLog do nothing.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:67: scoped_ptr<net::DnsTransaction>
transaction_;
On 2013/06/11 16:15:35, mmenke wrote:
> DISALLOW_COPY_AND_ASSIGN (And also the correspoding include).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:31:
base::WeakPtrFactory<TestDnsProbeRunnerCallback> weak_factory_;
On 2013/06/11 16:15:35, mmenke wrote:
> I don't think we need this.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:69: MessageLoopForIO
message_loop_;
On 2013/06/11 16:15:35, mmenke wrote:
> There was a recent post to Chromium-dev announcing TestBrowserThreadBundle,
and
> it recommended using it instead of creating individual message loops, even
when
> only one is needed.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:80: run_loop.RunUntilIdle();
On 2013/06/11 16:15:35, mmenke wrote:
> Think it's more common to just use:
> 
> RunLoop().RunUntilIdle();
> 
> And inline it.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:88:
SetRule(MockDnsClientRule::OK);
On 2013/06/11 16:15:35, mmenke wrote:
> I suggest turning this into a function, along the lines of:
> 
> RunDnsClientTest(MockDnsClientRule::OK, <async>, DnsProbeRunner::CORRECT);
> 
> And then call it once with each of the MockDnsClientRules, and once having it
> fail synchronously.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:88:
SetRule(MockDnsClientRule::OK);
On 2013/06/12 19:17:12, mmenke wrote:
> On 2013/06/11 16:15:35, mmenke wrote:
> > I suggest turning this into a function, along the lines of:
> > 
> > RunDnsClientTest(MockDnsClientRule::OK, <async>, DnsProbeRunner::CORRECT);
> > 
> > And then call it once with each of the MockDnsClientRules, and once having
it
> > fail synchronously.
> 
> Also, seems like one test should runs two probes with the same runner.

The new one runs them all with the same runner, in a row.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:92: TestDnsProbeRunnerCallback
cb;
On 2013/06/11 16:15:35, mmenke wrote:
> nit:  Google style discourages abbreviations, suggest just callback.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:10: #include
"chrome/browser/net/dns_probe_runner.h"
On 2013/06/11 16:15:35, mmenke wrote:
> nit:  Already used in header.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:11: #include
"chrome/common/net/net_error_info.h"
On 2013/06/11 16:15:35, mmenke wrote:
> nit:  Already used in header.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:14: #include
"net/base/network_change_notifier.h"
On 2013/06/11 16:15:35, mmenke wrote:
> nit:  Already used in header.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:38: const char kGooglePublicDns1[]   =
"8.8.8.8";
On 2013/06/12 19:17:12, mmenke wrote:
> nit:  Remove extra spaces.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:62: }
On 2013/06/11 16:15:35, mmenke wrote:
> This function is not currently being used, nor is this being used elsewhere. 
> Did we ever decide what to do here?

Nah, we didn't decide.  I'll remove it, and we can take a look at the data
lazily and alter the default (1) if it turns out it was bad.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:122: system_config.randomize_ports =
false;
On 2013/06/11 16:15:35, mmenke wrote:
> Think these two behaviors are worth mentioning in the header somewhere.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:133:
public_config.nameservers.push_back(MakeDnsEndPoint(kGooglePublicDns1));
On 2013/06/11 16:15:35, mmenke wrote:
> kGooglePublicDns1 -> kGooglePublicDns2

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:152: DCHECK_NE(STATE_PROBE_RUNNING,
state_);
On 2013/06/11 16:15:35, mmenke wrote:
> Think you can DCHECK_EQ(STATE_NO_RESULTS, state_); here.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:160:
public_runner_.RunProbe(base::Bind(callback, PUBLIC));
On 2013/06/11 16:15:35, mmenke wrote:
> Suggest a comment that it's theoretically possible for the probe to have
> finished by the time we're here.  Easy to overlook the (rare) sync case.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:169: case SYSTEM:
On 2013/06/11 16:15:35, mmenke wrote:
> DCHECK(system_runner_.is_running())?

No; that returns false during the callback.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:172: case PUBLIC:
On 2013/06/11 16:15:35, mmenke wrote:
> DCHECK(public_runner_.is_running());

Ditto.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:251:
UMA_HISTOGRAM_MEDIUM_TIMES("DnsProbe.Elapsed_BadConfig",
On 2013/06/12 19:17:12, mmenke wrote:
> These should all pretty much represent the same thing as the old histograms,
> right?  Other than the fact we're removing that field trial.

Right.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.h:65: chrome_common_net::DnsProbeStatus
EvaluateResults() const;
On 2013/06/11 16:15:35, mmenke wrote:
> Think a couple of these may be woth comments.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.h:70: bool ResultsExpired();
On 2013/06/12 19:17:12, mmenke wrote:
> nit:  const.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.h:75: chrome_common_net::DnsProbeStatus
result_;
On 2013/06/11 16:15:35, mmenke wrote:
> cached_result_, maybe?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.h:80: DnsProbeRunner::Result
public_result_;
On 2013/06/11 16:15:35, mmenke wrote:
> Think these are worth comments.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:41: run_loop.RunUntilIdle();
On 2013/06/12 19:17:12, mmenke wrote:
> Per earlier comment, suggest inlining this everywhere, and merging into one
> line.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:73: MessageLoopForIO
message_loop_;
On 2013/06/12 19:17:12, mmenke wrote:
> Other comment about how to handle message loops applies here as well.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:88: // TODO(ttuttle): More test
cases?
On 2013/06/12 19:17:12, mmenke wrote:
> Suggestions:  Sync failure, all other different results, and maybe a case with
> two callbacks called together?  Could also have a callback that triggers
another
> probe, but don't think that's too important.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:96: // Cached NXDOMAIN result
should persist.
On 2013/06/12 19:17:12, mmenke wrote:
> Shouldn't you change the result, so if the result is not cached, we get a new
> result?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:101: RunUntilIdle();
On 2013/06/12 19:17:12, mmenke wrote:
> Not needed, is it?

It is now.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:117: Probe();
On 2013/06/12 19:17:12, mmenke wrote:
> Should change the result for the second probe.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_test_util.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_test_util.cc:7: #include "base/memory/scoped_ptr.h"
On 2013/06/12 19:17:12, mmenke wrote:
> Not needed, appears in header.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_test_util.cc:8: #include "net/dns/dns_client.h"
On 2013/06/12 19:17:12, mmenke wrote:
> Not needed, if this appears in header (Of course, if it's removed from the
> header...).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_test_util.cc:11: #include "net/dns/dns_test_util.h"
On 2013/06/12 19:17:12, mmenke wrote:
> Not needed, appears in header.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_test_util.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_test_util.h:9: #include "net/dns/dns_client.h"
On 2013/06/12 19:17:12, mmenke wrote:
> I don't believe this is needed.  If it is, should get rid of the forward
> declaration.

It is required; scoped_ptr needs to know how to delete DnsClient, which it can't
find out from a forward declaration.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.h:70: // (Resent whenever an error page
commits, in case it is in a new renderer.)
On 2013/06/12 19:17:12, mmenke wrote:
> I think this comment is hard to follow.  What you mean is if there is a new
> renderer, sending it before commit would be too early.  What it sounds like to
> me is if it's and old renderer, we don't even need to send anything.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:74: );
On 2013/06/12 19:17:12, mmenke wrote:
> This should go on the previous line (x3).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/loc...
File chrome/common/localized_error.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/loc...
chrome/common/localized_error.cc:373: const LocalizedErrorMap
dns_probe_error_options[] = {
On 2013/06/12 19:17:12, mmenke wrote:
> Hmm...Naming scheme used for these is wrong, for consts.  Not a big change,
but
> this CL is already kinda huge without fixing that.

It is, but so is the one used for the other const arrays.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/loc...
chrome/common/localized_error.cc:447: error_code);
On 2013/06/12 19:17:12, mmenke wrote:
> We should DCHECK that this succeeds.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/net...
File chrome/common/net/net_error_info.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/net...
chrome/common/net/net_error_info.cc:37: && status < DNS_PROBE_MAX;
On 2013/06/12 19:17:12, mmenke wrote:
> nit:  Put "&&" on the previous line.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/net...
chrome/common/net/net_error_info.cc:42: static const char
kDnsProbeFieldTrialEnableGroupName[] = "enable";
On 2013/06/12 19:17:12, mmenke wrote:
> I believe it was mentioned on the mailing list that this creates bad code on
> some platforms, as it does the whole initialize-static-on-first-call thing. 
> "const char" is the way to go (Or const char?  Can't remember if one was
> preferred over the other).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/net...
chrome/common/net/net_error_info.cc:45: == kDnsProbeFieldTrialEnableGroupName;
On 2013/06/12 19:17:12, mmenke wrote:
> nit:  Put "==" on previous line, if it fits.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:25: #include
"third_party/WebKit/public/platform/WebURLError.h"
On 2013/06/12 19:17:12, mmenke wrote:
> Again, should remove duplicates also found in the header file.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:12: #include "googleurl/src/gurl.h"
On 2013/06/12 19:17:12, mmenke wrote:
> Don't think this is needed.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:53: // us to unit-test the state tracking
without having to mock out WebFrames
On 2013/06/12 19:17:12, mmenke wrote:
> nit:  Many reviewers don't like we/us in comments.  Life is easiest if we just
> appease them.  Suggest "this allows unit testings without having to...."

Done.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:69: // True if the last provisional load
start the helper saw was an error page.
On 2013/06/12 19:17:12, mmenke wrote:
> nit:  Suggest "True if the last started load was an error page."  (I think
using
> "start" as a noun makes the comment a little harder to follow).

Done.

[mmenke, 2013-06-13 15:00:11]

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:124: if (!async) {
On 2013/06/13 14:37:04, ttuttle wrote:
> On 2013/06/11 16:15:35, mmenke wrote:
> > Do we need this case / parameter?  Seems like we'd just be getting a
net_error
> > in this case, anyways, and the next code block would handle it.
> 
> I like having it explicit, but I'll drop it if you want.

I believe it's technically possible to get a sync success...  Just extremely
unlikely.

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:169: case SYSTEM:
On 2013/06/13 14:37:04, ttuttle wrote:
> On 2013/06/11 16:15:35, mmenke wrote:
> > DCHECK(system_runner_.is_running())?
> 
> No; that returns false during the callback.

DCHECK(!system_runner_.is_running())?  :)

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:172: case PUBLIC:
On 2013/06/13 14:37:04, ttuttle wrote:
> On 2013/06/11 16:15:35, mmenke wrote:
> > DCHECK(public_runner_.is_running());
> 
> Ditto.

DCHECK(!public_runner_.is_running())?  :)

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/loc...
File chrome/common/localized_error.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/common/loc...
chrome/common/localized_error.cc:373: const LocalizedErrorMap
dns_probe_error_options[] = {
On 2013/06/13 14:37:04, ttuttle wrote:
> On 2013/06/12 19:17:12, mmenke wrote:
> > Hmm...Naming scheme used for these is wrong, for consts.  Not a big change,
> but
> > this CL is already kinda huge without fixing that.
> 
> It is, but so is the one used for the other const arrays.

Sorry, my comment about fixing that meant fixing it for the others, too (Which I
would suggest in this CL, if it weren't so big already - best to keep to what
matters).

[szym, 2013-06-13 15:10:18]

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:124: if (!async) {
On 2013/06/13 15:00:12, mmenke wrote:
> On 2013/06/13 14:37:04, ttuttle wrote:
> > On 2013/06/11 16:15:35, mmenke wrote:
> > > Do we need this case / parameter?  Seems like we'd just be getting a
> net_error
> > > in this case, anyways, and the next code block would handle it.
> > 
> > I like having it explicit, but I'll drop it if you want.
> 
> I believe it's technically possible to get a sync success...  Just extremely
> unlikely.

Actually, DnsTransaction can never complete synchronously:
https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/dns_transa...

[mmenke, 2013-06-13 15:42:11]

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:124: if (!async) {
On 2013/06/13 15:10:18, szym wrote:
> On 2013/06/13 15:00:12, mmenke wrote:
> > On 2013/06/13 14:37:04, ttuttle wrote:
> > > On 2013/06/11 16:15:35, mmenke wrote:
> > > > Do we need this case / parameter?  Seems like we'd just be getting a
> > net_error
> > > > in this case, anyways, and the next code block would handle it.
> > > 
> > > I like having it explicit, but I'll drop it if you want.
> > 
> > I believe it's technically possible to get a sync success...  Just extremely
> > unlikely.
> 
> Actually, DnsTransaction can never complete synchronously:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/dns_transa...

Thanks!  Hmm...That was added to fix a crasher when it actually did succeed
synchronously, wasn't it?

[szym, 2013-06-13 15:44:44]

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/140001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:124: if (!async) {
On 2013/06/13 15:42:12, mmenke wrote:
> On 2013/06/13 15:10:18, szym wrote:
> > On 2013/06/13 15:00:12, mmenke wrote:
> > > On 2013/06/13 14:37:04, ttuttle wrote:
> > > > On 2013/06/11 16:15:35, mmenke wrote:
> > > > > Do we need this case / parameter?  Seems like we'd just be getting a
> > > net_error
> > > > > in this case, anyways, and the next code block would handle it.
> > > > 
> > > > I like having it explicit, but I'll drop it if you want.
> > > 
> > > I believe it's technically possible to get a sync success...  Just
extremely
> > > unlikely.
> > 
> > Actually, DnsTransaction can never complete synchronously:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/dns_transa...
> 
> Thanks!  Hmm...That was added to fix a crasher when it actually did succeed
> synchronously, wasn't it?

I'd like to say that it was by design, but yes, it was a crasher
http://crbug.com/120976

[mmenke, 2013-06-13 20:04:54]

Still need to go over the browser tests, and want to spend some more time
looking for races.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:160: SendInfo();
If it's a cross-site navigation, are we sending info to the right page?

https://codereview.chromium.org/13270005/diff/182001/chrome/common/net/net_er...
File chrome/common/net/net_error_info.h (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/common/net/net_er...
chrome/common/net/net_error_info.h:15: DNS_PROBE_POSSIBLE,
I suggest merging this with DNS_PROBE_STARTED, and call it DNS_PROBE_PENDING or
DNS_PROBE_WAIT or somesuch, a bit like ERR_IO_PENDING.

A "last_state_" of DNS_PROBE_POSSIBLE, in particular, seems a little weird.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:868: frame, error, is_post,
locale, &error_strings)) {
Don't really need an else/if here, and I don't think it clarifies things.  Can
just use:

if (!...) {
  blah();
}

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:870: // NetErrorHelper will
update it later.
nit:  Also, if you keep a comment here, get rid of the "we".

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:112: dns_error_active_ = true;
I think the state machine here is too confusing.

I suggest just three boolean variables:

is_loading_dns_error_page_:  Set to true here, set to false in OnStartLoad if
not an error page, and in OnCommitLoad.

is_non_updated_dns_error_page_:  Set to is_loading_dns_error_page_ in
OnCommitLoad.  Set to false after the page is updated with a final probe.

page_loaded_ is fine as-is.

Advantage with these is there's no ambiguity.  dns_error_active_ won't be true
for non-error pages while they're loading, for example.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:126: void
NetErrorHelper::OnFinishLoad(bool is_main_frame) {
This should check if it's a main frame, presumably.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:171: status_num >=
chrome_common_net::DNS_PROBE_MAX) {
Think we can just DCHECK on this, with the branch - generally, if something can
only happen in the case of a bug or random memory corruption, we shouldn't worry
about handling it.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:181: if (status ==
chrome_common_net::DNS_PROBE_POSSIBLE) {
Same as above comment - I think a DCHECK is sufficient.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:194: UpdateErrorPage();
Can this happen:

page_loaded_ starts as true (after a previous navigation).
OnStartLoad
OnFailLoad
OnNetErrorInfo (Before the error page starts to load)?
OnCommitLoad
...

If OnNetErrorInfo can occur before OnCommitLoad, then we're calling into the
wrong page.

If OnCommitLoad occurs asynchronously, this seems possible, if not likely.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:220:
render_view()->EvaluateScript(frame_xpath, js16, 0, false);
Should we do something to prevent too many updates?  I believe it may be
possible get a probe result from a previously displayed error page, and then get
two more results as we "run" a probe again in response to a new DNS error.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.h (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.h:63: chrome_common_net::DnsProbeStatus
last_status_;
nit:  Suggest renaming this last_probe_status_, to make it clear it's not the
status of the page load, or of the Helper.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:17:
mock_update_status_(chrome_common_net::DNS_PROBE_MAX) {}
Per Google style guide, should have virtual destructor.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:35: void
ReceiveStatus(DnsProbeStatus status) {
Suggest "ReceiveProbeStatus"

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:50: DnsProbeStatus
mock_update_status_;
I think this variable name is rather unclear.  I think both "status" and
"update" are a bit unclear.

Maybe probe_status_used_for_update_?

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:87: 
nit:  Remove blank line.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:94: 
Suggest throwing in a probe result.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:170: EXPECT_EQ(1,
update_count());
Should we update in this case?

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:195: FinishLoad(MAIN_FRAME);
Make sure no other updates here?

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:214: }
nit:  Line break.

https://codereview.chromium.org/13270005/diff/182001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:231: }
Maybe a test with two updated DNS error pages, with different statuses?

[szym, 2013-06-13 20:50:48]

dns_probe_* except dns_probe_browsertest looks good. The only major thing I
suggest is getting rid of ProbeType.

Maybe some comment on the tested scenarios would help me understand the
browsertest.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.cc (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:43: case net::OK:
nit: indent

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:134: callback_ = ProbeCallback();
Suggest: callback_.Reset()

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.h (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:42: // with the result (even if it is
known instantly).
Maybe add comment that must not be called again until callback is called.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:27: const
DnsProbeRunner::ProbeCallback& callback() const;
Put the accessor code inline.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:90: TEST_F(DnsProbeRunnerTest,
Null) {
All it does is test constructor+destructor. If that's intended, add a comment,
otherwise -- remove.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:106:
RunTest(MockDnsClientRule::FAIL_ASYNC, DnsProbeRunner::INCORRECT);
Consider (perhaps in a future CL) adding new MockDnsClientRule to test failures
like ERR_DNS_SERVER_FAILED. This is to improve test coverage here, but  would
probably be useful elsewhere.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.cc (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:125:
system_runner_.SetClient(system_client.Pass());
I don't think these wrapper functions help much. Maybe get rid of them.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:148: DCHECK(system_runner_.IsRunning());
I think this DCHECK will fail if the probe runner calls you back synchronously.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:160: system_result_ = result;
You could simplify this if you put the last result in DnsProbeRunner.

This way OnProbeComplete could be parameter-less, but most importantly you
wouldn't need to switch on ProbeType.

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.h (right):

https://codereview.chromium.org/13270005/diff/182001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.h:77: void ExpireResult();
nit: Suggest to name it ClearResult, FlushResult, AbandonResult, ForgetResult to
make it obvious that this is an unconditional action rather than "if expired,
remove it".

[Deprecated (see juliatuttle), 2013-06-18 19:36:37]

PTAL, mmenke (and anyone else who's interested)

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:43: case net::OK:
On 2013/06/13 20:50:48, szym wrote:
> nit: indent

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:134: callback_ = ProbeCallback();
On 2013/06/13 20:50:48, szym wrote:
> Suggest: callback_.Reset()

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:42: // with the result (even if it is
known instantly).
On 2013/06/13 20:50:48, szym wrote:
> Maybe add comment that must not be called again until callback is called.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:27: const
DnsProbeRunner::ProbeCallback& callback() const;
On 2013/06/13 20:50:48, szym wrote:
> Put the accessor code inline.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:90: TEST_F(DnsProbeRunnerTest,
Null) {
On 2013/06/13 20:50:48, szym wrote:
> All it does is test constructor+destructor. If that's intended, add a comment,
> otherwise -- remove.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:106:
RunTest(MockDnsClientRule::FAIL_ASYNC, DnsProbeRunner::INCORRECT);
On 2013/06/13 20:50:48, szym wrote:
> Consider (perhaps in a future CL) adding new MockDnsClientRule to test
failures
> like ERR_DNS_SERVER_FAILED. This is to improve test coverage here, but  would
> probably be useful elsewhere.

Yeah, it'd be useful to be able to specify an error code and an async flag.  Not
in this CL, though.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:125:
system_runner_.SetClient(system_client.Pass());
On 2013/06/13 20:50:48, szym wrote:
> I don't think these wrapper functions help much. Maybe get rid of them.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:148: DCHECK(system_runner_.IsRunning());
On 2013/06/13 20:50:48, szym wrote:
> I think this DCHECK will fail if the probe runner calls you back
synchronously.

The probe runner can't, anymore.  (There was already a bug here -- if the first
probe called us back asynchronously, we'd check and see both probe runners
stopped, assume we were done, and set state_ appropriately.  When we started the
second probe and *it* finished, state_ was already RESULT_CACHED, and a DCHECK
failed.)

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:160: system_result_ = result;
On 2013/06/13 20:50:48, szym wrote:
> You could simplify this if you put the last result in DnsProbeRunner.
> 
> This way OnProbeComplete could be parameter-less, but most importantly you
> wouldn't need to switch on ProbeType.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.h:77: void ExpireResult();
On 2013/06/13 20:50:48, szym wrote:
> nit: Suggest to name it ClearResult, FlushResult, AbandonResult, ForgetResult
to
> make it obvious that this is an unconditional action rather than "if expired,
> remove it".

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:160: SendInfo();
On 2013/06/13 20:04:55, mmenke wrote:
> If it's a cross-site navigation, are we sending info to the right page?

It doesn't matter; we re-send it when the error page commits just in case it was
cross-site.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/common/net...
File chrome/common/net/net_error_info.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/common/net...
chrome/common/net/net_error_info.h:15: DNS_PROBE_POSSIBLE,
On 2013/06/13 20:04:55, mmenke wrote:
> I suggest merging this with DNS_PROBE_STARTED, and call it DNS_PROBE_PENDING
or
> DNS_PROBE_WAIT or somesuch, a bit like ERR_IO_PENDING.
> 
> A "last_state_" of DNS_PROBE_POSSIBLE, in particular, seems a little weird.

The two are potentially different; if we ever add a spinner, it will be shown
for DNS_PROBE_STARTED but not DNS_PROBE_POSSIBLE.  The idea is that
DNS_PROBE_POSSIBLE should show the common error page elements, but not start a
spinner or anything that would flicker annoyingly if we immediately go to
DNS_PROBE_NOT_RUN.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/c...
chrome/renderer/chrome_content_renderer_client.cc:868: frame, error, is_post,
locale, &error_strings)) {
On 2013/06/13 20:04:55, mmenke wrote:
> Don't really need an else/if here, and I don't think it clarifies things.  Can
> just use:
> 
> if (!...) {
>   blah();
> }

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/c...
chrome/renderer/chrome_content_renderer_client.cc:870: // NetErrorHelper will
update it later.
On 2013/06/13 20:04:55, mmenke wrote:
> nit:  Also, if you keep a comment here, get rid of the "we".

Obsolete.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:112: dns_error_active_ = true;
On 2013/06/13 20:04:55, mmenke wrote:
> I think the state machine here is too confusing.
> 
> I suggest just three boolean variables:
> 
> is_loading_dns_error_page_:  Set to true here, set to false in OnStartLoad if
> not an error page, and in OnCommitLoad.
> 
> is_non_updated_dns_error_page_:  Set to is_loading_dns_error_page_ in
> OnCommitLoad.  Set to false after the page is updated with a final probe.
> 
> page_loaded_ is fine as-is.
> 
> Advantage with these is there's no ambiguity.  dns_error_active_ won't be true
> for non-error pages while they're loading, for example.

Done.

I had to clear page_loaded in OnFailLoad to make sure that, if we have two DNS
errors in a row, we don't consider the STARTED for the new page (which can come
as soon as we've failed a second time) intended for the old page.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:126: void
NetErrorHelper::OnFinishLoad(bool is_main_frame) {
On 2013/06/13 20:04:55, mmenke wrote:
> This should check if it's a main frame, presumably.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:171: status_num >=
chrome_common_net::DNS_PROBE_MAX) {
On 2013/06/13 20:04:55, mmenke wrote:
> Think we can just DCHECK on this, with the branch - generally, if something
can
> only happen in the case of a bug or random memory corruption, we shouldn't
worry
> about handling it.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:181: if (status ==
chrome_common_net::DNS_PROBE_POSSIBLE) {
On 2013/06/13 20:04:55, mmenke wrote:
> Same as above comment - I think a DCHECK is sufficient.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:194: UpdateErrorPage();
On 2013/06/13 20:04:55, mmenke wrote:
> Can this happen:
> 
> page_loaded_ starts as true (after a previous navigation).
> OnStartLoad
> OnFailLoad
> OnNetErrorInfo (Before the error page starts to load)?
> OnCommitLoad
> ...
> 
> If OnNetErrorInfo can occur before OnCommitLoad, then we're calling into the
> wrong page.
> 
> If OnCommitLoad occurs asynchronously, this seems possible, if not likely.

...ha, that is the exact race condition I just found.  Fixed (see above).

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:220:
render_view()->EvaluateScript(frame_xpath, js16, 0, false);
On 2013/06/13 20:04:55, mmenke wrote:
> Should we do something to prevent too many updates?  I believe it may be
> possible get a probe result from a previously displayed error page, and then
get
> two more results as we "run" a probe again in response to a new DNS error.

We could?  I don't think it'd be a huge deal if it occasionally updated twice.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:63: chrome_common_net::DnsProbeStatus
last_status_;
On 2013/06/13 20:04:55, mmenke wrote:
> nit:  Suggest renaming this last_probe_status_, to make it clear it's not the
> status of the page load, or of the Helper.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:17:
mock_update_status_(chrome_common_net::DNS_PROBE_MAX) {}
On 2013/06/13 20:04:55, mmenke wrote:
> Per Google style guide, should have virtual destructor.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:35: void
ReceiveStatus(DnsProbeStatus status) {
On 2013/06/13 20:04:55, mmenke wrote:
> Suggest "ReceiveProbeStatus"

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:50: DnsProbeStatus
mock_update_status_;
On 2013/06/13 20:04:55, mmenke wrote:
> I think this variable name is rather unclear.  I think both "status" and
> "update" are a bit unclear.
> 
> Maybe probe_status_used_for_update_?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:87: 
On 2013/06/13 20:04:55, mmenke wrote:
> nit:  Remove blank line.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:94: 
On 2013/06/13 20:04:55, mmenke wrote:
> Suggest throwing in a probe result.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:170: EXPECT_EQ(1,
update_count());
On 2013/06/13 20:04:55, mmenke wrote:
> Should we update in this case?

Yes; the initial strings provided to the error page don't include the "Hold on a
sec, we're trying to figure out what went wrong bit".  Once both of { the page
loads, we receive DNS_PROBE_STARTED } have happened, we update it to include
that.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:195: FinishLoad(MAIN_FRAME);
On 2013/06/13 20:04:55, mmenke wrote:
> Make sure no other updates here?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:214: }
On 2013/06/13 20:04:55, mmenke wrote:
> nit:  Line break.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/182001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:231: }
On 2013/06/13 20:04:55, mmenke wrote:
> Maybe a test with two updated DNS error pages, with different statuses?

Done.

[mmenke, 2013-06-20 15:47:36]

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:57: class
BrokenLinkDoctorProtocolHandlerDelegate {
Do we really need a delegate class?  Seems like we currently don't get anything
from this class.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:81: FilePath
GetMockLinkDoctorFilePath() const {
Suggest making this static, just to indicate it doesn't depend on anything. 
Alternatively, could just make it a global function.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:94: : io_thread_(NULL),
I'm assuming io_thread_ hasn't been created yet by the time this class is
created?  Could create it in SetUpOnMainThread(). Could then grab the IOThread
on construction.  Not a huge simplification, just one less thing to think about.
 Could also then make it automatically call SetUpOnIOThread on construction.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:98: virtual
~DnsProbeBrowserTestIOThreadHelper() {}
Suggest CHECKs on all these functions (Including set_link_doctor_net_error) to
make sure they're called on the IO thread.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:103: void
SetRules(MockDnsClientRule::Result system_good_result,
Think this function name could use improvement.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:129:
SetUrlRequestMocksEnabled(true);
Think it may just be simpler to install only the URLRequest mock classes we use,
rather than set up them all, and then remove one (One could imaging the way the
link doctor is mocked out by SetUrlRequestMocksEnabled changing, so I'd rather
not rely on it).

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:144: io_thread_ = NULL;
Should also call filter->ClearHandlers();

As a singleton, I believe the filter is leaked, so probably a good idea to make
sure what we added to it is cleaned up.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:204: Unretained(helper_)));
nit:  Suggest reversing the order of setup.  Doesn't matter here, but think it's
a good habit to get into.  Otherwise, can miss cases where it does matter.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:207: void
DnsProbeBrowserTest::SetLinkDoctorBroken(bool broken) {
Do we really need both this and SetLinkDoctorNetError?

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:241: bool
DnsProbeBrowserTest::TitleContains(const std::string& expected) {
Think it's worth mentioning why this is used instead of WebContents::GetTitle or
a TitleWatcher.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:254: return title.find(expected) !=
std::string::npos;
Is there a reason for not testing for an exact match?

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:282: EXPECT_TRUE(TitleContains("Mock
Link Doctor"));
Suggest adding a way to check if probes are queued and / or if a result is
cached, and checking them at the end of each test.

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:306:
EXPECT_TRUE(PageContains("DNS_PROBE_FINISHED_NO_INTERNET"));
Why are we guaranteed the probes have completed by this point?

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:319: } */
You plan to enable this?

https://codereview.chromium.org/13270005/diff/198001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:320: 
Other test suggestion:  Check the case that DNS probes are disabled.

[Deprecated (see juliatuttle), 2013-06-21 19:55:52]

PTAL, mmenke.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:57: class
BrokenLinkDoctorProtocolHandlerDelegate {
On 2013/06/20 15:47:36, mmenke wrote:
> Do we really need a delegate class?  Seems like we currently don't get
anything
> from this class.

It's what keeps BrokenLinkDoctorProtocolHandler and
DnsProbeBrowserTestIOThreadHelper from having circular dependencies.  It is
actually less code than moving the declaration of
DnsProbeBrowserTestIOThreadHelper here and then splitting out the method
definitions below.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:81: FilePath
GetMockLinkDoctorFilePath() const {
On 2013/06/20 15:47:36, mmenke wrote:
> Suggest making this static, just to indicate it doesn't depend on anything. 
> Alternatively, could just make it a global function.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:94: : io_thread_(NULL),
On 2013/06/20 15:47:36, mmenke wrote:
> I'm assuming io_thread_ hasn't been created yet by the time this class is
> created?  Could create it in SetUpOnMainThread(). Could then grab the IOThread
> on construction.  Not a huge simplification, just one less thing to think
about.
>  Could also then make it automatically call SetUpOnIOThread on construction.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:98: virtual
~DnsProbeBrowserTestIOThreadHelper() {}
On 2013/06/20 15:47:36, mmenke wrote:
> Suggest CHECKs on all these functions (Including set_link_doctor_net_error) to
> make sure they're called on the IO thread.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:103: void
SetRules(MockDnsClientRule::Result system_good_result,
On 2013/06/20 15:47:36, mmenke wrote:
> Think this function name could use improvement.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:129:
SetUrlRequestMocksEnabled(true);
On 2013/06/20 15:47:36, mmenke wrote:
> Think it may just be simpler to install only the URLRequest mock classes we
use,
> rather than set up them all, and then remove one (One could imaging the way
the
> link doctor is mocked out by SetUrlRequestMocksEnabled changing, so I'd rather
> not rely on it).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:144: io_thread_ = NULL;
On 2013/06/20 15:47:36, mmenke wrote:
> Should also call filter->ClearHandlers();
> 
> As a singleton, I believe the filter is leaked, so probably a good idea to
make
> sure what we added to it is cleaned up.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:204: Unretained(helper_)));
On 2013/06/20 15:47:36, mmenke wrote:
> nit:  Suggest reversing the order of setup.  Doesn't matter here, but think
it's
> a good habit to get into.  Otherwise, can miss cases where it does matter.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:207: void
DnsProbeBrowserTest::SetLinkDoctorBroken(bool broken) {
On 2013/06/20 15:47:36, mmenke wrote:
> Do we really need both this and SetLinkDoctorNetError?

No.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:241: bool
DnsProbeBrowserTest::TitleContains(const std::string& expected) {
On 2013/06/20 15:47:36, mmenke wrote:
> Think it's worth mentioning why this is used instead of WebContents::GetTitle
or
> a TitleWatcher.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:254: return title.find(expected) !=
std::string::npos;
On 2013/06/20 15:47:36, mmenke wrote:
> Is there a reason for not testing for an exact match?

No!  Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:282: EXPECT_TRUE(TitleContains("Mock
Link Doctor"));
On 2013/06/20 15:47:36, mmenke wrote:
> Suggest adding a way to check if probes are queued and / or if a result is
> cached, and checking them at the end of each test.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:306:
EXPECT_TRUE(PageContains("DNS_PROBE_FINISHED_NO_INTERNET"));
On 2013/06/20 15:47:36, mmenke wrote:
> Why are we guaranteed the probes have completed by this point?

We're not.  Fixed.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:319: } */
On 2013/06/20 15:47:36, mmenke wrote:
> You plan to enable this?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/198001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:320: 
On 2013/06/20 15:47:36, mmenke wrote:
> Other test suggestion:  Check the case that DNS probes are disabled.

Done.

[mmenke, 2013-06-24 16:20:00]

A couple more comments (Some actually written over the weekend, honest).  I'll
get back to you with more later today.  I expect mostly nits, though I'll
continue to look for races.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:67: virtual int
link_doctor_net_error() const = 0;
nit:  Virtual functions shouldn't be named this way.  Also, do we really need an
abstract class for this?  Can just give the BrokenLinkDoctorProtocolHandler a
function to set the result, and have the DnsProbeBrowserTestIOThreadHelper call
that directly, so don't need to have the two classes talk to each other.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:73: explicit
BrokenLinkDoctorProtocolHandler(
nit:  Explicit not needed (Unless you get rid of the second argument, per my
suggestion above).

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:129:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
Should use CHECKS (Or asserts) in tests, since we want the release build to fail
with them, too.  The CQ, in particular, only runs tests with release builds.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:195: int net_error_info_count_;
We're actually waiting / monitoring dns_probe_status, not net_error_info (x4).

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:285: // sent before this have been
applied.
Did you consider the title watcher?  The one concern with it is that if the
title is changed to the correct title and then to something else, it still
passes.  Not sure that's an issue for us.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:383:
FILE_PATH_LITERAL("iframe_dns_error.html");
Since this is only used in one test body, suggest just inlining it there.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:391: EXPECT_EQ(0,
net_error_info_count());
Think it's worth mentioning why these two tests won't flakily pass.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:406: }
Suggest a test with the link doctor diabled via prefs.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:406: }
Suggest a test with dns failing synchronously, just out of paranoia.  Know we
test it at the lower layer, too, but as that's a case where we risk a crash, as
opposed to just not updating the page properly, think it's worth extra testing.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.cc (right):

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.cc:91: DCHECK(!callback.is_null());
MAybe a DCHECK(!transaction_), too?

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.h (right):

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:41: // be called again // until the
callback is called (but may be called during
nit:  Remove bonux "//"

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.h (right):

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.h:34: NetErrorInfoSnoopCallback;
Since this takes a DnsProbeStatus rather than a NetErrorInfo, I think this
naming is confusing.  Fine with either renaming it, or changing its argument
type.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.h:44: const NetErrorInfoSnoopCallback&
net_error_snoop_callback);
Per Google style guide, functions that use this naming scheme should be inlined.
 I'm not actually sure about the overhead of copying a callback, though I
believe the data they hold is actually refcounted, so a "copy" is just
incrementing a reference and copying a pointer.

[Deprecated (see juliatuttle), 2013-06-25 16:45:04]

PTAL, mmenke.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:67: virtual int
link_doctor_net_error() const = 0;
On 2013/06/24 16:20:00, mmenke wrote:
> nit:  Virtual functions shouldn't be named this way.  Also, do we really need
an
> abstract class for this?  Can just give the BrokenLinkDoctorProtocolHandler a
> function to set the result, and have the DnsProbeBrowserTestIOThreadHelper
call
> that directly, so don't need to have the two classes talk to each other.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:73: explicit
BrokenLinkDoctorProtocolHandler(
On 2013/06/24 16:20:00, mmenke wrote:
> nit:  Explicit not needed (Unless you get rid of the second argument, per my
> suggestion above).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:129:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2013/06/24 16:20:00, mmenke wrote:
> Should use CHECKS (Or asserts) in tests, since we want the release build to
fail
> with them, too.  The CQ, in particular, only runs tests with release builds.

Done.  Do any of the DCHECKs in the non-test code want to change, also?

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:195: int net_error_info_count_;
On 2013/06/24 16:20:00, mmenke wrote:
> We're actually waiting / monitoring dns_probe_status, not net_error_info (x4).

Yeah, but what we receive is a NetErrorInfo message containing the
DnsProbeStatus.  (I can change the name if you want, but I named it for the
message it's waiting for.)

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:285: // sent before this have been
applied.
On 2013/06/24 16:20:00, mmenke wrote:
> Did you consider the title watcher?  The one concern with it is that if the
> title is changed to the correct title and then to something else, it still
> passes.  Not sure that's an issue for us.

I did; I like this better, but I'll change it if you want.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:383:
FILE_PATH_LITERAL("iframe_dns_error.html");
On 2013/06/24 16:20:00, mmenke wrote:
> Since this is only used in one test body, suggest just inlining it there.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:391: EXPECT_EQ(0,
net_error_info_count());
On 2013/06/24 16:20:00, mmenke wrote:
> Think it's worth mentioning why these two tests won't flakily pass.

Added to NoProbeInSubFrame.  ProbesDisabled explicitly expects NOT_RUN, so I
don't see how it could flakily pass (run a probe) while claiming it didn't run a
probe.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:406: }
On 2013/06/24 16:20:00, mmenke wrote:
> Suggest a test with the link doctor diabled via prefs.

That's tricky; we're currently gated on a field trial as well.  I can force
probes on to avoid the field trial, but that bypasses the pref as well.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:406: }
On 2013/06/24 16:20:00, mmenke wrote:
> Suggest a test with dns failing synchronously, just out of paranoia.  Know we
> test it at the lower layer, too, but as that's a case where we risk a crash,
as
> opposed to just not updating the page properly, think it's worth extra
testing.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.cc:91: DCHECK(!callback.is_null());
On 2013/06/24 16:20:00, mmenke wrote:
> MAybe a DCHECK(!transaction_), too?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:41: // be called again // until the
callback is called (but may be called during
On 2013/06/24 16:20:00, mmenke wrote:
> nit:  Remove bonux "//"

Done.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.h:34: NetErrorInfoSnoopCallback;
On 2013/06/24 16:20:00, mmenke wrote:
> Since this takes a DnsProbeStatus rather than a NetErrorInfo, I think this
> naming is confusing.  Fine with either renaming it, or changing its argument
> type.

There is no such type as a NetErrorInfo; I'll rename it to
DnsProbeStatusSnoopCallback.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.h:44: const NetErrorInfoSnoopCallback&
net_error_snoop_callback);
On 2013/06/24 16:20:00, mmenke wrote:
> Per Google style guide, functions that use this naming scheme should be
inlined.
>  I'm not actually sure about the overhead of copying a callback, though I
> believe the data they hold is actually refcounted, so a "copy" is just
> incrementing a reference and copying a pointer.

Done.

[mmenke, 2013-06-25 17:40:26]

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:129:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2013/06/25 16:45:04, ttuttle wrote:
> On 2013/06/24 16:20:00, mmenke wrote:
> > Should use CHECKS (Or asserts) in tests, since we want the release build to
> fail
> > with them, too.  The CQ, in particular, only runs tests with release builds.
> 
> Done.  Do any of the DCHECKs in the non-test code want to change, also?

My general feeling is that production code should avoid CHECKs unless there's
something we're particularly concerned about failing / regressing, just to avoid
bloating up the binary.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:391: EXPECT_EQ(0,
net_error_info_count());
On 2013/06/25 16:45:04, ttuttle wrote:
> On 2013/06/24 16:20:00, mmenke wrote:
> > Think it's worth mentioning why these two tests won't flakily pass.
> 
> Added to NoProbeInSubFrame.  ProbesDisabled explicitly expects NOT_RUN, so I
> don't see how it could flakily pass (run a probe) while claiming it didn't run
a
> probe.

You're right, was still thinking of the pre-callback version.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:406: }
On 2013/06/25 16:45:04, ttuttle wrote:
> On 2013/06/24 16:20:00, mmenke wrote:
> > Suggest a test with the link doctor diabled via prefs.
> 
> That's tricky; we're currently gated on a field trial as well.  I can force
> probes on to avoid the field trial, but that bypasses the pref as well.

The issue I'm concerned about isn't what we're gated on, it's what the link
doctor's second page load is gated on.  We don't even check that pref, do we?  I
just want to make sure we work when the error page has a single load, instead of
two loads.

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/227002/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.h:34: NetErrorInfoSnoopCallback;
On 2013/06/25 16:45:04, ttuttle wrote:
> On 2013/06/24 16:20:00, mmenke wrote:
> > Since this takes a DnsProbeStatus rather than a NetErrorInfo, I think this
> > naming is confusing.  Fine with either renaming it, or changing its argument
> > type.
> 
> There is no such type as a NetErrorInfo; I'll rename it to
> DnsProbeStatusSnoopCallback.

There is actually a "ChromeViewMsg_NetErrorInfo", which could theoretically be
referred to as "NetErrorInfo".  From the old function name, that's what I
expected it to take as an argument.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:360: switch
(last_dns_probe_status()) {
Please indent the body of switch statements - not sure if the style guide is
clear about it, but I find it makes the first case statement much more readable.
 Also seems to be more common in Chrome.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:402: }}
nit:  Split onto two lines.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:32: UNREACHABLE  // No response received
(timeout, network unreachable, etc.)
Nit:  If comments start with a capital, should generally end them in periods.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:46: void
RunTest(MockDnsClientRule::Result good_query_result,
nit:  We no longer do a bad query, so should get rid of the "good_" wherever it
appears in this file.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:71: }
I don't think we really need this one.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:137: ClearCachedResult();
nit:  Should consistently use either "result" or "results", instead of
ClearCachedResult()  and cached_result_, but STATE_NO_RESULTS.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:147: // do nothing; probe is already
running, and will call the callback
nit:  Capitalize + period.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:154: SetSystemClientToCurrentConfig();
If we're running a probe when the network changes, we'll use (And cache) the old
results.  Maybe we should reconsider?  Think it's fine to worry about it in
another CL.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:187:
public_config.nameservers.push_back(MakeDnsEndPoint(kGooglePublicDns1));
This should be kGooglePublicDns2

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:47: MockDnsClientRule::Result
public_good_query_result) {
nit:  Again, should probably ditch the goods

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:87: }
Think these Null tests are overkill.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_test_util.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_test_util.h:17: net::MockDnsClientRule::Result
good_result);
nit:  - good

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/common/net...
File chrome/common/net/net_error_info.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/common/net...
chrome/common/net/net_error_info.h:12: enum DnsProbeStatus {
Hmm...Speaking of there being no NetErrorInfo type...Perhaps this file should be
named dns_probe_status.h?

[mmenke, 2013-06-26 15:48:19]

First half of comments.  All nits except two browser test concerns.

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://codereview.chromium.org/13270005/diff/227002/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:285: // sent before this have been
applied.
On 2013/06/25 16:45:04, ttuttle wrote:
> On 2013/06/24 16:20:00, mmenke wrote:
> > Did you consider the title watcher?  The one concern with it is that if the
> > title is changed to the correct title and then to something else, it still
> > passes.  Not sure that's an issue for us.
> 
> I did; I like this better, but I'll change it if you want.

I've gone back and forth on this.  One one hand, it mirrors
DnsProbeBrowserTest::PageContains, so understanding it shouldn't add too much
overhead.

On the other hand, we already have a way for browser tests to wait for a
specific title.  If we used it, people familiar with that class don't have to
relearn another way of doing it in this file.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:68: explicit
BrokenLinkDoctorProtocolHandler(FilePath mock_link_doctor_file_path)
nit:  const FilePath&?

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:70: net_error_(net::OK) {}
nit:  Google style guide requires a virtual destructor for any class with
virtual functions.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:92: virtual
~DnsProbeBrowserTestIOThreadHelper() {}
nit:  virtual no longer needed.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:127:
protocol_handler_scoped_ptr_(protocol_handler_);
protocol_handler_scoped_ptr_ -> protocol_handler_scoped_ptr

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:127:
protocol_handler_scoped_ptr_(protocol_handler_);
Think it's a little cleaner to always have something own the protocol handler. 
Suggest creating it in the scoped pointer first, and then setting
protocol_handler.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:165: MockDnsClientRule::Result
public_good_result);
nit:  Don't need the goods.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:165: MockDnsClientRule::Result
public_good_result);
nit:  Indent here is a little weird.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:257: MockDnsClientRule::Result
public_good_result) {
nit:  Again, don't think "good" is needed any more.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:359: WaitForNetErrorInfo();
Re-entering the message loop in a loop like this is a bit of an anti-pattern
that's probably best to avoid.

Think it's a bit cleaner and simpler to have "WaitForNetErrorInfo" wait until we
send the final result for a probe, keeping an array of all intermediary results.
 Then we can just check the array at the end.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:366: if
(PageContains("DNS_PROBE_FINISHED_NO_INTERNET"))
This is needed because of the blank page before we load the link doctor page? 
This seems like a bad idea to me.  PageContains itself runs a message loop, at
which point more probe statuses can be sent.  It's suddenly not clear what order
things happen in.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner.h (right):

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:28: UNKNOWN,
I think it's worth adding a note that these are used in histograms.  So it's not
safe to remove entries, and new entries much be added to the bottom.  Otherwise,
I'd suggest removing UNKNOWN.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:42: // callback).
Instead of all the parentheses, use commas before the remarks, as both are
basically just compound sentences.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner.h:47: // a probe.  (It will not, of course,
affect the in-flight probe.)
Parenthesis not needed.  Also suggest removing the "of course" - it's not
actually self-evident that it doesn't affect the in-flight probe.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:36: DCHECK(!called_);
Think we can just use an EXPECT here, so it triggers in release builds (CHECKs
in unit tests are generally bad, unless we expect we'll enter an infinite loop).

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_runner_unittest.cc:60: if (query_result !=
MockDnsClientRule::FAIL_SYNC)
I don't think this conditional is needed any more.  Even in the sync result
case, we pass the result back asynchronously, and IsRunning() returns true in
the meantime.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service.cc (right):

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:73: // from DnsProbeJob to
DnsProbeRunner.
Have you done this?

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:93: case
chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN:
nit:  +indent.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:140: case STATE_NO_RESULT:
nit:  +indent.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service.cc:235: pending_callbacks_.clear();
nit:  Rather than copy + clear, maybe use swap?

https://codereview.chromium.org/13270005/diff/266001/chrome/common/net/net_er...
File chrome/common/net/net_error_info.h (right):

https://codereview.chromium.org/13270005/diff/266001/chrome/common/net/net_er...
chrome/common/net/net_error_info.h:12: enum DnsProbeStatus {
Perhaps this file should be named dns_probe_status.h?

[mmenke, 2013-06-26 16:31:28]

Not going to finish this today.  In focusing on a couple files, I'd kinda
forgotten this was a 2300 line CL.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:114: return;
nit:  return not needed.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:132: OnMainFrameDnsError();
Do we really need to start the probe and send the status on fail?  Seems like
doing this on commit instead would make life a little simpler - fewer redundant
SendInfo calls, for example.  Would start the probes a little later, but think
having a cleaner interface is worth it.

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:166: DCHECK(dns_probe_status_ !=
chrome_common_net::DNS_PROBE_STARTED);
DCHECK_NE (And reverse order)

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:211: void
NetErrorTabHelper::SendInfo() {
nit:  const?

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.h (right):

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.h:100: // but has not yet seen a new
page load successfully afterwards.
"load successfully" -> "commit successfully" (Can fail after commit, but before
load complete).

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/266001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:48: int mock_probe_count_;
nit:  Think this is a bit of a misnomer.  Suggest "running_probe_count_"

[Deprecated (see juliatuttle), 2013-06-26 22:23:55]

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:360: switch
(last_dns_probe_status()) {
On 2013/06/25 17:40:27, mmenke wrote:
> Please indent the body of switch statements - not sure if the style guide is
> clear about it, but I find it makes the first case statement much more
readable.
>  Also seems to be more common in Chrome.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:402: }}
On 2013/06/25 17:40:27, mmenke wrote:
> nit:  Split onto two lines.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:32: UNREACHABLE  // No response received
(timeout, network unreachable, etc.)
On 2013/06/25 17:40:27, mmenke wrote:
> Nit:  If comments start with a capital, should generally end them in periods.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:46: void
RunTest(MockDnsClientRule::Result good_query_result,
On 2013/06/25 17:40:27, mmenke wrote:
> nit:  We no longer do a bad query, so should get rid of the "good_" wherever
it
> appears in this file.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:71: }
On 2013/06/25 17:40:27, mmenke wrote:
> I don't think we really need this one.

I've had them crash before, but okay.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:137: ClearCachedResult();
On 2013/06/25 17:40:27, mmenke wrote:
> nit:  Should consistently use either "result" or "results", instead of
> ClearCachedResult()  and cached_result_, but STATE_NO_RESULTS.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:147: // do nothing; probe is already
running, and will call the callback
On 2013/06/25 17:40:27, mmenke wrote:
> nit:  Capitalize + period.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:154: SetSystemClientToCurrentConfig();
On 2013/06/25 17:40:27, mmenke wrote:
> If we're running a probe when the network changes, we'll use (And cache) the
old
> results.  Maybe we should reconsider?  Think it's fine to worry about it in
> another CL.

I'm mostly okay with that.  I think we might want to consider not caching it,
but I'm fine with displaying stale probe results.  (If we change networks, the
old NAME_NOT_RESOLVED page is no longer right either :)

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:187:
public_config.nameservers.push_back(MakeDnsEndPoint(kGooglePublicDns1));
On 2013/06/25 17:40:27, mmenke wrote:
> This should be kGooglePublicDns2

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:47: MockDnsClientRule::Result
public_good_query_result) {
On 2013/06/25 17:40:27, mmenke wrote:
> nit:  Again, should probably ditch the goods

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:87: }
On 2013/06/25 17:40:27, mmenke wrote:
> Think these Null tests are overkill.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_test_util.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_test_util.h:17: net::MockDnsClientRule::Result
good_result);
On 2013/06/25 17:40:27, mmenke wrote:
> nit:  - good

Done.

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/common/net...
File chrome/common/net/net_error_info.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/common/net...
chrome/common/net/net_error_info.h:12: enum DnsProbeStatus {
On 2013/06/25 17:40:27, mmenke wrote:
> Hmm...Speaking of there being no NetErrorInfo type...Perhaps this file should
be
> named dns_probe_status.h?

I am still planning for the eventuality that we pass other info (like a "user is
definitely not using a proxy" flag) for other errors.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:68: explicit
BrokenLinkDoctorProtocolHandler(FilePath mock_link_doctor_file_path)
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  const FilePath&?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:70: net_error_(net::OK) {}
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  Google style guide requires a virtual destructor for any class with
> virtual functions.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:92: virtual
~DnsProbeBrowserTestIOThreadHelper() {}
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  virtual no longer needed.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:127:
protocol_handler_scoped_ptr_(protocol_handler_);
On 2013/06/26 15:48:20, mmenke wrote:
> protocol_handler_scoped_ptr_ -> protocol_handler_scoped_ptr

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:127:
protocol_handler_scoped_ptr_(protocol_handler_);
On 2013/06/26 15:48:20, mmenke wrote:
> Think it's a little cleaner to always have something own the protocol handler.

> Suggest creating it in the scoped pointer first, and then setting
> protocol_handler.

Done, but it requires an extra cast.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:165: MockDnsClientRule::Result
public_good_result);
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  Don't need the goods.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:165: MockDnsClientRule::Result
public_good_result);
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  Indent here is a little weird.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:257: MockDnsClientRule::Result
public_good_result) {
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  Again, don't think "good" is needed any more.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:359: WaitForNetErrorInfo();
On 2013/06/26 15:48:20, mmenke wrote:
> Re-entering the message loop in a loop like this is a bit of an anti-pattern
> that's probably best to avoid.
> 
> Think it's a bit cleaner and simpler to have "WaitForNetErrorInfo" wait until
we
> send the final result for a probe, keeping an array of all intermediary
results.
>  Then we can just check the array at the end.

The problem is that the loop isn't, overall, waiting for the final result; it's
waiting for the page to be updated, and using the probe results as a way to know
when to check the page.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:366: if
(PageContains("DNS_PROBE_FINISHED_NO_INTERNET"))
On 2013/06/26 15:48:20, mmenke wrote:
> This is needed because of the blank page before we load the link doctor page? 
> This seems like a bad idea to me.  PageContains itself runs a message loop, at
> which point more probe statuses can be sent.  It's suddenly not clear what
order
> things happen in.

Hmm.  How do you think I should clarify this?  Basically, I want to say "check
the page for this string every time we get a probe status, until it contains
this text.  also, if you get any status other than this or that, it's bad".

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:28: UNKNOWN,
On 2013/06/26 15:48:20, mmenke wrote:
> I think it's worth adding a note that these are used in histograms.  So it's
not
> safe to remove entries, and new entries much be added to the bottom. 
Otherwise,
> I'd suggest removing UNKNOWN.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:42: // callback).
On 2013/06/26 15:48:20, mmenke wrote:
> Instead of all the parentheses, use commas before the remarks, as both are
> basically just compound sentences.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner.h:47: // a probe.  (It will not, of course,
affect the in-flight probe.)
On 2013/06/26 15:48:20, mmenke wrote:
> Parenthesis not needed.  Also suggest removing the "of course" - it's not
> actually self-evident that it doesn't affect the in-flight probe.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/dns_probe_runner_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:36: DCHECK(!called_);
On 2013/06/26 15:48:20, mmenke wrote:
> Think we can just use an EXPECT here, so it triggers in release builds (CHECKs
> in unit tests are generally bad, unless we expect we'll enter an infinite
loop).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_runner_unittest.cc:60: if (query_result !=
MockDnsClientRule::FAIL_SYNC)
On 2013/06/26 15:48:20, mmenke wrote:
> I don't think this conditional is needed any more.  Even in the sync result
> case, we pass the result back asynchronously, and IsRunning() returns true in
> the meantime.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:93: case
chrome_common_net::DNS_PROBE_FINISHED_UNKNOWN:
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  +indent.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:140: case STATE_NO_RESULT:
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  +indent.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:235: pending_callbacks_.clear();
On 2013/06/26 15:48:20, mmenke wrote:
> nit:  Rather than copy + clear, maybe use swap?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:114: return;
On 2013/06/26 16:31:28, mmenke wrote:
> nit:  return not needed.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:132: OnMainFrameDnsError();
On 2013/06/26 16:31:28, mmenke wrote:
> Do we really need to start the probe and send the status on fail?  Seems like
> doing this on commit instead would make life a little simpler - fewer
redundant
> SendInfo calls, for example.  Would start the probes a little later, but think
> having a cleaner interface is worth it.

We're going to have to track the fail anyway to know that we should start the
probe on a commit, and we're going to have to resend the status on each commit
to deal with the blank link doctor page.  I'm not sure how much simplification
we'd get out of this.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:166: DCHECK(dns_probe_status_ !=
chrome_common_net::DNS_PROBE_STARTED);
On 2013/06/26 16:31:28, mmenke wrote:
> DCHECK_NE (And reverse order)

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:211: void
NetErrorTabHelper::SendInfo() {
On 2013/06/26 16:31:28, mmenke wrote:
> nit:  const?

Can't; the underlying Send method isn't const.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.h:100: // but has not yet seen a new
page load successfully afterwards.
On 2013/06/26 16:31:28, mmenke wrote:
> "load successfully" -> "commit successfully" (Can fail after commit, but
before
> load complete).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:48: int mock_probe_count_;
On 2013/06/26 16:31:28, mmenke wrote:
> nit:  Think this is a bit of a misnomer.  Suggest "running_probe_count_"

Done.

[mmenke, 2013-06-27 14:49:36]

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/244002/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:154: SetSystemClientToCurrentConfig();
On 2013/06/26 22:23:56, ttuttle wrote:
> On 2013/06/25 17:40:27, mmenke wrote:
> > If we're running a probe when the network changes, we'll use (And cache) the
> old
> > results.  Maybe we should reconsider?  Think it's fine to worry about it in
> > another CL.
> 
> I'm mostly okay with that.  I think we might want to consider not caching it,
> but I'm fine with displaying stale probe results.  (If we change networks, the
> old NAME_NOT_RESOLVED page is no longer right either :)

My thinking is that a fresh result matters most around the point that
connectivity changes - mostly don't want to cache a stale result.  Not too
concerned about requests that fail right around the network change.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:359: WaitForNetErrorInfo();
On 2013/06/26 22:23:56, ttuttle wrote:
> On 2013/06/26 15:48:20, mmenke wrote:
> > Re-entering the message loop in a loop like this is a bit of an anti-pattern
> > that's probably best to avoid.
> > 
> > Think it's a bit cleaner and simpler to have "WaitForNetErrorInfo" wait
until
> we
> > send the final result for a probe, keeping an array of all intermediary
> results.
> >  Then we can just check the array at the end.
> 
> The problem is that the loop isn't, overall, waiting for the final result;
it's
> waiting for the page to be updated, and using the probe results as a way to
know
> when to check the page.

Ah, right...Think the simplest solution would be to have a result queue, and
exit the message loop at the end of the queue (And check each result as they
come in).  Seen this done before, to avoid this general code flow.

Also, non-determinism in tests is a big cause of flake.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:132: OnMainFrameDnsError();
On 2013/06/26 22:23:56, ttuttle wrote:
> On 2013/06/26 16:31:28, mmenke wrote:
> > Do we really need to start the probe and send the status on fail?  Seems
like
> > doing this on commit instead would make life a little simpler - fewer
> redundant
> > SendInfo calls, for example.  Would start the probes a little later, but
think
> > having a cleaner interface is worth it.
> 
> We're going to have to track the fail anyway to know that we should start the
> probe on a commit, and we're going to have to resend the status on each commit
> to deal with the blank link doctor page.  I'm not sure how much simplification
> we'd get out of this.

Right now, we may have soemthing like:

DNS_PROBE_STARTED
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET

or, if the probe returns quickly,

DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_FINISHED_NO_INTERNET

This seems *much* more regression prone than just:
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET

Given the two page loads, we may actually have:
DNS_PROBE_STARTED
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_FINISHED_NO_INTERNET

Or maybe:
DNS_PROBE_STARTED
DNS_PROBE_STARTED
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_FINISHED_NO_INTERNET

Or maybe:
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_FINISHED_NO_INTERNET

May also be possible to get:
DNS_PROBE_STARTED
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_STARTED
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET

Having code do weird stuff tends to make for regression prone code when future
coders don't understand the weird stuff that the code is doing.

Doing what I suggest would just result in just two possibilities:
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET

and

DNS_PROBE_STARTED
DNS_PROBE_STARTED
DNS_PROBE_FINISHED_NO_INTERNET

Still not perfect, but much simpler.

[Deprecated (see juliatuttle), 2013-06-28 16:59:45]

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:359: WaitForNetErrorInfo();
On 2013/06/27 14:49:37, mmenke wrote:
> On 2013/06/26 22:23:56, ttuttle wrote:
> > On 2013/06/26 15:48:20, mmenke wrote:
> > > Re-entering the message loop in a loop like this is a bit of an
anti-pattern
> > > that's probably best to avoid.
> > > 
> > > Think it's a bit cleaner and simpler to have "WaitForNetErrorInfo" wait
> until
> > we
> > > send the final result for a probe, keeping an array of all intermediary
> > results.
> > >  Then we can just check the array at the end.
> > 
> > The problem is that the loop isn't, overall, waiting for the final result;
> it's
> > waiting for the page to be updated, and using the probe results as a way to
> know
> > when to check the page.
> 
> Ah, right...Think the simplest solution would be to have a result queue, and
> exit the message loop at the end of the queue (And check each result as they
> come in).  Seen this done before, to avoid this general code flow.
> 
> Also, non-determinism in tests is a big cause of flake.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:366: if
(PageContains("DNS_PROBE_FINISHED_NO_INTERNET"))
On 2013/06/26 22:23:56, ttuttle wrote:
> On 2013/06/26 15:48:20, mmenke wrote:
> > This is needed because of the blank page before we load the link doctor
page? 
> > This seems like a bad idea to me.  PageContains itself runs a message loop,
at
> > which point more probe statuses can be sent.  It's suddenly not clear what
> order
> > things happen in.
> 
> Hmm.  How do you think I should clarify this?  Basically, I want to say "check
> the page for this string every time we get a probe status, until it contains
> this text.  also, if you get any status other than this or that, it's bad".

Obsolete.

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/dns_probe_service.cc:73: // from DnsProbeJob to
DnsProbeRunner.
On 2013/06/26 15:48:20, mmenke wrote:
> Have you done this?

I have now!

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/266001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:132: OnMainFrameDnsError();
On 2013/06/27 14:49:37, mmenke wrote:
> On 2013/06/26 22:23:56, ttuttle wrote:
> > On 2013/06/26 16:31:28, mmenke wrote:
> > > Do we really need to start the probe and send the status on fail?  Seems
> like
> > > doing this on commit instead would make life a little simpler - fewer
> > redundant
> > > SendInfo calls, for example.  Would start the probes a little later, but
> think
> > > having a cleaner interface is worth it.
> > 
> > We're going to have to track the fail anyway to know that we should start
the
> > probe on a commit, and we're going to have to resend the status on each
commit
> > to deal with the blank link doctor page.  I'm not sure how much
simplification
> > we'd get out of this.
> 
> Right now, we may have soemthing like:
> 
> DNS_PROBE_STARTED
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> or, if the probe returns quickly,
> 
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> This seems *much* more regression prone than just:
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> Given the two page loads, we may actually have:
> DNS_PROBE_STARTED
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> Or maybe:
> DNS_PROBE_STARTED
> DNS_PROBE_STARTED
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> Or maybe:
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> May also be possible to get:
> DNS_PROBE_STARTED
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_STARTED
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> Having code do weird stuff tends to make for regression prone code when future
> coders don't understand the weird stuff that the code is doing.
> 
> Doing what I suggest would just result in just two possibilities:
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> and
> 
> DNS_PROBE_STARTED
> DNS_PROBE_STARTED
> DNS_PROBE_FINISHED_NO_INTERNET
> 
> Still not perfect, but much simpler.

Done.

[mmenke, 2013-06-28 18:06:19]

Browser test (finally) done.  Sending just this since I found a bug, still need
to review the others.  Why does it always take me five times longer than I
expect to review stuff?  :(

https://codereview.chromium.org/13270005/diff/296001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.h (right):

https://codereview.chromium.org/13270005/diff/296001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.h:84: chrome_common_net::DnsProbeStatus
dns_probe_status_;
If this is never written to in unit tests, suggest just making a protected
accessor instead.

https://codereview.chromium.org/13270005/diff/296001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.h (right):

https://codereview.chromium.org/13270005/diff/296001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.h:43: static bool
GetErrorStringsForDnsProbe(
Think this needs a comment.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:59: class DelayingDnsProbeService :
public DnsProbeService {
Think this is worth a comment (Both what it does and why).

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:61: DelayingDnsProbeService() :
DnsProbeService() {}
nit:  " : DnsProbeService()" not needed.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:75: void CallDelayedCallbacks() {
BUG:  We could have an async call to CallCallbacks pending in DnsProbeService.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:177: 
nit:  Remove blank line.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:187: IOThread::Globals* globals =
io_thread_->globals();
optional:  Feels safest to me to just always keep the probe in a scoped_ptr.

scoped_ptr<DnsProbeService>
delaying_dns_probe_service(globals->dns_probe_service.release());

Generally feel like we should avoid calling delete, in favor of just always
having someone own an object explicitly (The "delete this" below is fine).

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:199: MockDnsClientRule::Result
public_result) {
CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));?

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:208: int link_doctor_net_error) {
CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));?

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:213: int
expected_delayed_callback_count) {
CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));?

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:216: if
(expected_delayed_callback_count >= 0) {
nit:  Is this needed?

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:266: CHECK(helper_);
optional:  This seems like overkill, since we never set it to NULL (Same goes
for the same check in the next function).

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:318: }
Hmm...Worth the effort of a test with link doctor disabled?  Disabling the link
doctor does disable the DNS probe...  unless the link doctor is disabled due to
lack of API keys.  Don't intend to delay the CL on this, and may not even be
worth doing at all, but thought I'd bring it up.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:344: MessageLoop::current()->Run();
RunLoop is now "right" way of doing this.

I believe in cases like this, the correct way to use the RunLoop is for the
DnsProbeBrowserTest to have a RunLoop as a member variable, and to exit the loop
by calling the RunLoop's Quit function (This makes nested message loops safe. 
Of course, since 99% of browser tests still use MessageLoops, or incorrectly use
RunLoops, doesn't actually work, but....)

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:356: bool
DnsProbeBrowserTest::TitleIs(const std::string& expected) {
Suggest making this return the title instead of a bool.  Then can use EXPECT_EQ,
and get useful output on failure.

Doesn't apply to next function, of course.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:402: EXPECT_EQ(0,
dns_probe_status_count());
Should we do something like "CallDelayedProbeCallbacks(0);"?  Admittedly, if
something goes wrong, we should still send PROBE_STARTED message, but can be a
bit more aggressive now as well.

Alternatively, on destruction, can have the DelayedProbeThing EXPECT that it has
no probes pending (Don't think CHECK is needed there, since we won't hang
because of something going wrong there).  This is probably the better option,
since it adds an extra check to all tests.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:405:
IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, OtherErrorWithoutLinkDoctor) {
Do we care about an error page that doesn't try to use the link doctor?  I don't
think we do, as there's no relevant difference there.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:414:
IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, NxdomainWithLinkDoctor) {
Suggest a comment describing what this particular test tests.  I just think
naming it Nxdomain when the mock DNS returns a valid address is a little
confusing, though I don't disagree with the test name.  Other tests are fine.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:419: EXPECT_TRUE(TitleIs("Mock Link
Doctor"));
We run probes and send messages here, think we should probably still let them
all run (Along with the probe) and make sure the title doesn't change.  Can't
see how it would, but paranoia is your friend.

[mmenke, 2013-06-28 20:43:27]

A couple more nits.  Keep on getting distracted, will not have time to finish up
today.  :(

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_test_util.cc (right):

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_test_util.cc:30:
rules.push_back(MockDnsClientRule("google.com", kTypeA, result));
Think it would be a bit better to have the probe running export this, so we
don't have to worry about mismatches if we ever change this.

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:158: if (dns_probe_status_ !=
chrome_common_net::DNS_PROBE_STARTED) {
Think this is worth a comment.  Something along the lines of "If multiple error
pages load for that one load failure, as is the case with the link doctor, this
will still only run a single probe.  The result will be sent to both pages, if
the probe completes before the second error page commits.  Otherwise, it will
just be sent to the second error page."

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/307001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:107: int probe_count() {
return tab_helper_.running_probe_count(); }
running_probe_count()  (probe_count going from 1 to 0 is just too weird,
otherwise)

https://codereview.chromium.org/13270005/diff/307001/chrome/common/net/net_er...
File chrome/common/net/net_error_info.h (right):

https://codereview.chromium.org/13270005/diff/307001/chrome/common/net/net_er...
chrome/common/net/net_error_info.h:30: DNS_PROBE_FINISHED_UNKNOWN,
Think we should have a comment that the order of the completed status codes
matters for histograms, and other status codes must all appear first.

https://codereview.chromium.org/13270005/diff/307001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/13270005/diff/307001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:150: last_fail_was_dns_error_ &&
last_start_was_error_page_;
nit:  Fix indent.

[mmenke, 2013-06-28 20:46:49]

Oh yea, and what about disabling it before landing?  Seems like that'd be a bit
tricky, due to the renderer-side code, and wanting to have the tests running. 
Could add a command line flag, but that seems a bit much, since we don't want to
keep it around.

[Deprecated (see juliatuttle), 2013-07-01 17:39:55]

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:59: class DelayingDnsProbeService :
public DnsProbeService {
On 2013/06/28 18:06:19, mmenke wrote:
> Think this is worth a comment (Both what it does and why).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:61: DelayingDnsProbeService() :
DnsProbeService() {}
On 2013/06/28 18:06:19, mmenke wrote:
> nit:  " : DnsProbeService()" not needed.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:75: void CallDelayedCallbacks() {
On 2013/06/28 18:06:19, mmenke wrote:
> BUG:  We could have an async call to CallCallbacks pending in DnsProbeService.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:177: 
On 2013/06/28 18:06:19, mmenke wrote:
> nit:  Remove blank line.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:187: IOThread::Globals* globals =
io_thread_->globals();
On 2013/06/28 18:06:19, mmenke wrote:
> optional:  Feels safest to me to just always keep the probe in a scoped_ptr.
> 
> scoped_ptr<DnsProbeService>
> delaying_dns_probe_service(globals->dns_probe_service.release());
> 
> Generally feel like we should avoid calling delete, in favor of just always
> having someone own an object explicitly (The "delete this" below is fine).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:199: MockDnsClientRule::Result
public_result) {
On 2013/06/28 18:06:19, mmenke wrote:
> CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:208: int link_doctor_net_error) {
On 2013/06/28 18:06:19, mmenke wrote:
> CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:213: int
expected_delayed_callback_count) {
On 2013/06/28 18:06:19, mmenke wrote:
> CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:216: if
(expected_delayed_callback_count >= 0) {
On 2013/06/28 18:06:19, mmenke wrote:
> nit:  Is this needed?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:266: CHECK(helper_);
On 2013/06/28 18:06:19, mmenke wrote:
> optional:  This seems like overkill, since we never set it to NULL (Same goes
> for the same check in the next function).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:318: }
On 2013/06/28 18:06:19, mmenke wrote:
> Hmm...Worth the effort of a test with link doctor disabled?  Disabling the
link
> doctor does disable the DNS probe...  unless the link doctor is disabled due
to
> lack of API keys.  Don't intend to delay the CL on this, and may not even be
> worth doing at all, but thought I'd bring it up.

Not in this CL, but sure.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:344: MessageLoop::current()->Run();
On 2013/06/28 18:06:19, mmenke wrote:
> RunLoop is now "right" way of doing this.
> 
> I believe in cases like this, the correct way to use the RunLoop is for the
> DnsProbeBrowserTest to have a RunLoop as a member variable, and to exit the
loop
> by calling the RunLoop's Quit function (This makes nested message loops safe. 
> Of course, since 99% of browser tests still use MessageLoops, or incorrectly
use
> RunLoops, doesn't actually work, but....)

I think I am doing something wrong; I tried sticking a RunLoop in the class and
it segfaulted (!) when I called Run.  (The same thing happened when I did
RunLoop().Run() directly.)

Can we look at this on Monday?

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:356: bool
DnsProbeBrowserTest::TitleIs(const std::string& expected) {
On 2013/06/28 18:06:19, mmenke wrote:
> Suggest making this return the title instead of a bool.  Then can use
EXPECT_EQ,
> and get useful output on failure.
> 
> Doesn't apply to next function, of course.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:402: EXPECT_EQ(0,
dns_probe_status_count());
On 2013/06/28 18:06:19, mmenke wrote:
> Should we do something like "CallDelayedProbeCallbacks(0);"?  Admittedly, if
> something goes wrong, we should still send PROBE_STARTED message, but can be a
> bit more aggressive now as well.
> 
> Alternatively, on destruction, can have the DelayedProbeThing EXPECT that it
has
> no probes pending (Don't think CHECK is needed there, since we won't hang
> because of something going wrong there).  This is probably the better option,
> since it adds an extra check to all tests.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:405:
IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, OtherErrorWithoutLinkDoctor) {
On 2013/06/28 18:06:19, mmenke wrote:
> Do we care about an error page that doesn't try to use the link doctor?  I
don't
> think we do, as there's no relevant difference there.

This is mostly here to ensure that GetErrorStringForDnsProbe isn't erroneously
returning true and mangling all error pages.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:414:
IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, NxdomainWithLinkDoctor) {
On 2013/06/28 18:06:19, mmenke wrote:
> Suggest a comment describing what this particular test tests.  I just think
> naming it Nxdomain when the mock DNS returns a valid address is a little
> confusing, though I don't disagree with the test name.  Other tests are fine.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:419: EXPECT_TRUE(TitleIs("Mock Link
Doctor"));
On 2013/06/28 18:06:19, mmenke wrote:
> We run probes and send messages here, think we should probably still let them
> all run (Along with the probe) and make sure the title doesn't change.  Can't
> see how it would, but paranoia is your friend.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
File chrome/browser/net/dns_probe_test_util.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_test_util.cc:30:
rules.push_back(MockDnsClientRule("google.com", kTypeA, result));
On 2013/06/28 20:43:27, mmenke wrote:
> Think it would be a bit better to have the probe running export this, so we
> don't have to worry about mismatches if we ever change this.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:158: if (dns_probe_status_ !=
chrome_common_net::DNS_PROBE_STARTED) {
On 2013/06/28 20:43:27, mmenke wrote:
> Think this is worth a comment.  Something along the lines of "If multiple
error
> pages load for that one load failure, as is the case with the link doctor,
this
> will still only run a single probe.  The result will be sent to both pages, if
> the probe completes before the second error page commits.  Otherwise, it will
> just be sent to the second error page."

That's not what this check is for; this check is to make sure we don't start a
bunch of probes if we get a bunch of failures (e.g. if the user keeps typing in
new URLs that also fail while we're running probes for earlier ones).

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:107: int probe_count() {
return tab_helper_.running_probe_count(); }
On 2013/06/28 20:43:27, mmenke wrote:
> running_probe_count()  (probe_count going from 1 to 0 is just too weird,
> otherwise)

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/common/net...
File chrome/common/net/net_error_info.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/common/net...
chrome/common/net/net_error_info.h:30: DNS_PROBE_FINISHED_UNKNOWN,
On 2013/06/28 20:43:27, mmenke wrote:
> Think we should have a comment that the order of the completed status codes
> matters for histograms, and other status codes must all appear first.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:150: last_fail_was_dns_error_ &&
last_start_was_error_page_;
On 2013/06/28 20:43:27, mmenke wrote:
> nit:  Fix indent.

Done.

[mmenke, 2013-07-01 18:11:44]

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/307001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:344: MessageLoop::current()->Run();
On 2013/07/01 17:39:55, ttuttle wrote:
> On 2013/06/28 18:06:19, mmenke wrote:
> > RunLoop is now "right" way of doing this.
> > 
> > I believe in cases like this, the correct way to use the RunLoop is for the
> > DnsProbeBrowserTest to have a RunLoop as a member variable, and to exit the
> loop
> > by calling the RunLoop's Quit function (This makes nested message loops
safe. 
> > Of course, since 99% of browser tests still use MessageLoops, or incorrectly
> use
> > RunLoops, doesn't actually work, but....)
> 
> I think I am doing something wrong; I tried sticking a RunLoop in the class
and
> it segfaulted (!) when I called Run.  (The same thing happened when I did
> RunLoop().Run() directly.)
> 
> Can we look at this on Monday?

Sorry, I thought I'd told you, I'm taking a week off.  I'll be back on the 10th
(Next Wednesday).

[mmenke, 2013-07-12 16:14:48]

Renderer comments.  Other than these, I'm happy with it.  I'll get the
(hopefully final) set of browser side comments later today, except perhaps the
browser_tests.

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:89: }
Hmm...we're not testing either this or the stuff we use to generate the new
page's contents...  If is_failed_post_ broke, for instance, we wouldn't catch
it.  Worth testing?

I'm also no a big fan of splitting up with failed load updating between here and
OnFailLoad.

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:138: forwarding_probe_results_ = false;
Any reason not to just set forwarding to false here?

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:138: forwarding_probe_results_ = false;
BUG:  Need "last_probe_status_ = chrome_common_net::DNS_PROBE_POSSIBLE"
somewhere, other than constructor.  Maybe here?

Should have a unit test for this.

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:150: last_fail_was_dns_error_ &&
last_start_was_error_page_;
No known way to set this to false for the link doctor, right?

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:216:
LocalizedError::GetStrings(GetUpdatedError(),
Hmm...between fail and commit, we'd actually display the wrong URL and
last_probe_status_ values.  Doesn't look like there's a simple way to fix it,
without either adding significantly more complexity.  Probably not worth
worrying about, unless there's a quick fix I'm missing, that can't end up with
us getting stuck at a sending probe page.

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:20: EXPECT_UPDATE
Think some comments are needed.  A detailed one for EXPECT_UPDATE.  Other two
groups can just have one short comment each.

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:108: bool update_expected =
false;
Think this is worth a comment.

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:109: int update_count =
page_update_count();
expected_updated_count?

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:145: DCHECK(!update_expected);
DCHECKs shouldn't be used in tests.  Think we're fine just using EXPECT on
these.

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:155:
EXPECT_EQ(last_status_received, displayed_probe_status());
Think there should be a comment about this somewhere (Either here, or where
last_status_received is declared, or where update_expected is declared).

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:168: }
nit:  Can you just share this code with the update_expected case?  I don't think
we get much from separating them out.

[Deprecated (see juliatuttle), 2013-07-12 17:42:48]

PTAL, mmenke.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:89: }
On 2013/07/12 16:14:48, mmenke wrote:
> Hmm...we're not testing either this or the stuff we use to generate the new
> page's contents...  If is_failed_post_ broke, for instance, we wouldn't catch
> it.  Worth testing?

I'm not sure what a good way to test this would be; I guess we could expose
last_error_ and is_failed_post_ somehow and check them in the browser test?

> I'm also no a big fan of splitting up with failed load updating between here
and
> OnFailLoad.

I'm not either, but I'm not sure of another way we can unittest the state
machine without having some way to create an entire mock WebFrame and simulate
it loading.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:138: forwarding_probe_results_ = false;
On 2013/07/12 16:14:48, mmenke wrote:
> BUG:  Need "last_probe_status_ = chrome_common_net::DNS_PROBE_POSSIBLE"
> somewhere, other than constructor.  Maybe here?

We do that in OnFailLoad, when a DNS error occurs.

> Should have a unit test for this.

For which?

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:138: forwarding_probe_results_ = false;
On 2013/07/12 16:14:48, mmenke wrote:
> Any reason not to just set forwarding to false here?

Hmm, I guess not.  If we have two DNS error pages in a row, we don't actually
want to forward results while the second page is committed but not loaded.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:150: last_fail_was_dns_error_ &&
last_start_was_error_page_;
On 2013/07/12 16:14:48, mmenke wrote:
> No known way to set this to false for the link doctor, right?

Nope.  As far as I can tell, the loads are identical for the blank link doctor
page and the real error page.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:216:
LocalizedError::GetStrings(GetUpdatedError(),
On 2013/07/12 16:14:48, mmenke wrote:
> Hmm...between fail and commit, we'd actually display the wrong URL and
> last_probe_status_ values.  Doesn't look like there's a simple way to fix it,
> without either adding significantly more complexity.  Probably not worth
> worrying about, unless there's a quick fix I'm missing, that can't end up with
> us getting stuck at a sending probe page.

I don't think so?

Until the error page commits, we're still showing the previous,
successfully-loaded page.
After an error page commits but before it finishes loading, we'll be showing the
error page (either a blank pre-link-doctor page or the actual error page), with
DNS_PROBE_POSSIBLE strings.
After the error page finishes loading, we'll update it to either
DNS_PROBE_RUNNING or _NOT_RUN or _FINISHED_*.

This sounds no different than any other error page, except for the update?

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:20: EXPECT_UPDATE
On 2013/07/12 16:14:48, mmenke wrote:
> Think some comments are needed.  A detailed one for EXPECT_UPDATE.  Other two
> groups can just have one short comment each.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:108: bool update_expected =
false;
On 2013/07/12 16:14:48, mmenke wrote:
> Think this is worth a comment.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:109: int update_count =
page_update_count();
On 2013/07/12 16:14:48, mmenke wrote:
> expected_updated_count?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:145: DCHECK(!update_expected);
On 2013/07/12 16:14:48, mmenke wrote:
> DCHECKs shouldn't be used in tests.  Think we're fine just using EXPECT on
> these.

EXPECT doesn't make sense to me -- if this DCHECK fails, it means that someone
provided an erroneous test case, not that the test failed.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:155:
EXPECT_EQ(last_status_received, displayed_probe_status());
On 2013/07/12 16:14:48, mmenke wrote:
> Think there should be a comment about this somewhere (Either here, or where
> last_status_received is declared, or where update_expected is declared).

Done.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:168: }
On 2013/07/12 16:14:48, mmenke wrote:
> nit:  Can you just share this code with the update_expected case?  I don't
think
> we get much from separating them out.

Done.

[mmenke, 2013-07-12 18:02:11]

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:89: }
On 2013/07/12 17:42:49, ttuttle wrote:
> On 2013/07/12 16:14:48, mmenke wrote:
> > Hmm...we're not testing either this or the stuff we use to generate the new
> > page's contents...  If is_failed_post_ broke, for instance, we wouldn't
catch
> > it.  Worth testing?
> 
> I'm not sure what a good way to test this would be; I guess we could expose
> last_error_ and is_failed_post_ somehow and check them in the browser test?
> 
> > I'm also no a big fan of splitting up with failed load updating between here
> and
> > OnFailLoad.
> 
> I'm not either, but I'm not sure of another way we can unittest the state
> machine without having some way to create an entire mock WebFrame and simulate
> it loading.

We could just pass error and is_failed_post into OnFailLoad, and then pass them
in for the tests.  UpdateErrorPage could take the updated error and
is_failed_post as inputs.

May make things too ugly, I haven't spent much time thinking about it.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:138: forwarding_probe_results_ = false;
On 2013/07/12 17:42:49, ttuttle wrote:
> On 2013/07/12 16:14:48, mmenke wrote:
> > BUG:  Need "last_probe_status_ = chrome_common_net::DNS_PROBE_POSSIBLE"
> > somewhere, other than constructor.  Maybe here?
> 
> We do that in OnFailLoad, when a DNS error occurs.

Hmm...I even searched the page for that.  How did I miss it?  :(

> > Should have a unit test for this.
> 
> For which?

A test that depends on resetting the probe status.  Your current two load test
gets the probe start status before the load stops, so it would pass even if you
weren't resetting the status.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:145: DCHECK(!update_expected);
On 2013/07/12 17:42:49, ttuttle wrote:
> On 2013/07/12 16:14:48, mmenke wrote:
> > DCHECKs shouldn't be used in tests.  Think we're fine just using EXPECT on
> > these.
> 
> EXPECT doesn't make sense to me -- if this DCHECK fails, it means that someone
> provided an erroneous test case, not that the test failed.

DCHECKs are not run by trybots, by default.  Also, if a DCHECK fails, all other
tests, and whatever information they may have provided, is lost.

[mmenke, 2013-07-12 19:51:21]

Just the browser tests left.  :)

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_service_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:59:
EXPECT_FALSE(callback_called());
nit:  I don't think this EXPECT is needed, since we just called Reset(), and
SetRules doesn't start any probes.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:72: DnsProbeStatus
callback_result() { return callback_result_; }
nit: Both these can be const.  Actually, are they even needed?

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_service_unittest.cc:75: void
ProbeCallback(DnsProbeStatus result) {
Maybe EXPECT_FALSE(callback_called_)?

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:116: }
Shouldn't we clear dns_error_page_committed_ if dns_error_active is false?  If
we have a DNS error and then a non-DNS error, looks like we'd still be sending
info.  This would be benign, since the renderer would ignore the extra messages,
and we wouldn't trigger any extra probes.  Regardless, seems like a bad idea. 
Think we can easily unit test it.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:118: dns_error_active_ = false;
optional:  Could we just clear this in DidStartProvisionalLoadForFrame, if
is_error_page_ is false?  This works and is fine, just wondering if it would be
more consistent to do so.  Sorry if I asked this before.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:151: // If we're being tested, we
don't have a WebContents.
nit:  Some people feel strongly about not using "we" in comments.  Suggest just
using "this" instead.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:40: running_probe_count_++;
Couldn't this just be a bool that we can assert is false here?

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:99: bogus_error_description,
nit:  Can we just pass in "string16()" instead?

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:155:
StartProvisionalLoad(MAIN_FRAME, ERROR_PAGE);
Think it's worth mentioning double error page loads like this happen in the case
of the link doctor being enabled.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:225: // Send result even if
we've started a new page load; the error page is still
nit:  --we, per other comment.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:320:
TEST_F(NetErrorTabHelperTest, MultipleProbesWithErrorPage) {
nit:  Think the title could be clearer.  "TwoSequentialDnsErrorsWithProbes",
maybe?

https://codereview.chromium.org/13270005/diff/351001/chrome/common/net/net_er...
File chrome/common/net/net_error_info.h (right):

https://codereview.chromium.org/13270005/diff/351001/chrome/common/net/net_er...
chrome/common/net/net_error_info.h:36: DNS_PROBE_FINISHED_UNKNOWN,
optional:  Think DNS_PROBE_INCONCLUSIVE may be clearer.  At least I'm always
left scratching my head when I see this one.

[mmenke, 2013-07-12 20:08:59]

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:84: int delayed_probe_count() const
{ return delayed_probes_.size(); }
Suggest verifying this and StartDelayedProbes are called on the IO thread,
mostly for documentation purposes.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:96: class
BrokenLinkDoctorProtocolHandler
Since this cal also handle non-broken link doctors, may just want to call it
LinkDoctorProtocolHandler.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:253: std::list<DnsProbeStatus>
dns_probe_statuses_;
Think it's worth commenting on this one.

https://codereview.chromium.org/13270005/diff/351001/chrome/browser/net/dns_p...
chrome/browser/net/dns_probe_browsertest.cc:388: 
optional nit:  Think this linebreak makes more sense after
"dns_probe_status_count_++;"

[Deprecated (see juliatuttle), 2013-07-12 20:55:54]

PTAL, mmenke.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:89: }
On 2013/07/12 18:02:12, mmenke wrote:
> On 2013/07/12 17:42:49, ttuttle wrote:
> > On 2013/07/12 16:14:48, mmenke wrote:
> > > Hmm...we're not testing either this or the stuff we use to generate the
new
> > > page's contents...  If is_failed_post_ broke, for instance, we wouldn't
> catch
> > > it.  Worth testing?
> > 
> > I'm not sure what a good way to test this would be; I guess we could expose
> > last_error_ and is_failed_post_ somehow and check them in the browser test?
> > 
> > > I'm also no a big fan of splitting up with failed load updating between
here
> > and
> > > OnFailLoad.
> > 
> > I'm not either, but I'm not sure of another way we can unittest the state
> > machine without having some way to create an entire mock WebFrame and
simulate
> > it loading.
> 
> We could just pass error and is_failed_post into OnFailLoad, and then pass
them
> in for the tests.  UpdateErrorPage could take the updated error and
> is_failed_post as inputs.
> 
> May make things too ugly, I haven't spent much time thinking about it.

That seems ugly, and it still wouldn't check the actual logic that goes from a
WebFrame to is_failed_post.  I'm not as worried that we'd overwrite
is_failed_post or fail to record it properly.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper.cc:138: forwarding_probe_results_ = false;
On 2013/07/12 18:02:12, mmenke wrote:
> On 2013/07/12 17:42:49, ttuttle wrote:
> > On 2013/07/12 16:14:48, mmenke wrote:
> > > BUG:  Need "last_probe_status_ = chrome_common_net::DNS_PROBE_POSSIBLE"
> > > somewhere, other than constructor.  Maybe here?
> > 
> > We do that in OnFailLoad, when a DNS error occurs.
> 
> Hmm...I even searched the page for that.  How did I miss it?  :(
> 
> > > Should have a unit test for this.
> > 
> > For which?
> 
> A test that depends on resetting the probe status.  Your current two load test
> gets the probe start status before the load stops, so it would pass even if
you
> weren't resetting the status.

Added another test that repeats the "DNS_PROBE_FINISHED after FinishLoad" test
case twice.

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
File chrome/renderer/net/net_error_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/320001/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:145: DCHECK(!update_expected);
On 2013/07/12 18:02:12, mmenke wrote:
> On 2013/07/12 17:42:49, ttuttle wrote:
> > On 2013/07/12 16:14:48, mmenke wrote:
> > > DCHECKs shouldn't be used in tests.  Think we're fine just using EXPECT on
> > > these.
> > 
> > EXPECT doesn't make sense to me -- if this DCHECK fails, it means that
someone
> > provided an erroneous test case, not that the test failed.
> 
> DCHECKs are not run by trybots, by default.  Also, if a DCHECK fails, all
other
> tests, and whatever information they may have provided, is lost.

Alright, changed to an ASSERT.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
File chrome/browser/net/dns_probe_browsertest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:84: int delayed_probe_count() const
{ return delayed_probes_.size(); }
On 2013/07/12 20:09:00, mmenke wrote:
> Suggest verifying this and StartDelayedProbes are called on the IO thread,
> mostly for documentation purposes.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:96: class
BrokenLinkDoctorProtocolHandler
On 2013/07/12 20:09:00, mmenke wrote:
> Since this cal also handle non-broken link doctors, may just want to call it
> LinkDoctorProtocolHandler.

BreakableLinkDoctorProtocolHandler :)

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:253: std::list<DnsProbeStatus>
dns_probe_statuses_;
On 2013/07/12 20:09:00, mmenke wrote:
> Think it's worth commenting on this one.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/dns_probe_browsertest.cc:388: 
On 2013/07/12 20:09:00, mmenke wrote:
> optional nit:  Think this linebreak makes more sense after
> "dns_probe_status_count_++;"

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
File chrome/browser/net/dns_probe_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:59:
EXPECT_FALSE(callback_called());
On 2013/07/12 19:51:22, mmenke wrote:
> nit:  I don't think this EXPECT is needed, since we just called Reset(), and
> SetRules doesn't start any probes.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:72: DnsProbeStatus
callback_result() { return callback_result_; }
On 2013/07/12 19:51:22, mmenke wrote:
> nit: Both these can be const.  Actually, are they even needed?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/dns_probe_service_unittest.cc:75: void
ProbeCallback(DnsProbeStatus result) {
On 2013/07/12 19:51:22, mmenke wrote:
> Maybe EXPECT_FALSE(callback_called_)?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:116: }
On 2013/07/12 19:51:22, mmenke wrote:
> Shouldn't we clear dns_error_page_committed_ if dns_error_active is false?  If
> we have a DNS error and then a non-DNS error, looks like we'd still be sending
> info.  This would be benign, since the renderer would ignore the extra
messages,
> and we wouldn't trigger any extra probes.  Regardless, seems like a bad idea. 
> Think we can easily unit test it.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:118: dns_error_active_ = false;
On 2013/07/12 19:51:22, mmenke wrote:
> optional:  Could we just clear this in DidStartProvisionalLoadForFrame, if
> is_error_page_ is false?  This works and is fine, just wondering if it would
be
> more consistent to do so.  Sorry if I asked this before.

What would it be more consistent with?  Right now all we do in Start is remember
whether it's an error page or not.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:151: // If we're being tested, we
don't have a WebContents.
On 2013/07/12 19:51:22, mmenke wrote:
> nit:  Some people feel strongly about not using "we" in comments.  Suggest
just
> using "this" instead.

Fixed, here and elsewhere.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:40: running_probe_count_++;
On 2013/07/12 19:51:22, mmenke wrote:
> Couldn't this just be a bool that we can assert is false here?

Sure.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:99: bogus_error_description,
On 2013/07/12 19:51:22, mmenke wrote:
> nit:  Can we just pass in "string16()" instead?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:155:
StartProvisionalLoad(MAIN_FRAME, ERROR_PAGE);
On 2013/07/12 19:51:22, mmenke wrote:
> Think it's worth mentioning double error page loads like this happen in the
case
> of the link doctor being enabled.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:225: // Send result even if
we've started a new page load; the error page is still
On 2013/07/12 19:51:22, mmenke wrote:
> nit:  --we, per other comment.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:320:
TEST_F(NetErrorTabHelperTest, MultipleProbesWithErrorPage) {
On 2013/07/12 19:51:22, mmenke wrote:
> nit:  Think the title could be clearer.  "TwoSequentialDnsErrorsWithProbes",
> maybe?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/common/net...
File chrome/common/net/net_error_info.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/common/net...
chrome/common/net/net_error_info.h:36: DNS_PROBE_FINISHED_UNKNOWN,
On 2013/07/12 19:51:22, mmenke wrote:
> optional:  Think DNS_PROBE_INCONCLUSIVE may be clearer.  At least I'm always
> left scratching my head when I see this one.

Done.

[mmenke, 2013-07-12 21:07:48]

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/351001/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:118: dns_error_active_ = false;
On 2013/07/12 20:55:55, ttuttle wrote:
> On 2013/07/12 19:51:22, mmenke wrote:
> > optional:  Could we just clear this in DidStartProvisionalLoadForFrame, if
> > is_error_page_ is false?  This works and is fine, just wondering if it would
> be
> > more consistent to do so.  Sorry if I asked this before.
> 
> What would it be more consistent with?  Right now all we do in Start is
remember
> whether it's an error page or not.

It would be more consistent with the current state of the RenderView.

[mmenke, 2013-07-15 16:33:21]

LGTM

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper.cc (right):

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:89: }
On 2013/07/12 20:55:55, ttuttle wrote:
> On 2013/07/12 18:02:12, mmenke wrote:
> > On 2013/07/12 17:42:49, ttuttle wrote:
> > > On 2013/07/12 16:14:48, mmenke wrote:
> > > > Hmm...we're not testing either this or the stuff we use to generate the
> new
> > > > page's contents...  If is_failed_post_ broke, for instance, we wouldn't
> > catch
> > > > it.  Worth testing?
> > > 
> > > I'm not sure what a good way to test this would be; I guess we could
expose
> > > last_error_ and is_failed_post_ somehow and check them in the browser
test?
> > > 
> > > > I'm also no a big fan of splitting up with failed load updating between
> here
> > > and
> > > > OnFailLoad.
> > > 
> > > I'm not either, but I'm not sure of another way we can unittest the state
> > > machine without having some way to create an entire mock WebFrame and
> simulate
> > > it loading.
> > 
> > We could just pass error and is_failed_post into OnFailLoad, and then pass
> them
> > in for the tests.  UpdateErrorPage could take the updated error and
> > is_failed_post as inputs.
> > 
> > May make things too ugly, I haven't spent much time thinking about it.
> 
> That seems ugly, and it still wouldn't check the actual logic that goes from a
> WebFrame to is_failed_post.  I'm not as worried that we'd overwrite
> is_failed_post or fail to record it properly.

My concern isn't so much that we'd overwrite it, but that we could end up not
setting it in some future update.

https://codereview.chromium.org/13270005/diff/320001/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper.cc:216:
LocalizedError::GetStrings(GetUpdatedError(),
On 2013/07/12 17:42:49, ttuttle wrote:
> On 2013/07/12 16:14:48, mmenke wrote:
> > Hmm...between fail and commit, we'd actually display the wrong URL and
> > last_probe_status_ values.  Doesn't look like there's a simple way to fix
it,
> > without either adding significantly more complexity.  Probably not worth
> > worrying about, unless there's a quick fix I'm missing, that can't end up
with
> > us getting stuck at a sending probe page.
> 
> I don't think so?
> 
> Until the error page commits, we're still showing the previous,
> successfully-loaded page.
> After an error page commits but before it finishes loading, we'll be showing
the
> error page (either a blank pre-link-doctor page or the actual error page),
with
> DNS_PROBE_POSSIBLE strings.
> After the error page finishes loading, we'll update it to either
> DNS_PROBE_RUNNING or _NOT_RUN or _FINISHED_*.
> 
> This sounds no different than any other error page, except for the update?

A page fails with a DNS issue, we commit the page.  User navigates again in the
same frame and we use the same RenderView.  New navigation fails, also with a
DNS error, DidFailProvisionalLoad updates is_failed_post_ and the URL, and we
then get the probe result.

We update the original, committed, page for the failed post using the URL and
post information for the first page load, not the second.

When the second page commits, we update it with the correct information.

https://codereview.chromium.org/13270005/diff/357002/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://codereview.chromium.org/13270005/diff/357002/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper.cc:108: // but // ensures that the
status will make it to the real error page, even
nit:  Remove extra "//"

https://codereview.chromium.org/13270005/diff/357002/chrome/browser/net/net_e...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/357002/chrome/browser/net/net_e...
chrome/browser/net/net_error_tab_helper_unittest.cc:137: // Test full-blown DNS
error page loads.  Note that the helper (can) see two
optional nit: Not sure what meaning "full-blown" adds, also a bit concerned
about non-native speakers not knowing the idiomatic expression.  Maybe just
"full"?

https://codereview.chromium.org/13270005/diff/357002/chrome/renderer/net/net_...
File chrome/renderer/net/net_error_helper_unittest.cc (right):

https://codereview.chromium.org/13270005/diff/357002/chrome/renderer/net/net_...
chrome/renderer/net/net_error_helper_unittest.cc:409: LOAD_FINISH,
EXPECT_UPDATE, STATUS_FINISHED
Hmm...This test, and a bunch of others, skip STATUS_STARTED.  Should we include
it, to more accurately reflect how things actually work?  I don't feel
paritucularly strongly about it, just thought I'd mention it.

[Deprecated (see juliatuttle), 2013-07-15 19:33:37]

https://chromiumcodereview.appspot.com/13270005/diff/357002/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/357002/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper.cc:108: // but // ensures that the
status will make it to the real error page, even
On 2013/07/15 16:33:22, mmenke wrote:
> nit:  Remove extra "//"

Done.

https://chromiumcodereview.appspot.com/13270005/diff/357002/chrome/browser/ne...
File chrome/browser/net/net_error_tab_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/357002/chrome/browser/ne...
chrome/browser/net/net_error_tab_helper_unittest.cc:137: // Test full-blown DNS
error page loads.  Note that the helper (can) see two
On 2013/07/15 16:33:22, mmenke wrote:
> optional nit: Not sure what meaning "full-blown" adds, also a bit concerned
> about non-native speakers not knowing the idiomatic expression.  Maybe just
> "full"?

Done.

https://chromiumcodereview.appspot.com/13270005/diff/357002/chrome/renderer/n...
File chrome/renderer/net/net_error_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/357002/chrome/renderer/n...
chrome/renderer/net/net_error_helper_unittest.cc:409: LOAD_FINISH,
EXPECT_UPDATE, STATUS_FINISHED
On 2013/07/15 16:33:22, mmenke wrote:
> Hmm...This test, and a bunch of others, skip STATUS_STARTED.  Should we
include
> it, to more accurately reflect how things actually work?  I don't feel
> paritucularly strongly about it, just thought I'd mention it.

I do cover cases with STARTED and FINISHED above, but I want to make sure we
work well if we only see a FINISHED (which can happen if the probe result is
cached), even if it's arbitrarily delayed.

[James Hawkins, 2013-07-15 19:52:45]

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/common/loc...
File chrome/common/localized_error.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/common/loc...
chrome/common/localized_error.cc:449: DCHECK(map);
The cases above don't DCHECK; why are we DCHECKing here?

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/common/net...
File chrome/common/net/net_error_info.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/common/net...
chrome/common/net/net_error_info.h:50: const char* DnsProbeStatusToString(int
status);
nit: Document these functions and params.

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:65: virtual void UpdateErrorPage();
What is this an implementation of?

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:79: // This is an odd variable.
Optional nit: Remove this part of the comment; it starts the reading off w/
confusion.

[Deprecated (see juliatuttle), 2013-07-15 20:22:27]

PTAL, jhawkins.

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/common/loc...
File chrome/common/localized_error.cc (right):

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/common/loc...
chrome/common/localized_error.cc:449: DCHECK(map);
On 2013/07/15 19:52:45, James Hawkins wrote:
> The cases above don't DCHECK; why are we DCHECKing here?

I know for sure that it is a mistake if a dns-probe error gets passed into this
that is not in the map, so I want to know about it.  I do not know that for the
existing cases.

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/common/net...
File chrome/common/net/net_error_info.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/common/net...
chrome/common/net/net_error_info.h:50: const char* DnsProbeStatusToString(int
status);
On 2013/07/15 19:52:45, James Hawkins wrote:
> nit: Document these functions and params.

Done.

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:65: virtual void UpdateErrorPage();
On 2013/07/15 19:52:45, James Hawkins wrote:
> What is this an implementation of?

It's not.  UpdateErrorPage is virtual so we can mock it out in a subclass for
unittesting.

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:79: // This is an odd variable.
On 2013/07/15 19:52:45, James Hawkins wrote:
> Optional nit: Remove this part of the comment; it starts the reading off w/
> confusion.

Done.

[James Hawkins, 2013-07-15 20:28:30]

LGTM with request.

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/renderer/n...
File chrome/renderer/net/net_error_helper.h (right):

https://chromiumcodereview.appspot.com/13270005/diff/365002/chrome/renderer/n...
chrome/renderer/net/net_error_helper.h:65: virtual void UpdateErrorPage();
On 2013/07/15 20:22:28, ttuttle wrote:
> On 2013/07/15 19:52:45, James Hawkins wrote:
> > What is this an implementation of?
> 
> It's not.  UpdateErrorPage is virtual so we can mock it out in a subclass for
> unittesting.

Please document that.

[palmer, 2013-07-15 21:17:27]

messages.h LGTM

[commit-bot: I haz the power, 2013-07-15 22:26:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035

[commit-bot: I haz the power, 2013-07-16 02:21:09]

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

[commit-bot: I haz the power, 2013-07-16 02:36:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035

[commit-bot: I haz the power, 2013-07-16 04:17:43]

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

[commit-bot: I haz the power, 2013-07-16 17:08:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035

[commit-bot: I haz the power, 2013-07-16 18:26:34]

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

[commit-bot: I haz the power, 2013-07-16 20:19:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035

[commit-bot: I haz the power, 2013-07-17 00:06:18]

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

[commit-bot: I haz the power, 2013-07-17 00:26:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035

[commit-bot: I haz the power, 2013-07-17 05:23:15]

Change committed as 211950