Replace manual GoogleURLTracker update request with call to utility function.

Replace manual GoogleURLTracker update request with call to utility function.

This removes the forced Google URL re-check on a profile reset.  The old behavior was added to correct cases where users said "no" to a server request to use a new TLD.  Nowadays we don't prompt about this, so users can't be stuck on "the wrong" server.

BUG=244291
TEST=none

Review-Url: https://codereview.chromium.org/2799083003
Cr-Commit-Position: refs/heads/master@{#462944}
Committed: https://chromium.googlesource.com/chromium/src/+/a078dc8d4c1abc3ff76ce6188ad986686ab24908

[Peter Kasting, 2017-04-06 08:29:47]

pkasting@chromium.org changed reviewers:
+ vasilii@chromium.org

[Peter Kasting, 2017-04-06 08:30:25]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 08:30:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 09:16:54]

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

[commit-bot: I haz the power, 2017-04-06 09:16:55]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-04-06 09:33:18]

https://codereview.chromium.org/2799083003/diff/1/components/search_engines/t...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2799083003/diff/1/components/search_engines/t...
components/search_engines/template_url_service.cc:693:
RequestGoogleURLTrackerServerCheckIfNecessary();
There is a behavior change here. ProfileResetter calls RequestServerCheck(true).
RequestGoogleURLTrackerServerCheckIfNecessary calls RequestServerCheck(false).
Thus, this method is probably a noop.

[Peter Kasting, 2017-04-06 09:43:26]

https://codereview.chromium.org/2799083003/diff/1/components/search_engines/t...
File components/search_engines/template_url_service.cc (right):

https://codereview.chromium.org/2799083003/diff/1/components/search_engines/t...
components/search_engines/template_url_service.cc:693:
RequestGoogleURLTrackerServerCheckIfNecessary();
On 2017/04/06 09:33:17, vasilii wrote:
> There is a behavior change here. ProfileResetter calls
RequestServerCheck(true).
> RequestGoogleURLTrackerServerCheckIfNecessary calls RequestServerCheck(false).
> Thus, this method is probably a noop.

I missed that.

