Switch to using the new Link Doctor API.

Switch to using the new Link Doctor API.

The new API allows Link Doctor results to be integrated into Chrome's
own error pages.

BUG=64832
R=pkasting@chromium.org, thakis@chromium.org, tsepez@chromium.org, ttuttle@chromium.org

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

[mmenke, 2014-02-04 16:43:19]

[+thlx]:  Please review localized_error and neterror changes.  Link Doctor
basically provides suggestions for mistyped URLs in some cases (It's the funky
error page when you go to, say, http://someInvalidUrl/), and we want to
integrate those results into a normal Chrome error page.  I'd love for a cleaner
way to pass along the new suggestions, but I don't see one, happy to hear
suggestions.  Could have localized_error or the Javascript deal with the JSON we
get from the server, but I think both of those would be a little uglier, also
seems like the code to create the query and parse the response should be in the
same file.


[+pkasting]:  Please review google_util changes.  The origin country is needed
for
https://cs.corp.google.com/#piper///depot/google3/linkdoctor/fixurlserver/fix...
for the same purpose as the "TLD param".


[+tsepez]:  Please review render_messages changes.  All the new information was
basically embedded in the old URL, with the following exceptions:

1)  The base URL has changed (It's now http://www.googleapis.com/rpc).  It's
just needed to use a bogus URL in unit tests, and disable the Link Doctor by
passing along an empty URL.
2)  The API key was added.  Currently, it's not available in the renderer
process.  We could change that instead of adding a parameter, though we'd have
to pass the relevant command line flag(s) to the renderer process as well, to do
it correctly.  I'm fine with either option.

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/resour...
File chrome/renderer/resources/neterror.css (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/resour...
chrome/renderer/resources/neterror.css:255:
url('../../app/theme/default_200_percent/common/omnibox_search_button_loupe.png')
2x);
Not sure if this should be renamed so it's not modified in ignorance of this new
use, or if there's some standard procedure to follow when reusing an icon, or
what.

[Deprecated (see juliatuttle), 2014-02-04 17:44:21]

Have some nits.

