Refactor SearchGeolocationService to make it testable

Refactor SearchGeolocationService to make it testable

This refactors SearchGeolocationService to make it testable. Search engine
related functionality, which cannot easily be mocked out in unittests, is moved
into a delegate class.

A couple of other minor fixes are made:
-An IsOffTheRecord check is made into a DCHECK since the class should never be
constructed for an off the record profile.
-The CCTLD is no longer stored because it was never used.
-An extra DCHECK is added in SetDSEGeolocationSetting to ensure the setting is
only changed when Google is the current search engine.

BUG=674398
Review-Url: https://codereview.chromium.org/2637953003
Cr-Commit-Position: refs/heads/master@{#445304}
Committed: https://chromium.googlesource.com/chromium/src/+/f34fbb504afba2fa86f1dd7bbfd90e0134cbe639

[raymes, 2017-01-17 22:31:51]

Description was changed from

==========
Refactor SearchGeolocationService to make it testable

This refactors SearchGeolocationService to make a testable. Search engine
related functionality, which cannot easily be mocked out in unittests, is moved
into a delegate class.

A couple of other minor fixes are made:
-Previously the pref was never being registered. It is now registered in the
service factory.
-An IsOffTheRecord check is made into a DCHECK since the class should never be
constructed for an off the record profile.
-The CCTLD is no longer stored because it was never used.
-An extra DCHECK is added in SetDSEGeolocationSetting to ensure the setting is
only changed when Google is the current search engine.

BUG=674398
==========

to

==========
Refactor SearchGeolocationService to make it testable

This refactors SearchGeolocationService to make a testable. Search engine
related functionality, which cannot easily be mocked out in unittests, is moved
into a delegate class.

A couple of other minor fixes are made:
-An IsOffTheRecord check is made into a DCHECK since the class should never be
constructed for an off the record profile.
-The CCTLD is no longer stored because it was never used.
-An extra DCHECK is added in SetDSEGeolocationSetting to ensure the setting is
only changed when Google is the current search engine.

BUG=674398
==========

[raymes, 2017-01-17 22:32:11]

Description was changed from

==========
Refactor SearchGeolocationService to make it testable

This refactors SearchGeolocationService to make a testable. Search engine
related functionality, which cannot easily be mocked out in unittests, is moved
into a delegate class.

A couple of other minor fixes are made:
-An IsOffTheRecord check is made into a DCHECK since the class should never be
constructed for an off the record profile.
-The CCTLD is no longer stored because it was never used.
-An extra DCHECK is added in SetDSEGeolocationSetting to ensure the setting is
only changed when Google is the current search engine.

BUG=674398
==========

to

==========
Refactor SearchGeolocationService to make it testable

This refactors SearchGeolocationService to make it testable. Search engine
related functionality, which cannot easily be mocked out in unittests, is moved
into a delegate class.

A couple of other minor fixes are made:
-An IsOffTheRecord check is made into a DCHECK since the class should never be
constructed for an off the record profile.
-The CCTLD is no longer stored because it was never used.
-An extra DCHECK is added in SetDSEGeolocationSetting to ensure the setting is
only changed when Google is the current search engine.

BUG=674398
==========

[raymes, 2017-01-17 22:41:06]

raymes@chromium.org changed reviewers:
+ dominickn@chromium.org

[raymes, 2017-01-17 22:41:07]

Hey Dom, I know you don't have any context on this stuff but could you just do a
pass anyway before I send to an owner? Thanks!

[dominickn, 2017-01-18 00:11:18]

lgtm % nits

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.cc
(right):

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:67:
return url::Origin(origin_url);
Nit: this seems to be a much more common pattern in non-test code

return url::Origin(
    GURL(UIThreadSearchTermsData(profile_).GoogleBaseURLValue()));

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:155:
DCHECK(!profile_->IsOffTheRecord());
Can this DCHECK be made in the constructor? The off-the-record nature of the
profile should stay constant throughout its lifetime right?

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.h
(right):

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:8:
#include "base/callback.h"
Nit: you can just use callback_forward.h here I think, with callback.h included
in the cc file.

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:43: //
Returns the origin of the current CCTLD of the default search engine. If
"Returns the origin of the current Google CCTLD if Google is the default search
engine. Otherwise returns a unique Origin."

?

[raymes, 2017-01-18 04:07:24]

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.cc
(right):

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:67:
return url::Origin(origin_url);
On 2017/01/18 00:11:18, dominickn wrote:
> Nit: this seems to be a much more common pattern in non-test code
> 
> return url::Origin(
>     GURL(UIThreadSearchTermsData(profile_).GoogleBaseURLValue()));

Done.

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.cc:155:
DCHECK(!profile_->IsOffTheRecord());
On 2017/01/18 00:11:18, dominickn wrote:
> Can this DCHECK be made in the constructor? The off-the-record nature of the
> profile should stay constant throughout its lifetime right?

Done.

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
File chrome/browser/android/search_geolocation/search_geolocation_service.h
(right):

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:8:
#include "base/callback.h"
On 2017/01/18 00:11:18, dominickn wrote:
> Nit: you can just use callback_forward.h here I think, with callback.h
included
> in the cc file.

Done.

https://codereview.chromium.org/2637953003/diff/40001/chrome/browser/android/...
chrome/browser/android/search_geolocation/search_geolocation_service.h:43: //
Returns the origin of the current CCTLD of the default search engine. If
On 2017/01/18 00:11:18, dominickn wrote:
> "Returns the origin of the current Google CCTLD if Google is the default
search
> engine. Otherwise returns a unique Origin."
> 
> ?

Done.

[raymes, 2017-01-18 04:08:58]

raymes@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[raymes, 2017-01-18 04:08:58]

+dfalcantara for OWNERS

[raymes, 2017-01-18 04:09:13]

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

[commit-bot: I haz the power, 2017-01-18 04:09:29]

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-01-18 04:42:55]

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

