[NTP Snippets] Don't reschedule background fetching on every startup

[NTP Snippets] Don't reschedule background fetching on every startup

Instead, store the currently-scheduled intervals in prefs, and only reschedule when something changed.

BUG=646842
Committed: https://crrev.com/990b825d970ab5998c535d36666365105bc4acd7
Cr-Commit-Position: refs/heads/master@{#421158}

[Marc Treib, 2016-09-22 16:30:12]

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

[Marc Treib, 2016-09-22 16:30:13]

PTAL!

[jkrcal, 2016-09-23 10:28:34]

https://codereview.chromium.org/2363753002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2363753002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:95: // Reschedules the
fetching of snippets.
Maybe expanding the comment? That it does forced rescheduling, where it is
supposed to be called from, etc.

https://codereview.chromium.org/2363753002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2363753002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:251: interval_fallback !=
old_interval_fallback) {
What if:
 - the current fallback is scheduled, say, to 24h,
 - it is right after upgrade, i.e. the pref has the default value 0,
 - the new fallback interval is set to be 0?
 (and the same for the wifi interval)

To me it looks it ends up doing nothing. It should reschedule (in fact
unschedule) the fallback. Hm?

It is a bit niche, still cannot you make the default values for the prefs be -1
or MAX_INT?

[Marc Treib, 2016-09-23 11:19:13]

https://codereview.chromium.org/2363753002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2363753002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:95: // Reschedules the
fetching of snippets.
On 2016/09/23 10:28:34, jkrcal wrote:
> Maybe expanding the comment? That it does forced rescheduling, where it is
> supposed to be called from, etc.

I added the "forced" part. IMO where it is supposed to be called from isn't good
to mention in a comment, it's bound to get out of date.

https://codereview.chromium.org/2363753002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2363753002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:251: interval_fallback !=
old_interval_fallback) {
On 2016/09/23 10:28:34, jkrcal wrote:
> What if:
>  - the current fallback is scheduled, say, to 24h,
>  - it is right after upgrade, i.e. the pref has the default value 0,

Prefs do survive the upgrade, so they won't be reset to their default value.
The actual scheduled tasks don't survive, so right now we might end up in a
situation where we think (via prefs) that things are scheduled, but they're
actually not. That will be addressed by the next CL when I implement
GcmTaskService.onInitializeTasks, see
https://developers.google.com/android/reference/com/google/android/gms/gcm/Gc...

>  - the new fallback interval is set to be 0?
>  (and the same for the wifi interval)
> 
> To me it looks it ends up doing nothing. It should reschedule (in fact
> unschedule) the fallback. Hm?
> 
> It is a bit niche, still cannot you make the default values for the prefs be
-1
> or MAX_INT?

Per above, I think having 0 (= "not scheduled") as the default makes sense.

[jkrcal, 2016-09-23 11:24:54]

lgtm, thanks! 
(and sorry for the delay before the first review)

https://codereview.chromium.org/2363753002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/ntp_snippets_bridge.cc (right):

https://codereview.chromium.org/2363753002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/ntp_snippets_bridge.cc:95: // Reschedules the
fetching of snippets.
On 2016/09/23 11:19:13, Marc Treib wrote:
> On 2016/09/23 10:28:34, jkrcal wrote:
> > Maybe expanding the comment? That it does forced rescheduling, where it is
> > supposed to be called from, etc.
> 
> I added the "forced" part. IMO where it is supposed to be called from isn't
good
> to mention in a comment, it's bound to get out of date.

Acknowledged.

https://codereview.chromium.org/2363753002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/2363753002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:251: interval_fallback !=
old_interval_fallback) {
On 2016/09/23 11:19:13, Marc Treib wrote:
> On 2016/09/23 10:28:34, jkrcal wrote:
> > What if:
> >  - the current fallback is scheduled, say, to 24h,
> >  - it is right after upgrade, i.e. the pref has the default value 0,
> 
> Prefs do survive the upgrade, so they won't be reset to their default value.
> The actual scheduled tasks don't survive, so right now we might end up in a
> situation where we think (via prefs) that things are scheduled, but they're
> actually not. That will be addressed by the next CL when I implement
> GcmTaskService.onInitializeTasks, see
>
https://developers.google.com/android/reference/com/google/android/gms/gcm/Gc...
> 
> >  - the new fallback interval is set to be 0?
> >  (and the same for the wifi interval)
> > 
> > To me it looks it ends up doing nothing. It should reschedule (in fact
> > unschedule) the fallback. Hm?
> > 
> > It is a bit niche, still cannot you make the default values for the prefs be
> -1
> > or MAX_INT?
> 
> Per above, I think having 0 (= "not scheduled") as the default makes sense.

I see, thanks!

[Marc Treib, 2016-09-27 08:24:16]

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

[commit-bot: I haz the power, 2016-09-27 08:24:34]

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-09-27 09:06:18]

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

[commit-bot: I haz the power, 2016-09-27 09:06:19]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-09-27 09:28:32]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-09-27 09:28:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org
Link to the patchset: https://codereview.chromium.org/2363753002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2016-09-27 09:28:49]

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-09-27 09:33:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-27 09:35:08]

Description was changed from

==========
[NTP Snippets] Don't reschedule background fetching on every startup

Instead, store the currently-scheduled intervals in prefs, and only reschedule
when something changed.

BUG=646842
==========

to

==========
[NTP Snippets] Don't reschedule background fetching on every startup

Instead, store the currently-scheduled intervals in prefs, and only reschedule
when something changed.

BUG=646842

Committed: https://crrev.com/990b825d970ab5998c535d36666365105bc4acd7
Cr-Commit-Position: refs/heads/master@{#421158}
==========

[commit-bot: I haz the power, 2016-09-27 09:35:09]

Patchset 3 (id:??) landed as
https://crrev.com/990b825d970ab5998c535d36666365105bc4acd7
Cr-Commit-Position: refs/heads/master@{#421158}