Also: As a general naming trend, I feel like you should s/Link Doctor/FixURL/
(since that's what the new service is called) and
s/AlternateErrorPage/ErrorPageSuggestions/ or something like that (since it's no
longer loading an entirely different page).

https://codereview.chromium.org/137623011/diff/1670001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8864: +        Do you mean: <ph
name="LINK">&lt;a jsvalues="href:urlCorrection"
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere-spelled-correctly.com</ex></ph>?
Should this be "Did you mean", to match the phrasing on search results pages?

https://codereview.chromium.org/137623011/diff/1670001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8869: +      <message
name="IDS_ERRORPAGES_SUGGESTION_GOOGLE_SEARCH" desc="When a page fails to load,
sometimes we suggest a specific Google search.  This appears jsut above abar
that takes text input.">
typo: jsut above abar -> just above a bar

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:56: // Expands the more box on the
currently displayed error page.
Technically it'd be nicer if you searched for the element by content ("More")
and clicked it, in case the name or id changes, but it's not a huge deal.

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:69: base::RemoveChars(error_string,
"net:", &error_string);
It bothers me that this relies on the fact that the error codes themselves never
contain 'n', 'e', 't', or ':'.  Can you look for and drop "net::" specifically?

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:166: // Navigates back in the history
and waits for |num_navigations| to occur.
GoBack(2) to me reads as "go back twice"; I'd prefer GoBackAndWaitForNavigations
or something like that.

https://codereview.chromium.org/137623011/diff/1670001/chrome/common/localize...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/common/localize...
chrome/common/localized_error.cc:671: // TODO(mmenke):  Investigate showing this
when there are link doctor results.
I wouldn't show it along with link doctor results -- if we can get link doctor
results, it's probably not a local network issue?

https://codereview.chromium.org/137623011/diff/1670001/chrome/common/localize...
File chrome/common/localized_error.h (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/common/localize...
chrome/common/localized_error.h:42: // Prefix to prepend to search terms. 
Search box is only shown if this is
Chrome prompts the user for a search engine at first setup -- can we respect
that here?

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/net/ne...
File chrome/renderer/net/net_error_helper_core.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper_core.cc:178: // Doesn't seem like a good
idea to show these.
What if that's what the user was looking for?  (Is the FixURL server clever
enough to return (soft) porn results only when the original URL indicated that
the user was looking for that?)

[Tom Sepez, 2014-02-04 18:14:20]

Messages LGTM - browser-to-renderer message, right?

[mmenke, 2014-02-04 18:15:37]

On 2014/02/04 18:14:20, Tom Sepez wrote:
> Messages LGTM - browser-to-renderer message, right?

Yea, browser-to-renderer.  Thanks!

[mmenke, 2014-02-04 19:43:56]

Thanks for the suggestions, ttuttle!  I agree that we should update the term
"alternate error page".  I've replaced it with "Link Doctor [suggestions]"
everywhere, except the pref (So we don't lose settings).

About Link Doctor vs FixURL Service:  It seems to actually be "the Link Doctor
FixURL Service".  Since the information link is still "http://go/linkdoctor",
and it lives in google3/linkdoctor/, I think the Link Doctor name is the best
one to use, for searchability.  If you still disagree, I can switch.

https://codereview.chromium.org/137623011/diff/1670001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8864: +        Do you mean: <ph
name="LINK">&lt;a jsvalues="href:urlCorrection"
jscontent="urlCorrectionForDisplay"&gt;&lt;/a&gt;<ex>www.somewhere-spelled-correctly.com</ex></ph>?
On 2014/02/04 17:44:21, ttuttle wrote:
> Should this be "Did you mean", to match the phrasing on search results pages?

Done.

https://codereview.chromium.org/137623011/diff/1670001/chrome/app/generated_r...
chrome/app/generated_resources.grd:8869: +      <message
name="IDS_ERRORPAGES_SUGGESTION_GOOGLE_SEARCH" desc="When a page fails to load,
sometimes we suggest a specific Google search.  This appears jsut above abar
that takes text input.">
On 2014/02/04 17:44:21, ttuttle wrote:
> typo: jsut above abar -> just above a bar

Done

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:56: // Expands the more box on the
currently displayed error page.
On 2014/02/04 17:44:21, ttuttle wrote:
> Technically it'd be nicer if you searched for the element by content ("More")
> and clicked it, in case the name or id changes, but it's not a huge deal.

I'm not sure...Then I'd be depending on an english language string in
generated_resources, rather than on neterror.html.  Tests are by default run in
english, but that still seems a little weird, though we do that elsewhere.

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:69: base::RemoveChars(error_string,
"net:", &error_string);
On 2014/02/04 17:44:21, ttuttle wrote:
> It bothers me that this relies on the fact that the error codes themselves
never
> contain 'n', 'e', 't', or ':'.  Can you look for and drop "net::"
specifically?

I agree that this is weird and ugly, but it's done in 3 other places (Including
in localized_error), so I'd prefer to match them for now.  I'd actually like to
add a function to net/ to exclude the "net::", or add a parameter to
ErrorToString to exclude it in another CL.

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:166: // Navigates back in the history
and waits for |num_navigations| to occur.
On 2014/02/04 17:44:21, ttuttle wrote:
> GoBack(2) to me reads as "go back twice"; I'd prefer
GoBackAndWaitForNavigations
> or something like that.

I agree, done.

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/google/...
chrome/browser/google/google_util.cc:112: return "gb";
Added support for "cat", which seems to be the only other Google TLD without a
CLDR country code.

https://codereview.chromium.org/137623011/diff/1670001/chrome/common/localize...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/common/localize...
chrome/common/localized_error.cc:671: // TODO(mmenke):  Investigate showing this
when there are link doctor results.
On 2014/02/04 17:44:21, ttuttle wrote:
> I wouldn't show it along with link doctor results -- if we can get link doctor
> results, it's probably not a local network issue?

Think you're right, removed comment.

https://codereview.chromium.org/137623011/diff/1670001/chrome/common/localize...
File chrome/common/localized_error.h (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/common/localize...
chrome/common/localized_error.h:42: // Prefix to prepend to search terms. 
Search box is only shown if this is
On 2014/02/04 17:44:21, ttuttle wrote:
> Chrome prompts the user for a search engine at first setup -- can we respect
> that here?

No, we can't, without a fair bit of extra work.  We'd have to support POSTs,
GETs, different search parameter names, filling in multiple places in URLs with
the search parameter, weird google params (Like {google:baseURL}), etc.  The
code to to all that is currently based on the profile, and is exclusive to the
browser process.

We could add search information in yet another format, but then custom set
search engines wouldn't work, and we'd need to include that information for all
search engines we suggest (Which vary by locale).

While I agree that we ideally should do this, I don't think it should block
shutting down the current Link Doctor service.  Also not sure it's something the
networking team should really be worrying about.  May be simplest to just remove
it, if the default search engine isn't Google.

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/net/ne...
File chrome/renderer/net/net_error_helper_core.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper_core.cc:178: // Doesn't seem like a good
idea to show these.
On 2014/02/04 17:44:21, ttuttle wrote:
> What if that's what the user was looking for?  (Is the FixURL server clever
> enough to return (soft) porn results only when the original URL indicated that
> the user was looking for that?)

In my experience, FixURL isn't really that clever.  Since Chrome has no safe
search settings, I think we should just exclude this for now.  If we really show
porn links, should talk to Chris and the UI guys about it.

[Deprecated (see juliatuttle), 2014-02-04 20:20:20]

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:56: // Expands the more box on the
currently displayed error page.
On 2014/02/04 19:43:56, mmenke wrote:
> On 2014/02/04 17:44:21, ttuttle wrote:
> > Technically it'd be nicer if you searched for the element by content
("More")
> > and clicked it, in case the name or id changes, but it's not a huge deal.
> 
> I'm not sure...Then I'd be depending on an english language string in
> generated_resources, rather than on neterror.html.  Tests are by default run
in
> english, but that still seems a little weird, though we do that elsewhere.

Maybe search for it by id then?  I don't like that it's calling straight into
JavaScript to emulate a UI interaction.  (If that doesn't bother you, feel free
to ignore this.)

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:69: base::RemoveChars(error_string,
"net:", &error_string);
On 2014/02/04 19:43:56, mmenke wrote:
> On 2014/02/04 17:44:21, ttuttle wrote:
> > It bothers me that this relies on the fact that the error codes themselves
> never
> > contain 'n', 'e', 't', or ':'.  Can you look for and drop "net::"
> specifically?
> 
> I agree that this is weird and ugly, but it's done in 3 other places
(Including
> in localized_error), so I'd prefer to match them for now.  I'd actually like
to
> add a function to net/ to exclude the "net::", or add a parameter to
> ErrorToString to exclude it in another CL.

Alright.

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/net/ne...
File chrome/renderer/net/net_error_helper_core.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper_core.cc:178: // Doesn't seem like a good
idea to show these.
On 2014/02/04 19:43:56, mmenke wrote:
> On 2014/02/04 17:44:21, ttuttle wrote:
> > What if that's what the user was looking for?  (Is the FixURL server clever
> > enough to return (soft) porn results only when the original URL indicated
that
> > the user was looking for that?)
> 
> In my experience, FixURL isn't really that clever.  Since Chrome has no safe
> search settings, I think we should just exclude this for now.  If we really
show
> porn links, should talk to Chris and the UI guys about it.

