[Most Visited] Check for Sync state when using SuggestionsService.

[Most Visited] Check for Sync state when using SuggestionsService.

BUG=386454
TBR=erikwright
TEST=SuggestionsServiceTest

[Mathieu, 2014-08-15 01:47:43]

Can you have a look? Will polish tomorrow, may not be perfect.

Thanks :)

[manzagop (departed), 2014-08-15 13:01:52]

Cool stuff.

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:205: // as an observer.
Perhaps explain why this is needed: ensuring the state can be changed even if
suggestions are only ever requested when initialization is incomplete (the user
only visits the NTP on startup).

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:206: ProfileSyncService*
profile_sync_service =
I said yesterday I thought this would be null is the user is not signed in. This
seems wrong: it seems it only checks for a commandline flag.

There is however a IsSyncEnabledAndLoggedIn()

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:209: sync_initialized_ =
profile_sync_service->sync_initialized();
sync_initialized_ doesn't need to be a class member. It's always set to a new
value before being used.

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:355: if (sync &&
sync->sync_initialized()) {
We should double check the sync service gets initialized even when sync is
disabled.

https://codereview.chromium.org/473123002/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/473123002/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service.cc:170:
callback.Run(SuggestionsProfile());
Why not clear the cache, then push back requestors and serve from cache?

You need to clear the cache so we "remember" it's disabled. Otherwise you'll
serve the suggestions the next time you're uninitialized.

https://codereview.chromium.org/473123002/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_service_unittest.cc (right):

https://codereview.chromium.org/473123002/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service_unittest.cc:392: 
Test the cache was cleared by calling a second time with true, true?

[Mathieu, 2014-08-20 14:21:32]

Hi, 

I changed the API to an enum, please have a look.

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
File chrome/browser/android/most_visited_sites.cc (right):

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:205: // as an observer.
On 2014/08/15 13:01:52, manzagop wrote:
> Perhaps explain why this is needed: ensuring the state can be changed even if
> suggestions are only ever requested when initialization is incomplete (the
user
> only visits the NTP on startup).

Done.

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:206: ProfileSyncService*
profile_sync_service =
On 2014/08/15 13:01:52, manzagop wrote:
> I said yesterday I thought this would be null is the user is not signed in.
This
> seems wrong: it seems it only checks for a commandline flag.
> 
> There is however a IsSyncEnabledAndLoggedIn()

Done.

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:209: sync_initialized_ =
profile_sync_service->sync_initialized();
On 2014/08/15 13:01:52, manzagop wrote:
> sync_initialized_ doesn't need to be a class member. It's always set to a new
> value before being used.

Done.

https://codereview.chromium.org/473123002/diff/20001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:355: if (sync &&
sync->sync_initialized()) {
On 2014/08/15 13:01:52, manzagop wrote:
> We should double check the sync service gets initialized even when sync is
> disabled.

Done.

https://codereview.chromium.org/473123002/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/473123002/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service.cc:170:
callback.Run(SuggestionsProfile());
On 2014/08/15 13:01:52, manzagop wrote:
> Why not clear the cache, then push back requestors and serve from cache?
> 
> You need to clear the cache so we "remember" it's disabled. Otherwise you'll
> serve the suggestions the next time you're uninitialized.

Done.

https://codereview.chromium.org/473123002/diff/20001/components/suggestions/s...
File components/suggestions/suggestions_service_unittest.cc (right):

https://codereview.chromium.org/473123002/diff/20001/components/suggestions/s...
components/suggestions/suggestions_service_unittest.cc:392: 
On 2014/08/15 13:01:52, manzagop wrote:
> Test the cache was cleared by calling a second time with true, true?

Done.

[manzagop (departed), 2014-08-20 15:43:51]

Good stuff. Main comment is about ongoing requests and the blacklist store.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_service.cc:169:
suggestions_store_->ClearSuggestions();
Also cancel any ongoing request and serve the waiting requestors? 

Well... not sure if it's a blacklist request. Also, if we have stuff in the
blacklist store, we'll keep kicking off new requests as we get responses. So
we'll keep filling the cache.

Basically, what's the stance on stopping syncing (all / only history). Does it
apply to the content of the blacklist store, or is the user expectation that it
should be uploaded since it was blacklisted before the change?

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_service.h:69: // fetch request unless a
pending one exists. To prevent multiple requests,
I guess we shouldn't issue a request in the serve from cache state.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
File components/suggestions/suggestions_utils.cc (right):

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.cc:10: bool sync_enabled,
Perhaps put this argument first, since it's the first thing you check. Reading
in this order, you're led to believe you might need to initialize before you can
tell if it's enabled.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.cc:14: // Sync is enabled.
Not sure about the location of this comment. Same below.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
File components/suggestions/suggestions_utils.h (right):

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:10: // Establishes the different sync
states that users of  SuggestionsService can
nit: double space "of  Sugg...".

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:13: enum SyncState {
Nit: how about FetchType: CACHE, SERVER, CLEAR.

Basically, something that centers on the SuggestionsService instead of on Sync.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:15: // is unknown (since not
initialized).
Maybe document the fetch type, instead of what motivates it? Eg Don't issue a
server request, instead serve suggestions from cache if any.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:19: INITIALIZED_ENABLED_HISTORY,
Update suggestions from the server. Serve from cache on timeout.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:23: DISABLED,
Do not issue a server request. Clear the cache.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:26: SyncState GetSyncState(bool
sync_initialized,
Perhaps this is where you can document what conditions lead to which FetchType.

[Mathieu, 2014-08-20 18:41:30]

Thanks, your input is appreciated!

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_service.cc:169:
suggestions_store_->ClearSuggestions();
On 2014/08/20 15:43:51, manzagop wrote:
> Also cancel any ongoing request and serve the waiting requestors? 
> 
> Well... not sure if it's a blacklist request. Also, if we have stuff in the
> blacklist store, we'll keep kicking off new requests as we get responses. So
> we'll keep filling the cache.
> 
> Basically, what's the stance on stopping syncing (all / only history). Does it
> apply to the content of the blacklist store, or is the user expectation that
it
> should be uploaded since it was blacklisted before the change?

We shouldn't interact with the server anymore when sync is disabled.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
File components/suggestions/suggestions_service.h (right):

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_service.h:69: // fetch request unless a
pending one exists. To prevent multiple requests,
On 2014/08/20 15:43:51, manzagop wrote:
> I guess we shouldn't issue a request in the serve from cache state.

Done. Reworded comment.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
File components/suggestions/suggestions_utils.cc (right):

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.cc:10: bool sync_enabled,
On 2014/08/20 15:43:51, manzagop wrote:
> Perhaps put this argument first, since it's the first thing you check. Reading
> in this order, you're led to believe you might need to initialize before you
can
> tell if it's enabled.

Done.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.cc:14: // Sync is enabled.
On 2014/08/20 15:43:51, manzagop wrote:
> Not sure about the location of this comment. Same below.

Done.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
File components/suggestions/suggestions_utils.h (right):

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:10: // Establishes the different sync
states that users of  SuggestionsService can
On 2014/08/20 15:43:51, manzagop wrote:
> nit: double space "of  Sugg...".

Done.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:13: enum SyncState {
On 2014/08/20 15:43:51, manzagop wrote:
> Nit: how about FetchType: CACHE, SERVER, CLEAR.
> 
> Basically, something that centers on the SuggestionsService instead of on
Sync.
> 

Ack. Discussed offline. I don't want users of SuggestionsService to focus on
behavior, but rather on Sync State because it enforces the API and reduces the
possibility of someone just passing the literal SERVER enum as argument.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:15: // is unknown (since not
initialized).
On 2014/08/20 15:43:51, manzagop wrote:
> Maybe document the fetch type, instead of what motivates it? Eg Don't issue a
> server request, instead serve suggestions from cache if any.

Done.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:19: INITIALIZED_ENABLED_HISTORY,
On 2014/08/20 15:43:51, manzagop wrote:
> Update suggestions from the server. Serve from cache on timeout.

Done.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:23: DISABLED,
On 2014/08/20 15:43:51, manzagop wrote:
> Do not issue a server request. Clear the cache.

Done.

https://codereview.chromium.org/473123002/diff/40001/components/suggestions/s...
components/suggestions/suggestions_utils.h:26: SyncState GetSyncState(bool
sync_initialized,
On 2014/08/20 15:43:51, manzagop wrote:
> Perhaps this is where you can document what conditions lead to which
FetchType.

Acknowledged.

[manzagop (departed), 2014-08-20 19:28:14]

https://codereview.chromium.org/473123002/diff/60001/components/suggestions/s...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/473123002/diff/60001/components/suggestions/s...
components/suggestions/suggestions_service.cc:174:
callback.Run(SuggestionsProfile());
Also serve the waiting requestors?

https://codereview.chromium.org/473123002/diff/60001/components/suggestions/s...
File components/suggestions/suggestions_utils.h (right):

https://codereview.chromium.org/473123002/diff/60001/components/suggestions/s...
components/suggestions/suggestions_utils.h:26: // Behavior: Do not issue a
server request. Clear the cache.
Also "Serve empty suggestions"? To make it clear the callback still gets called.

[manzagop (departed), 2014-08-20 19:28:49]

LGTM up to the requestors thing.

[Mathieu, 2014-08-20 21:37:48]

Thanks P-A.

Newton, can you look at the changes to most_visited_sites? We're enforcing a
sync state check now for SuggestionsService, for privacy requirements.
MostVisitedSites therefore needs to listen to sync changes and update
accordingly. Happy to discuss more if needed.

https://codereview.chromium.org/473123002/diff/60001/components/suggestions/s...
File components/suggestions/suggestions_service.cc (right):

https://codereview.chromium.org/473123002/diff/60001/components/suggestions/s...
components/suggestions/suggestions_service.cc:174:
callback.Run(SuggestionsProfile());
On 2014/08/20 19:28:14, manzagop wrote:
> Also serve the waiting requestors?

Done.

https://codereview.chromium.org/473123002/diff/60001/components/suggestions/s...
File components/suggestions/suggestions_utils.h (right):

https://codereview.chromium.org/473123002/diff/60001/components/suggestions/s...
components/suggestions/suggestions_utils.h:26: // Behavior: Do not issue a
server request. Clear the cache.
On 2014/08/20 19:28:14, manzagop wrote:
> Also "Serve empty suggestions"? To make it clear the callback still gets
called.

Done.

[newt (away), 2014-08-20 21:49:55]

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

https://codereview.chromium.org/473123002/diff/80001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:262: mv_source_ ==
SUGGESTIONS_SERVICE;
extra space

https://codereview.chromium.org/473123002/diff/80001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:350: QueryMostVisitedURLs();
If we're showing TopSites instead of using the SuggestionService, won't this
cause unnecessary updates? i.e. shouldn't this be:

    if (mv_source_ == SUGGESTIONS_SERVICE) {
        QueryMostVisitedURLs();
    }

[Mathieu, 2014-08-20 22:23:56]

Thanks!

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

https://codereview.chromium.org/473123002/diff/80001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:262: mv_source_ ==
SUGGESTIONS_SERVICE;
On 2014/08/20 21:49:55, newt wrote:
> extra space

Hmm sorry I don't see it. It's indented by 4...

https://codereview.chromium.org/473123002/diff/80001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:350: QueryMostVisitedURLs();
On 2014/08/20 21:49:55, newt wrote:
> If we're showing TopSites instead of using the SuggestionService, won't this
> cause unnecessary updates? i.e. shouldn't this be:
> 
>     if (mv_source_ == SUGGESTIONS_SERVICE) {
>         QueryMostVisitedURLs();
>     }

If we served TopSites because let's say sync was disabled, we still want to call
QueryMostVisitedURLs when the sync state changes to enabled, in case it uses
SuggestionsService. QueryMostVisitedURLs will take the right decision, and if
SuggestionsService is disabled still, it will return quickly.

[newt (away), 2014-08-20 22:38:56]

lgtm

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

https://codereview.chromium.org/473123002/diff/80001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:262: mv_source_ ==
SUGGESTIONS_SERVICE;
On 2014/08/20 22:23:56, Mathieu Perreault wrote:
> On 2014/08/20 21:49:55, newt wrote:
> > extra space
> 
> Hmm sorry I don't see it. It's indented by 4...

Sorry, my font is weird and doesn't always show underlines. I though it said
"mv_source  ==", but it's really "mv_source_ =="

https://codereview.chromium.org/473123002/diff/80001/chrome/browser/android/m...
chrome/browser/android/most_visited_sites.cc:350: QueryMostVisitedURLs();
On 2014/08/20 22:23:56, Mathieu Perreault wrote:
> On 2014/08/20 21:49:55, newt wrote:
> > If we're showing TopSites instead of using the SuggestionService, won't this
> > cause unnecessary updates? i.e. shouldn't this be:
> > 
> >     if (mv_source_ == SUGGESTIONS_SERVICE) {
> >         QueryMostVisitedURLs();
> >     }
> 
> If we served TopSites because let's say sync was disabled, we still want to
call
> QueryMostVisitedURLs when the sync state changes to enabled, in case it uses
> SuggestionsService. QueryMostVisitedURLs will take the right decision, and if
> SuggestionsService is disabled still, it will return quickly.

Ok, I understand that this is needed in some cases. It'll still cause
unnecessary updates in some cases, but that's harder to fix, and not a huge
problem. The Java view code is pretty efficient with handling unneeded updates.

[Mathieu, 2014-08-20 23:16:13]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-08-20 23:17:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/473123002/80001

[commit-bot: I haz the power, 2014-08-21 00:47:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)

[Mathieu, 2014-08-21 00:48:19]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2014-08-21 00:52:06]

TBR erikwright for suggestions.gypi (I should really make myself an owner of
that)

[Mathieu, 2014-08-21 00:52:12]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-08-21 00:54:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/473123002/80001

[commit-bot: I haz the power, 2014-08-21 00:58:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[commit-bot: I haz the power, 2014-08-21 01:01:57]

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

[commit-bot: I haz the power, 2014-08-21 01:02:00]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Mathieu, 2014-08-21 12:49:17]

The CQ bit was checked by mathp@chromium.org

[commit-bot: I haz the power, 2014-08-21 12:50:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mathp@chromium.org/473123002/80001

[Mathieu, 2014-08-21 17:13:12]

The CQ bit was unchecked by mathp@chromium.org

[Mathieu, 2014-08-21 17:20:32]

On 2014/08/21 17:13:12, Mathieu Perreault wrote:
> The CQ bit was unchecked by mailto:mathp@chromium.org

Change committed.