SuggestionsService: implement SyncServiceObserver

SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

Also remove a bunch of null checks on SuggestionsService - SuggestionsServiceFactory::GetForProfile will never return null.

BUG=591973
Committed: https://crrev.com/16070ce69cfb26481240995705c8a1a206fd829c
Cr-Commit-Position: refs/heads/master@{#380603}

[Marc Treib, 2016-03-07 20:35:24]

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

[Marc Treib, 2016-03-07 20:35:24]

PTAL!

[Marc Treib, 2016-03-07 20:44:14]

https://codereview.chromium.org/1770843002/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (left):

https://codereview.chromium.org/1770843002/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:456: // Suggestions service is
enabled, initiate a query.
I don't think suggestions_service can be null. Certainly the comment was
wrong/misleading, it being non-null doesn't mean it's actually active.

https://codereview.chromium.org/1770843002/diff/40001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1770843002/diff/40001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:772: if (observer_.is_null())
We can get here before the observer is set.

https://codereview.chromium.org/1770843002/diff/40001/chrome/browser/search/s...
File chrome/browser/search/suggestions/suggestions_source.cc (right):

https://codereview.chromium.org/1770843002/diff/40001/chrome/browser/search/s...
chrome/browser/search/suggestions/suggestions_source.cc:125:
suggestions_service->FetchSuggestionsData();
This is kinda silly - we start a network request but don't actually do anything
with the result; it might be there after a page reload. But this preserves the
existing behavior, and for a debugging page it's probably "good enough".

https://codereview.chromium.org/1770843002/diff/40001/chrome/browser/ui/app_l...
File
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc
(right):

https://codereview.chromium.org/1770843002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/suggestions/suggestions_search_provider.cc:48:
suggestions_service_->GetSuggestionsDataFromCache();
Before, this would also start a network request; now it doesn't - it just shows
what you saw the last time on the NTP (unless you signed out in the meantime
etc). I think this is what we want.

[Mathieu, 2016-03-08 14:52:34]

I like it! suggestions/ lgtm with a question

https://codereview.chromium.org/1770843002/diff/80001/components/suggestions/...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/1770843002/diff/80001/components/suggestions/...
components/suggestions/suggestions_service.cc:375:
IssueRequestIfNoneOngoing(BuildSuggestionsURL());
So the first OnStateChanged that is called in the constructor of this class will
always have |callback_list_| empty, so no network request. If there are no
future sync state changes, this may never be called in the lifetime of the
browser? Aren't we worried about stale suggestions?

This is why all calls to get suggestions from cache were accompanied with a
network request. To make sure the next caller would have the updated data. Up to
you if you'd like to change it.

[Marc Treib, 2016-03-08 15:31:17]

https://codereview.chromium.org/1770843002/diff/80001/components/suggestions/...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/1770843002/diff/80001/components/suggestions/...
components/suggestions/suggestions_service.cc:375:
IssueRequestIfNoneOngoing(BuildSuggestionsURL());
On 2016/03/08 14:52:34, Mathieu Perreault wrote:
> So the first OnStateChanged that is called in the constructor of this class
will
> always have |callback_list_| empty, so no network request. If there are no
> future sync state changes, this may never be called in the lifetime of the
> browser? Aren't we worried about stale suggestions?

The main reason I put this in was to avoid lots of calls to /chromesuggestions
that aren't needed, because the SuggestionsService isn't actually used (ie on
desktop so far). Just "on every Chrome startup" amounts to thousands of QPS,
which triggers all sorts of alarms (speaking from personal experience here :D).

> This is why all calls to get suggestions from cache were accompanied with a
> network request. To make sure the next caller would have the updated data. Up
to
> you if you'd like to change it.

IMO when a caller wants fresh data, they should request it. Note that in the
previous system, you'd also get stale data, and there wasn't even a way to wait
for the new data - you'd just have to hope that next time it will be fresher.

[Marc Treib, 2016-03-08 15:37:58]

treib@chromium.org changed reviewers:
+ jennyz@chromium.org, newt@chromium.org, rdevlin.cronin@chromium.org

[Marc Treib, 2016-03-08 15:37:59]

+newt for most_visited_sites.h/cc
+devlin for preferences_private_apitest.cc
+jennyz for suggestions_search_provider.cc

PTAL!

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
(right):

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc:81:
return;
Before, this test was assuming that nobody else would add a SyncServiceObserver.
This CL introduces one that is created on browser startup, so we'd get here
twice, |initialized_state_violation_| would get set to true, and the test would
fail.

[Devlin, 2016-03-08 17:14:59]

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
(right):

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc:81:
return;
On 2016/03/08 15:37:59, Marc Treib wrote:
> Before, this test was assuming that nobody else would add a
SyncServiceObserver.
> This CL introduces one that is created on browser startup, so we'd get here
> twice, |initialized_state_violation_| would get set to true, and the test
would
> fail.

This seems...wrong.  What exactly is this trying to test?  What happens when
there are more observers or clients that call some of these functions?  What if
we end up adding our observer earlier?  This just seems like the wrong pattern
to be testing.

[Marc Treib, 2016-03-08 17:33:22]

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
(right):

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc:81:
return;
On 2016/03/08 17:14:59, Devlin wrote:
> On 2016/03/08 15:37:59, Marc Treib wrote:
> > Before, this test was assuming that nobody else would add a
> SyncServiceObserver.
> > This CL introduces one that is created on browser startup, so we'd get here
> > twice, |initialized_state_violation_| would get set to true, and the test
> would
> > fail.
> 
> This seems...wrong.  What exactly is this trying to test?  What happens when
> there are more observers or clients that call some of these functions?  What
if
> we end up adding our observer earlier?  This just seems like the wrong pattern
> to be testing.

I'm reverse engineering, so take everything I say with a big grain of salt :)

There's two tests below for which this is relevant. One sets sync_initialized to
false, the other doesn't (note how it's initially set to true above). So this is
testing that AddObserver only gets called if IsSyncActive (above) says false.
Which, incidentally, is exactly what happens in
PreferencesPrivateGetSyncCategoriesWithoutPassphraseFunction: It registers
itself as a SyncServiceObserver iff IsSyncActive() is false.