Alright.  I'll tinker with it and see how clever it is.  I'd like it if we could
end up typo-correcting all sites if possible.

[mmenke, 2014-02-04 20:31:09]

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
File chrome/browser/errorpage_browsertest.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/browser/errorpa...
chrome/browser/errorpage_browsertest.cc:56: // Expands the more box on the
currently displayed error page.
On 2014/02/04 20:20:20, ttuttle wrote:
> On 2014/02/04 19:43:56, mmenke wrote:
> > On 2014/02/04 17:44:21, ttuttle wrote:
> > > Technically it'd be nicer if you searched for the element by content
> ("More")
> > > and clicked it, in case the name or id changes, but it's not a huge deal.
> > 
> > I'm not sure...Then I'd be depending on an english language string in
> > generated_resources, rather than on neterror.html.  Tests are by default run
> in
> > english, but that still seems a little weird, though we do that elsewhere.
> 
> Maybe search for it by id then?  I don't like that it's calling straight into
> JavaScript to emulate a UI interaction.  (If that doesn't bother you, feel
free
> to ignore this.)

Getting it by its id and simulating a click sounds perfectly reasonable to me. 
Done.

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/net/ne...
File chrome/renderer/net/net_error_helper_core.cc (right):

https://codereview.chromium.org/137623011/diff/1670001/chrome/renderer/net/ne...
chrome/renderer/net/net_error_helper_core.cc:178: // Doesn't seem like a good
idea to show these.
On 2014/02/04 20:20:20, ttuttle wrote:
> On 2014/02/04 19:43:56, mmenke wrote:
> > On 2014/02/04 17:44:21, ttuttle wrote:
> > > What if that's what the user was looking for?  (Is the FixURL server
clever
> > > enough to return (soft) porn results only when the original URL indicated
> that
> > > the user was looking for that?)
> > 
> > In my experience, FixURL isn't really that clever.  Since Chrome has no safe
> > search settings, I think we should just exclude this for now.  If we really
> show
> > porn links, should talk to Chris and the UI guys about it.
> 
> Alright.  I'll tinker with it and see how clever it is.  I'd like it if we
could
> end up typo-correcting all sites if possible.

May also want to see if we're already excluding porn links.  If we're currently
displaying them, then I'd have less concern about removing the checks, since
we'd be matching current behavior.

[Peter Kasting, 2014-02-04 22:49:04]

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (left):

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:79: // 'no' for that.
Why did you remove this comment?

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:103: NOTREACHED();
Do not handle DCHECK failure.  Instead of "if (...) { NOTREACHED(); return",
just do DCHECK(...) and unconditionally move on.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:106: std::string tld =
google_domain.substr(last_dot + 1);
Nit: I'm uneasy calling this the "tld", since it's not, necessarily.  (The old
returned value, by contrast, was a real TLD.)

Perhaps this should be called the "country component".

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:110: // The CLDR code for the UK is "GB".
What does "CLDR" mean?  Explain it or don't use this abbreviation.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:120: GURL url =
GoogleURLTracker::GoogleURL(profile);
Nit: This can just be inlined into the last line of the function.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:122: // The url returned by the tracker
does not include search path.
Nit: search path -> the "/search" path

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
File chrome/browser/google/google_util.h (right):

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.h:44: std::string
GetGoogleCountryCodeCode(Profile* profile);
"Code" is repeated twice.

[mmenke, 2014-02-04 23:52:02]

Thanks for the comments!

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (left):

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:79: // 'no' for that.
On 2014/02/04 22:49:04, Peter Kasting wrote:
> Why did you remove this comment?

Mistake, thanks for catching that.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:103: NOTREACHED();
On 2014/02/04 22:49:04, Peter Kasting wrote:
> Do not handle DCHECK failure.  Instead of "if (...) { NOTREACHED(); return",
> just do DCHECK(...) and unconditionally move on.