[commit-bot: I haz the power, 2017-01-18 04:42:56]

Dry run: This issue passed the CQ dry run.

[gone, 2017-01-19 18:02:05]

rs lgtm

[raymes, 2017-01-22 21:44:30]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-01-22 21:44:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2637953003/#ps60001
(title: "Refactor for test")

[commit-bot: I haz the power, 2017-01-22 21:44:44]

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-01-22 22:40:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485121470853290,
"parent_rev": "728aa8652fcdef20b24213d96eeb7db6a2543748", "commit_rev":
"f34fbb504afba2fa86f1dd7bbfd90e0134cbe639"}

[commit-bot: I haz the power, 2017-01-22 22:40:58]

Description was changed from

==========
Refactor SearchGeolocationService to make it testable

This refactors SearchGeolocationService to make it testable. Search engine
related functionality, which cannot easily be mocked out in unittests, is moved
into a delegate class.

A couple of other minor fixes are made:
-An IsOffTheRecord check is made into a DCHECK since the class should never be
constructed for an off the record profile.
-The CCTLD is no longer stored because it was never used.
-An extra DCHECK is added in SetDSEGeolocationSetting to ensure the setting is
only changed when Google is the current search engine.

BUG=674398
==========

to

==========
Refactor SearchGeolocationService to make it testable

This refactors SearchGeolocationService to make it testable. Search engine
related functionality, which cannot easily be mocked out in unittests, is moved
into a delegate class.

A couple of other minor fixes are made:
-An IsOffTheRecord check is made into a DCHECK since the class should never be
constructed for an off the record profile.
-The CCTLD is no longer stored because it was never used.
-An extra DCHECK is added in SetDSEGeolocationSetting to ensure the setting is
only changed when Google is the current search engine.

BUG=674398

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

[commit-bot: I haz the power, 2017-01-22 22:40:58]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/f34fbb504afba2fa86f1dd7bbfd9...