[SuggestionsService] Split SuggestionsService interface from impl

[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to prevent behavioral changes.

BUG=619584
Committed: https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2
Cr-Commit-Position: refs/heads/master@{#439195}

[mastiz, 2016-12-13 12:52:58]

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

[commit-bot: I haz the power, 2016-12-13 12:53:09]

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, 2016-12-13 12:58:03]

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

[commit-bot: I haz the power, 2016-12-13 12:58:03]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[mastiz, 2016-12-13 13:01:27]

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

[commit-bot: I haz the power, 2016-12-13 13:01:59]

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

[mastiz, 2016-12-13 13:03:15]

Description was changed from

==========
[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

BUG=619584
==========

to

==========
[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to avoid any behavioral changes.

BUG=619584
==========

[mastiz, 2016-12-13 13:03:46]

Description was changed from

==========
[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to avoid any behavioral changes.

BUG=619584
==========

to

==========
[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to prevent behavioral changes.

BUG=619584
==========

[commit-bot: I haz the power, 2016-12-13 13:08:37]

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

[commit-bot: I haz the power, 2016-12-13 13:08:38]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mastiz, 2016-12-13 13:20:03]

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

[commit-bot: I haz the power, 2016-12-13 13:20:19]

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, 2016-12-13 13:35:13]

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

[commit-bot: I haz the power, 2016-12-13 13:35:14]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mastiz, 2016-12-13 13:39:14]

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

[commit-bot: I haz the power, 2016-12-13 13:39:24]

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

[mastiz, 2016-12-13 13:39:44]

The CQ bit was unchecked by mastiz@chromium.org

[mastiz, 2016-12-13 13:39:59]

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

[commit-bot: I haz the power, 2016-12-13 13:40:17]

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, 2016-12-13 14:20:03]

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

[commit-bot: I haz the power, 2016-12-13 14:20:04]

Dry run: This issue passed the CQ dry run.

[mastiz, 2016-12-13 14:51:20]

mastiz@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-12-13 15:12:07]

lgtm

https://codereview.chromium.org/2568133005/diff/60001/components/suggestions/...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/2568133005/diff/60001/components/suggestions/...
components/suggestions/suggestions_service.h:100: class SuggestionsServiceImpl :
public SuggestionsService,
Hm, I think nobody expect the factory (and tests) need to know about the Impl.
Would it make sense to move it to a separate file?

[mastiz, 2016-12-13 16:16:58]

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

[commit-bot: I haz the power, 2016-12-13 16:17:17]

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

[mastiz, 2016-12-13 16:18:50]

https://codereview.chromium.org/2568133005/diff/60001/components/suggestions/...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/2568133005/diff/60001/components/suggestions/...
components/suggestions/suggestions_service.h:100: class SuggestionsServiceImpl :
public SuggestionsService,
On 2016/12/13 15:12:07, Marc Treib wrote:
> Hm, I think nobody expect the factory (and tests) need to know about the Impl.
> Would it make sense to move it to a separate file?

Done, moved impl to dedicated file, ptal. git cl format mangled a few lines, I
can revert those if you prefer.

[Marc Treib, 2016-12-13 16:30:27]

https://codereview.chromium.org/2568133005/diff/60001/components/suggestions/...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/2568133005/diff/60001/components/suggestions/...
components/suggestions/suggestions_service.h:100: class SuggestionsServiceImpl :
public SuggestionsService,
On 2016/12/13 16:18:49, mastiz wrote:
> On 2016/12/13 15:12:07, Marc Treib wrote:
> > Hm, I think nobody expect the factory (and tests) need to know about the
Impl.
> > Would it make sense to move it to a separate file?
> 
> Done, moved impl to dedicated file, ptal. git cl format mangled a few lines, I
> can revert those if you prefer.

Nah that's fine, IMO it's generally a Good Thing to have everything git cl
formatted.

[Marc Treib, 2016-12-13 16:33:04]

Ah, and just for completeness' sake, still LGTM

[commit-bot: I haz the power, 2016-12-13 18:21:20]

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

[commit-bot: I haz the power, 2016-12-13 18:21:21]

Dry run: This issue passed the CQ dry run.

[mastiz, 2016-12-14 09:00:54]

mastiz@chromium.org changed reviewers:
+ jennyz@chromium.org, sdefresne@chromium.org

[mastiz, 2016-12-14 09:00:55]

jennyz@chromium.org: Please review changes in chrome/browser/ui

sdefresne@chromium.org: Please review changes in 
ios/

[sdefresne, 2016-12-14 11:56:22]

ios/ lgtm

[mastiz, 2016-12-15 08:26:26]

Friendly ping for jennyz@.

[mastiz, 2016-12-16 08:36:32]

Friendly ping for jennyz@, thanks.

[jennyz, 2016-12-16 18:00:08]

lgtm

[mastiz, 2016-12-16 18:27:15]

The CQ bit was checked by mastiz@chromium.org

[commit-bot: I haz the power, 2016-12-16 18:28:04]

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, 2016-12-16 21:41:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481912835680760,
"parent_rev": "40430cf24bb710e7a76a98da674afe7508f2b470", "commit_rev":
"84725b13715ed7fd584e1258a1bbfe640d42511a"}

[commit-bot: I haz the power, 2016-12-16 21:42:09]

Description was changed from

==========
[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to prevent behavioral changes.

BUG=619584
==========

to

==========
[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to prevent behavioral changes.

BUG=619584

Review-Url: https://codereview.chromium.org/2568133005
==========

[commit-bot: I haz the power, 2016-12-16 21:42:11]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-16 21:45:11]

Description was changed from

==========
[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to prevent behavioral changes.

BUG=619584

Review-Url: https://codereview.chromium.org/2568133005
==========

to

==========
[SuggestionsService] Split SuggestionsService interface from impl

The introduction of an interface allows dependent classes
(primarily MostVisitedSites) to inject doubles in tests, since it's
otherwise hard due to the number of dependencies.

A small signature change is adopted: GetSuggestionsDataFromCache() now
returns base::Optional, which allows clients to distinguish cache
misses from cache containing an empty tileset. Client code has been
updated to prevent behavioral changes.

BUG=619584

Committed: https://crrev.com/d5d7b4a47a878e25b55294cf09751340aaf302e2
Cr-Commit-Position: refs/heads/master@{#439195}
==========

[commit-bot: I haz the power, 2016-12-16 21:45:12]

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