Done.  Looks like GoogleURLTracker is smart enough to ensure we don't get
something without any periods in it, but if it weren't, I'd argue that this code
would be the way to go, since we get the URL from a remote Google service, which
we wouldn't want to be able to crash Chrome.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:106: std::string tld =
google_domain.substr(last_dot + 1);
On 2014/02/04 22:49:04, Peter Kasting wrote:
> Nit: I'm uneasy calling this the "tld", since it's not, necessarily.  (The old
> returned value, by contrast, was a real TLD.)
> 
> Perhaps this should be called the "country component".

Done, though per IANA, uk is a TLD:
http://data.iana.org/TLD/tlds-alpha-by-domain.txt.  It's just not a registry
controlled domain.

I also removed the call to "registry_controlled_domains::GetDomainAndRegistry",
since it's no longer needed.  Switched to using the host.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:110: // The CLDR code for the UK is "GB".
On 2014/02/04 22:49:04, Peter Kasting wrote:
> What does "CLDR" mean?  Explain it or don't use this abbreviation.

Done.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:120: GURL url =
GoogleURLTracker::GoogleURL(profile);
On 2014/02/04 22:49:04, Peter Kasting wrote:
> Nit: This can just be inlined into the last line of the function.

Done.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:122: // The url returned by the tracker
does not include search path.
On 2014/02/04 22:49:04, Peter Kasting wrote:
> Nit: search path -> the "/search" path

Done.

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
File chrome/browser/google/google_util.h (right):

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.h:44: std::string
GetGoogleCountryCodeCode(Profile* profile);
On 2014/02/04 22:49:04, Peter Kasting wrote:
> "Code" is repeated twice.

Fixed.

[Peter Kasting, 2014-02-05 00:01:38]

LGTM

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:103: NOTREACHED();
On 2014/02/04 23:52:02, mmenke wrote:
> On 2014/02/04 22:49:04, Peter Kasting wrote:
> > Do not handle DCHECK failure.  Instead of "if (...) { NOTREACHED(); return",
> > just do DCHECK(...) and unconditionally move on.
> 
> Done.  Looks like GoogleURLTracker is smart enough to ensure we don't get
> something without any periods in it, but if it weren't, I'd argue that this
code
> would be the way to go, since we get the URL from a remote Google service,
which
> we wouldn't want to be able to crash Chrome.

If that were the case, we wouldn't want NOTREACHED(), since NOTREACHED() asserts
that it is impossible to reach under any condition.

https://codereview.chromium.org/137623011/diff/2040002/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/2040002/chrome/browser/google/...
chrome/browser/google/google_util.cc:99: const std::string google_domain =
GoogleURLTracker::GoogleURL(profile).host();
Nit: Perhaps |google_hostname| might be less confusing than |google_domain|.

https://codereview.chromium.org/137623011/diff/2040002/chrome/browser/google/...
chrome/browser/google/google_util.cc:120: // The url returned by the tracker
does not include "/search" path.
Nit: include -> include the

[mmenke, 2014-02-05 00:04:55]

Thanks!

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/1960001/chrome/browser/google/...
chrome/browser/google/google_util.cc:103: NOTREACHED();
On 2014/02/05 00:01:39, Peter Kasting wrote:
> On 2014/02/04 23:52:02, mmenke wrote:
> > On 2014/02/04 22:49:04, Peter Kasting wrote:
> > > Do not handle DCHECK failure.  Instead of "if (...) { NOTREACHED();
return",
> > > just do DCHECK(...) and unconditionally move on.
> > 
> > Done.  Looks like GoogleURLTracker is smart enough to ensure we don't get
> > something without any periods in it, but if it weren't, I'd argue that this
> code
> > would be the way to go, since we get the URL from a remote Google service,
> which
> > we wouldn't want to be able to crash Chrome.
> 
> If that were the case, we wouldn't want NOTREACHED(), since NOTREACHED()
asserts
> that it is impossible to reach under any condition.

Makes sense.

https://codereview.chromium.org/137623011/diff/2040002/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/2040002/chrome/browser/google/...
chrome/browser/google/google_util.cc:99: const std::string google_domain =
GoogleURLTracker::GoogleURL(profile).host();
On 2014/02/05 00:01:39, Peter Kasting wrote:
> Nit: Perhaps |google_hostname| might be less confusing than |google_domain|.

Done.

https://codereview.chromium.org/137623011/diff/2040002/chrome/browser/google/...
chrome/browser/google/google_util.cc:120: // The url returned by the tracker
does not include "/search" path.
On 2014/02/05 00:01:39, Peter Kasting wrote:
> Nit: include -> include the

Done.

[mmenke, 2014-02-06 15:21:59]

Ttuttle, thakis:  Ping!

[(unused - use chromium), 2014-02-06 17:37:50]

Which bits am I supposed to look at?


On Thu, Feb 6, 2014 at 7:21 AM, <mmenke@chromium.org> wrote:

> Ttuttle, thakis:  Ping!
>
> https://codereview.chromium.org/137623011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2014-02-06 17:39:48]

On 2014/02/06 17:37:50, thakis wrote:
> Which bits am I supposed to look at?

