Remove the GoogleURLTracker infobar functionality entirely.

Remove the GoogleURLTracker infobar functionality entirely.

Now instead of prompting the user which search domain they would like when they
perform a search after Chrome has detected a new search domain, we just accept
what searchdomaincheck tells us immediately.

If the domain changes with existing search pages open, we don't reload those
pages on the new domain, as we did with the infobar if you clicked the button to
change domains; doing so could interrupt what the user is doing and will
certainly be surprising.  We just allow future searches to use the new domain.

BUG=421174
TEST=A machine moved among different countries should automatically change search domains to the local domains (on network switch or Chrome restart) without prompting the user

Committed: https://crrev.com/cd7c9d17c3d471e00136542092f167fb60e4244f
Cr-Commit-Position: refs/heads/master@{#310395}

[Peter Kasting, 2014-12-20 02:18:06]

pkasting@chromium.org changed reviewers:
+ brettw@chromium.org, isherman@chromium.org, mmenke@chromium.org

[Peter Kasting, 2014-12-20 02:18:07]

isherman: chrome/browser/google/, components/google/
brettw: Other stuff
mmenke: FYI for chrome/browser/ui/navigation_correction_tab_observer.cc

https://codereview.chromium.org/808253006/diff/20001/chrome/browser/google/go...
File chrome/browser/google/google_url_tracker_factory.cc (right):

https://codereview.chromium.org/808253006/diff/20001/chrome/browser/google/go...
chrome/browser/google/google_url_tracker_factory.cc:48: kLastPromptedGoogleURL);
I think this is the right way to delete this pref??

https://codereview.chromium.org/808253006/diff/20001/chrome/browser/ui/naviga...
File chrome/browser/ui/navigation_correction_tab_observer.cc (right):

https://codereview.chromium.org/808253006/diff/20001/chrome/browser/ui/naviga...
chrome/browser/ui/navigation_correction_tab_observer.cc:47:
google_url_tracker->RequestServerCheck(false);
This is a seeming omission I noticed while cleaning things up; if we're actually
using a Google-based service, and listening for changes to the Google URL, we
should ensure the GoogleURLTracker actually checks for URL updates.

https://codereview.chromium.org/808253006/diff/20001/components/google/core/b...
File components/google/core/browser/google_url_tracker.cc (right):

https://codereview.chromium.org/808253006/diff/20001/components/google/core/b...
components/google/core/browser/google_url_tracker.cc:71: return
callback_list_.Add(cb);
Same code as before, just moved up here to match the .h declaration order.

[brettw, 2014-12-30 03:06:07]

lgtm

