SuggestionsService: don't automatically fetch on startup

SuggestionsService: don't automatically fetch on startup

Expected impact:
- On mobile: Before, we would fetch during startup only if an NTP
  happened to be open, i.e. semi-randomly. Now we don't fetch at
  all. This might decrease tile freshness for the first NTP in some
  cases, but the difference should be minimal.
- On desktop, without experiments: No impact, since this code isn't
  used.
- On desktop, with NTPTilesInInstantService: Before, we'd always fetch
  during startup, after Sync finishes initialization. Like on mobile,
  this might decrease freshness in some cases, but only minimally.

On both mobile and desktop, this will significantly reduce traffic to
the server; see the bug for numbers.

BUG=718845
Review-Url: https://codereview.chromium.org/2866013002
Cr-Commit-Position: refs/heads/master@{#469999}
Committed: https://chromium.googlesource.com/chromium/src/+/a114fa410cb49bf11df659f51763420838c49605

[Marc Treib, 2017-05-08 13:43:33]

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

[commit-bot: I haz the power, 2017-05-08 13:43:47]

Dry run: CQ is trying da patch.

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

[Marc Treib, 2017-05-08 13:45:54]

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

[Marc Treib, 2017-05-08 13:45:54]

This is what I'd suggest: Fetch on sign-in, but not during startup. Also remove
the "only if observers" restriction since it doesn't make sense, now that we
don't (want to) update NTPs in-place anyway.

[mastiz, 2017-05-08 14:18:32]

LGTM!

Can you please elaborate the CL description to cover which impact is expected,
per platform? From what I understood:
- Mobile will issue less fetches only if an NTP is opened (semi-random),
possibly leading to more stale suggestions (IMO negligible).
- Desktop doesn't currently use this codepath (right?).

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
File components/suggestions/suggestions_service_impl.cc (right):

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
components/suggestions/suggestions_service_impl.cc:308: if (new_sync_state ==
SYNC_OR_HISTORY_SYNC_DISABLED) {
Optional: How about using a switch with |new_sync_state|? One way would be to
store |old_sync_state|, assign to |sync_state_| early, such that you can use
return statements within the switch (and avoid |result|).

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
components/suggestions/suggestions_service_impl.cc:326: RefreshAction action =
RefreshSyncState();
Optional: avoid the temporary variable.

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
components/suggestions/suggestions_service_impl.cc:359: // requests if a request
happens to be ongoing.
Nit: I'm not a very fan of this TODO phrasing, since there's really no proposed
action. I'd suggest:
// This will silently swallow blacklist and clearblacklist
// requests if a request happens to be ongoing.
// TODO(treib): <Do this or that>.

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
File components/suggestions/suggestions_service_impl_unittest.cc (right):

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
components/suggestions/suggestions_service_impl_unittest.cc:330:
EXPECT_CALL(callback, Run(_)).Times(0);
I should've mentioned this earlier (as part of the StrictMock discussion), but
what you probably want is to move this expectation immediately after the
callback is instantiated. Right now the callback could've been called within
AddCallback() and the test would pass.

[commit-bot: I haz the power, 2017-05-08 14:33:10]

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

[commit-bot: I haz the power, 2017-05-08 14:33:12]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-05-08 14:42:29]

Description was changed from

==========
SuggestionsService: don't automatically fetch on startup

BUG=718845
==========

to

==========
SuggestionsService: don't automatically fetch on startup

Expected impact:
- On mobile: Before, we would fetch during startup only if an NTP
  happened to be open, i.e. semi-randomly. Now we don't fetch at
  all. This might decrease tile freshness for the first NTP in some
  cases, but the difference should be minimal.
- On desktop, without experiments: No impact, since this code isn't
  used.
- On desktop, with NTPTilesInInstantService: Before, we'd always fetch
  during startup, after Sync finishes initialization. Like on mobile,
  this might decrease freshness in some cases, but probably not very
  significantly.

On both mobile and desktop, this should significantly reduce traffic to
the server; see the bug for numbers.

BUG=718845
==========

[Marc Treib, 2017-05-08 14:44:59]

Description was changed from

==========
SuggestionsService: don't automatically fetch on startup

Expected impact:
- On mobile: Before, we would fetch during startup only if an NTP
  happened to be open, i.e. semi-randomly. Now we don't fetch at
  all. This might decrease tile freshness for the first NTP in some
  cases, but the difference should be minimal.
- On desktop, without experiments: No impact, since this code isn't
  used.
- On desktop, with NTPTilesInInstantService: Before, we'd always fetch
  during startup, after Sync finishes initialization. Like on mobile,
  this might decrease freshness in some cases, but probably not very
  significantly.

On both mobile and desktop, this should significantly reduce traffic to
the server; see the bug for numbers.

BUG=718845
==========

to

==========
SuggestionsService: don't automatically fetch on startup

Expected impact:
- On mobile: Before, we would fetch during startup only if an NTP
  happened to be open, i.e. semi-randomly. Now we don't fetch at
  all. This might decrease tile freshness for the first NTP in some
  cases, but the difference should be minimal.
- On desktop, without experiments: No impact, since this code isn't
  used.
- On desktop, with NTPTilesInInstantService: Before, we'd always fetch
  during startup, after Sync finishes initialization. Like on mobile,
  this might decrease freshness in some cases, but probably not very
  significantly.

On both mobile and desktop, this will significantly reduce traffic to
the server; see the bug for numbers.

BUG=718845
==========

[Marc Treib, 2017-05-08 14:45:25]

Description was changed from

==========
SuggestionsService: don't automatically fetch on startup

Expected impact:
- On mobile: Before, we would fetch during startup only if an NTP
  happened to be open, i.e. semi-randomly. Now we don't fetch at
  all. This might decrease tile freshness for the first NTP in some
  cases, but the difference should be minimal.
- On desktop, without experiments: No impact, since this code isn't
  used.
- On desktop, with NTPTilesInInstantService: Before, we'd always fetch
  during startup, after Sync finishes initialization. Like on mobile,
  this might decrease freshness in some cases, but probably not very
  significantly.

On both mobile and desktop, this will significantly reduce traffic to
the server; see the bug for numbers.

BUG=718845
==========

to

==========
SuggestionsService: don't automatically fetch on startup

Expected impact:
- On mobile: Before, we would fetch during startup only if an NTP
  happened to be open, i.e. semi-randomly. Now we don't fetch at
  all. This might decrease tile freshness for the first NTP in some
  cases, but the difference should be minimal.
- On desktop, without experiments: No impact, since this code isn't
  used.
- On desktop, with NTPTilesInInstantService: Before, we'd always fetch
  during startup, after Sync finishes initialization. Like on mobile,
  this might decrease freshness in some cases, but only minimally.

On both mobile and desktop, this will significantly reduce traffic to
the server; see the bug for numbers.

BUG=718845
==========

[Marc Treib, 2017-05-08 14:57:46]

Thanks! All addressed.

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
File components/suggestions/suggestions_service_impl.cc (right):

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
components/suggestions/suggestions_service_impl.cc:308: if (new_sync_state ==
SYNC_OR_HISTORY_SYNC_DISABLED) {
On 2017/05/08 14:18:32, mastiz wrote:
> Optional: How about using a switch with |new_sync_state|? One way would be to
> store |old_sync_state|, assign to |sync_state_| early, such that you can use
> return statements within the switch (and avoid |result|).

Done.

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
components/suggestions/suggestions_service_impl.cc:326: RefreshAction action =
RefreshSyncState();
On 2017/05/08 14:18:32, mastiz wrote:
> Optional: avoid the temporary variable.

Done.

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
components/suggestions/suggestions_service_impl.cc:359: // requests if a request
happens to be ongoing.
On 2017/05/08 14:18:32, mastiz wrote:
> Nit: I'm not a very fan of this TODO phrasing, since there's really no
proposed
> action. I'd suggest:
> // This will silently swallow blacklist and clearblacklist
> // requests if a request happens to be ongoing.
> // TODO(treib): <Do this or that>.

Done.

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
File components/suggestions/suggestions_service_impl_unittest.cc (right):

https://codereview.chromium.org/2866013002/diff/1/components/suggestions/sugg...
components/suggestions/suggestions_service_impl_unittest.cc:330:
EXPECT_CALL(callback, Run(_)).Times(0);
On 2017/05/08 14:18:32, mastiz wrote:
> I should've mentioned this earlier (as part of the StrictMock discussion), but
> what you probably want is to move this expectation immediately after the
> callback is instantiated. Right now the callback could've been called within
> AddCallback() and the test would pass.

Done.

[Marc Treib, 2017-05-08 14:57:51]

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

[commit-bot: I haz the power, 2017-05-08 14:59:14]

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-05-08 15:57:59]

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

[commit-bot: I haz the power, 2017-05-08 15:57:59]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-05-08 16:03:08]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2017-05-08 16:03:08]