Sorry, I called you "thlx" in my earlier email.  :)

Please review localized_error and neterror.html changes.  Link Doctor
basically provides suggestions for mistyped URLs in some cases (It's the funky
error page when you go to, say, http://someInvalidUrl/), and we want to
integrate those results into a normal Chrome error page.  I'd love for a cleaner
way to pass along the new suggestions, but I don't see one, happy to hear
suggestions.  Could have localized_error or the Javascript deal with the JSON we
get from the server, but I think both of those would be a little uglier, also
seems like the code to create the query and parse the response should be in the
same file.

[Nico, 2014-02-06 17:58:31]

my 4 files lgtm

Some optional comments below.

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
chrome/common/localized_error.cc:624: if (params) {
This whole chunk feels a bit awkward. Maybe this'd be nicer as

  if (!params)
    params = ErrorPageParams::DefaultParams()

(but then you eithe rneed to return a singleton object; or do a per-value copy,
so I understand why you didn't do that. If this is the best it can be, maybe it
could be in a self-contained helper function?)

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
chrome/common/localized_error.cc:640: error_strings->Set("suggestions",
suggestions);
Oh I see, that's why it's a pointer I suppose. Hm.

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
File chrome/common/localized_error.h (right):

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
chrome/common/localized_error.h:40: scoped_ptr<base::ListValue>
override_suggestions;
optional: could maybe just be a ListValue, not a pointer to one? (as is, this
inherits the DISALLOW_COPY_AND_ASSIGN from scoped_ptr, but that's kind of subtle
and possibly not needed either)

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
chrome/common/localized_error.h:46: // Default search terms.  Ignored if
|search_prefix| is invalid.
You probably mean |search_url| here

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
File chrome/renderer/resources/neterror.css (right):

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
chrome/renderer/resources/neterror.css:220: /* Prevents a space between
controls. */
You can probably get the same by setting margin padding border on the controls
to 0 (not that there's anything wrong with flex)

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
chrome/renderer/resources/neterror.css:255:
url('../../app/theme/default_200_percent/common/omnibox_search_button_loupe.png')
2x);
Hm, is this how we do this these days? I thought the options were:

1. Load image from chrome-theme:// url if it's in the resource package (and then
there's some helper that adds the 1x 2x stuff automatically)

2. Use resources/foo.png and resources/2x/foo.png and then the 2x is added by
the grit inlining script

3. Add webkit-image-set via js, in which case there's a wrapper function
somewhere.

I _think_ we try to not hardcode the 1x 2x (since android has other scale
factors too, etc)

…hm, I was about to say 'this is documented at
http://dev.chromium.org/developers/design-documents/high-dpi-resources ', but
that only lists what you have. cs.chromium.org for "webkit-image-set
file:chrome" also finds lots of similar-looking stuff, so I suppose this is fine
as is.

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
File chrome/renderer/resources/neterror.html (right):

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
chrome/renderer/resources/neterror.html:51: jsvalues=".value:searchTerms" />
hm, maybe the search query param name should be part of search_url and be passed
in? (Part of search_url in the cc file, passed as separate parameter here).
Hardcoding q here seems a bit iffy.

[(unused - use chromium), 2014-02-06 17:58:51]

On Thu, Feb 6, 2014 at 9:39 AM, <mmenke@chromium.org> wrote:

> On 2014/02/06 17:37:50, thakis wrote:
>
>> Which bits am I supposed to look at?
>>
>
> Sorry, I called you "thlx" in my earlier email.  :)
>

Ah, I see, sorry :-)

Done.