[mmenke, 2015-01-05 15:35:51]

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
File chrome/browser/ui/navigation_correction_tab_observer.cc (right):

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
chrome/browser/ui/navigation_correction_tab_observer.cc:44: if
(google_util::IsGoogleDomainUrl(GetNavigationCorrectionURL(),
Think this needs a comment.

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
chrome/browser/ui/navigation_correction_tab_observer.cc:44: if
(google_util::IsGoogleDomainUrl(GetNavigationCorrectionURL(),
It's possible that navigation corrections are disabled.  If multiple
RequestServerCheck(false) calls don't result in lots of redundant checks, could
we move this into NavigationCorrectionTabObserver::GetNavigationCorrectionURL,
and only do it just above the "google_util::LinkDoctorBaseURL" call?

Would still be best if we could cancel the registration if the option is later
disabled, but only ever requesting checks if it's enabled at some point during
chrome's execution seems better than nothing.

[Peter Kasting, 2015-01-05 20:15:48]

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
File chrome/browser/ui/navigation_correction_tab_observer.cc (right):

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
chrome/browser/ui/navigation_correction_tab_observer.cc:44: if
(google_util::IsGoogleDomainUrl(GetNavigationCorrectionURL(),
On 2015/01/05 15:35:51, mmenke wrote:
> Think this needs a comment.

What would you propose?  (The code seems pretty clear to me so I'm not sure what
you'd like clarified.)

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
chrome/browser/ui/navigation_correction_tab_observer.cc:44: if
(google_util::IsGoogleDomainUrl(GetNavigationCorrectionURL(),
On 2015/01/05 15:35:51, mmenke wrote:
> It's possible that navigation corrections are disabled.  If multiple
> RequestServerCheck(false) calls don't result in lots of redundant checks,
could
> we move this into NavigationCorrectionTabObserver::GetNavigationCorrectionURL,
> and only do it just above the "google_util::LinkDoctorBaseURL" call?

They won't result in redundant checks, but moving this there means we could
return the wrong domain from at least the first call to that function, since the
check (obviously) completes asynchronously.

OTOH, I think if someone starts with corrections disabled, then later enables
them, it won't necessarily cause us to do a server check.  So maybe we should
have something like this in both places.

> Would still be best if we could cancel the registration if the option is later
> disabled, but only ever requesting checks if it's enabled at some point during
> chrome's execution seems better than nothing.

Yeah, the chance of turning off corrections + then seeing a network change or
something similar that would actually trigger another check seems low enough to
not really worry about.

[mmenke, 2015-01-05 21:01:53]

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
File chrome/browser/ui/navigation_correction_tab_observer.cc (right):

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
chrome/browser/ui/navigation_correction_tab_observer.cc:44: if
(google_util::IsGoogleDomainUrl(GetNavigationCorrectionURL(),
On 2015/01/05 20:15:47, Peter Kasting wrote:
> On 2015/01/05 15:35:51, mmenke wrote:
> > It's possible that navigation corrections are disabled.  If multiple
> > RequestServerCheck(false) calls don't result in lots of redundant checks,
> could
> > we move this into
NavigationCorrectionTabObserver::GetNavigationCorrectionURL,
> > and only do it just above the "google_util::LinkDoctorBaseURL" call?
> 
> They won't result in redundant checks, but moving this there means we could
> return the wrong domain from at least the first call to that function, since
the
> check (obviously) completes asynchronously.
> 
> OTOH, I think if someone starts with corrections disabled, then later enables
> them, it won't necessarily cause us to do a server check.  So maybe we should
> have something like this in both places.

Something in both places seems reasonable (Or something here and in
NavigationCorrectionTabObserver::OnEnabledChanged, actually).

> > Would still be best if we could cancel the registration if the option is
later
> > disabled, but only ever requesting checks if it's enabled at some point
during
> > chrome's execution seems better than nothing.
> 
> Yeah, the chance of turning off corrections + then seeing a network change or
> something similar that would actually trigger another check seems low enough
to
> not really worry about.

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/ui/naviga...
chrome/browser/ui/navigation_correction_tab_observer.cc:44: if
(google_util::IsGoogleDomainUrl(GetNavigationCorrectionURL(),
On 2015/01/05 20:15:47, Peter Kasting wrote:
> On 2015/01/05 15:35:51, mmenke wrote:
> > Think this needs a comment.
> 
> What would you propose?  (The code seems pretty clear to me so I'm not sure
what
> you'd like clarified.)

I'd suggest "// If navigation corrections will be retrieved from Google, need to
get the correct Google domain to use."

I think neither "google_url_tracker" nor "RequestServerCheck" are particularly
clear.  I would have guessed google url tracker was for separating out full
Google URLs used by Chrome and not Chromium, rather than for figuring out which
Google domain to use, and checking a server sounds more like checking it a
server is servicing requests to me...  Which left me rather confused when
reading this code, until I looked at the comments in the relevant file.

[Ilya Sherman, 2015-01-06 00:36:28]

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/google/DEPS
File chrome/browser/google/DEPS (right):

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/google/DE...
chrome/browser/google/DEPS:5: ],
Is any of this still needed now that the code has been componentized?

[Ilya Sherman, 2015-01-06 00:39:58]

LGTM % nits.

https://codereview.chromium.org/808253006/diff/10039/components/google/core/b...
File components/google/core/browser/google_url_tracker.h (right):

https://codereview.chromium.org/808253006/diff/10039/components/google/core/b...
components/google/core/browser/google_url_tracker.h:27: // change.  The current
values is saved to prefs.
nit: "values is" -> "value is"

https://codereview.chromium.org/808253006/diff/10039/components/google/core/b...
File components/google/core/browser/google_util.cc (right):

https://codereview.chromium.org/808253006/diff/10039/components/google/core/b...
components/google/core/browser/google_util.cc:82: url::Component query(0,
static_cast<int>(str.length())), key, value;
nit: Would checked_cast be better?

[Peter Kasting, 2015-01-06 00:54:26]

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/google/DEPS
File chrome/browser/google/DEPS (right):

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/google/DE...
chrome/browser/google/DEPS:5: ],
On 2015/01/06 00:36:28, Ilya Sherman wrote:
> Is any of this still needed now that the code has been componentized?

Can you clarify what the "now that the code has been componentized" part means? 
AFAIK this file was added when the code was componentized so I'm not sure what's
changed that you're referring to.

I assume that iOS is still using this class and thus that we still want to
protect against use of web_contents.h, but I don't think I added this to begin
with so I'm not really the expert.

https://codereview.chromium.org/808253006/diff/10039/components/google/core/b...
File components/google/core/browser/google_util.cc (right):

https://codereview.chromium.org/808253006/diff/10039/components/google/core/b...
components/google/core/browser/google_util.cc:82: url::Component query(0,
static_cast<int>(str.length())), key, value;
On 2015/01/06 00:39:58, Ilya Sherman wrote:
> nit: Would checked_cast be better?

I don't think it's necessary.  This just makes explicit a conversion that was
previously implicit, and which we have in many other places; if we can ever get
strings > 2 GB in parts of the UI a whole bunch of code is going to break.  From
glancing at ExtractQueryKeyValue() I think the consequence in that case is just
going to be that this doesn't extract anything anyway, which seems like a fine
failure mode.

[Ilya Sherman, 2015-01-06 01:19:35]

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/google/DEPS
File chrome/browser/google/DEPS (right):

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/google/DE...
chrome/browser/google/DEPS:5: ],
On 2015/01/06 00:54:26, Peter Kasting wrote:
> On 2015/01/06 00:36:28, Ilya Sherman wrote:
> > Is any of this still needed now that the code has been componentized?
> 
> Can you clarify what the "now that the code has been componentized" part
means? 
> AFAIK this file was added when the code was componentized so I'm not sure
what's
> changed that you're referring to.
> 
> I assume that iOS is still using this class and thus that we still want to
> protect against use of web_contents.h, but I don't think I added this to begin
> with so I'm not really the expert.

I guess to be clearer, this rule seems to now only apply to the
google_url_tracker_factory.* files, which I don't think are used on iOS.  It
would be good to double-check with the iOS team, but I'm pretty sure that this
entire DEPS file can be removed.

[Peter Kasting, 2015-01-06 01:27:51]

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/google/DEPS
File chrome/browser/google/DEPS (right):

https://codereview.chromium.org/808253006/diff/10039/chrome/browser/google/DE...
chrome/browser/google/DEPS:5: ],
On 2015/01/06 01:19:35, Ilya Sherman wrote:
> On 2015/01/06 00:54:26, Peter Kasting wrote:
> > On 2015/01/06 00:36:28, Ilya Sherman wrote:
> > > Is any of this still needed now that the code has been componentized?
> > 
> > Can you clarify what the "now that the code has been componentized" part
> means? 
> > AFAIK this file was added when the code was componentized so I'm not sure
> what's
> > changed that you're referring to.
> > 
> > I assume that iOS is still using this class and thus that we still want to
> > protect against use of web_contents.h, but I don't think I added this to
begin
> > with so I'm not really the expert.
> 
> I guess to be clearer, this rule seems to now only apply to the
> google_url_tracker_factory.* files, which I don't think are used on iOS.  It
> would be good to double-check with the iOS team, but I'm pretty sure that this
> entire DEPS file can be removed.

Never mind, I was misunderstanding some things (and I think you are too --
"google_url_tracker\." won't match against "google_url_tracker_factory.*").

This file is dead and has been dead since componentization.  I'll remove it.

[Peter Kasting, 2015-01-07 18:59:46]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-01-07 19:00:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/808253006/50001

[commit-bot: I haz the power, 2015-01-07 19:06:43]

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

[commit-bot: I haz the power, 2015-01-07 19:06:45]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Peter Kasting, 2015-01-07 19:42:25]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-01-07 19:43:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/808253006/70001

[commit-bot: I haz the power, 2015-01-07 21:39:51]

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

[commit-bot: I haz the power, 2015-01-07 21:39:53]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Kasting, 2015-01-07 21:49:21]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2015-01-07 21:50:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/808253006/70001

[commit-bot: I haz the power, 2015-01-07 22:40:44]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2015-01-07 22:41:45]

Patchset 5 (id:??) landed as
https://crrev.com/cd7c9d17c3d471e00136542092f167fb60e4244f
Cr-Commit-Position: refs/heads/master@{#310395}