Add autodetection of "intranet" redirection, for ISPs etc. that send typos an...

Add autodetection of "intranet" redirection, for ISPs etc. that send typos and nonexistent addresses to custom pages, and plumb it to the code that puts up infobars when users type in a search that appears to be an intranet address, so we don't show these for erroneous cases.

BUG=31556
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35807

[Peter Kasting, 2010-01-07 00:56:51]

This isn't tested or in fact even compiled yet, but I figured it'd be OK to
start taking a look at it.

[Evan Martin, 2010-01-07 01:14:58]

Seems all right.  I worry about perceived overhead of all of these HTTP
requests.

http://codereview.chromium.org/525079/diff/3001/32
File chrome/browser/first_run.h (right):

http://codereview.chromium.org/525079/diff/3001/32#newcode141
chrome/browser/first_run.h:141: // #1292702).
This bug number is probably not too useful anymore.  :)

http://codereview.chromium.org/525079/diff/3001/37
File chrome/browser/intranet_redirect_detector.cc (right):

http://codereview.chromium.org/525079/diff/3001/37#newcode63
chrome/browser/intranet_redirect_detector.cc:63: void
IntranetRedirectDetector::StartFetchesIfPossible() {
What prevents this from starting multiple times?  Say ::Observe happens after
::FinishSleep

http://codereview.chromium.org/525079/diff/3001/37#newcode74
chrome/browser/intranet_redirect_detector.cc:74: url_string += ('a' +
base::RandInt(0, 'z' - 'a'));
This will randomly generate "obscene" URLs sometimes.  I wonder if people will
notice?  It might be a good idea to only use consonants, or to intersperse some
numbers.  Though even then you can get mthrfckr

http://codereview.chromium.org/525079/diff/3001/37#newcode93
chrome/browser/intranet_redirect_detector.cc:93: // Delete the fetcher on this
function's exit.
This comment perhaps belongs above L97, was confused by it at first.

[Peter Kasting, 2010-01-07 01:31:15]

http://codereview.chromium.org/525079/diff/3001/37
File chrome/browser/intranet_redirect_detector.cc (right):

http://codereview.chromium.org/525079/diff/3001/37#newcode63
chrome/browser/intranet_redirect_detector.cc:63: void
IntranetRedirectDetector::StartFetchesIfPossible() {
On 2010/01/07 01:14:59, Evan Martin wrote:
> What prevents this from starting multiple times?  Say ::Observe happens after
> ::FinishSleep

The default request context only becomes available once, so if Observe() happens
after FinishSleep(), FinishSleep() won't have been able to initiate fetching,
since |request_context_available_| will have been false.

I added a DCHECK().

http://codereview.chromium.org/525079/diff/3001/37#newcode74
chrome/browser/intranet_redirect_detector.cc:74: url_string += ('a' +
base::RandInt(0, 'z' - 'a'));
On 2010/01/07 01:14:59, Evan Martin wrote:
> This will randomly generate "obscene" URLs sometimes.  I wonder if people will
> notice?

It's not as if these are visible in the UI.  And we already send these HEAD
requests for every one-word entry people type in the address bar -- that's much
more likely to generate such things :)

http://codereview.chromium.org/525079/diff/3001/37#newcode93
chrome/browser/intranet_redirect_detector.cc:93: // Delete the fetcher on this
function's exit.
On 2010/01/07 01:14:59, Evan Martin wrote:
> This comment perhaps belongs above L97, was confused by it at first.

But this entire block is only concerned with deleting the fetcher.  The comment
is accurate.

[Peter Kasting, 2010-01-08 03:43:41]

Added a resolver proc for testing to prevent us from sending out real network
requests in that case.  Adding eroman as a reviewer for that stuff.  I also
touched a lot of files because I eliminated the need for a header in one file
and that made it necessary in others.

[eroman, 2010-01-08 04:04:01]

LG on the HostResolverProc side of things.

http://codereview.chromium.org/525079/diff/59/70
File chrome/browser/intranet_redirect_detector.cc (right):

http://codereview.chromium.org/525079/diff/59/70#newcode166
chrome/browser/intranet_redirect_detector.cc:166: return ((host.length() ==
IntranetRedirectDetector::kNumCharsInHostnames) &&
style-nit: i am not a fan of long ternaries...

http://codereview.chromium.org/525079/diff/59/69
File chrome/browser/intranet_redirect_detector.h (right):

http://codereview.chromium.org/525079/diff/59/69#newcode97
chrome/browser/intranet_redirect_detector.h:97: class
IntranetRedirectHostResolverProc : public net::HostResolverProc {
Since this is test-only code, it would be better if it lived in a test-only
file.

http://codereview.chromium.org/525079/diff/59/78
File chrome/test/in_process_browser_test.h (right):

http://codereview.chromium.org/525079/diff/59/78#newcode153
chrome/test/in_process_browser_test.h:153: intranet_redirect_host_resolver_;
Doesn't seem this variable is necessary -- when chaining the other guy will hold
a reference to its previous.

http://codereview.chromium.org/525079/diff/59/77
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/525079/diff/59/77#newcode41
net/base/host_resolver_proc.cc:41: previous_proc_ = (GetLastProc(proc) == this)
? current_previous : proc;
I don't think this test is worth doing, but don't feel too strong about it.

http://codereview.chromium.org/525079/diff/59/77#newcode54
net/base/host_resolver_proc.cc:54: last_proc = last_proc->previous_proc_;
nit: indent by 2 spaces.

[Peter Kasting, 2010-01-08 04:52:01]

http://codereview.chromium.org/525079/diff/59/69
File chrome/browser/intranet_redirect_detector.h (right):

http://codereview.chromium.org/525079/diff/59/69#newcode97
chrome/browser/intranet_redirect_detector.h:97: class
IntranetRedirectHostResolverProc : public net::HostResolverProc {
On 2010/01/08 04:04:01, eroman wrote:
> Since this is test-only code, it would be better if it lived in a test-only
> file.

I don't agree.  Conceptually this is closely tied with the detector itself, and
we have a large number of places in our code where we have some test-only
members or small classes mixed in.  I don't see any harm in putting this here.

http://codereview.chromium.org/525079/diff/59/78
File chrome/test/in_process_browser_test.h (right):

http://codereview.chromium.org/525079/diff/59/78#newcode153
chrome/test/in_process_browser_test.h:153: intranet_redirect_host_resolver_;
On 2010/01/08 04:04:01, eroman wrote:
> Doesn't seem this variable is necessary -- when chaining the other guy will
hold
> a reference to its previous.

Ah, didn't realize that.  That does make things cleaner.

http://codereview.chromium.org/525079/diff/59/77
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/525079/diff/59/77#newcode41
net/base/host_resolver_proc.cc:41: previous_proc_ = (GetLastProc(proc) == this)
? current_previous : proc;
On 2010/01/08 04:04:01, eroman wrote:
> I don't think this test is worth doing, but don't feel too strong about it.

As I mentioned on IM, it turns out we need this (or at least a weak form of it),
unless you want to rewrite default handling to be "implied".

http://codereview.chromium.org/525079/diff/59/77#newcode54
net/base/host_resolver_proc.cc:54: last_proc = last_proc->previous_proc_;
On 2010/01/08 04:04:01, eroman wrote:
> nit: indent by 2 spaces.

Good catch

[eroman, 2010-01-08 05:20:56]

lgtm

> unless you want to rewrite default handling to be "implied".

I will look into doing that as a subsequent iteration.

[Peter Kasting, 2010-01-08 05:22:41]

On 2010/01/08 05:20:56, eroman wrote:
> > unless you want to rewrite default handling to be "implied".
> 
> I will look into doing that as a subsequent iteration.

TBH, it seems OK as-is to me...