Why did the old code set force=true?  I don't see why that should be needed. 
(And if it's not needed, I can rip that arg out.)

[vasilii, 2017-04-06 11:00:54]

On 2017/04/06 09:43:26, Peter Kasting wrote:
>
https://codereview.chromium.org/2799083003/diff/1/components/search_engines/t...
> File components/search_engines/template_url_service.cc (right):
> 
>
https://codereview.chromium.org/2799083003/diff/1/components/search_engines/t...
> components/search_engines/template_url_service.cc:693:
> RequestGoogleURLTrackerServerCheckIfNecessary();
> On 2017/04/06 09:33:17, vasilii wrote:
> > There is a behavior change here. ProfileResetter calls
> RequestServerCheck(true).
> > RequestGoogleURLTrackerServerCheckIfNecessary calls
RequestServerCheck(false).
> > Thus, this method is probably a noop.
> 
> I missed that.
> 
> Why did the old code set force=true?  I don't see why that should be needed. 
> (And if it's not needed, I can rip that arg out.)

I'd ask the opposite question. Why calling RequestServerCheck(false) or calling
it at all if Chrome is supposed to handle automatically all the interesting
events like network change/default search engine change.
"Reset settings" is an explicit user gesture. If there is a bug in the automated
logic, this would be the only way to reset the Google URL.

[Peter Kasting, 2017-04-06 19:47:29]

(Note, if possible please try to reply on the original codereview comment rather
than with the general messaging interface)

On 2017/04/06 11:00:54, vasilii wrote:
> On 2017/04/06 09:43:26, Peter Kasting wrote:
> >
>
https://codereview.chromium.org/2799083003/diff/1/components/search_engines/t...
> > File components/search_engines/template_url_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/2799083003/diff/1/components/search_engines/t...
> > components/search_engines/template_url_service.cc:693:
> > RequestGoogleURLTrackerServerCheckIfNecessary();
> > On 2017/04/06 09:33:17, vasilii wrote:
> > > There is a behavior change here. ProfileResetter calls
> > RequestServerCheck(true).
> > > RequestGoogleURLTrackerServerCheckIfNecessary calls
> RequestServerCheck(false).
> > > Thus, this method is probably a noop.
> > 
> > I missed that.
> > 
> > Why did the old code set force=true?  I don't see why that should be needed.

> > (And if it's not needed, I can rip that arg out.)
> 
> I'd ask the opposite question. Why calling RequestServerCheck(false) or
calling
> it at all if Chrome is supposed to handle automatically all the interesting
> events like network change/default search engine change.

For user privacy/peace of mind, we don't actually request the Google TLD if no
consumers need it.  RequestServerCheck() means "a consumer would actually use
this value, so we need to go ahead and check it if we haven't".

Specifically, the current consumers are the navigation error fixup logic and the
default search engine.  If a user has the former turned off and the latter set
to Bing, Chrome will never ping this service.  If the user then resets their
settings, toggling the default search engine back to Google, we need to tell the
GoogleURLTracker that it should go ahead and do the ping.  So we need to call
RequestServerCheck().

> "Reset settings" is an explicit user gesture. If there is a bug in the
automated
> logic, this would be the only way to reset the Google URL.

Restarting Chrome would do it.  But more importantly, is that the actual purpose
of this code -- to try and work around possible bugs in GoogleURLTracker?  That
doesn't seem very compelling.

[Peter Kasting, 2017-04-07 02:05:51]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 02:06:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Kasting, 2017-04-07 02:08:10]

The CQ bit was checked by pkasting@chromium.org to run a CQ dry run

[Peter Kasting, 2017-04-07 02:08:29]

On 2017/04/06 19:47:29, Peter Kasting wrote:
> Restarting Chrome would do it.  But more importantly, is that the actual
purpose
> of this code -- to try and work around possible bugs in GoogleURLTracker? 
That
> doesn't seem very compelling.

I looked into the history here.  Looks like this was from
https://bugs.chromium.org/p/chromium/issues/detail?id=244291 .

I'm not sure the motivation to do this is as strong now, since we've removed URL
prompting, and we'll always switch to whatever the server says to use.  (Also,
practically speaking, I believe the server always says to use google.com these
days?)  So there's no way for a user to decline an update and then get stuck on
the wrong domain, which is the main thing we were trying to fix before.

So I think it's OK to remove this.  (Done.)

[commit-bot: I haz the power, 2017-04-07 02:08:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 03:03:09]

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

[commit-bot: I haz the power, 2017-04-07 03:03:10]

Dry run: This issue passed the CQ dry run.

[vasilii, 2017-04-07 09:22:39]

For me the check returns ".google.de"

Implementation LGTM. You need to update the description and set the bug number.

[Peter Kasting, 2017-04-07 18:47:57]

Description was changed from

==========
Replace manual GoogleURLTracker update request with call to utility function.

This should not be a behavior change, just a simplification.

BUG=none
TEST=none
==========

to

==========
Replace manual GoogleURLTracker update request with call to utility function.

This removes the forced Google URL re-check on a profile reset.  The old
behavior was added to correct cases where users said "no" to a server request to
use a new TLD.  Nowadays we don't prompt about this, so users can't be stuck on
"the wrong" server.

BUG=244291
TEST=none
==========

[Peter Kasting, 2017-04-07 18:48:35]

On 2017/04/07 09:22:39, vasilii wrote:
> For me the check returns ".google.de"

Huh, I guess they haven't changed that then.  Interesting.

> Implementation LGTM. You need to update the description and set the bug
number.

I reused the old bug number since it seemed most appropriate.

[Peter Kasting, 2017-04-07 18:48:40]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2017-04-07 18:49:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 18:55:54]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491590920617330,
"parent_rev": "381d77efd050a4ef04eba2ce52f06741ac0d1107", "commit_rev":
"a078dc8d4c1abc3ff76ce6188ad986686ab24908"}

[commit-bot: I haz the power, 2017-04-07 18:56:47]

Description was changed from

==========
Replace manual GoogleURLTracker update request with call to utility function.

This removes the forced Google URL re-check on a profile reset.  The old
behavior was added to correct cases where users said "no" to a server request to
use a new TLD.  Nowadays we don't prompt about this, so users can't be stuck on
"the wrong" server.

BUG=244291
TEST=none
==========

to

==========
Replace manual GoogleURLTracker update request with call to utility function.

This removes the forced Google URL re-check on a profile reset.  The old
behavior was added to correct cases where users said "no" to a server request to
use a new TLD.  Nowadays we don't prompt about this, so users can't be stuck on
"the wrong" server.

BUG=244291
TEST=none

Review-Url: https://codereview.chromium.org/2799083003
Cr-Commit-Position: refs/heads/master@{#462944}
Committed:
https://chromium.googlesource.com/chromium/src/+/a078dc8d4c1abc3ff76ce6188ad9...
==========

[commit-bot: I haz the power, 2017-04-07 18:56:48]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a078dc8d4c1abc3ff76ce6188ad9...