I agree that this test setup is less than optimal, as evidenced by the fact that
it was broken by a totally unrelated change. Still, the new code is IMO slightly
less wrong than the old code.

[newt (away), 2016-03-08 18:58:39]

So, does SuggestionsServiceFactory::GetForProfile(profile_) always return a
non-null value now? That'd be helpful to mention in the CL description to
understand the refactoring in most_visited_sites.cc.

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:252: OnSuggestionsProfileAvailable(
If there are no locally cached results, then OnSuggestionsProfileAvailable()
will call InitiateTopSitesQuery(). Is that what we want, or will that cause
duplicate work?

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:775: if (observer_.is_null())
why this change?

[Devlin, 2016-03-08 20:06:23]

lgtm

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/extensio...
File
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc
(right):

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/preferences_private/preferences_private_apitest.cc:81:
return;
On 2016/03/08 17:33:22, Marc Treib wrote:
> On 2016/03/08 17:14:59, Devlin wrote:
> > On 2016/03/08 15:37:59, Marc Treib wrote:
> > > Before, this test was assuming that nobody else would add a
> > SyncServiceObserver.
> > > This CL introduces one that is created on browser startup, so we'd get
here
> > > twice, |initialized_state_violation_| would get set to true, and the test
> > would
> > > fail.
> > 
> > This seems...wrong.  What exactly is this trying to test?  What happens when
> > there are more observers or clients that call some of these functions?  What
> if
> > we end up adding our observer earlier?  This just seems like the wrong
pattern
> > to be testing.
> 
> I'm reverse engineering, so take everything I say with a big grain of salt :)
> 
> There's two tests below for which this is relevant. One sets sync_initialized
to
> false, the other doesn't (note how it's initially set to true above). So this
is
> testing that AddObserver only gets called if IsSyncActive (above) says false.
> Which, incidentally, is exactly what happens in
> PreferencesPrivateGetSyncCategoriesWithoutPassphraseFunction: It registers
> itself as a SyncServiceObserver iff IsSyncActive() is false.
> 
> I agree that this test setup is less than optimal, as evidenced by the fact
that
> it was broken by a totally unrelated change. Still, the new code is IMO
slightly
> less wrong than the old code.

I still think that it's not testing anything useful, and, judging by the fact
that it's making you update the test and me wonder about it, it makes me think
we should just delete it.  I guess we can hold off on that for now and just go
with this, even though I think it's silly.  (You're right that it's no worse
than what was there.)

Relatedly, do we even need this API?  Maybe we can delete it all! :)

[Marc Treib, 2016-03-09 09:54:41]

Description was changed from

==========
SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

BUG=591973
==========

to

==========
SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

Also remove a bunch of null checks on SuggestionsService - it's never null.

BUG=591973
==========

[Marc Treib, 2016-03-09 10:02:10]

Description was changed from

==========
SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

Also remove a bunch of null checks on SuggestionsService - it's never null.

BUG=591973
==========

to

==========
SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

Also remove a bunch of null checks on SuggestionsService -
SuggestionsServiceFactory::GetForProfile will never return null.

BUG=591973
==========

[Marc Treib, 2016-03-09 10:03:49]