>
> Please review localized_error and neterror.html changes.  Link Doctor
>
> basically provides suggestions for mistyped URLs in some cases (It's the
> funky
> error page when you go to, say, http://someInvalidUrl/), and we want to
> integrate those results into a normal Chrome error page.  I'd love for a
> cleaner
> way to pass along the new suggestions, but I don't see one, happy to hear
> suggestions.  Could have localized_error or the Javascript deal with the
> JSON we
> get from the server, but I think both of those would be a little uglier,
> also
> seems like the code to create the query and parse the response should be
> in the
> same file.
>
> https://codereview.chromium.org/137623011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2014-02-06 22:00:32]

Thanks for the feedback!

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
File chrome/common/localized_error.cc (right):

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
chrome/common/localized_error.cc:624: if (params) {
On 2014/02/06 17:58:32, Nico wrote:
> This whole chunk feels a bit awkward. Maybe this'd be nicer as
> 
>   if (!params)
>     params = ErrorPageParams::DefaultParams()
> 
> (but then you eithe rneed to return a singleton object; or do a per-value
copy,
> so I understand why you didn't do that. If this is the best it can be, maybe
it
> could be in a self-contained helper function?)

Done.  Separated the code to add a reload suggestion into a function as well, so
it'll be called on both paths, which now share no code beyond this point.

This seemed cleaner than having the function set use_default_suggestions,
suggest_reload, and suggestions and update error_strings, though it does mean
there are two different places where we call error_strings->Set("suggestions",
suggestions);

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
File chrome/common/localized_error.h (right):

https://codereview.chromium.org/137623011/diff/2590001/chrome/common/localize...
chrome/common/localized_error.h:46: // Default search terms.  Ignored if
|search_prefix| is invalid.
On 2014/02/06 17:58:32, Nico wrote:
> You probably mean |search_url| here

Done.

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
File chrome/renderer/resources/neterror.css (right):

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
chrome/renderer/resources/neterror.css:220: /* Prevents a space between
controls. */
On 2014/02/06 17:58:32, Nico wrote:
> You can probably get the same by setting margin padding border on the controls
> to 0 (not that there's anything wrong with flex)

Tried that, didn't work (Well, didn't set 0 margin on the button, because that
shrinks the button).

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
chrome/renderer/resources/neterror.css:255:
url('../../app/theme/default_200_percent/common/omnibox_search_button_loupe.png')
2x);
On 2014/02/06 17:58:32, Nico wrote:
> Hm, is this how we do this these days? I thought the options were:
> 
> 1. Load image from chrome-theme:// url if it's in the resource package (and
then
> there's some helper that adds the 1x 2x stuff automatically)

Since this is loaded in normal renderer processes, we don't have permissions to
access chrome-theme URLs, so have to embed the images.

> 2. Use resources/foo.png and resources/2x/foo.png and then the 2x is added by
> the grit inlining script

I can't find any examples of this.

> 3. Add webkit-image-set via js, in which case there's a wrapper function
> somewhere.

No permissions to access chrome-theme URLs.

> I _think_ we try to not hardcode the 1x 2x (since android has other scale
> factors too, etc)
> 
> …hm, I was about to say 'this is documented at
> http://dev.chromium.org/developers/design-documents/high-dpi-resources ', but
> that only lists what you have. http://cs.chromium.org for "webkit-image-set
> file:chrome" also finds lots of similar-looking stuff, so I suppose this is
fine
> as is.

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
File chrome/renderer/resources/neterror.html (right):

https://codereview.chromium.org/137623011/diff/2590001/chrome/renderer/resour...
chrome/renderer/resources/neterror.html:51: jsvalues=".value:searchTerms" />
On 2014/02/06 17:58:32, Nico wrote:
> hm, maybe the search query param name should be part of search_url and be
passed
> in? (Part of search_url in the cc file, passed as separate parameter here).
> Hardcoding q here seems a bit iffy.

Done, no argument that it was iffy.  I was worried about stuffing a string with
spaces or non-ASCII characters directly into a string and using it as a URL, but
I suppose we're smart enough to do that correctly.

[mmenke, 2014-02-07 19:56:47]

ttuttle:  Ping!

[Deprecated (see juliatuttle), 2014-02-08 01:51:30]

lgtm with or without nits.

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/google/...
chrome/browser/google/google_util.cc:79: // Google does not yet recognize 'nb'
for Norwegian Bokmal, but it uses
nit: Move this comment one line down, right above the if.  (Also, is there a
central place listing "Google locale vs. real locale" differences?)

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/google/...
chrome/browser/google/google_util.cc:115: if (country_code == "cat")
...and of course nyan.cat is as expected.  I love the Internet.

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:333:
LinkDoctorTabObserver::RegisterProfilePrefs(registry);
nit: Re-alphabetize.

[mmenke, 2014-02-08 01:57:48]

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/google/...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/google/...
chrome/browser/google/google_util.cc:79: // Google does not yet recognize 'nb'
for Norwegian Bokmal, but it uses
On 2014/02/08 01:51:31, ttuttle wrote:
> nit: Move this comment one line down, right above the if.  (Also, is there a
> central place listing "Google locale vs. real locale" differences?)

Done.

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/prefs/b...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/137623011/diff/3330001/chrome/browser/prefs/b...
chrome/browser/prefs/browser_prefs.cc:333:
LinkDoctorTabObserver::RegisterProfilePrefs(registry);
On 2014/02/08 01:51:31, ttuttle wrote:
> nit: Re-alphabetize.

Done, thanks!  Completely missed the comment.

[mmenke, 2014-02-10 15:25:00]

pkasting:  Sorry, I completely forgot about getting an owners review for a
couple files.  Mind also taking a look at the chrome/browser/ui/ changes? 
They're closely related to the google_util ones.  Thanks!

[Peter Kasting, 2014-02-10 23:19:46]

LGTM

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
File chrome/browser/ui/link_doctor_tab_observer.cc (right):

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.cc:76: GURL url;
Nit: Remove this temp; return GURL() or google_util::LinkDoctorBaseURL()
directly below.

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.cc:77: // Disable aLink Doctor
suggestions when in Incognito mode.
Nit: aLink -> Link

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.cc:78: if
(profile_->IsOffTheRecord())
Nit: I'd combine this statement (and its comment) with the next one.

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.cc:97: rfh->GetRoutingID(),
Nit: Indent 4, not 14

You may list more than one arg per line if you wish.

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
File chrome/browser/ui/link_doctor_tab_observer.h (right):

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.h:20: // Per-tab class to implement
Link Doctor functionality.
I'm mildly concerned about using the phrase "link doctor" everywhere.

First, do we explain what that means anywhere?  If so we should probably cite
the relevant header file here; otherwise we should document on this class what
that actually means.  This existing one-line comment doesn't really tell us
anything the class name doesn't already.

Second, do we really want to use this phrase in general?  With the old code we'd
purposefully chosen "alternate error page" because it was both more descriptive
(for laymen) and more generic; LinkDoctor was kind of an internal Google name,
and could be changed, or we could modify Chrome to support other services or
whatever.

I'm not staunchly opposed to this name, I just want to make sure the above
factors have been considered.

[mmenke, 2014-02-10 23:29:02]

Quick response to the major issue you raised, which I think is a perfectly valid
point.

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
File chrome/browser/ui/link_doctor_tab_observer.h (right):

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.h:20: // Per-tab class to implement
Link Doctor functionality.
On 2014/02/10 23:19:46, Peter Kasting wrote:
> I'm mildly concerned about using the phrase "link doctor" everywhere.
> 
> First, do we explain what that means anywhere?  If so we should probably cite
> the relevant header file here; otherwise we should document on this class what
> that actually means.  This existing one-line comment doesn't really tell us
> anything the class name doesn't already.
> 
> Second, do we really want to use this phrase in general?  With the old code
we'd
> purposefully chosen "alternate error page" because it was both more
descriptive
> (for laymen) and more generic; LinkDoctor was kind of an internal Google name,
> and could be changed, or we could modify Chrome to support other services or
> whatever.
> 
> I'm not staunchly opposed to this name, I just want to make sure the above
> factors have been considered.

The "alternate error page" was a page served off of a Google server, in place of
a locally generated error page (See http://invalidurl/ vs http:/0.0.0.0:7/, with
"use a service to resolve navigation errors" enabled in chrome://settings).

This service is being discontinued.  Instead, we download a list of suggested
URLs in JSON format, wrap them with text from chrome, and replace the
suggestions of the current net error pages, so it's no longer really an
"alternate" error page, but rather a normal error page with alternate
suggestions.  Given that we're depending on very specific JSON parameters in the
request and response, I don't think it can even remotely be considered some sort
general error page service

I'll add some text documenting what the service is and does, and I'm certainly
not set on this name by any means, but I don't think "alternate error page" is
accurate any more.

[mmenke, 2014-02-10 23:40:34]

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
File chrome/browser/ui/link_doctor_tab_observer.h (right):

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.h:20: // Per-tab class to implement
Link Doctor functionality.
Maybe call it the NavigationCorrectionObserver, and referring to it in text and
some variables as the "navigation correction service"?  Using "service" in the
class names seems confusing.  That uses similar wording to the configuration
option, and gets away from using the Link Doctor name.

[mmenke, 2014-02-11 16:02:18]

Thanks for the feedback!  I'll plan to do the suggestion rename
(NavigationCorrectionObserver / navigation correction service) later today,
unless I hear otherwise from you.

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
File chrome/browser/ui/link_doctor_tab_observer.cc (right):

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.cc:76: GURL url;
On 2014/02/10 23:19:46, Peter Kasting wrote:
> Nit: Remove this temp; return GURL() or google_util::LinkDoctorBaseURL()
> directly below.

Done.

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.cc:77: // Disable aLink Doctor
suggestions when in Incognito mode.
On 2014/02/10 23:19:46, Peter Kasting wrote:
> Nit: aLink -> Link

Done.

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.cc:78: if
(profile_->IsOffTheRecord())
On 2014/02/10 23:19:46, Peter Kasting wrote:
> Nit: I'd combine this statement (and its comment) with the next one.

Done.

https://codereview.chromium.org/137623011/diff/3690001/chrome/browser/ui/link...
chrome/browser/ui/link_doctor_tab_observer.cc:97: rfh->GetRoutingID(),
On 2014/02/10 23:19:46, Peter Kasting wrote:
> Nit: Indent 4, not 14
> 
> You may list more than one arg per line if you wish.

Done.

[mmenke, 2014-02-26 17:06:39]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-02-26 17:06:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/137623011/4760001

[commit-bot: I haz the power, 2014-02-26 17:07:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 17:07:05]

Failed to apply patch for
chrome/browser/ui/navigation_correction_tab_observer.cc:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 5
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: chrome/browser/ui/navigation_correction_tab_observer.cc
  |===================================================================
  |--- chrome/browser/ui/navigation_correction_tab_observer.cc	(revision 251807)
  |+++ chrome/browser/ui/navigation_correction_tab_observer.cc	(working copy)
  --------------------------
  No file to patch.  Skipping patch.
  5 out of 5 hunks ignored

Patch:   N   chrome/browser/ui/navigation_correction_tab_observer.cc
Index: chrome/browser/ui/navigation_correction_tab_observer.cc
===================================================================
--- chrome/browser/ui/navigation_correction_tab_observer.cc	(revision 251807)
+++ chrome/browser/ui/navigation_correction_tab_observer.cc	(working copy)
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/ui/alternate_error_tab_observer.h"
+#include "chrome/browser/ui/navigation_correction_tab_observer.h"
 
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/chrome_notification_types.h"
@@ -15,14 +15,15 @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
+#include "google_apis/google_api_keys.h"
 
 using content::RenderFrameHost;
 using content::RenderViewHost;
 using content::WebContents;
 
-DEFINE_WEB_CONTENTS_USER_DATA_KEY(AlternateErrorPageTabObserver);
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(NavigationCorrectionTabObserver);
 
-AlternateErrorPageTabObserver::AlternateErrorPageTabObserver(
+NavigationCorrectionTabObserver::NavigationCorrectionTabObserver(
     WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext()))
{
@@ -31,8 +32,7 @@
     pref_change_registrar_.Init(prefs);
     pref_change_registrar_.Add(
         prefs::kAlternateErrorPagesEnabled,
-        base::Bind(&AlternateErrorPageTabObserver::
-                       OnAlternateErrorPagesEnabledChanged,
+        base::Bind(&NavigationCorrectionTabObserver::OnEnabledChanged,
                    base::Unretained(this)));
   }
 
@@ -40,11 +40,11 @@
                  content::Source<Profile>(profile_->GetOriginalProfile()));
 }
 
-AlternateErrorPageTabObserver::~AlternateErrorPageTabObserver() {
+NavigationCorrectionTabObserver::~NavigationCorrectionTabObserver() {
 }
 
 // static
-void AlternateErrorPageTabObserver::RegisterProfilePrefs(
+void NavigationCorrectionTabObserver::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* prefs) {
   prefs->RegisterBooleanPref(prefs::kAlternateErrorPagesEnabled,
                              true,
@@ -54,48 +54,46 @@

////////////////////////////////////////////////////////////////////////////////
 // WebContentsObserver overrides
 
-void AlternateErrorPageTabObserver::RenderViewCreated(
+void NavigationCorrectionTabObserver::RenderViewCreated(
     RenderViewHost* render_view_host) {
-  UpdateAlternateErrorPageURL(render_view_host);
+  UpdateNavigationCorrectionInfo(render_view_host);
 }
 

////////////////////////////////////////////////////////////////////////////////
 // content::NotificationObserver overrides
 
-void AlternateErrorPageTabObserver::Observe(
+void NavigationCorrectionTabObserver::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   DCHECK_EQ(chrome::NOTIFICATION_GOOGLE_URL_UPDATED, type);
-  UpdateAlternateErrorPageURL(web_contents()->GetRenderViewHost());
+  UpdateNavigationCorrectionInfo(web_contents()->GetRenderViewHost());
 }
 

////////////////////////////////////////////////////////////////////////////////
 // Internal helpers
 
-GURL AlternateErrorPageTabObserver::GetAlternateErrorPageURL() const {
-  GURL url;
-  // Disable alternate error pages when in Incognito mode.
-  if (profile_->IsOffTheRecord())
-    return url;
-
-  if (profile_->GetPrefs()->GetBoolean(prefs::kAlternateErrorPagesEnabled)) {
-    url = google_util::LinkDoctorBaseURL();
-    if (!url.is_valid())
-      return url;
-    url = google_util::AppendGoogleLocaleParam(url);
-    url = google_util::AppendGoogleTLDParam(profile_, url);
+GURL NavigationCorrectionTabObserver::GetNavigationCorrectionURL() const {
+  // Disable navigation corrections when the preference is disabled or when in
+  // Incognito mode.
+  if (!profile_->GetPrefs()->GetBoolean(prefs::kAlternateErrorPagesEnabled) ||
+      profile_->IsOffTheRecord()) {
+    return GURL();
   }
-  return url;
+
+  return google_util::LinkDoctorBaseURL();
 }
 
-void AlternateErrorPageTabObserver::OnAlternateErrorPagesEnabledChanged() {
-  UpdateAlternateErrorPageURL(web_contents()->GetRenderViewHost());
+void NavigationCorrectionTabObserver::OnEnabledChanged() {
+  UpdateNavigationCorrectionInfo(web_contents()->GetRenderViewHost());
 }
 
-void AlternateErrorPageTabObserver::UpdateAlternateErrorPageURL(
+void NavigationCorrectionTabObserver::UpdateNavigationCorrectionInfo(
     RenderViewHost* rvh) {
   RenderFrameHost* rfh = rvh->GetMainFrame();
-  rfh->Send(new ChromeViewMsg_SetAltErrorPageURL(
-                rfh->GetRoutingID(), GetAlternateErrorPageURL()));
+  rfh->Send(new ChromeViewMsg_SetNavigationCorrectionInfo(
+      rfh->GetRoutingID(), GetNavigationCorrectionURL(),
+      google_util::GetGoogleLocale(),
+      google_util::GetGoogleCountryCode(profile_), google_apis::GetAPIKey(),
+      google_util::GetGoogleSearchURL(profile_)));
 }

[mmenke, 2014-02-26 17:44:03]

One of the bots doesn't like the rename.

[mmenke, 2014-02-28 20:26:45]

Committed patchset #21 manually as r254207 (presubmit successful).

[mmenke, 2014-02-28 22:10:34]

On 2014/02/28 20:26:45, mmenke wrote:
> Committed patchset #21 manually as r254207 (presubmit successful).

Patch was reverted due to a line I forgot to add back when responding to a
comment, I've now added it back.