Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13270005:
Display DNS probe results. (Closed)
|
Created:
7 years, 8 months ago by Deprecated (see juliatuttle) Modified:
7 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionDisplay 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
Patch Set 1 #
Total comments: 33
Patch Set 2 : Round two. #
Total comments: 42
Patch Set 3 : #Patch Set 4 : Fix unittest #Patch Set 5 : Add part of a browser test #Patch Set 6 : More work on browsertest #Patch Set 7 : Make fewer assumptions and more noise, so we are robust with Link Doctor #Patch Set 8 : More work on browsertests #Patch Set 9 : Fix DnsProbeService unittest #Patch Set 10 : Rebase and fix a couple of things #Patch Set 11 : rebase and fix one thing #Patch Set 12 : Fix dns_probe_service_unittest #Patch Set 13 : Fix FilePath initalization on Windows #
Total comments: 38
Patch Set 14 : Add half-baked DnsProbeRunner for debugging #Patch Set 15 : First try at DnsProbeRunner #Patch Set 16 : rebase #Patch Set 17 : Fix up tests #Patch Set 18 : sigh #Patch Set 19 : unit tests for NetError{,Tab}Helper #Patch Set 20 : unit tests for NetError{Tab,}Helper #Patch Set 21 : ...actually add the unittests #
Total comments: 23
Patch Set 22 : Refactor a bit again (DnsProbeRunner is now tied to a single DnsClient), other fixes #
Total comments: 134
Patch Set 23 : rebase onto lkgr #Patch Set 24 : Tweak things, clean up unit tests #
Total comments: 60
Patch Set 25 : Rework NetErrorHelper state machine a little #
Total comments: 30
Patch Set 26 : Make browser test less flaky #Patch Set 27 : Fix browser test nits #
Total comments: 31
Patch Set 28 : Fix more browser test issues #
Total comments: 27
Patch Set 29 : Fix moar nits #
Total comments: 56
Patch Set 30 : #Patch Set 31 : #
Total comments: 2
Patch Set 32 : Don't break DnsProbe histograms #
Total comments: 45
Patch Set 33 : #
Total comments: 30
Patch Set 34 : rebase #Patch Set 35 : #Patch Set 36 : #
Total comments: 33
Patch Set 37 : #
Total comments: 6
Patch Set 38 : fix style nits #
Total comments: 9
Patch Set 39 : Fix jhawkins' issues #Patch Set 40 : Fix one last nit #Messages
Total messages: 68 (0 generated)
PTAL, mmenke.
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.
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.
I'll get to this this afternoon.
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"><span jscontent="productName"></span><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?
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"><span jscontent="productName"></span><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.
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.
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.
PTAL, mmenke.
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.
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.
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.
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.
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.
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.
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?
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)?
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)?
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.
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.
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.
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).
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.
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).
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...
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?
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
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?
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".
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.
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.
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.
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.
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.
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?
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?
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_"
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.
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.
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.
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.
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.
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.
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.
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).
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.
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.
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.
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.
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_++;"
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.
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.
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.
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.
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.
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.
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.
messages.h LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035
Retried try job too often on win7_aura for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/13270005/372035
Message was sent while issue was closed.
Change committed as 211950 |