SuggestionsServiceFactory::GetForProfile(profile_) would never return null, even
before this CL. Sorry for not mentioning this before; I've updated the CL
description now.
(I don't know why the null checks were there in the first place, I'm guessing it
could actually be null at some point in the past?)

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/android/...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:252: OnSuggestionsProfileAvailable(
On 2016/03/08 18:58:39, newt wrote:
> If there are no locally cached results, then OnSuggestionsProfileAvailable()
> will call InitiateTopSitesQuery(). Is that what we want, or will that cause
> duplicate work?

Yup, that is what we want, and also the previous behavior: If there are no
results from SuggestionsService (yet), then we'll show TopSites instead in the
meantime.

https://codereview.chromium.org/1770843002/diff/80001/chrome/browser/android/...
chrome/browser/android/most_visited_sites.cc:775: if (observer_.is_null())
On 2016/03/08 18:58:39, newt wrote:
> why this change?

We can now be called back with results, and hence try to notify the observer,
before the observer is set from the Java side.
(Actually, it looks to me that this could already happen before, but apparently
it never did...)

[Marc Treib, 2016-03-09 13:04:20]

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

[Marc Treib, 2016-03-09 13:04:21]

+stevenjb for suggestions_search_provider.cc, since jennyz seems to be inactive.
PTAL, thanks!

[newt (away), 2016-03-09 18:12:03]

most_visited_sites lgtm

[newt (away), 2016-03-09 18:12:58]

On 2016/03/09 10:03:49, Marc Treib wrote:
> SuggestionsServiceFactory::GetForProfile(profile_) would never return null,
even
> before this CL. Sorry for not mentioning this before; I've updated the CL
> description now.
> (I don't know why the null checks were there in the first place, I'm guessing
it
> could actually be null at some point in the past?)

Yeah, when it was first added, it could be null if the suggestions service was
disabled. It seems that's not the case anymore.

[stevenjb, 2016-03-09 18:27:06]

lgtm

[Marc Treib, 2016-03-10 09:22:17]

treib@chromium.org changed reviewers:
+ pavely@chromium.org, sdefresne@chromium.org

[Marc Treib, 2016-03-10 09:22:18]

+pavely for new dependency from components/suggestions to components/sync_driver

+sdefresne for components/suggestions.gypi. (We should probably have an entry in
components/OWNERS for this file, so we don't need to bother a top-level
components owner for this kind of thing. I'll send that out as a separate CL.)

[sdefresne, 2016-03-10 10:14:50]

components/suggestions.gypi lgtm

[sdefresne, 2016-03-10 10:15:21]

... provided that the bots are happy.

[Marc Treib, 2016-03-10 10:21:28]

On 2016/03/10 10:15:21, sdefresne wrote:
> ... provided that the bots are happy.

They are, see Patch Set 5 :)
(The failed presubmit check here is just because of missing owners)
Thanks!

[sdefresne, 2016-03-10 10:27:33]

On 2016/03/10 at 10:21:28, treib wrote:
> On 2016/03/10 10:15:21, sdefresne wrote:
> > ... provided that the bots are happy.
> 
> They are, see Patch Set 5 :)
> (The failed presubmit check here is just because of missing owners)
> Thanks!

Well, patchset 5 was only run against two bots, there's a lot more!

[pavely, 2016-03-10 23:58:49]

lgtm

[Marc Treib, 2016-03-11 08:54:42]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-03-11 08:54:43]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
rdevlin.cronin@chromium.org, newt@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/1770843002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-11 08:55:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1770843002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1770843002/100001

[commit-bot: I haz the power, 2016-03-11 09:05:12]

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

[commit-bot: I haz the power, 2016-03-11 09:05:14]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Marc Treib, 2016-03-11 10:39:42]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-03-11 10:39:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, pavely@chromium.org, newt@chromium.org,
mathp@chromium.org, stevenjb@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/1770843002/#ps120001
(title: "iOS")

[commit-bot: I haz the power, 2016-03-11 10:40:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1770843002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1770843002/120001

[sdefresne, 2016-03-11 10:51:00]

ios/ lgtm, but if you had sent the CL to all bots, you could have caught this
earlier.

[Marc Treib, 2016-03-11 10:58:09]

On 2016/03/11 10:51:00, sdefresne wrote:
> ios/ lgtm, but if you had sent the CL to all bots, you could have caught this
> earlier.

Thanks, and sorry for the unnecessary noise!
That was a misguided attempt at saving resources...

[commit-bot: I haz the power, 2016-03-11 11:57:49]

Description was changed from

==========
SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

Also remove a bunch of null checks on SuggestionsService -
SuggestionsServiceFactory::GetForProfile will never return null.

BUG=591973
==========

to

==========
SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

Also remove a bunch of null checks on SuggestionsService -
SuggestionsServiceFactory::GetForProfile will never return null.

BUG=591973
==========

[commit-bot: I haz the power, 2016-03-11 11:57:51]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-11 11:59:05]

Description was changed from

==========
SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

Also remove a bunch of null checks on SuggestionsService -
SuggestionsServiceFactory::GetForProfile will never return null.

BUG=591973
==========

to

==========
SuggestionsService: implement SyncServiceObserver
so that its clients don't have to.

Also remove a bunch of null checks on SuggestionsService -
SuggestionsServiceFactory::GetForProfile will never return null.

BUG=591973

Committed: https://crrev.com/16070ce69cfb26481240995705c8a1a206fd829c
Cr-Commit-Position: refs/heads/master@{#380603}
==========

[commit-bot: I haz the power, 2016-03-11 11:59:07]

Patchset 7 (id:??) landed as
https://crrev.com/16070ce69cfb26481240995705c8a1a206fd829c
Cr-Commit-Position: refs/heads/master@{#380603}