The patchset sent to the CQ was uploaded after l-g-t-m from mastiz@chromium.org
Link to the patchset: https://codereview.chromium.org/2866013002/#ps20001
(title: "review")

[commit-bot: I haz the power, 2017-05-08 16:04:01]

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-05-08 16:10:06]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1494259388387470,
"parent_rev": "a5c39d39a0e9a28a246db3ad016e4490fed01041", "commit_rev":
"a114fa410cb49bf11df659f51763420838c49605"}

[commit-bot: I haz the power, 2017-05-08 16:10:13]

Description was changed from

==========
SuggestionsService: don't automatically fetch on startup

Expected impact:
- On mobile: Before, we would fetch during startup only if an NTP
  happened to be open, i.e. semi-randomly. Now we don't fetch at
  all. This might decrease tile freshness for the first NTP in some
  cases, but the difference should be minimal.
- On desktop, without experiments: No impact, since this code isn't
  used.
- On desktop, with NTPTilesInInstantService: Before, we'd always fetch
  during startup, after Sync finishes initialization. Like on mobile,
  this might decrease freshness in some cases, but only minimally.

On both mobile and desktop, this will significantly reduce traffic to
the server; see the bug for numbers.

BUG=718845
==========

to

==========
SuggestionsService: don't automatically fetch on startup

Expected impact:
- On mobile: Before, we would fetch during startup only if an NTP
  happened to be open, i.e. semi-randomly. Now we don't fetch at
  all. This might decrease tile freshness for the first NTP in some
  cases, but the difference should be minimal.
- On desktop, without experiments: No impact, since this code isn't
  used.
- On desktop, with NTPTilesInInstantService: Before, we'd always fetch
  during startup, after Sync finishes initialization. Like on mobile,
  this might decrease freshness in some cases, but only minimally.

On both mobile and desktop, this will significantly reduce traffic to
the server; see the bug for numbers.

BUG=718845

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

[commit-bot: I haz the power, 2017-05-08 16:10:19]

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